Skip to content

Hash cache key so reserved characters do not cause keys to be rejected.#116

Open
mulldug wants to merge 1 commit intomainfrom
fix/invalid-cache-key
Open

Hash cache key so reserved characters do not cause keys to be rejected.#116
mulldug wants to merge 1 commit intomainfrom
fix/invalid-cache-key

Conversation

@mulldug
Copy link
Collaborator

@mulldug mulldug commented Mar 4, 2026

Cache keys were being rejected because they contained characters that are reserved in PSR-6 cache keys. The fix is to hash the key.

Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix — the md5() hashing correctly eliminates PSR-6 reserved characters (/ in particular) from cache keys.

However, this PR is missing a regression test. Without one, the md5() call could be accidentally reverted with no test failure to catch it.

Suggested test coverage (integration test using the real cache backend):

  1. Cache write/read with reserved characters — call getClientByIdCacheable with a client ID containing / (e.g. .-_~87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x.openstack.client from TestSeeder), assert it returns the correct client without throwing InvalidArgumentException. Call it a second time to verify the cached entry is also retrievable.

  2. Distinct cache entries for different IDs — call getClientByIdCacheable with two different client IDs that both contain /, assert they return different clients. This confirms the hashing doesn't accidentally collapse distinct keys.

These can go in a new tests/OAuth2ClientCacheTest.php extending BrowserKitTestCase (which seeds the test data and connects to Redis).

@smarcet
Copy link
Collaborator

smarcet commented Mar 4, 2026

@mulldug please

  1. we do follow Conventional Commits messages ( see https://www.conventionalcommits.org/en/v1.0.0/)
  2. please provide unit test to verify the fix and add it to the CI

Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mulldug please review comments

…ed character violation

  symfony/cache v7+ enforces PSR-6 key validation in all adapters, including
  array. Client IDs may contain reserved characters (e.g. '/'), which caused
  getClientByIdCacheable() to throw InvalidArgumentException and return a
  server_error 400 during token exchange.

  Replace the raw client ID with its md5 hash in the cache key so the key
  is always a valid hex string, regardless of the client ID's content.

  Adds OAuth2ClientCacheTest to cover retrieval with reserved-character IDs
  and to confirm distinct IDs hash to distinct cache keys.
@mulldug mulldug force-pushed the fix/invalid-cache-key branch from 94793b4 to c686d19 Compare March 4, 2026 22:20
@mulldug
Copy link
Collaborator Author

mulldug commented Mar 5, 2026

Tests have been added and the commit message updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants