Skip to content

refactor(token): simplify JWT validation and enforce token type checks#87

Merged
appleboy merged 3 commits intomainfrom
token
Mar 8, 2026
Merged

refactor(token): simplify JWT validation and enforce token type checks#87
appleboy merged 3 commits intomainfrom
token

Conversation

@appleboy
Copy link
Copy Markdown
Member

@appleboy appleboy commented Mar 8, 2026

Summary

  • Extract ParseJWT as an exported method for type-agnostic JWT signature verification and claims extraction, enabling callers (e.g. ID token tests) to parse tokens without type constraints
  • Enforce strict token type checking: ValidateToken requires type == "access", ValidateRefreshToken requires type == "refresh", each returning precise error messages
  • Replace the error-parameter anti-pattern (invalidErr/expiredErr passed into shared helpers) with base errors + mapRefreshError translator in both LocalTokenProvider and HTTPTokenProvider
  • Move ScopeSet from token/idtoken.go to util/string.go, remove duplicate parseScopeSet from handlers, and eliminate services → token import dependency in authorization.go
  • Add TokenCategoryAccess/TokenCategoryRefresh constants, remove dead ErrTokenValidation/ErrTokenReused sentinels
  • Fix HTTPTokenProvider.GenerateClientCredentialsToken to use ClientCredentialsTokenExpiration instead of standard JWT expiration

Test plan

  • All existing tests pass (internal/token, internal/handlers, internal/services)
  • New tests for ValidateToken rejecting refresh and ID tokens with specific error messages
  • New tests for ValidateRefreshToken rejecting access tokens and returning refresh-specific errors
  • New table-driven tests for mapRefreshError covering direct, wrapped, and unrelated errors
  • New TestScopeSet in util/string_test.go with edge cases (empty, single, multiple, extra whitespace)
  • Lint passes with zero issues

🤖 Generated with Claude Code

- Extract ParseJWT for type-agnostic signature verification and claims extraction
- Enforce strict type checking in ValidateToken and ValidateRefreshToken
- Replace error-parameter pattern with base errors and mapRefreshError translator
- Move ScopeSet to util package and remove duplicate parseScopeSet
- Add token category constants and remove unused error sentinels
- Fix HTTPTokenProvider.GenerateClientCredentialsToken expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 8, 2026 08:14
- Add check for absolute URLs that have a scheme but no host (e.g. "http:")
- These should be rejected as unsafe redirects

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors JWT validation logic in the token package to enforce strict token type checks, extract a ParseJWT method for type-agnostic parsing, and consolidate the ScopeSet utility to internal/util/string.go. It also fixes a bug where HTTPTokenProvider.GenerateClientCredentialsToken was incorrectly using the standard JWT expiration instead of the dedicated client-credentials expiration.

Changes:

  • Adds TokenCategoryAccess/TokenCategoryRefresh constants and removes dead error sentinels (ErrTokenValidation, ErrTokenReused); ValidateToken/ValidateRefreshToken now enforce strict type claim checks, and a ParseJWT method is exposed for type-agnostic parsing (e.g., ID tokens in tests).
  • Consolidates the ScopeSet helper from token/idtoken.go and handlers/oidc.go into util/string.go, eliminating the services → token import dependency in authorization.go.
  • Fixes HTTPTokenProvider.GenerateClientCredentialsToken to use ClientCredentialsTokenExpiration instead of delegating to GenerateToken (which used JWTExpiration).

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/util/string.go Adds ScopeSet as a shared utility
internal/util/string_test.go Table-driven tests for ScopeSet including edge cases
internal/token/types.go Adds TokenCategoryAccess and TokenCategoryRefresh constants
internal/token/errors.go Removes unused ErrTokenValidation and ErrTokenReused sentinels
internal/token/idtoken.go Removes ScopeSet (now in util) and its strings import
internal/token/local.go Extracts ParseJWT, keyFunc; adds mapRefreshError; ValidateToken/ValidateRefreshToken delegate to ParseJWT with strict type checks
internal/token/http_api.go Removes invalidErr/expiredErr params from callValidateAPI; fixes GenerateClientCredentialsToken to use correct expiration; ValidateRefreshToken uses mapRefreshError
internal/token/local_test.go New tests for type-checking in ValidateToken/ValidateRefreshToken, mapRefreshError coverage, and parseIDTokenClaims helper using ParseJWT
internal/services/token.go Switches from token.ScopeSet to util.ScopeSet
internal/services/token_test.go Updates ID token claim tests to use ParseJWT; updates error assertion
internal/services/authorization.go Uses util.ScopeSet instead of inline map-building loop
internal/handlers/oidc.go Uses util.ScopeSet; removes local parseScopeSet
internal/handlers/oidc_test.go Removes tests for parseScopeSet (now covered by util/string_test.go)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 8, 2026

Codecov Report

❌ Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/token/local.go 87.80% 3 Missing and 2 partials ⚠️
internal/token/http_api.go 73.33% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

- Verify HTTPTokenProvider.GenerateClientCredentialsToken sends
  expires_in from ClientCredentialsTokenExpiration, not JWTExpiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@appleboy appleboy merged commit c4f5003 into main Mar 8, 2026
17 checks passed
@appleboy appleboy deleted the token branch March 8, 2026 08:27
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