fix(security): reject non-first-party tokens, stop ROLE_SERVICE inference (#312)#325
Open
dmytrocraft wants to merge 4 commits into
Open
fix(security): reject non-first-party tokens, stop ROLE_SERVICE inference (#312)#325dmytrocraft wants to merge 4 commits into
dmytrocraft wants to merge 4 commits into
Conversation
…ence (#312) AccessTokenValidator silently elevated any RS256 JWT lacking both `iss` and `roles` to ROLE_SERVICE and skipped issuer/audience validation for that token class. Because the Lexik first-party JWT and the League OAuth2 server share a single RSA keypair, a League OAuth2 access token (aud=client_id, no iss/roles) passed signature verification and was promoted to ROLE_SERVICE, granting any low-privilege client or end user access to POST /api/users/batch (bulk import). - Validate issuer and audience unconditionally for every access token; tokens whose iss != vilnacrm-user-service or aud !contains vilnacrm-api are rejected. - Default an absent roles claim to the least-privilege ROLE_USER; never infer ROLE_SERVICE from missing claims (CWE-1188 insecure default). - Add explicit API Platform `security: is_granted('ROLE_SERVICE')` on the bulk-import operation as defense-in-depth. - Flip the unit test that encoded the vulnerable behaviour to assert rejection; add regression tests for OAuth2-shaped tokens and the absent-roles case. Closes #312 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR hardens JWT/OAuth2 token validation to prevent silent escalation to ROLE_SERVICE: it enforces unconditional issuer/audience checks, requires sid when roles are present, defaults missing roles to ROLE_USER, updates tests and integration expectations, and adds an API Platform operation-level ROLE_SERVICE guard for bulk-import. ChangesAccess Token Escalation Vulnerability Fix
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
The integration test `testOauthClientCredentialsAccessTokenCanReachProtected ServiceRoute` encoded the vulnerable behaviour — it asserted a League OAuth2 client_credentials access token COULD reach POST /api/users/batch. With the ROLE_SERVICE escalation fixed, that token is now rejected with 401, so the test is renamed and flipped to assert the secure outcome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 551
Lines 9541 9538 -3
=========================================
- Hits 9541 9538 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 6 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client as External Client
participant Gateway as API Gateway / Firewall
participant ApiPlatform as API Platform (User.yaml)
participant Validator as AccessTokenValidator
participant JwtDecoder as Lexik JWT Decoder
participant Resolver as AccessTokenUserResolver
participant Db as Database
Note over Client,Db: NEW: Mandatory Issuer/Audience Validation Flow
Client->>Gateway: POST /api/users/batch + Authorization: Bearer <token>
Gateway->>Gateway: Extract JWT token from header
alt Token is League OAuth2 token (aud=client_id, no iss)
Gateway->>Validator: validate(token)
Validator->>JwtDecoder: decode(token) → payload
JwtDecoder-->>Validator: {sub, aud=client_id, exp, nbf}
Validator->>Validator: validateClaims()
Validator->>Validator: validateIssuerAndAudience()
Note over Validator: Issuer missing → REJECTED
Validator->>Gateway: Throw InvalidAccessTokenException
Gateway-->>Client: 401 Unauthorized
else Token is first-party with explicit roles
Gateway->>Validator: validate(token)
Validator->>JwtDecoder: decode(token) → payload
JwtDecoder-->>Validator: {iss=vilnacrm-user-service, aud=vilnacrm-api, sid, roles=[ROLE_SERVICE]}
Validator->>Validator: validateClaims()
Validator->>Validator: validateIssuerAndAudience() ✓
Validator->>Validator: validateSessionBinding() ✓
Validator->>Validator: extractRoles()
Note over Validator: roles present → use explicit roles
Validator-->>Gateway: {subject, sid, roles=[ROLE_SERVICE]}
Gateway->>Resolver: resolveUser(payload)
Resolver-->>Gateway: User with ROLE_SERVICE
Gateway->>ApiPlatform: Check is_granted('ROLE_SERVICE')
ApiPlatform-->>Gateway: Granted
Gateway->>Db: Execute batch import
Db-->>Gateway: Users created
Gateway-->>Client: 201 Created
else Token has no roles claim (valid first-party)
Gateway->>Validator: validate(token)
Validator->>JwtDecoder: decode(token) → payload
JwtDecoder-->>Validator: {iss=vilnacrm-user-service, aud=vilnacrm-api, sid}
Validator->>Validator: validateClaims()
Validator->>Validator: validateIssuerAndAudience() ✓
Validator->>Validator: validateSessionBinding()
Note over Validator: No roles claim → skip sid check
Validator->>Validator: extractRoles()
Note over Validator: roles absent → default to ROLE_USER
Validator-->>Gateway: {subject, sid, roles=[ROLE_USER]}
Gateway->>Resolver: resolveUser(payload)
Resolver-->>Gateway: User with ROLE_USER
Gateway->>ApiPlatform: Check is_granted('ROLE_SERVICE')
ApiPlatform-->>Gateway: Denied
Gateway-->>Client: 403 Forbidden
else Token has roles but no sid
Gateway->>Validator: validate(token)
Validator->>JwtDecoder: decode(token) → payload
JwtDecoder-->>Validator: {iss=vilnacrm-user-service, aud=vilnacrm-api, roles=[ROLE_SERVICE]}
Validator->>Validator: validateClaims()
Validator->>Validator: validateIssuerAndAudience() ✓
Validator->>Validator: validateSessionBinding()
Note over Validator: roles present but sid missing → REJECTED
Validator->>Gateway: Throw InvalidAccessTokenException
Gateway-->>Client: 401 Unauthorized
end
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Shared/Infrastructure/Validator/AccessTokenValidator.php (1)
103-107: ⚡ Quick winRemove inline explanatory comments from
src/**/*.php.Please replace these inline comments with self-explanatory naming/tests (or small method extraction), per repository rules for source files.
As per coding guidelines,
src/**/*.php: “Remove inline comments; write self-explanatory code with clear naming. Extract helper methods instead of using comments for explanation.”Also applies to: 216-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Shared/Infrastructure/Validator/AccessTokenValidator.php` around lines 103 - 107, Replace the inline explanatory comments in AccessTokenValidator (the block describing unconditional issuer/audience validation for every token) by extracting the check into a small, well-named method and/or using self-explanatory variable names so the behavior is clear without comments; e.g., create a method on AccessTokenValidator like ensureFirstPartyTokenConstraintsOrValidateIssuerAndAudience() (or validateIssuerAndAudienceForFirstPartyTokens()) and move the conditional logic there, update any callers (e.g., validate or validateAccessToken) to use that method, and add a unit test covering the League OAuth2 case (aud=client_id, no iss) to preserve behavior — apply the same refactor for the other analogous comment block in this class.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Shared/Infrastructure/Validator/AccessTokenValidator.php`:
- Around line 115-118: The validateSessionBinding function currently uses
isset($payload['roles']) which treats an explicit roles: null as absent; change
the check to explicitly reject a roles key with null by using
array_key_exists('roles', $payload) and throwing
CustomUserMessageAuthenticationException when array_key_exists(...) &&
$payload['roles'] === null; also update the code paths that use the
($payload['roles'] ?? null) expression (lines where roles fallback is applied)
to distinguish "key missing" vs "key present but null" by using
array_key_exists('roles', $payload) to decide fallback to ROLE_USER only when
the key is truly absent, and to reject when the key exists but is null.
---
Nitpick comments:
In `@src/Shared/Infrastructure/Validator/AccessTokenValidator.php`:
- Around line 103-107: Replace the inline explanatory comments in
AccessTokenValidator (the block describing unconditional issuer/audience
validation for every token) by extracting the check into a small, well-named
method and/or using self-explanatory variable names so the behavior is clear
without comments; e.g., create a method on AccessTokenValidator like
ensureFirstPartyTokenConstraintsOrValidateIssuerAndAudience() (or
validateIssuerAndAudienceForFirstPartyTokens()) and move the conditional logic
there, update any callers (e.g., validate or validateAccessToken) to use that
method, and add a unit test covering the League OAuth2 case (aud=client_id, no
iss) to preserve behavior — apply the same refactor for the other analogous
comment block in this class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cee6846-98ad-4d26-8cd6-3fbc48fe8cde
📒 Files selected for processing (6)
config/api_platform/resources/User.yamlspecs/security-312-accesstoken-role-service-escalation/prd.mdspecs/security-312-accesstoken-role-service-escalation/stories.mdsrc/Shared/Infrastructure/Validator/AccessTokenValidator.phptests/Integration/Auth/PasswordGrantIntegrationTest.phptests/Unit/Shared/Infrastructure/Validator/AccessTokenValidatorIssuerAudienceTest.php
Reject explicit `roles: null` as malformed claims (CodeRabbit) by using array_key_exists to distinguish a truly absent roles claim (defaults to least-privilege ROLE_USER) from an explicit null/invalid one (rejected). Remove inline explanatory comments per repository src/**/*.php convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security fix — Closes #312 (CRITICAL)
Vulnerability
AccessTokenValidatorsilently elevated any RS256 JWT lacking bothissandrolestoROLE_SERVICE, and skipped issuer/audience validation for that token class. The Lexik first-party JWT and the League OAuth2 server share one RSA keypair, so a League OAuth2 access token (aud=client_id, noiss/roles) passed signature verification and was promoted toROLE_SERVICE— the sole gate forPOST /api/users/batch(bulk user import). Any low-privilege OAuth2 client (client_credentials) orpassword-grant end user could therefore bulk-provision accounts. CWE-269 / CWE-287 / CWE-345.Fix
validateClaims()callsvalidateIssuerAndAudience()unconditionally. League OAuth2 tokens (aud=client_id, noiss) are now rejected.extractRoles()defaults a missingrolesclaim to least-privilegeROLE_USER;ROLE_SERVICEmust be positively asserted in a signed first-party token (CWE-1188).security: is_granted('ROLE_SERVICE')on thecreate_batch_httpoperation, so the restriction survives firewall misconfiguration.testValidateReturnsServiceRoleForOauthTokenWithoutIss→testValidateRejectsOauthTokenWithoutIss) to assert rejection; added regression tests for OAuth2-shaped tokens, the api-audience-without-issuer case, the absent-roles→ROLE_USER default, and the still-valid explicit service token.Compatibility
Legitimate first-party tokens (user and service) carry
iss=vilnacrm-user-service,aud=vilnacrm-api,sid, and explicitroles(seeTestAccessTokenFactory), so existing Behat/integration ROLE_SERVICE and ROLE_USER flows validate unchanged.Local verification
BMAD
Spec bundle:
specs/security-312-accesstoken-role-service-escalation/(prd.md, stories.md) with FR/NFR + positive/negative/edge test mapping.🤖 Generated with Claude Code
Summary by cubic
Fixes #312 by rejecting non‑first‑party JWTs and removing implicit
ROLE_SERVICE. Adds a service‑role gate to bulk import to prevent OAuth2 token escalation.issandaudfor every token; reject tokens withoutissor with non‑vilnacrm-apiaudience.ROLE_SERVICE; missingrolesdefaults toROLE_USER, explicitroles: nullis rejected, andsidis required whenrolesis present.security: "is_granted('ROLE_SERVICE')"toPOST /api/users/batchfor defense‑in‑depth.client_credentialstokens now get 401 at the batch endpoint; add regressions for absent‑roles→ROLE_USER, api‑audience without issuer, androles: null.Written for commit cec441f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests