Skip to content

Harden the resource-server handlers (security review follow-up) - #19

Merged
eliasjpr merged 1 commit into
masterfrom
claude/azu-crystal-framework-plan-phqt6e-bearer2
Oct 23, 2026
Merged

eliasjpr merged 1 commit into
masterfrom
claude/azu-crystal-framework-plan-phqt6e-bearer2

Conversation

@eliasjpr

Copy link
Copy Markdown
Collaborator

Follow-up to #18: the findings of the independent security review of the resource-server handlers.

Changes by finding

  1. context.authly_claims! did not exist (the README used it). Added: it raises Authly::MissingClaims with a clear message when Bearer did not authenticate the request. README example now matches.
  2. JWKS cache header. The body is built first; Cache-Control: public, max-age=300 is set only on success, no-store on the 500.
  3. Path matching was fail-open and compared the raw path. New Authly::Handler::PathFilter:
    • Protect everything by default; except: ["/health"] lists open paths (documented as the primary mode). prefixes: stays as an allowlist with a loud doc warning (every unlisted path is open); prefixes: [] is refused (it would protect nothing).
    • Paths are percent-decoded, \ is treated as /, // is collapsed and ./.. resolved before comparing: /%61pi/x, //api/x, /x/../api/x are all /api/x. Whole-segment matching, trailing slash ignored (/api/ covers /api).
    • Case policy: prefixes ignore case (/API/x is protected), except is exact (/HEALTH is not skipped by /health): both choices lean toward protecting.
    • RequireScope takes prefixes:/except: too.
  4. Store outage no longer looks like a bad token. Errors from the token store (Redis down) raise Authly::VerificationUnavailable; Bearer answers 503 + Retry-After: 1 + no-store, no WWW-Authenticate, so clients do not re-authenticate. Still fails closed. Only the store calls are classified this way: a malformed or forged token (including odd base64) stays a 401. OpaqueToken#valid? no longer swallows every exception.
  5. JWTToken#valid? logged every bad token with Log.error; now Log.debug with the exception class only, no token content.
  6. CORS. Documented ordering (CORS handler before Bearer). Optional pass_preflight: true lets OPTIONS with Access-Control-Request-Method through; off by default.
  7. Confused deputy.
    • Access tokens (JWT and opaque) now carry aud = config.access_token_audience (default base_url) and JWT access tokens carry token_type: "access_token" (refresh tokens "refresh_token").
    • StrategyTokenVerifier requires token_type == "access_token"; tokens issued before this change (no token_type, but with scope) are accepted only with legacy_tokens: true, for the length of access_ttl after upgrade. A token with any other token_type is refused even then.
    • audience: is enforced when set; the rdoc example and README are fixed and say prominently to set it. Without audience: any aud passes. Limit: one audience per authorization server (no RFC 8707 resource indicators).
  8. Config threading. StrategyTokenVerifier.new(config) now uses that configuration for the key, algorithm, issuer, token strategy and store (it calls JWT.decode and the store directly instead of the global-bound helpers). The token is decoded once per request.

Nits

  • Metadata issuer (base_url) vs token iss (config.issuer): documented in the README (not changed).
  • JWKS: the algorithm must fit the key (RS* needs RSA; ES256/384/512 need EC on P-256/384/521), otherwise refused.
  • Repeated access_token query values are refused (400); query-token responses get Cache-Control: private and Referrer-Policy: no-referrer; README warns about URL logging.
  • Found while writing the HEAD spec: ResponseHelper.write printed a body for HEAD requests, which desynchronizes a kept-alive connection (the client read the body as the next response). It now omits the body for HEAD; this affects every Authly handler using the helper.

Specs added (spec/bearer_hardening_spec.cr, spec/jwks_spec.cr)

Unit and over-the-wire path cases (//api, /API, /api/, /%61pi/x, /x/../api/x, look-alikes); keep-alive reuse after a skipped path; HEAD; OPTIONS and preflight; store failure for JWT and opaque (503, Retry-After, recovery, unknown token still 401, malformed token not a 503); runtime store swap; audience and token_type (including refresh tokens and legacy flag); config threading with a separate Configuration; Bearer<TAB>token; any: [] with all; query-token headers and duplicates; JWKS cache and error headers; JWKS key/alg mismatch; authly_claims!.

Verification (Crystal 1.22.0, the floor and latest)

  • crystal tool format --check: clean
  • Ameba (crystal run lib/ameba/bin/ameba.cr, as CI): 89 files, 0 failures
  • spec/grants/*_spec.cr: 31 examples, 0 failures
  • spec/*_spec.cr: 314 examples, 0 failures
  • Mutation checks (each makes the suite fail): no percent-decoding (5 failures), case-sensitive prefixes (3), no .. resolution (5), store error read as "revoked" (3), no token_type requirement (2), no audience check (2), verifier using the global config (1), HEAD body (1 error), authly_claims! not raising (1), JWKS key/alg check removed (1), header plus query token accepted (1).

Behaviour changes to be aware of

  • Bearer/RequireScope without prefixes: already protected everything; what changes is that prefixes: no longer has a segment-only raw comparison and except: is the documented mode.
  • Access tokens now include aud and token_type; verifiers need legacy_tokens: true while pre-upgrade tokens are still alive.
  • A store outage is now 503 at Bearer (it used to be 401).

Limits

InMemoryStore is not thread-safe; revocation is only seen where the store is shared; userinfo/introspect still turn all errors into "invalid"/inactive (not touched here).


Generated by Claude Code

Protect everything by default with except:, normalize paths, 503 on store outages, aud and token_type on access tokens, audience enforcement, config threading, JWKS key/alg check, HEAD without body, CORS preflight option, claims bang.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f
@eliasjpr
eliasjpr merged commit 32249e2 into master Oct 23, 2026
3 checks passed
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