From 38ae744434dd273d5e7b7cc3490f44be7f970cb4 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 21:36:54 +1000 Subject: [PATCH 1/6] test(saml): pin what the assertion Conditions actually enforce (ID-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SAML assertion is a *bearer* credential — anyone holding the bytes can present them. Everything that stops a captured assertion being reused lives in `Conditions` and the surrounding envelope, and every one of those checks in `crystal-saml` is guarded by a "skip if not configured" clause (`validate_audience` returns true when `sp_entity_id` is nil, `validate_destination` when the ACS URL is nil, `validate_issuer` when `idp_entity_id` is nil). Which of them are live therefore depends entirely on what `external_providers.cr#build_saml` passes, and nothing asserted that. This pins it — both where it holds and where it does not. Matters directly for UCLA, which is a SAML/Shibboleth deployment. Enforced, now covered: * NotOnOrAfter in the past -> refused * NotBefore in the future -> refused * Audience naming another SP -> refused (sp_entity_id comes from strat.issuer; blank would disable it) * Destination naming another -> refused deployment's ACS * valid window -> admitted (the control — without it a build that refused everything would pass all five rejection cases) Gaps, pinned so they stay deliberate rather than accidental: * **The declared Issuer is never validated.** `build_saml` passes sp_entity_id, idp_cert and idp_cert_fingerprint but no `idp_entity_id`, so `validate_issuer` short-circuits and `` is decorative. Not currently exploitable — the signature must still verify against the strat's pinned cert — but it becomes load-bearing the moment a strat trusts more than one key or a cert is reused across IdPs. Fixing it needs an `idp_entity_id` column and a value for every existing strat: a migration, not a code change. * **No one-time use.** There is no assertion-ID cache, and the SAML callback deliberately skips the session-state check, so nothing binds a response to a request we issued. The same bytes replay until `NotOnOrAfter` — which is the only bound. Ruby behaved the same way and Shibboleth issues short windows, so this is the accepted SAML bearer model rather than a regression. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/saml_callbacks_spec.cr | 205 ++++++++++++++++++++++++ 1 file changed, 205 insertions(+) diff --git a/spec/controllers/saml_callbacks_spec.cr b/spec/controllers/saml_callbacks_spec.cr index fe0859b..06a2029 100644 --- a/spec/controllers/saml_callbacks_spec.cr +++ b/spec/controllers/saml_callbacks_spec.cr @@ -326,5 +326,210 @@ module PlaceOS::Auth strat.try &.destroy end end + + # ---- ID-04: what the Conditions actually buy us -------------------- + # + # A SAML assertion is a *bearer* credential: anyone holding the bytes can + # present them. Everything that stops a captured assertion being reused + # lives in `Conditions` and the surrounding envelope, and every one of + # those checks in `crystal-saml` is guarded by a "skip if not configured" + # clause. So which of them are live depends entirely on what + # `external_providers.cr#build_saml` passes — this block pins that, both + # where it holds and where it does not. + # + # Matters directly for UCLA, which is a SAML/Shibboleth deployment. + describe "assertion conditions (ID-04)", tags: "saml-conditions" do + acs = "http://localhost/auth/adfs/callback" + + strat_with_cert = -> { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + strat = ::PlaceOS::Model::SamlAuthentication.new + strat.name = "cond-saml-#{Random.rand(99999)}" + strat.issuer = "https://sp.example.test/cond-#{Random.rand(99999)}" + strat.idp_sso_target_url = "https://idp.example.test/sso" + strat.assertion_consumer_service_url = acs + strat.uid_attribute = "email" + strat.idp_cert = Spec::SamlFixtures.idp_cert_pem + strat.authority_id = authority.id + strat.save! + strat + } + + post_assertion = ->(strat : ::PlaceOS::Model::SamlAuthentication, saml_response : String) { + client.post( + "/auth/adfs/callback?id=#{URI.encode_www_form(strat.id.as(String))}", + headers: HTTP::Headers{ + "Host" => "localhost", + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: "SAMLResponse=#{URI.encode_www_form(saml_response)}&RelayState=#{URI.encode_www_form("/backoffice/")}", + ) + } + + lookup_for = ->(email : String) { + ::PlaceOS::Model::UserAuthLookup.where(uid: email, provider: "adfs").first? + } + + refused = ->(result : HTTP::Client::Response, email : String) { + lookup = lookup_for.call(email) + location = result.headers["Location"]? + rejected = result.status_code >= 400 || location.try(&.includes?("/auth/failure")) || false + if lookup || !rejected + fail "ASSERTION WAS NOT REJECTED — status=#{result.status_code} " \ + "location=#{location.inspect} lookup_created=#{!lookup.nil?} " \ + "body=#{result.body[0, 200].inspect}" + end + } + + admitted = ->(result : HTTP::Client::Response, email : String) { + lookup = lookup_for.call(email) + if lookup.nil? + fail "ASSERTION WAS NOT ACCEPTED — status=#{result.status_code} " \ + "location=#{result.headers["Location"]?.inspect} body=#{result.body[0, 200].inspect}" + end + lookup.not_nil! + } + + cleanup = ->(email : String) { + if lookup = lookup_for.call(email) + if user_id = lookup.user_id + ::PlaceOS::Model::User.find?(user_id).try &.destroy + end + lookup.destroy + end + } + + # The control for the whole block. Without it, a build that refused + # every assertion would pass all the rejection cases below. + it "admits an assertion inside its validity window" do + strat = strat_with_cert.call + email = "saml-cond-ok-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 5.minutes.ago, not_on_or_after: 30.minutes.from_now)) + + admitted.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion whose NotOnOrAfter has passed" do + # This is the ONLY thing bounding replay of a captured assertion — + # see the one-time-use case at the end of this block. + strat = strat_with_cert.call + email = "saml-cond-exp-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 2.hours.ago, not_on_or_after: 1.hour.ago)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion that is not valid yet (NotBefore in the future)" do + strat = strat_with_cert.call + email = "saml-cond-early-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + not_before: 1.hour.from_now, not_on_or_after: 2.hours.from_now)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion minted for a different Service Provider" do + # `validate_audience` compares against `settings.sp_entity_id`, which + # `build_saml` sets from `strat.issuer`. If that were ever left blank + # the check short-circuits to `true` and any assertion the IdP issued + # for ANY service protected by the same IdP would log the bearer in + # here. This is the assertion that catches that. + strat = strat_with_cert.call + email = "saml-cond-aud-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: "https://someone-else.example.test/metadata", email: email)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "refuses an assertion addressed to a different Destination" do + # `validate_destination` compares the Response's `Destination` against + # the configured ACS URL, so an assertion captured from another + # deployment's endpoint cannot be posted here. + strat = strat_with_cert.call + email = "saml-cond-dest-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: "https://elsewhere.example.test/auth/adfs/callback", + audience: strat.issuer.as(String), email: email)) + + refused.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + # ---- gaps, pinned so they stay deliberate ---------------------- + + it "does NOT validate the declared Issuer — trust rests on the cert alone" do + # `crystal-saml`'s `validate_issuer` short-circuits to true when + # `settings.idp_entity_id` is nil, and `build_saml` never passes it + # (`external_providers.cr#build_saml` sets sp_entity_id, idp_cert, + # idp_cert_fingerprint … but no idp_entity_id). So `` is + # decorative here. + # + # That is not currently exploitable: the signature must still verify + # against the strat's pinned `idp_cert`, so an attacker cannot mint + # one. It becomes load-bearing the moment a strat trusts more than one + # key, or a cert is reused across IdPs. Pinned rather than fixed + # because adding the check needs an `idp_entity_id` column and a + # value for every existing strat — a migration, not a code change. + strat = strat_with_cert.call + email = "saml-cond-iss-#{Random.rand(99999)}@localhost" + xml = Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email, + idp_entity_id: "https://not-the-configured-idp.example.test/metadata")) + + admitted.call(post_assertion.call(strat, Spec::SamlFixtures.encode(xml)), email) + ensure + cleanup.call(email) if email + strat.try &.destroy + end + + it "does NOT enforce one-time use — the same assertion replays until it expires" do + # There is no assertion-ID cache, and the SAML callback deliberately + # skips the session-state check (see "callback state check" above), so + # nothing binds a response to a request we issued. An assertion + # captured in transit, from a proxy log or from browser history can be + # POSTed again by anyone until `NotOnOrAfter`. + # + # The legacy Ruby service behaved the same way, and Shibboleth issues + # short windows, so this is the accepted SAML bearer-token model + # rather than a regression. It is pinned here so the exposure is a + # recorded decision — and so that if someone adds a replay cache, this + # spec is what tells them the behaviour changed. + strat = strat_with_cert.call + email = "saml-cond-replay-#{Random.rand(99999)}@localhost" + encoded = Spec::SamlFixtures.encode( + Spec::SamlFixtures.signed(Spec::SamlFixtures.response_xml( + acs_url: acs, audience: strat.issuer.as(String), email: email))) + + first = admitted.call(post_assertion.call(strat, encoded), email) + second_result = post_assertion.call(strat, encoded) + + # Same bytes, accepted again, resolving to the same identity. + second = admitted.call(second_result, email) + second.id.should eq first.id + ensure + cleanup.call(email) if email + strat.try &.destroy + end + end end end From d04cc577d31714db1b10427a3a0e7e4f1e72df0a Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 21:46:41 +1000 Subject: [PATCH 2/6] =?UTF-8?q?test(auth):=20pin=20the=20refresh=20scope?= =?UTF-8?q?=20semantics=20=E2=80=94=20neither=20widened=20nor=20narrowed?= =?UTF-8?q?=20(RF-10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §6 says a refresh request's scope "MUST NOT include any scope not originally granted", and Doorkeeper enforced exactly that (`RefreshTokenRequest#validate_scope` checks the request against `refresh_token.scopes`). Nothing here asserted what auth.cr does, and the two obvious guesses are both wrong. What it actually does: **the `scope` parameter is ignored on refresh.** `oauth.cr`'s refresh branch calls `Authly.access_token(grant_type:, client_id:, client_secret:, refresh_token:)` and never forwards `scope`, so `Grant#@scope` is nil and `Grant#scope` falls through to the scope recovered from the refresh token. I went looking for a privilege escalation and did not find one. The reason is worth writing down, because the near-miss is real: `validate_scope!` checks the requested scope with `Authly.clients.allowed_scopes?`, which asks "may this client ever hold this scope" — the CLIENT REGISTRATION — not "was this scope granted to this token". If the parameter were forwarded, a client registered for more than it was granted could widen its own token on refresh. It is only safe because the parameter never arrives. Pinned, not fixed. The current behaviour errs safe: a client can never gain a scope it was not granted. Making narrowing work means forwarding the parameter, which without a granted-scope subset check turns a safe divergence into a real escalation. That is a change to make deliberately. Five cases, on a client REGISTERED for `public users` whose user grants only `public` — the gap is where widening would become visible: * asks `public users`, granted `public` -> 200, token carries `public` * asks `users`, granted `public` -> 200, token carries `public` * asks `public`, granted `public users` -> 200, token carries BOTH (the divergence) * asks exactly what was granted -> 200, unchanged * asks for nothing -> 200, granted scope preserved (the behaviour the scope-recovery patch exists to protect — the 2026-07-25 revert — asserted here so a future narrowing check cannot regress it into an empty scope) Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/refresh_semantics_spec.cr | 163 +++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/spec/controllers/refresh_semantics_spec.cr b/spec/controllers/refresh_semantics_spec.cr index 2c1e75a..a24d50d 100644 --- a/spec/controllers/refresh_semantics_spec.cr +++ b/spec/controllers/refresh_semantics_spec.cr @@ -359,5 +359,168 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- RF-10: a refresh may narrow the grant, never widen it --------- + # + # RFC 6749 §6: "The requested scope MUST NOT include any scope not + # originally granted by the resource owner." Doorkeeper enforced exactly + # that — `RefreshTokenRequest#validate_scope` checks the requested scope + # against `refresh_token.scopes`, the scope the grant actually carried. + # + # authly checks the requested scope against the CLIENT REGISTRATION only + # (`Authly.clients.allowed_scopes?`), which is a different question: it + # asks "may this client ever hold this scope", not "was this scope + # granted to this token". A client registered for more than it was + # granted could therefore widen its own token on refresh. + describe "scope narrowing and widening (RF-10)" do + # A client REGISTERED for two scopes, whose user grants only one. + # That gap is the whole point: it is where widening becomes visible. + two_scope_grant = ->(granted : String) { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + + redirect = "https://rf10.example/cb-#{Random.rand(999_999)}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "rf10-#{Random.rand(999_999)}" + app.redirect_uri = redirect + app.scopes = "public users" + app.confidential = true + app.owner_id = user.id.as(String) + app.save! + + cookie = Spec.signin!(client, user, password) + authorize_path = String.build do |io| + io << "/auth/authorize?response_type=code" + io << "&client_id=" << URI.encode_www_form(app.uid.as(String)) + io << "&redirect_uri=" << URI.encode_www_form(redirect) + io << "&scope=" << URI.encode_www_form(granted) + end + authorized = client.get(authorize_path, headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => cookie, + }) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + exchanged = client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + }) + exchanged.status_code.should eq 200 + body = JSON.parse(exchanged.body) + # The grant really is narrower than the registration. + scopes_of.call(decode.call(body["access_token"].as_s)).should eq granted.split + {user, app, body["refresh_token"].as_s} + } + + refresh_asking = ->(app : ::PlaceOS::Model::DoorkeeperApplication, token : String, scope : String) { + client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "refresh_token") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("refresh_token", token) + fp.add("scope", scope) + }) + } + + it "does NOT widen the grant when the refresh asks for more" do + # The security-relevant half. Granted `public`, registered for + # `public users`, asking for `users` back. + user, app, refresh_token = two_scope_grant.call("public") + + result = refresh_asking.call(app, refresh_token, "public users") + + # Accepted — but the token that comes back carries the GRANTED + # scope, not the requested one. No escalation. + result.status_code.should eq 200 + claims = decode.call(JSON.parse(result.body)["access_token"].as_s) + scopes_of.call(claims).should eq ["public"] + scopes_of.call(claims).should_not contain "users" + claims["sub"].as_s.should eq user.id.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + + it "does NOT widen even when the extra scope is the only one asked for" do + user, app, refresh_token = two_scope_grant.call("public") + + result = refresh_asking.call(app, refresh_token, "users") + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "does NOT narrow either — the scope parameter is ignored on refresh (DIVERGENCE)" do + # RFC 6749 §6 lets a client refresh into a NARROWER scope, and + # Doorkeeper honoured it (`RefreshTokenRequest#validate_scope` + # checks the request against `refresh_token.scopes`). auth.cr honours + # neither direction, for one reason: `oauth.cr`'s refresh branch + # calls `Authly.access_token(grant_type:, client_id:, client_secret:, + # refresh_token:)` and never forwards `scope`, so `Grant#@scope` is + # nil and `Grant#scope` falls through to the scope recovered from the + # refresh token. + # + # Pinned rather than fixed, deliberately. The current behaviour errs + # SAFE — a client can never gain a scope it was not granted. Making + # narrowing work means forwarding the parameter, and at that point + # `validate_scope!` only checks the CLIENT REGISTRATION + # (`Authly.clients.allowed_scopes?`), which asks "may this client ever + # hold this scope", not "was it granted to this token". Forwarding it + # without also adding a granted-scope subset check would convert this + # safe divergence into a real privilege escalation. That is a change + # worth making deliberately, not as a side effect. + user, app, refresh_token = two_scope_grant.call("public users") + + result = refresh_asking.call(app, refresh_token, "public") + + result.status_code.should eq 200 + # Asked for `public`, handed back `public users`. + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "returns exactly the granted scope when the refresh asks for it" do + user, app, refresh_token = two_scope_grant.call("public users") + + result = refresh_asking.call(app, refresh_token, "public users") + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + + it "still carries the granted scope when the refresh asks for nothing" do + # The existing behaviour the scope-recovery patch exists to protect + # (the 2026-07-25 revert). Asserted here too so a narrowing check + # cannot regress it into an empty scope. + user, app, refresh_token = two_scope_grant.call("public users") + + result = client.post("/auth/token", headers: form_headers, body: URI::Params.build { |fp| + fp.add("grant_type", "refresh_token") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("refresh_token", refresh_token) + }) + + result.status_code.should eq 200 + scopes_of.call(decode.call(JSON.parse(result.body)["access_token"].as_s)).should eq ["public", "users"] + ensure + app.try &.destroy + user.try &.destroy + end + end end end From f937c7601ae7adfaaa19d931734e860dc74347bb Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 21:56:02 +1000 Subject: [PATCH 3/6] test(auth): pin the PKCE method handling and the missing nonce (PK-05, PK-06, OI-09, OI-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three capability questions where what the service advertises, what the RFC requires and what the code does are all slightly different. None is exploitable; all three are the kind of thing an integrator discovers the hard way, so they are now written down as assertions. **PK-05 / OI-09 — `plain` is accepted though only S256 is advertised.** Discovery says `code_challenge_methods_supported: ["S256"]`; `normalize_code_challenge` transforms S256 alone and passes every other method through, so authly compares a `plain` challenge verbatim and the exchange succeeds. RFC 7636 §7.2 tells a server that supports S256 to refuse `plain`, and the reason bites here: with `plain` the challenge IS the verifier, and the challenge travels in the authorize URL — nginx's log, the browser history, any `Referer`. Not exploitable across clients (the method is baked into the code at authorize time, so a legitimate client's code cannot be downgraded), but a capability document is a promise, and this one is not kept. Also asserts `plain` still checks the verifier, so the case above cannot be read as "PKCE is ignored". **PK-06 — an unknown method mints an unredeemable code.** `S512` is accepted at authorize (302, code issued) and then refused at the exchange. It fails CLOSED, which is what matters. What it costs is diagnosability: the error is 401 `unauthorized_client`, which tells an integrator their client may not use this grant type when the real problem is one query parameter they chose. RFC 7636 §4.4.1 would reject it at the authorize endpoint with `invalid_request`. Same misdirection class as XO-03 and the undecodable-grant 500. **OI-04 — `nonce` is silently dropped.** `/auth/authorize` has no `nonce` parameter, `Authly::Code` has no field for it, and `Owner#id_token` never emits it. OIDC Core §3.1.3.7 step 11 says an RP that sent a nonce MUST verify it comes back, so any RP library that sends one — most do, even on the code flow — will reject our ID token. Safe direction (login refused, never wrongly accepted) and no PlaceOS client sends one, which is why it has never surfaced. The spec asserts the missing claim alongside a well-formed `sub`/`aud`, so it reads as "this claim is absent", not "the flow is broken". Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/oidc_id_token_spec.cr | 76 ++++++++++++++ spec/controllers/pkce_spec.cr | 132 +++++++++++++++++++++++++ 2 files changed, 208 insertions(+) diff --git a/spec/controllers/oidc_id_token_spec.cr b/spec/controllers/oidc_id_token_spec.cr index 6d606b4..c2061ae 100644 --- a/spec/controllers/oidc_id_token_spec.cr +++ b/spec/controllers/oidc_id_token_spec.cr @@ -460,5 +460,81 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- OI-04: nonce --------------------------------------------------- + + describe "nonce is not supported (OI-04)" do + it "drops a nonce sent to the authorize endpoint instead of echoing it" do + # OIDC Core §3.1.2.1 makes `nonce` OPTIONAL for the code flow, and + # §3.1.3.7 step 11 says that if the client sent one, it MUST verify + # the same value comes back in the ID token. auth.cr never captures + # it: `/auth/authorize` has no `nonce` parameter, `Authly::Code` + # has no field for it, and `AuthlyAdapter::Owner#id_token` never + # emits one (there is a comment saying as much). + # + # The consequence is a real interop limit, not a cosmetic gap: an RP + # library that sends `nonce` by default — most do, even on the code + # flow — will reject our ID token as tampered. It is a *safe* + # failure (login refused, never wrongly accepted), and no PlaceOS + # client sends one today, which is why it has never been noticed. + # Pinned so that anyone integrating an external RP finds this here + # rather than in a support ticket, and so an implementation lands + # with a test already waiting for it. + user, app, redirect, cookie, password = nil, nil, nil, nil, nil + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + user = ::PlaceOS::Model::Generator.user(authority) + password = "bcrypt-please-#{Random.rand(99999)}" + user.password = password + user.save! + + redirect = "https://oidc.example/cb/#{UUID.random}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "oidc-nonce-#{Random.rand(99999)}" + app.redirect_uri = redirect + app.scopes = "openid public" + app.owner_id = user.id.as(String) + app.confidential = true + app.save! + + cookie = Spec.signin!(client, user, password) + the_nonce = "nonce-#{Random.rand(999_999_999)}" + authorized = client.get( + "/auth/oauth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(redirect)}" \ + "&scope=#{URI.encode_www_form("openid public")}" \ + "&nonce=#{URI.encode_www_form(the_nonce)}", + headers: HTTP::Headers{"Host" => "localhost", "Cookie" => cookie}, + ) + + # An unknown parameter must not break the flow — the login still + # works, which is exactly why the missing claim is easy to miss. + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = form_post.call("/auth/token", { + "grant_type" => "authorization_code", + "client_id" => app.uid.as(String), + "client_secret" => app.secret, + "code" => code, + "redirect_uri" => redirect, + }) + token.status_code.should eq 200 + + id_token = JSON.parse(token.body)["id_token"].as_s + id_token.should_not be_empty + payload, _ = JWT.decode(id_token, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + + # The claim an OIDC-conformant RP would check, and its absence. + payload.as_h.has_key?("nonce").should be_false + # Positive control: the ID token IS otherwise well-formed, so this is + # a missing claim and not a broken flow. + payload["sub"].as_s.should eq user.id.as(String) + payload["aud"].as_s.should eq app.uid.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/pkce_spec.cr b/spec/controllers/pkce_spec.cr index c5ef4b2..1e110e4 100644 --- a/spec/controllers/pkce_spec.cr +++ b/spec/controllers/pkce_spec.cr @@ -251,5 +251,137 @@ module PlaceOS::Auth app.try &.destroy user.try &.destroy end + + # ---- PK-05 / PK-06 / OI-09: which methods are really accepted ------ + # + # Discovery advertises `code_challenge_methods_supported: ["S256"]` + # (`oauth.cr`, Discovery::Response). A capability document is a promise + # about what the server does, so what it *actually* accepts had better + # match — and the challenge method is not a detail: with `plain`, the + # challenge and the verifier are the same string, and the challenge + # travels in the authorize URL. Anything that reads that URL — nginx's + # access log, the browser's history, a `Referer` — holds the verifier. + # That is precisely why RFC 7636 §7.2 tells a server supporting S256 to + # refuse `plain`. + describe "code_challenge_method handling (PK-05, PK-06, OI-09)" do + authorize_with = ->(app : ::PlaceOS::Model::DoorkeeperApplication, redirect : String, cookie : String, challenge : String, method : String) { + query = String.build do |io| + io << "/auth/authorize?response_type=code" + io << "&client_id=" << URI.encode_www_form(app.uid.as(String)) + io << "&redirect_uri=" << URI.encode_www_form(redirect) + io << "&scope=public" + io << "&code_challenge=" << URI.encode_www_form(challenge) + io << "&code_challenge_method=" << URI.encode_www_form(method) + end + client.get(query, headers: HTTP::Headers{"Host" => "localhost", "Cookie" => cookie}) + } + + exchange = ->(app : ::PlaceOS::Model::DoorkeeperApplication, redirect : String, code : String, verifier : String) { + client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + fp.add("code_verifier", verifier) + }) + } + + it "advertises S256 as the only supported method" do + result = client.get("/.well-known/openid-configuration", + headers: HTTP::Headers{"Host" => "localhost"}) + result.status_code.should eq 200 + JSON.parse(result.body)["code_challenge_methods_supported"] + .as_a.map(&.as_s).should eq ["S256"] + end + + it "accepts a `plain` challenge anyway (PK-05 — DIVERGENCE from the advertised set)" do + # The advertised set says S256 only; the implementation takes `plain` + # too. `normalize_code_challenge` deliberately transforms S256 alone + # and passes every other method through untouched, and authly then + # compares the verifier verbatim. + # + # Not currently exploitable across clients: the method is baked into + # the code at authorize time, so an attacker cannot downgrade a + # *legitimate* client's code — the client picks its own method. The + # exposure is that a naive or hostile client can opt itself into a + # scheme where the authorize URL carries its own proof of possession, + # while discovery tells integrators that cannot happen. + redirect = "https://spa.example/cb-plain-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + verifier = "plain-verifier-#{Random.rand(999_999)}" + + authorized = authorize_with.call(app, redirect, cookie, verifier, "plain") + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, verifier) + token.status_code.should eq 200 + JSON.parse(token.body)["access_token"].as_s.should_not be_empty + ensure + app.try &.destroy + user.try &.destroy + end + + it "still checks a `plain` challenge against the verifier" do + # The control for the case above: `plain` is accepted, but it is not + # a no-op — a wrong verifier is still refused. Without this, the + # test above would equally describe a build that ignored PKCE. + redirect = "https://spa.example/cb-plainbad-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + + authorized = authorize_with.call(app, redirect, cookie, "the-real-verifier", "plain") + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, "not-the-verifier") + token.status_code.should_not eq 200 + JSON.parse(token.body)["error"].as_s.should_not be_empty + ensure + app.try &.destroy + user.try &.destroy + end + + it "mints an unredeemable code for an unknown method (PK-06)" do + # RFC 7636 §4.3 makes `code_challenge_method` a value the server must + # understand, and §4.4.1 says an unsupported one should be rejected + # at the AUTHORIZE endpoint with `invalid_request`. auth.cr passes it + # through instead: `normalize_code_challenge` transforms S256 alone, + # every other method is stored verbatim, and the code is minted. + # + # It fails CLOSED, which is the part that matters — the exchange is + # refused, so an unknown method can never weaken the check. What it + # costs is diagnosability: the client gets a 302 and a code that + # looks fine, then a 401 `unauthorized_client` one round trip later. + # That code says "this client may not use this grant type", which + # sends an integrator to their client registration when the actual + # problem is one query parameter they chose. Same misdirection class + # as the boot-time `oauth_tokens` check (XO-03) and the undecodable + # -grant 500 — pinned here so the cost is visible if anyone decides + # to move the rejection to the authorize endpoint where it belongs. + redirect = "https://spa.example/cb-s512-#{Random.rand(99999)}" + user, app, password = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + verifier = "s512-verifier-#{Random.rand(999_999)}" + + authorized = authorize_with.call(app, redirect, cookie, verifier, "S512") + # Accepted here — this is the part RFC 7636 says should have failed. + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = exchange.call(app, redirect, code, verifier) + token.status_code.should eq 401 + JSON.parse(token.body)["error"].as_s.should eq "unauthorized_client" + ensure + app.try &.destroy + user.try &.destroy + end + end end end From a38547813575cc1194990c05c6f13bbeaef39bd4 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 22:05:11 +1000 Subject: [PATCH 4/6] test(auth): pin the session and introspection tenant boundaries (MT-03, IR-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One auth.cr process serves every authority (eight on dev), so every tenant boundary is enforced in code rather than by deployment. `bearer_credentials_spec.cr` covers the token half (AZ-06/MT-02). This covers the two boundaries either side of it. **MT-03 — the session cookie is NOT bound to an authority.** `new_session` stores `uid`, `exp` and `iat` and nothing else; `session_user` resolves it with a bare `User.find?(uid)`; and the cookie is encrypted with one process-wide `COOKIE_SESSION_SECRET`. So a cookie minted at authority A decrypts and validates at authority B, and `/auth/authority` reports `session: true`. Pinned rather than fixed, for two reasons. A browser never does this on its own — cookies are host-scoped, so B never receives A's cookie; it needs an attacker who already holds the cookie value, at which point they hold the session anyway. And it yields no usable credential: the second spec drives the whole authorize+exchange against authority B with a `localhost` user's session and asserts the token comes back with `aud: localhost`, then that `/auth/userinfo` at B refuses it 401. So the tenant boundary lives on the TOKEN, not on the session. That is a design fact worth recording — anyone adding a session-only surface to auth.cr (an admin page, a form post) inherits it and needs its own check. The `aud` assertion is the one that fails first if that stops being true. **IR-04 — cross-tenant introspection answers `{"active": false}`.** RFC 7662 §2.2 wants exactly that: not an error, and not the token's metadata. `introspection_revocation_spec` covers the cross-application case inside one tenant; this is the same guard across authorities, where a leak would cross an organisational boundary. Asserts the absence of `scope`, `client_id` and `exp` from the response and that neither the foreign user id nor the foreign client id appears anywhere in the body — plus a control proving a client can still introspect its own token, without which an always-false build would pass. Adds `signin_at`, since `Spec.signin!` hardcodes `Host: localhost` and therefore cannot sign a user in at any other authority. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/multi_tenant_spec.cr | 264 ++++++++++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 spec/controllers/multi_tenant_spec.cr diff --git a/spec/controllers/multi_tenant_spec.cr b/spec/controllers/multi_tenant_spec.cr new file mode 100644 index 0000000..2402a39 --- /dev/null +++ b/spec/controllers/multi_tenant_spec.cr @@ -0,0 +1,264 @@ +require "../helper" +require "jwt" + +module PlaceOS::Auth + # Multi-tenant boundaries — PPT-2536 test-matrix rows MT-03 and IR-04. + # + # One auth.cr process serves every authority (eight on dev), so every + # tenant boundary here is enforced in code rather than by deployment. + # `bearer_credentials_spec.cr` covers the token half (AZ-06/MT-02): a token + # minted for one authority is refused at another. This covers the two + # boundaries either side of it — the *session* that precedes a token, and + # introspection, which reads other people's tokens for a living. + describe OAuth, tags: "multi-tenant" do + decode = ->(token : String) { + payload, _ = JWT.decode(token, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + payload + } + + # A second authority, with a user and a password, alongside `localhost`. + other_tenant = -> { + authority = ::PlaceOS::Model::Generator.authority + authority.domain = "tenant-#{Random.rand(999_999)}.example" + authority.save! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + {authority, user, password} + } + + local_user = -> { + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + password = "bcrypt-please-#{Random.rand(999_999)}" + user = ::PlaceOS::Model::Generator.user(authority) + user.password = password + user.save! + {authority, user, password} + } + + # `Spec.signin!` hardcodes `Host: localhost`, so it cannot sign a user + # in at any other authority — which is exactly what these rows need. + signin_at = ->(host : String, user : ::PlaceOS::Model::User, password : String) { + result = client.post("/auth/signin", + headers: HTTP::Headers{"Host" => host, "Content-Type" => "application/json"}, + body: {email: user.email.to_s, password: password}.to_json) + raise "signin at #{host} failed: #{result.status_code} #{result.body}" unless {202, 303}.includes?(result.status_code) + session = result.cookies[PlaceOS::Auth::SESSION_COOKIE_NAME]? + raise "signin at #{host} set no session cookie" if session.nil? + "#{session.name}=#{session.value}" + } + + make_app = ->(owner : ::PlaceOS::Model::User, slug : String) { + redirect = "https://mt.example/cb-#{slug}-#{Random.rand(999_999)}" + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "mt-#{slug}-#{Random.rand(999_999)}" + app.redirect_uri = redirect + app.scopes = "public" + app.owner_id = owner.id.as(String) + app.confidential = true + app.save! + {app, redirect} + } + + # ---- MT-03: the session is not bound to an authority ---------------- + + describe "session / authority binding (MT-03)" do + it "accepts a session cookie issued by one authority at another (PINNED)" do + # `new_session` stores `uid`, `exp` and `iat` — no authority — and + # `session_user` resolves it with a bare `User.find?(uid)`. The + # session cookie is encrypted with ONE process-wide + # `COOKIE_SESSION_SECRET`, so a cookie minted while signed in at + # authority A decrypts and validates at authority B. + # + # Why this is a boundary worth writing down rather than a hole to + # panic about: a browser will never do this by itself — cookies are + # host-scoped, so B never receives A's cookie. It needs an attacker + # who already holds the cookie value, and at that point they hold the + # session regardless. And it does not yield a usable credential: the + # token minted from it carries the USER's authority in `aud` (see the + # next case), and `ensure_matching_domain` then refuses it at B. + # + # So the tenant boundary is enforced on the TOKEN, not on the + # session. Pinned because that is a real design fact — anyone adding + # a session-only surface to auth.cr (an admin page, a form post) + # inherits this and would need their own check. + _, user, password = local_user.call + cookie = Spec.signin!(client, user, password) + foreign_authority, _, _ = other_tenant.call + + result = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => foreign_authority.domain, "Cookie" => cookie, + }) + + result.status_code.should eq 200 + JSON.parse(result.body)["session"].as_bool.should be_true + ensure + user.try &.destroy + foreign_authority.try &.destroy + end + + it "stamps the token with the USER's authority, not the request Host" do + # The control that makes the case above safe, and the assertion that + # would fail first if it stopped being true. A code minted while + # talking to authority B, with a session belonging to a `localhost` + # user, still produces a token whose `aud` is `localhost` — so + # `ensure_matching_domain` refuses it at B. + _, user, password = local_user.call + foreign_authority, _, _ = other_tenant.call + app, redirect = make_app.call(user, "aud") + cookie = Spec.signin!(client, user, password) + + authorized = client.get( + "/auth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(redirect)}&scope=public", + headers: HTTP::Headers{"Host" => foreign_authority.domain, "Cookie" => cookie}, + ) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + token = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => foreign_authority.domain, + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("code", code) + fp.add("redirect_uri", redirect) + }) + token.status_code.should eq 200 + + access = JSON.parse(token.body)["access_token"].as_s + claims = decode.call(access) + # Minted while addressing the foreign authority, but bound to the + # user's own. + claims["aud"].as_s.should eq "localhost" + claims["aud"].as_s.should_not eq foreign_authority.domain + claims["sub"].as_s.should eq user.id.as(String) + + # And therefore unusable at the authority it was requested through. + refused = client.get("/auth/userinfo", headers: HTTP::Headers{ + "Host" => foreign_authority.domain, "Authorization" => "Bearer #{access}", + }) + refused.status_code.should eq 401 + ensure + app.try &.destroy + user.try &.destroy + foreign_authority.try &.destroy + end + end + + # ---- IR-04: introspection across tenants ---------------------------- + + describe "cross-tenant introspection (IR-04)" do + it "reports another tenant's token as inactive rather than describing it" do + # RFC 7662 §2.2: an introspection response for a token the caller is + # not entitled to see must be `{"active": false}` — not an error, and + # certainly not the token's metadata. `introspection_revocation_spec` + # covers the cross-*application* case within one tenant; this is the + # same guard across authorities, where a leak would cross an + # organisational boundary rather than an app one. + _, local, local_password = local_user.call + foreign_authority, foreign_user, foreign_password = other_tenant.call + + victim_app, victim_redirect = make_app.call(foreign_user, "victim") + caller_app, _ = make_app.call(local, "caller") + + # Mint a real token for the foreign tenant's app. + foreign_cookie = signin_at.call(foreign_authority.domain, foreign_user, foreign_password) + authorized = client.get( + "/auth/authorize?response_type=code" \ + "&client_id=#{URI.encode_www_form(victim_app.uid.as(String))}" \ + "&redirect_uri=#{URI.encode_www_form(victim_redirect)}&scope=public", + headers: HTTP::Headers{"Host" => foreign_authority.domain, "Cookie" => foreign_cookie}, + ) + authorized.status_code.should eq 302 + code = URI::Params.parse(authorized.headers["Location"].split('?', 2).last)["code"] + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => foreign_authority.domain, + "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "authorization_code") + fp.add("client_id", victim_app.uid.as(String)) + fp.add("client_secret", victim_app.secret) + fp.add("code", code) + fp.add("redirect_uri", victim_redirect) + }) + issued.status_code.should eq 200 + victim_token = JSON.parse(issued.body)["access_token"].as_s + + # A different tenant's client asks about it, authenticating properly + # as itself. + result = client.post("/auth/introspect", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("token", victim_token) + fp.add("client_id", caller_app.uid.as(String)) + fp.add("client_secret", caller_app.secret) + }) + + result.status_code.should eq 200 + body = JSON.parse(result.body) + body["active"].as_bool.should be_false + # Nothing about the token may leak alongside the `false` — not the + # subject, not the scope, not the owning client. + body.as_h.has_key?("scope").should be_false + body.as_h.has_key?("client_id").should be_false + body.as_h.has_key?("exp").should be_false + result.body.should_not contain foreign_user.id.as(String) + result.body.should_not contain victim_app.uid.as(String) + ensure + victim_app.try &.destroy + caller_app.try &.destroy + local.try &.destroy + foreign_user.try &.destroy + foreign_authority.try &.destroy + end + + it "still describes the token to its own client (the control)" do + # Without this, a build whose introspection always answered + # `{"active": false}` would pass the case above. + _, local, _ = local_user.call + app, redirect = make_app.call(local, "self") + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + issued.status_code.should eq 200 + own_token = JSON.parse(issued.body)["access_token"].as_s + + result = client.post("/auth/introspect", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("token", own_token) + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + }) + + result.status_code.should eq 200 + JSON.parse(result.body)["active"].as_bool.should be_true + ensure + app.try &.destroy + local.try &.destroy + end + end + end +end From 89523266b5e97cf250a8dc75ab7c6ec23f2d43c0 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 22:14:43 +1000 Subject: [PATCH 5/6] test(auth): pin the deny-path gate, the userinfo branches and the missing kid (AU-07, OI-05, OI-06, OI-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **AU-07** — `DELETE /auth/authorize` (the deny path) is session-gated like the grant path, and bounces to `/auth/login` rather than emitting `access_denied`. That distinction is the point: an unauthenticated deny that redirected to the client would let anyone who can reach the endpoint fabricate a user's refusal, and the client would record it as a decision the user made. Asserts the redirect goes to login and that nothing reached the client. **OI-06** — `/auth/userinfo` answers GET and POST identically, and its `sub` matches the `sub` of the ID token from the same grant (OIDC Core §5.3.2). `discovery_spec` proved both verbs are ROUTED; this proves they agree. **OI-05** — a token that outlives its user returns `404 {"error":""}`, not the `unknown subject` 401 the code appears to intend. `authorize!` calls `User.find`, which RAISES `PgORM::Error::RecordNotFound` rather than returning nil; that escapes the `rescue e : JWT::Error` around it and lands on the base controller's 404 handler, so `userinfo`'s own guard is never reached by this route (it still covers the guest-scope path, which skips the user lookup). Diagnosability, not security: RFC 6750 §3.1 and OIDC Core §5.3.3 want 401 `invalid_token`, and an RP reading 404 concludes the ENDPOINT is missing — it may disable userinfo rather than refresh the token. The body says `{"error":""}` because RecordNotFound carries no message. Pinned rather than fixed because the fix is a semantic decision (is a userless token authenticated at all?) that should be made with the guest-scope path in view. Reachable in production by any deletion or tenant teardown inside the 2-hour access-token window. **OI-11** — our tokens carry NO `kid` in the JOSE header. `Authly.jwt_encode` is `JWT.encode(payload, key, alg)`, which emits `alg` and `typ` only, so a token names no key while the JWKS publishes one with an RFC 7638 `kid`. It works today only because there is exactly one key and libraries fall back to it. That makes it a latent blocker on key rotation: the moment the JWKS holds two keys a kid-less token is ambiguous and strict RPs reject it, so this has to be fixed BEFORE a rotation, not during one. Same class as the missing `nonce` — fine for PlaceOS's own clients, a surprise for a conformant RP. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/authorize_flow_spec.cr | 25 ++++++++ spec/controllers/jwks_spec.cr | 59 ++++++++++++++++++ spec/controllers/oidc_id_token_spec.cr | 79 +++++++++++++++++++++++++ 3 files changed, 163 insertions(+) diff --git a/spec/controllers/authorize_flow_spec.cr b/spec/controllers/authorize_flow_spec.cr index 23edd73..aca09dc 100644 --- a/spec/controllers/authorize_flow_spec.cr +++ b/spec/controllers/authorize_flow_spec.cr @@ -84,6 +84,31 @@ module PlaceOS::Auth user.try &.destroy end + it "bounces an unauthenticated caller to login rather than denying (AU-07)" do + # The deny path is gated on the session exactly like the grant path. + # It matters that it BOUNCES rather than emitting `access_denied`: an + # unauthenticated deny that redirected to the client would let anyone + # who can reach the endpoint fabricate a user's refusal, and the + # client would take it as a decision the user made. + user, password, app = make_app.call + query = "redirect_uri=#{URI.encode_www_form(app.redirect_uri.as(String))}" \ + "&client_id=#{URI.encode_www_form(app.uid.as(String))}&response_type=code&state=abc" + + result = client.delete("/auth/oauth/authorize?#{query}", + headers: HTTP::Headers{"Host" => "localhost"}) + + result.status_code.should eq 303 + location = result.headers["Location"] + location.should eq "/auth/login" + # Nothing reached the client — no decision was invented on the + # user's behalf. + location.should_not contain "access_denied" + location.should_not start_with app.redirect_uri.as(String) + ensure + app.try &.destroy + user.try &.destroy + end + it "refuses to redirect to a URI not registered for the client (no open redirect)" do user, password, app = make_app.call cookie = Spec.signin!(client, user, password) diff --git a/spec/controllers/jwks_spec.cr b/spec/controllers/jwks_spec.cr index 5f14517..c994e97 100644 --- a/spec/controllers/jwks_spec.cr +++ b/spec/controllers/jwks_spec.cr @@ -1,4 +1,5 @@ require "../helper" +require "base64" module PlaceOS::Auth # JWKS parity for the Doorkeeper-openid_connect mount (PPT-2536). @@ -32,5 +33,63 @@ module PlaceOS::Auth n.size.should eq 342 n.should_not start_with "A" # a leading zero byte would encode as "A..." end + + # ---- OI-11: does a token point at the key that signed it? ---------- + + it "issues tokens whose header carries no kid (OI-11 — DIVERGENCE)" do + # An RP validating our tokens fetches the JWKS and has to pick a key. + # The standard way is `kid`: OIDC Core §10.1 says the header SHOULD + # carry one when the JWKS publishes more than one key, and most + # libraries look for it unconditionally. + # + # `Authly.jwt_encode` is `JWT.encode(payload, key, alg)`, which emits + # `{"alg":"RS256","typ":"JWT"}` and nothing else, so our tokens name no + # key at all. It works today only because the JWKS publishes exactly + # ONE key and every sane library falls back to "try the only one". + # + # Two things make this worth pinning rather than shrugging at. It is a + # latent blocker on key rotation: the moment the JWKS holds two keys, a + # kid-less token is ambiguous and strict RPs reject it — so rotation + # needs this fixed FIRST, not during. And it is the same class as the + # missing `nonce` (OI-04): fine for PlaceOS's own clients, a surprise + # for anyone integrating a conformant RP. + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "jwks-kid-#{Random.rand(999_999)}" + app.redirect_uri = "https://jwks.example/cb-#{Random.rand(999_999)}" + app.scopes = "public" + app.confidential = true + app.owner_id = "authority-owner" + app.save! + + issued = client.post("/auth/token", + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + issued.status_code.should eq 200 + access = JSON.parse(issued.body)["access_token"].as_s + + # Decode the JOSE header without verifying — that is exactly what an + # RP does before it knows which key to verify with. + header = JSON.parse(String.new(Base64.decode(access.split('.').first + "=="))) + header["alg"].as_s.should eq "RS256" + header["typ"].as_s.should eq "JWT" + header.as_h.has_key?("kid").should be_false + + # The key it WOULD have named, and the reason a fallback works today: + # there is exactly one. + keys = JSON.parse( + client.get("/auth/oauth/discovery/keys", headers: HTTP::Headers{"Host" => "localhost"}).body + )["keys"].as_a + keys.size.should eq 1 + keys.first["kid"].as_s.should_not be_empty + ensure + app.try &.destroy + end end end diff --git a/spec/controllers/oidc_id_token_spec.cr b/spec/controllers/oidc_id_token_spec.cr index c2061ae..2ebd7a3 100644 --- a/spec/controllers/oidc_id_token_spec.cr +++ b/spec/controllers/oidc_id_token_spec.cr @@ -536,5 +536,84 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- OI-05 / OI-06: the userinfo endpoint --------------------------- + + describe "userinfo (OI-05, OI-06)" do + it "answers GET and POST identically, with a sub matching the id_token" do + # OIDC Core §5.3 requires both verbs, and §5.3.2 requires the + # `sub` returned here to match the `sub` of the ID token issued in + # the same grant. An RP that keys its user records off userinfo + # while validating the ID token separately breaks silently if the + # two ever disagree — `discovery_spec.cr` proves both verbs are + # ROUTED; this proves they agree, and with what. + user, app, _redirect, token = authorize_and_exchange.call("openid public") + access = token["access_token"].as_s + id_payload, _ = JWT.decode(token["id_token"].as_s, + ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + + headers = HTTP::Headers{"Host" => "localhost", "Authorization" => "Bearer #{access}"} + via_get = client.get("/auth/userinfo", headers: headers) + via_post = client.post("/auth/userinfo", headers: headers) + + via_get.status_code.should eq 200 + via_post.status_code.should eq 200 + JSON.parse(via_get.body).should eq JSON.parse(via_post.body) + + subject = JSON.parse(via_get.body)["sub"].as_s + subject.should eq user.id.as(String) + subject.should eq id_payload["sub"].as_s + ensure + app.try &.destroy + user.try &.destroy + end + + it "404s with an empty error once the user behind the token is gone (OI-05 — DIVERGENCE)" do + # A token can outlive its user: a deletion, or a tenant teardown, + # inside the 2-hour access-token window. What comes back is + # `404 {"error":""}`. + # + # Why 404 and not the `unknown subject` 401 the code appears to + # intend: `authorize!` calls `::PlaceOS::Model::User.find(...)`, + # which RAISES `PgORM::Error::RecordNotFound` rather than returning + # nil. That escapes the `rescue e : JWT::Error` around it and lands + # on the base controller's RecordNotFound handler — so + # `OAuth#userinfo`'s own `raise Error::Unauthorized.new("unknown + # subject")` guard is never reached by this route. (It still covers + # the guest-scope path, which skips the user lookup entirely.) + # + # Two problems, both diagnosability rather than security. RFC 6750 + # §3.1 and OIDC Core §5.3.3 want 401 `invalid_token` here — an RP + # reading 404 concludes the *endpoint* is missing and may disable + # userinfo entirely, rather than refreshing the token. And the body + # carries `{"error":""}`, because `RecordNotFound` is raised with no + # message, so nothing in the response says what was not found. + # + # Pinned rather than fixed: changing it means either making + # `authorize!` tolerate the missing row (which is a real semantic + # decision about whether a userless token is authenticated at all) + # or catching RecordNotFound per-controller. Worth doing + # deliberately, with the guest-scope path considered alongside. + user, app, _redirect, token = authorize_and_exchange.call("openid public") + access = token["access_token"].as_s + headers = HTTP::Headers{"Host" => "localhost", "Authorization" => "Bearer #{access}"} + + # Control: it works while the user exists, so the change below is + # attributable to the deletion and nothing else. + client.get("/auth/userinfo", headers: headers).status_code.should eq 200 + + user.destroy + user = nil + + result = client.get("/auth/userinfo", headers: headers) + result.status_code.should eq 404 + JSON.parse(result.body)["error"].as_s.should be_empty + # Whatever else changes, no claims may leak for a user that is gone. + result.body.should_not contain "\"sub\"" + ensure + app.try &.destroy + user.try &.destroy + end + end end end From 214d44bda65ce5055da38a8fd4b63b6b1c1613d7 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 22:23:53 +1000 Subject: [PATCH 6/6] test(auth): pin the code lifetime, the session-timeout override and mount parity (AU-12, SC-09, TK-09) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **AU-12** — authorization codes carry the 10-minute Doorkeeper lifetime. Asserted by decoding a real code and checking `exp - iat == 600`, not by reading the constant back to itself. A cutover must not silently shorten or lengthen the window a half-finished login has to complete in. The rejection side already lives in `token_disclosure_spec.cr`. **SC-09** — `Authority#internals["session_timeout"]` overrides `SESSION_TIMEOUT_MINUTES`. A tenant setting a shorter window is making a security decision, so the override being *read* is the row. The session's `exp` is sealed inside the encrypted cookie and cannot be inspected from outside, so this drives it with a negative window: the same signin shows `session: true` under the default and `session: false` with the override in place. The control and the assertion are in one example, so an ignored override cannot pass. **TK-09** — `/auth/token` and `/auth/oauth/token` behave identically. They are stacked annotations on one method today and so cannot diverge, but that is an implementation detail and the drop-in promise is behavioural: this fails if anyone splits them to deprecate one or to hang a filter on one. Compares the success envelope shape, `Cache-Control`, `Pragma`, token_type and expires_in, asserts both tokens actually verify, and compares the REJECTION — status, error code and `WWW-Authenticate` — so parity is not proven on the happy path alone. Co-Authored-By: Claude Opus 5 (1M context) --- spec/controllers/authorize_validation_spec.cr | 38 ++++++++++ spec/controllers/oauth_alias_spec.cr | 71 +++++++++++++++++++ spec/controllers/sessions_spec.cr | 48 +++++++++++++ 3 files changed, 157 insertions(+) diff --git a/spec/controllers/authorize_validation_spec.cr b/spec/controllers/authorize_validation_spec.cr index 10ebe3d..a3a4567 100644 --- a/spec/controllers/authorize_validation_spec.cr +++ b/spec/controllers/authorize_validation_spec.cr @@ -808,5 +808,43 @@ module PlaceOS::Auth user.try &.destroy end end + + # ---- AU-12: the authorization code's lifetime ---------------------- + + describe "code expiry (AU-12)" do + it "mints codes with the 10-minute Doorkeeper lifetime" do + # Doorkeeper's `authorization_code_expires_in` default is 10 minutes + # and the legacy service never overrode it, so a cutover must not + # silently shorten or lengthen the window a half-finished login has + # to complete in. `authly_adapter.cr` sets `config.code_ttl = + # 10.minutes`; this asserts the value REACHES the code rather than + # asserting the constant back to itself. + # + # The rejection side — an expired code refused with 400 + # `invalid_grant` rather than a 500 — lives in + # `token_disclosure_spec.cr`. + ::Authly.config.code_ttl.should eq 10.minutes + + redirect = "https://au12.example/cb-#{Random.rand(999_999)}" + user, password = make_user.call + app = make_app.call(redirect) + cookie = Spec.signin!(client, user, password) + + result = authorize.call(cookie, { + "response_type" => "code", + "client_id" => app.uid.as(String), + "redirect_uri" => redirect, + "scope" => "public", + }) + result.status_code.should eq 302 + code = URI::Params.parse(result.headers["Location"].split('?', 2).last)["code"] + + payload, _ = JWT.decode(code, ::Authly.config.public_key.as(String), JWT::Algorithm::RS256) + (payload["exp"].as_i64 - payload["iat"].as_i64).should eq 600 + ensure + app.try &.destroy + user.try &.destroy + end + end end end diff --git a/spec/controllers/oauth_alias_spec.cr b/spec/controllers/oauth_alias_spec.cr index 9cfc83b..a7d245b 100644 --- a/spec/controllers/oauth_alias_spec.cr +++ b/spec/controllers/oauth_alias_spec.cr @@ -84,5 +84,76 @@ module PlaceOS::Auth result.status_code.should eq 303 result.headers["Location"].should eq "/auth/login" end + + # ---- TK-09: the two token mounts must not drift apart -------------- + + it "answers the short and legacy token paths identically (TK-09)" do + # `/auth/token` is what ts-client calls; `/auth/oauth/token` is the + # Doorkeeper mount external integrators hardcoded. They are stacked + # route annotations on ONE method today, so they cannot diverge — but + # that is an implementation detail, and the drop-in promise is that + # both behave the same. This is what fails if anyone ever splits them, + # to deprecate one or to hang a filter on just one. + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + user = ::PlaceOS::Model::Generator.user(authority) + user.save! + app = ::PlaceOS::Model::DoorkeeperApplication.new + app.name = "alias-parity-#{Random.rand(999_999)}" + app.redirect_uri = "https://alias.example/cb-#{Random.rand(999_999)}" + app.scopes = "public" + app.confidential = true + app.owner_id = user.id.as(String) + app.save! + + request = ->(path : String) { + client.post(path, + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: URI::Params.build { |fp| + fp.add("grant_type", "client_credentials") + fp.add("client_id", app.uid.as(String)) + fp.add("client_secret", app.secret) + fp.add("scope", "public") + }) + } + + short = request.call("/auth/token") + legacy = request.call("/auth/oauth/token") + + short.status_code.should eq 200 + legacy.status_code.should eq 200 + + # Same envelope and the same cache headers. The tokens themselves + # differ — each call mints a new one — so compare shape, not bytes. + JSON.parse(short.body).as_h.keys.sort!.should eq JSON.parse(legacy.body).as_h.keys.sort! + short.headers["Cache-Control"].should eq legacy.headers["Cache-Control"] + short.headers["Pragma"].should eq legacy.headers["Pragma"] + JSON.parse(short.body)["token_type"].as_s.should eq JSON.parse(legacy.body)["token_type"].as_s + JSON.parse(short.body)["expires_in"].as_i.should eq JSON.parse(legacy.body)["expires_in"].as_i + + # Both tokens actually verify — a shape match on two broken tokens + # would otherwise pass. + ::Authly.valid?(JSON.parse(short.body)["access_token"].as_s).should be_true + ::Authly.valid?(JSON.parse(legacy.body)["access_token"].as_s).should be_true + + # Identical REJECTION too, not just identical success. + bad = ->(path : String) { + client.post(path, + headers: HTTP::Headers{ + "Host" => "localhost", "Content-Type" => "application/x-www-form-urlencoded", + }, + body: "grant_type=client_credentials&client_id=ghost-#{Random.rand(999_999)}&client_secret=x") + } + bad_short = bad.call("/auth/token") + bad_legacy = bad.call("/auth/oauth/token") + bad_short.status_code.should eq bad_legacy.status_code + bad_short.status_code.should eq 401 + JSON.parse(bad_short.body)["error"].as_s.should eq JSON.parse(bad_legacy.body)["error"].as_s + bad_short.headers["WWW-Authenticate"].should eq bad_legacy.headers["WWW-Authenticate"] + ensure + app.try &.destroy + user.try &.destroy + end end end diff --git a/spec/controllers/sessions_spec.cr b/spec/controllers/sessions_spec.cr index bee7562..f806ea5 100644 --- a/spec/controllers/sessions_spec.cr +++ b/spec/controllers/sessions_spec.cr @@ -275,6 +275,54 @@ module PlaceOS::Auth end end + # ---- SC-09: per-authority session timeout --------------------------- + + describe "per-authority session timeout (SC-09)" do + it "honours internals.session_timeout over the env default" do + # `authority_session_timeout` reads `Authority#internals` + # ["session_timeout"] and falls back to `SESSION_TIMEOUT_MINUTES`. + # A tenant that sets a shorter window is making a security decision, + # so the override actually being read is the whole point of the row. + # + # Driving it with a NEGATIVE window is what makes this observable in + # a spec: the session's `exp` is inside the encrypted cookie and + # cannot be read from outside, but a session that was already expired + # when it was issued shows up immediately as `session: false`, while + # the same signin under the default shows `session: true`. + authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil! + original = authority.internals.dup + password = "ok-password-1234" + user = create_user.call(password) + + # Control first, under the default timeout. + cookie = Spec.signin!(client, user, password) + live = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => cookie, + }) + JSON.parse(live.body)["session"].as_bool.should be_true + + authority.internals = authority.internals.merge({ + "session_timeout" => JSON::Any.new(-1_i64), + }) + authority.save! + + expired_cookie = Spec.signin!(client, user, password) + result = client.get("/auth/authority", headers: HTTP::Headers{ + "Host" => "localhost", "Cookie" => expired_cookie, + }) + + result.status_code.should eq 200 + # Issued and already past its window — the override was read. + JSON.parse(result.body)["session"].as_bool.should be_false + ensure + if authority + authority.internals = original || {} of String => JSON::Any + authority.save! + end + user.try &.destroy + end + end + describe "GET /auth/login" do it "redirects to the authority's login_url with {{url}} substituted" do authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil!