PPT-2536: authorize-endpoint validation specs (AU-02..AU-06, AU-13) - #18
Merged
Conversation
PPT-2536 test-matrix rows for the authorization endpoint. 12 new examples
in spec/controllers/authorize_validation_spec.cr:
AU-02 implicit/hybrid/`none`/`CODE` response types rejected with
`unsupported_response_type` and no Location, on a *registered*
client (oauth_spec.cr only covered the unregistered `client_id=x`
case, where the 400 is not attributable to the response type)
AU-03 unregistered / protocol-relative / javascript: / data: / userinfo
redirect URIs produce a non-redirectable 401, never a Location;
empty redirect_uri is the distinct 400 invalid_redirect_uri branch
AU-04 exact-match redirect validation: scheme, host case, subdomain,
port, trailing dot, extra query/path/fragment, percent-encoded and
whitespace near-misses all rejected
AU-05 the three redirect shapes real dev clients register — custom
scheme, http://localhost:*, LAN IP — accepted from a multi-URI
registration, plus the full native code exchange with no secret
AU-06 `state` echoed byte-identical on both the grant and the deny
redirect using a reserved-character value, and omitted entirely
when the client sends none
Every rejection table carries a positive control in the same example, so a
fixture that stopped minting codes at all cannot make the table pass
vacuously; every redirect asserts its TARGET, not just the status code.
Two known divergences are recorded as `pending` rather than silently
skipped, because both need a decision, not just a test:
AU-11 authorization codes are stateless JWTs with no consumption store,
so they are replayable for their full 10-minute TTL. Ruby
Doorkeeper revoked the grant on first use (RFC 6749 4.1.2).
Fixing it interacts with the RF-05 double-submit finding.
AU-13 the code carries no `client_id` claim; cross-client redemption is
blocked only via `redirect_uri`. Two clients with overlapping
registered URIs defeat it (RFC 6749 4.1.3). The passing AU-13
cases pin both directions of the redirect-mediated binding.
Also fixes a vacuous assertion in hostile_input_spec.cr: the open-redirect
case ran unauthenticated, so `Location?.try(&.includes?("evil.example"))
.should_not be_true` was asserting against the /auth/login bounce and would
have passed even if the signed-in path leaked a code. Now asserts the
positive (303 to exactly /auth/login).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cross-client case asserted 401 unauthorized_client; the endpoint returns 400 invalid_redirect_uri. authly's validate! runs valid_redirect? BEFORE client_authorized?, so client B presenting client A's redirect_uri trips the redirect check and never reaches the client check. The security property is unchanged and still asserted — B is refused and no access_token is issued, with client A redeeming the same code afterwards as a positive control so the refusal cannot pass for the wrong reason. Only which guard fires differs, and pinning it means a reordering upstream surfaces as a deliberate decision rather than a silent change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
P1 coverage for the OAuth authorize endpoint. Test-only. 12 examples.
none/case-variant response types → 400unsupported_response_typewith no Location, on a registered client, plus acodepositive control//evil,javascript:,data:, userinfo-trick redirects → 401 with no Location; emptyredirect_uri→ the distinct 400 branchcom.placeos.booking.panel://),localhost:8080and LAN-IP redirects mint codes and echo that exact URI, plus a full native exchange with no secretstateechoed byte-identical (a b&c=d?e#f/g+h%21) on grant and deny redirects; absent when not sentExisting coverage was found and not duplicated —
oauth_spec.cr:294already coveredresponse_type=token, but with an unregisteredclient_id, so the 400 wasn't attributable to the response type. These use a registered client with a control.Correction made during review
AU-13 originally asserted
401 unauthorized_client; the endpoint returns 400invalid_redirect_uri. authly'svalidate!runsvalid_redirect?beforeclient_authorized?, so client B presenting A's URI trips the redirect check and never reaches the client check. The security property is unchanged and still asserted — B refused, noaccess_token, with A redeeming the same code afterwards as a positive control.Also fixed here
hostile_input_spec.cr:94was vacuous: it assertedLocationdidn't containevil.exampleon an unauthenticated authorize request, which 303s to/auth/login— so the expression wasfalse.should_not be_trueand never exercised redirect validation at all. Now asserts the positive (303+ exactLocation).Defect pinned as
pendingAU-11 — authorization codes are replayable. No code store exists; the code is a self-contained JWT and nothing marks it spent, so one code mints unlimited token pairs for its full 10-minute TTL. RFC 6749 §4.1.2 and a Doorkeeper parity break. Not fixed here: it interacts with RF-05 (the SPA boot race double-submits), so single-use has to be spend-once-return-same-token or a grace window, not revoke-the-family.
186 examples, 0 failures, 3 pending.🤖 Generated with Claude Code