Skip to content

[Security] Allow enums in SignatureHasher::computeSignatureHash() #60302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: 7.3
Choose a base branch
from

Conversation

BenMorel
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Currently, using remember_me.signature_properties on a property holding an enum fails with:

The property path "status" on the user object "App\Security\SecurityUser" must return a value that can be cast to a string, but "App(...)\UserStatus" was returned.

We can add support for enums in one of three ways:

  1. support all enums, and use the enum case name
  2. support only backed enums, and use the enum case backing value (int|string)
  3. support all enums, and use enum case name + backing value if available (most sensitive to changes)

In the current PR I went with option 2, but I'm thinking that option 3 could be better:

Pros:

  • it would support all enums
  • it would detect changes such as exchanging values of 2 enum cases

Cons:

  • it would invalidate the token if an enum case is renamed

I would welcome feedback here.

@derrabus
Copy link
Member

Tests? 🙃

@OskarStark OskarStark changed the title Allow enums in SignatureHasher::computeSignatureHash() Allow enums in SignatureHasher::computeSignatureHash() Apr 30, 2025
@carsonbot carsonbot changed the title Allow enums in SignatureHasher::computeSignatureHash() [Security] Allow enums in SignatureHasher::computeSignatureHash() Apr 30, 2025
@stof
Copy link
Member

stof commented Apr 30, 2025

Only option 2 makes sense to me. If you need your enum to have a canonical scalar representation, you should use a backed enum (that's what they are about).

  • it would detect changes such as exchanging values of 2 enum cases

that's not something we need to detect IMO (your DB would also have issues in such case, and this would require an insane architecture IMO)

@BenMorel
Copy link
Contributor Author

Alright then, thanks for your input, @stof.
@derrabus I'll add tests for the signature hasher, I was actually surprised to find none.

@BenMorel BenMorel force-pushed the signature_hasher_enum branch 3 times, most recently from 5c6d8d2 to 554fb1f Compare May 21, 2025 11:34
@BenMorel BenMorel force-pushed the signature_hasher_enum branch from 554fb1f to 8b14913 Compare May 21, 2025 12:09
@BenMorel BenMorel force-pushed the signature_hasher_enum branch from 3815106 to d13454c Compare May 21, 2025 12:38
@BenMorel
Copy link
Contributor Author

@chalasr @derrabus @stof Ready for review.

I've split the PR into 3 commits:

  • Commit 1 adds the feature, and adds missing tests for SignatureHasher.
    As you can see, tests are rather ugly as in its current state, SignatureHasher uses a hardcoded combination of hash_*() functions, base64_encode() and strtr(), with no mechanism in place to mock these:
    ['someValue', ['arbitraryValue'], 'kfMzZgYYD1oeqSSW7m0k94VuRvS7LeHcKq-PKU8WD7k~0nuV8X2IlHqxDdPRNOLP-wp_v2KdVL9dNYJ0_557fGc~'],
    ['someValue', ['identifier', 'arbitraryValue'], 'myxvvho8WkMuOcMMeuRlZQFe58TNDQFgDrVFb8SZ50g~iJ4d_Agaa0AaCHZinVr_zZCgR2nSZgokvXIkv7ne1b4~'],
  • Commit 2 proposes introducing HasherInterface and HashContextInterface, which are now mockable, and allow to see what is hashed exactly in tests:
    - ['someValue', ['arbitraryValue'], 'kfMzZgYYD1oeqSSW7m0k94VuRvS7LeHcKq-PKU8WD7k~0nuV8X2IlHqxDdPRNOLP-wp_v2KdVL9dNYJ0_557fGc~'],
    - ['someValue', ['identifier', 'arbitraryValue'], 'myxvvho8WkMuOcMMeuRlZQFe58TNDQFgDrVFb8SZ50g~iJ4d_Agaa0AaCHZinVr_zZCgR2nSZgokvXIkv7ne1b4~'],
    + ['someValue', ['arbitraryValue'], true, 'HMAC(HASH(:someValue):1234567890:username,s3cr3t)HASH(:someValue)'],
    + ['someValue', ['identifier', 'arbitraryValue'], true, 'HMAC(HASH(:username:someValue):1234567890:username,s3cr3t)HASH(:username:someValue)'],
  • Commit 3 fixes a missing dependency on symfony/property-access, highlighted by the newly added tests for SignatureHasher.

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

Successfully merging this pull request may close these issues.

5 participants