Repository navigation
Harden the resource-server handlers (security review follow-up) - #19
Merged
eliasjpr merged 1 commit intoOct 23, 2026
Merged
Conversation
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
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
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.
Follow-up to #18: the findings of the independent security review of the resource-server handlers.
Changes by finding
context.authly_claims!did not exist (the README used it). Added: it raisesAuthly::MissingClaimswith a clear message whenBearerdid not authenticate the request. README example now matches.Cache-Control: public, max-age=300is set only on success,no-storeon the 500.Authly::Handler::PathFilter: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).\is treated as/,//is collapsed and./..resolved before comparing:/%61pi/x,//api/x,/x/../api/xare all/api/x. Whole-segment matching, trailing slash ignored (/api/covers/api).prefixesignore case (/API/xis protected),exceptis exact (/HEALTHis not skipped by/health): both choices lean toward protecting.RequireScopetakesprefixes:/except:too.Authly::VerificationUnavailable;Beareranswers 503 +Retry-After: 1+no-store, noWWW-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.JWTToken#valid?logged every bad token withLog.error; nowLog.debugwith the exception class only, no token content.Bearer). Optionalpass_preflight: trueletsOPTIONSwithAccess-Control-Request-Methodthrough; off by default.aud=config.access_token_audience(defaultbase_url) and JWT access tokens carrytoken_type: "access_token"(refresh tokens"refresh_token").StrategyTokenVerifierrequirestoken_type == "access_token"; tokens issued before this change (notoken_type, but withscope) are accepted only withlegacy_tokens: true, for the length ofaccess_ttlafter upgrade. A token with any othertoken_typeis refused even then.audience:is enforced when set; the rdoc example and README are fixed and say prominently to set it. Withoutaudience:anyaudpasses. Limit: one audience per authorization server (no RFC 8707 resource indicators).StrategyTokenVerifier.new(config)now uses that configuration for the key, algorithm, issuer, token strategy and store (it callsJWT.decodeand the store directly instead of the global-bound helpers). The token is decoded once per request.Nits
issuer(base_url) vs tokeniss(config.issuer): documented in the README (not changed).access_tokenquery values are refused (400); query-token responses getCache-Control: privateandReferrer-Policy: no-referrer; README warns about URL logging.ResponseHelper.writeprinted 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 andtoken_type(including refresh tokens and legacy flag); config threading with a separateConfiguration;Bearer<TAB>token;any: []withall; 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: cleancrystal run lib/ameba/bin/ameba.cr, as CI): 89 files, 0 failuresspec/grants/*_spec.cr: 31 examples, 0 failuresspec/*_spec.cr: 314 examples, 0 failures..resolution (5), store error read as "revoked" (3), notoken_typerequirement (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/RequireScopewithoutprefixes:already protected everything; what changes is thatprefixes:no longer has a segment-only raw comparison andexcept:is the documented mode.audandtoken_type; verifiers needlegacy_tokens: truewhile pre-upgrade tokens are still alive.Bearer(it used to be 401).Limits
InMemoryStoreis not thread-safe; revocation is only seen where the store is shared;userinfo/introspectstill turn all errors into "invalid"/inactive (not touched here).Generated by Claude Code