Skip to content

fix(sdk): verify JWT claims before returning them - #121

Merged
omercnet merged 2 commits into
mainfrom
descope-php-getclaims-signature-check
Jul 2, 2026
Merged

fix(sdk): verify JWT claims before returning them#121
omercnet merged 2 commits into
mainfrom
descope-php-getclaims-signature-check

Conversation

@omercnet

@omercnet omercnet commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

  • make getClaims return only claims from validateJWT, so JWKS kid lookup and RSA signature verification run first
  • reject expired signed tokens before returning claims
  • make the unverified token parser internal-only
  • add regression tests for forged RS256 payload claims and expired signed claims, and update README wording

Verification

  • docker run --rm -v "$PWD":/app -w /app composer:2 php -l src/SDK/Token/Extractor.php
  • docker run --rm -v "$PWD":/app -w /app composer:2 php -l src/tests/DescopeSDKTest.php
  • docker run --rm -v "$PWD":/app -w /app composer:2 composer test
  • manual QA: local JWKS mock + public DescopeSDK::getClaims accepted a signed token, rejected an expired signed token, and rejected a forged token with TokenException

@shuni-bot-dev

shuni-bot-dev Bot commented Jul 2, 2026

Copy link
Copy Markdown

🐕 Review complete — View session on Shuni Portal 🐾

@shuni-bot-dev

shuni-bot-dev Bot commented Jul 2, 2026

Copy link
Copy Markdown

🐕 Suggested Reviewers

The review assignment prioritizes recent contributors who have worked on the affected files, with a focus on those demonstrating breadth across the changed areas. Since only one contributor has recent activity on all relevant files, they are recommended for thorough review.

Reviewer Reason
gaokevin1 Recently contributed 24 commits covering all changed files, including tests and the main extraction logic, making them well-versed in these areas.
gaokevin1 Has recent activity on both the source code and tests, ensuring familiarity with the recent modifications and validation processes.
gaokevin1 Their recent contributions span across the key files involved in this PR, providing comprehensive coverage from implementation to testing.
gaokevin1 Consistent recent activity on the affected files indicates strong knowledge of the codebase areas impacted by this update.
gaokevin1 Extensive recent involvement with the files ensures they are well-positioned to review both logic changes and test updates efficiently.

Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best.

@shuni-bot-dev shuni-bot-dev Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐕 Shuni's Review

Makes getClaims run full JWKS signature verification (via validateJWT) instead of returning the unverified payload, and locks down the raw parser. Solid security fix — good bones!

Sniffed out 1 issue:

  • 1 🟢 LOW: getClaims verifies signature/issuer but not expiration

Retry logic, privatizing parseToken (no external callers), and the forged-token regression test all check out. Woof!

Comment thread src/SDK/Token/Extractor.php
@omercnet
omercnet requested a review from gaokevin1 July 2, 2026 12:19
@omercnet omercnet changed the title Verify JWT claims before returning them fix(sdk): verify JWT claims before returning them Jul 2, 2026
@omercnet
omercnet force-pushed the descope-php-getclaims-signature-check branch from 2d2cc6e to d75f37e Compare July 2, 2026 12:25
@omercnet
omercnet enabled auto-merge (squash) July 2, 2026 13:37
@omercnet
omercnet merged commit a7864e2 into main Jul 2, 2026
13 checks passed
@omercnet
omercnet deleted the descope-php-getclaims-signature-check branch July 2, 2026 13:44
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