Skip to content

auth: add and enforce token_type, bind refresh to access, derive live perms#3082

Open
xilosada wants to merge 1 commit into
auth-secret-verifyfrom
auth-token-semantics
Open

auth: add and enforce token_type, bind refresh to access, derive live perms#3082
xilosada wants to merge 1 commit into
auth-secret-verifyfrom
auth-token-semantics

Conversation

@xilosada

Copy link
Copy Markdown
Member

Stacked on #3079 (base branch: auth-secret-verify). Review/merge after it.

What

  • Access and refresh tokens carry a distinct token_type; verification enforces it, so a refresh token is not accepted as a bearer credential.
  • The refresh endpoint requires the access and refresh tokens to share a subject.
  • Verified permissions are intersected with the key's current permission set instead of trusting the token's snapshot.

Existing tokens are invalidated on upgrade; clients re-authenticate.

Tests

cargo test -p mero-auth green (76 passed): token_type enforcement, subject-binding, live-permission re-derivation.

… perms

- access and refresh tokens carry a distinct token_type and verification
  enforces it (a refresh token is not accepted as a bearer credential)
- the refresh endpoint requires the access and refresh tokens to share a subject
- verified permissions are intersected with the key's current permission set
  rather than trusting the snapshot baked into the token

Existing tokens are invalidated on upgrade; clients re-authenticate.
@github-actions

Copy link
Copy Markdown

Your PR title does not adhere to the Conventional Commits convention:

<type>(<scope>): <subject>

Common errors to avoid:

  1. The title must be in lower case.
  2. Allowed type values are: build, ci, docs, feat, fix, perf, refactor, test.

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 50.7s


🟡 Warning (2)

1. Double decode of access token on every refresh request

File: crates/auth/src/api/handlers/auth.rs (line 293-320) | Consensus: 1/1 agents ✓

The refresh_token_handler calls verify_token (which calls decode_with_secrets with validate_exp=true) and then immediately calls verify_expired_access_claims (which calls decode_with_secrets with validate_exp=false) on the same access token string. This means every refresh request decodes and signature-verifies the access token JWT twice (and potentially iterates through backup secrets twice). Beyond the performance cost, the two calls are not atomic — a race condition exists where the token could expire between the first and second call (extremely unlikely in practice but theoretically possible). More concretely, the second call is redundant: the token type check in verify_expired_access_claims is already implied by the first verify_token call succeeding (which enforces TokenType::Access). The only new information from the second call is the sub claim, which could be extracted from the first call's error path or by decoding without verification.

Suggested fix:

Restructure the handler to decode the access token once with `validate_exp=false` and `TokenType::Access` enforcement, then separately check if it is expired. This avoids the double decode and the theoretical TOCTOU window.

Found by: security-reviewer

2. Expired access token accepted even when still valid path is taken

File: crates/auth/src/auth/token/jwt.rs (line 370-385) | Consensus: 1/1 agents ✓

In decode_with_secrets, when validate_exp is false, an ExpiredSignature error from jsonwebtoken is mapped to AuthError::TokenExpired and returned early. However, when validate_exp is false, jsonwebtoken should not raise ExpiredSignature at all — the library skips expiry validation entirely when validate_exp = false. This means the early-return branch for ExpiredSignature is dead code in the validate_exp = false path, but more importantly: if somehow an expired token is passed to verify_expired_access_claims, it will succeed (correct), but a non-expired token will also succeed (also correct). The real concern is the inverse: verify_expired_access_claims is called in refresh_token_handler after the handler has already confirmed the access token is expired via verify_token. But verify_expired_access_claims itself does not enforce that the token IS expired — it only skips expiry enforcement. A still-valid (non-expired) access token would also pass verify_expired_access_claims. The handler guards against this with the prior verify_token check that returns early if the token is still valid, so the overall flow is correct. However, verify_expired_access_claims is misleadingly named — it does not verify the token is expired, only that it is an access token with a valid signature. This could lead to misuse if called from other contexts in the future.

Suggested fix:

Rename `verify_expired_access_claims` to `decode_access_claims_ignore_expiry` or add a doc comment explicitly warning that this method does NOT assert the token is expired. Alternatively, add an assertion that the token's `exp` is in the past before returning claims.

Found by: security-reviewer

💡 Suggestion (2)

1. Empty secrets list produces a misleading error message

File: crates/auth/src/auth/token/jwt.rs (line 354-388) | Consensus: 1/1 agents ✓

In decode_with_secrets, if secrets is an empty slice, the loop body never executes, last_signature_err remains None, and the function returns Err(AuthError::InvalidToken("No verification secret available".to_string())). This is correct behavior, but the condition is silently swallowed — there is no warn! or error! log when no secrets are available, which would make diagnosing a misconfigured secret manager difficult in production.

Suggested fix:

Add a `tracing::error!("No JWT verification secrets available")` log before returning the error when `secrets` is empty.

Found by: security-reviewer

2. HashSet constructed from Vec on every token verification

File: crates/auth/src/auth/token/jwt.rs (line 482-500) | Consensus: 1/1 agents ✓

In verify_token_string, a HashSet<&String> is built from key.permissions on every call to intersect with the token's claimed permissions. For keys with many permissions and high-frequency token verification, this allocation occurs on every request. Since key.permissions is a Vec<String>, the HashSet construction is O(n) per verification.

Suggested fix:

If `key.permissions` is expected to remain small (< ~20 entries), a linear scan with `contains` on the Vec is simpler and avoids the allocation. If it can be large, consider storing permissions as a `HashSet` in the `Key` model directly, or caching the set. For the current scale this is a minor concern.

Found by: security-reviewer


🤖 Generated by MeroReviewer | Review ID: review-75e0976f

@meroreviewer

meroreviewer Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 architecture/crates/auth.html: Files matching crates/auth/** were changed but architecture/crates/auth.html was not updated (per source_to_docs_mapping).

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions Bot added the Stale label Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant