feat(0186): sign in with Discord — identity only, scope identify, session cookie - #220
Merged
Merged
Conversation
Four routes on the existing axum router under task 0183's gated prefix — /auth/login, /auth/callback, /auth/me, /auth/logout — so the whole slice is an empty 404 until PORTAL_ENABLED is flipped. No new crate, no second gateway integration (ADR 0008, 2026-08-07 meeting). Authorization Code with PKCE (S256) and scope exactly `identify`, requested in the authorize URL and verified again on the token response so a Developer Portal registration that drifts wider fails closed rather than quietly collecting more (ADR 0010). `state` is signed and bound to the browser by a second signed cookie carrying the same nonce plus the PKCE verifier. That pair is what refuses a callback with no cookie, a mismatched nonce (login-CSRF), a forged signature, a crossed action or an expired window. Replay is closed by clearing the pending cookie on every path out of the handler, before the outcome is known, so a second presentation of the same code+state has nothing left to match against. The `state` carries an action slot even though only sign-in exists — task 0189 needs it and adding it later would mean re-deriving the signing format while a deployed portal holds live cookies in the old one. The session is a signed HttpOnly + Secure + SameSite=Lax cookie scoped to /api-tokens/, carrying the Discord user ID, a display username and an expiry enforced server-side. No Discord access or refresh token is persisted: `current_user` consumes the token by value, so the handler cannot reach it afterwards. One key signs all three token kinds, which is safe only because the MAC is domain-separated — without it a `state`, which the holder reads out of their own address bar, would verify as a session cookie. The client secret lives in Secrets Manager and reaches the Lambda through the Parameters & Secrets extension it already loads for its ClickHouse certificate; the environment carries only the name, computed by a helper beside `mtlsSecretName` (ADR 0007, Tranche 3 AC 6). The read is skipped entirely while the portal is closed — reading it unconditionally would fail Lambda init on a deployment where the secret does not exist yet, and one router serves every route group, so that would take /v1 down to protect four routes that answer 404 either way. The gateway throttle and the CloudFront cookie forwarding this task requires turned out to be already correct from task 0184. Both were verified by measurement rather than re-implemented, and both are now guarded: three new assertions in verify-openapi-routes.mjs pin the CloudFront policies and the access-log cookie setting, and a source scan pins the methodSettings entries to BOTH arms of `if (cacheEnabled)` — a synth cannot see that one, because production runs the cache on and only ever exercises one arm. Frontend is a link and a line of text. Signed-out and cancelled are plain text, not screens; styling is task 0193's. Verified: the live Discord round-trip by Adam against his own registered application, locally with the flag on. 87 automated tests, 65 new. Everything else — the session through CloudFront — needs task 0205's gateway deploy, since these four routes sit at depth 3 and the deployed mapping covers 1-2.
A mutation sweep over the fifteen security properties this slice claims found two the suite did not detect. This closes the one that is closable. Deleting the `pending.action != state.action` comparison in `state_token::accept` left the whole suite green. With a single `Action` variant the mismatch arm is unreachable, and the test that looked like it covered it — signing a payload with an unknown action string — fails earlier, at deserialization, and reports BadSignature. So the check was never exercised. That defeats the reason the slot is carried now rather than later. Task 0189 adds the second real action, and the point of paying for the field early is that the comparison is already there AND already tested when it does. `Action` therefore gains a `#[cfg(test)]` variant. It is not 0189's `issue` arriving early: `Action::parse` never yields it, so `/auth/login` cannot mint a round-trip for it, and it is compiled out of every shipped binary. It exists only to make the mismatch arm reachable. Re-running the mutation now fails exactly one test. The second miss — swapping `ct_eq` for `==` on the MAC comparison — is left uncovered and recorded instead. Both are functionally identical; only timing separates them, and a statistical timing assertion is too flaky to be worth its false failures. Code review is the control there.
Two findings from the review pass, both in the "reports an error and leaves the user worse off" family. **A stranger could cancel someone else's sign-in.** The callback cleared the pending-login cookie before it verified anything, so every unverifiable shape took an in-flight login down with it: `?error=x`, `?state=<garbage>`, `?code=x`, or no parameters at all. `SameSite=Lax` sends that cookie on a top-level GET navigation, which is exactly what any third-party page can cause, so a victim part-way through signing in could be knocked back to `invalid_state` by a page they merely visited. Nothing was disclosed and no session could be issued — a denial of login rather than a break — but it cost an attacker one link and the victim saw only that signing in "did not work". The fix is an ordering one. `state` is now verified first, on every path including the cancellation, and only then is the cookie dropped. RFC 6749 §4.1.2.1 has the authorization server echo `state` on the error response too, so an error callback carrying none did not come from Discord finishing our round-trip and has no business acting on this browser's state. `refuse_state` no longer emits a cookie header at all. Replay is unaffected: every path from a verified `state` onwards still clears, so a second presentation of the same code+state still finds nothing to match against. That half now has a test of its own rather than riding on the first. **The portal offered no way out of a failed session check.** When `/auth/me` answered anything but a session, the page reported the error and removed the sign-in control, leaving a dead end the visitor could escape only by guessing at a reload. Signing in is a fresh top-level navigation and does not depend on the request that just failed, so the control stays. +3 integration tests, +1 frontend test. The suite that asserted a refusal DOES clear the cookie now asserts the opposite, which is the behaviour change: that assertion was pinning the defect.
…nment Two findings from code review. **A data race in the integration suite.** `Endpoints::from_env` ran inside `app()`, so `std::env::var` executed on every router construction — fifteen of them in `tests/portal_auth.rs` — while thirteen others called `std::env::set_var` to point a router at the mock Discord. libtest runs those on parallel threads, so a reader could be walking `environ` while `setenv` reallocated it. That is the undefined behaviour `set_var` was made `unsafe` for in edition 2024, and it does not surface as a failing test: it is an intermittent segfault that aborts the whole binary. Serialising the readers behind the same lock would have worked. Threading the value through `AppConfig` removes the race by construction instead, and takes the `unsafe` block out of the test file entirely. It also closes the quieter half of the same problem, which no lock would have: a router built by `signed_in_app` captured whatever base URL another test had last written, and was harmless only because none of those tests reached the token exchange. Production is unaffected — `AppConfig::from_env` still reads the same two overrides, and `compute-stack.ts` still sets neither, so the deployed handler takes Discord's real endpoints as before. **The sign-out re-read bypassed its own staleness guard.** `load()` returns a canceller and the mount effect wired it up, but `onSignOut` called `load()` and dropped the return value, leaving that request's `live` flag with nothing to flip it. The canceller now lives in a ref: one mechanism covers both the supersede case and the unmount case, rather than depending on which caller remembered. Cosmetic under React 18, which made the stray `setState` a silent no-op — but it defeated the guard the comment above the effect claims is there.
The recorded section claimed "no High or Medium defect in the code", which was true when it was written and wrong twice over afterwards: the action-slot comparison was dead code, and the callback cleared the pending-login cookie before verifying anything. It also still listed the missing sign-in control as open after it had been fixed. Replaced with a ledger that separates fixed from open and names the commit for each fix, so a future session reads what the code actually does rather than what the first pass believed. Adds the two findings that arrived later — the CloudFront Set-Cookie response direction, and the one mutation no test can catch — and the mutation-coverage numbers, which are the evidence that the suite detects defects rather than merely passing.
The runbook told an operator how to provision the secret but only gestured at the local round-trip, and nothing said which port the redirect URI belongs to — which is the single thing most likely to be got wrong, because it is the Vite port and not the API's. `packages/prices-api/README.md` gains the full procedure: the Discord registration, the shape of `.portal-oauth.json` with placeholder values, the two commands, what a working run looks like both in the browser and over curl, and a symptom table. Every command in it was run as written. Two things worth stating that were not written down anywhere: - **No ClickHouse is required.** `serve` builds a lazy plaintext client that never connects unless a `/v1` route is hit, and it ignores `CH_ENABLED` entirely — that variable is a Lambda-only knob. Earlier notes passed `CH_ENABLED=false` as though it mattered. - **The proxy is what makes the browser see one origin**, which is the property `SameSite=Lax` depends on. A separate backend port breaks the session in a way no test in this repo can see, so it is not an arbitrary bit of dev ergonomics. The runbook's section 4 now points at this rather than keeping a second, thinner copy, and holds only what is genuinely its own: adding the second redirect URI to the registration, and deciding whether to remove it afterwards. The env table gains the five variables task 0186 introduced, including which of them CDK sets and which exist only as a local or test seam.
Collaborator
Review notes — Discord sign-inReviewed The security-critical parts hold up. PKCE S256 matches the RFC vector, Two things I checked by measurement rather than by reading, both clean:
Three findings below. 1. Every Discord OAuth error is reported to the visitor as "cancelled", and nothing is logged —
|
**1. Every Discord OAuth error was reported as "cancelled", and none was logged.** The branch was `if query.error.is_some()`, so `invalid_scope` — what Discord returns when the Developer Portal registration has drifted from `discord::SCOPE` — landed every visitor on "Sign-in cancelled." with nothing in CloudWatch. That is the same drift the token-response scope check exists to catch, arriving by the one door that check never sees, and it presents as every visitor changing their mind forever. `access_denied` alone is now a cancellation: silent, unchanged page. Everything else, including values Discord may invent later, is a logged failure landing on `?signin=failed`, which the portal renders as its own state. The `error` value is attacker-controlled, so it reaches the log truncated and stripped of anything outside printable ASCII, and it never reaches the URL at all — both landing states are literals. **2. `Endpoints::from_env()` was live in the deployed Lambda.** Setting `DISCORD_API_BASE` moved the endpoint in a `--features lambda` build, proved by running one. `exchange_code` then posts `client_secret` and the authorization `code` to a host of the setter's choosing, which exfiltrates the secret with no `GetSecretValue` of their own in CloudTrail — the Lambda does its usual read and posts the result out. `lambda:UpdateFunctionConfiguration` is enough, and that is a permission distinct from `UpdateFunctionCode`. The overrides are now compiled out under the `lambda` feature. The comment that claimed "no deployed configuration in which these are attacker- or operator-reachable" is replaced with what the code enforces; `compute-stack.ts` setting neither is now a second line rather than the only one. `PORTAL_OAUTH_SECRET_FILE` is gated the same way, and it deserves it more than it first appears: the same permission attaches LAYERS, whose contents unpack to `/opt`. One permission therefore places a chosen file and points this at it, handing over `session_signing_key` and with it the ability to mint a session for any Discord ID — worse than the endpoint override, not milder. `redirect` no longer `expect`s its `Location`. The authorize URL is the one target not built from literals alone, and a value carrying a newline panicked the task: no response written, connection dropped, `curl` reporting `000`, and on Lambda an invocation error for what is a configuration fault. **3. A malformed callback answered `502 discord_unavailable`.** These routes are keyless and throttled at 10 req/s, so anyone could call `/auth/login`, take the `state` and replay it with no `code` to manufacture 5xx — polluting the alarms 0204 is building and making a real Discord outage indistinguishable from a script. Now a `400` with `invalid_query`; the pending cookie still goes, because `state` has verified by then. The `cfg(test)` `Action::TestOther` arm follows. Every new behaviour is declared as a property a mutation can break — the gap that let all three ship. Eight mutations, eight caught. The logging one needed the decision extracted into `refuse_oauth_error` so a test can drive it with a capturing subscriber: "there is a `warn!` here" is exactly the claim that survives its own deletion.
…sign-in-identity-only
…lper CI failed on `cargo test --workspace` with `missing fields portal_endpoints and portal_oauth in initializer of AppConfig` at `tests/common/mod.rs:18` — a file that does not exist on this branch. Task 0119 merged into `develop` twelve commits ago and added it, and a `pull_request` build tests the MERGE of the branch with its target rather than the branch alone. Green locally, red in CI, and correctly so: the merge result is what would land. `portal_enabled` is false in that helper, so the portal routes answer an empty 404 whatever the credentials say; `None` and the default endpoints are the shape every non-portal test wants. Third time an `AppConfig` field has broken test literals — 0119 hit it with `portal_enabled` in c7bf795, this branch hit it twice. `tests/common` is the structural answer for the suites 0119 owns; the portal suites still spell the struct out, which is a cost worth noticing rather than one worth a refactor mid-review. Verified on the merge result rather than on either side: the exact four CI Rust steps (fmt, check, the clippy set 0119 extended to include prices-api, and `test --workspace` — 89 suites), the Nx targets, the Lambda build, synth, all three OpenAPI guards, and the 61-assertion adversarial suite. The three review fixes still hold.
adamkoot
force-pushed
the
feat/0186_discord-sign-in-identity-only
branch
from
August 18, 2026 12:46
add38a3 to
0866f9e
Compare
Task 0119 merged into `develop` with `ValidatedQuery`/`ValidatedPath`/
`ValidatedJson` — opt-in wrappers so every rejection answers in the
`ErrorEnvelope` voice rather than axum's `text/plain`. The data handlers adopted
them; the portal routes, written on a branch cut before that landed, did not. So
this is drift the merge created rather than anything either side got wrong.
It matters more here than the "consistency" framing suggests. A `text/plain`
rejection on these routes is read by task 0185's bundle, whose `getJson` reports
a non-JSON body as "could not reach the portal backend" — the failure mode that
file is explicit about, because it is also what a broken CloudFront behaviour
ordering looks like. A caller's own duplicated query key would therefore present
to them as an outage, and to anyone reading the page as a routing regression.
Measured before and after on `?code=a&code=b`:
before 400 text/plain Failed to deserialize query string: …
after 400 application/json {"code":"invalid_query", …}
Both handlers, both directions pinned: reverting either to plain `Query` fails
the new test. Valid queries are unaffected — `/auth/login` still redirects,
`?action=signin` still redirects, `?action=issue` still refuses at 400.
Every field on both query types stays `Option`, so an unknown key is still
ignored rather than rejected. Discord may add parameters to a callback, and a
sign-in that broke because of one would be self-inflicted.
…edger The ledger was written after my own review passes and before karczuRF's, so it claimed five fixes where there are now nine, and said nothing about the finding that mattered most: the endpoint overrides being live in the deployed Lambda. Adds F6-F9 and a section the previous version had no reason to contain — what the mutation sweeps did NOT find. None of the three external findings would have been caught by any of them, because each concerned behaviour never declared as a property: 'only a cancellation is silent', 'the Lambda ignores the overrides', 'a client error is not a 5xx'. A sweep proves the properties you wrote are enforced and says nothing about the ones you did not think to write. Recording that bounds the method for whoever reads these numbers next and might otherwise read 24-of-25 as coverage.
…w states Two gaps the last two changes opened and nothing covered. **`ValidatedQuery` must not out-rank task 0183's gate.** The extractor rejects before the handler but after routing; the gate is a layer, so it runs first. That ordering is the only reason a malformed query on a CLOSED portal still answers an empty 404 rather than a 400 naming `invalid_query` — which would say "this route exists and parses input", the disclosure the gate is built to prevent. Nothing about `ValidatedQuery`'s own contract guarantees it; it is a property of how the two compose in `portal::apply`, and it now has a test. Moving the gate behind the merge fails three tests. **The docs did not know about `?signin=failed`.** Worth more than tidiness on the runbook side: scope drift is now caught in two places that fire at different moments. A registration asking for LESS than the code requests is refused by Discord at the authorize step and never returns a code — `invalid_scope`, the new landing state, a log line, and no token exchange. One asking for MORE gets as far as the granted-scope check. An operator debugging drift needs to know which log line points at which, and the runbook exists for exactly that failure. The README symptom table gains the states the review fixes introduced: `invalid_scope`, the cancelled/failed split, `invalid_query`, and `sign_in_misconfigured`.
karczuRF
approved these changes
Aug 18, 2026
adamkoot
added a commit
that referenced
this pull request
Aug 18, 2026
0185 (#218) and 0186 (#220) are both merged to develop, so archive them and promote 0187, whose prerequisite is the session cookie 0186 issues. 0185 closes with six of seven criteria; the deploy-on-merge one was withdrawn with Adam on 2026-08-14. 0186 closes with nine of ten: the session surviving CloudFront is archived open on 0205's deploy, since the four sign-in routes sit at depth 3 and the deployed gateway maps only depth 1-2. Neither spawns a backlog task — every follow-up already has one.
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
2 participants
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.
Third slice of the self-service onboarding epic. Adds Discord sign-in to the
existing
prices-apiaxum router — identity only, scopeidentify, a sessioncookie. No key is issued, no eligibility is checked, nothing is stored.
Closes the code half of task 0186. Ships behind [0183]'s
PORTAL_ENABLEDflag, which stays
falsein production.What this adds
Four routes under
/api-tokens/api/, so [0183]'s prefix gate covers themwithout knowing they exist:
GET /auth/loginstate+ PKCE, redirects to DiscordGET /auth/callbackstate, exchanges the code, issues a sessionGET /auth/mePOST /auth/logoutNew module tree
packages/prices-api/src/portal/auth/—crypto,state_token,session,cookies,discord,secret. No new crate, nosecond gateway integration, no second build (ADR 0008, 2026-08-07 meeting).
Frontend is a link and a line of text. Signed-out and cancelled are plain
text, not screens. Styling is [0193]'s.
The security-relevant decisions
stateis bound to the browser, not just signed./auth/loginmints twovalues from one nonce: the
stateparameter that travels to Discord, and asigned
HttpOnlycookie holding the same nonce plus the PKCE verifier. Asingle signed
statewould not catch the interesting attack — an attacker whocalls
/auth/loginthemselves holds a genuine, correctly-signedstate, andpasting it into a victim's browser is the whole login-CSRF. Their nonce does
not match the victim's cookie.
Replay is closed by clearing the pending cookie before the outcome is
known, on every path out of the callback. A second presentation of the same
code+statehas nothing left to match against, so nothing here needs aused-nonce table. Discord's codes are single-use too, but that is Discord's
guarantee to keep, not ours to rely on.
One key signs three token kinds, which is only safe with domain
separation. The MAC covers
context ‖ 0x00 ‖ payload. Without it astate—which the holder reads out of their own address bar — would verify as a
session cookie, and
/auth/loginwould be a session vending machine.No Discord token is persisted.
current_usertakes the token by value,so after that line the handler cannot reach it. The session carries exactly
three fields and a test asserts on the serialized form, so adding a fourth is
what fails rather than a reviewer having to notice.
Scope is verified, not just requested. Scopes are declared in the Developer
Portal and in the authorize URL, so the two can disagree and only the token
response shows the real grant. Compared as a whole string, so
identify guildsis refused (ADR 0010).
The
actionslot is carried and checked now, though only sign-in exists.[0189] binds issuance and rework to a round-trip, and adding the field then
would mean re-deriving the signing format while a deployed portal holds live
cookies in the old one.
Secrets
The client secret lives in Secrets Manager and reaches the Lambda through the
Parameters & Secrets extension it already loads for its ClickHouse
certificate. The environment carries only the name, computed by
portalOauthSecretName()besidemtlsSecretName(ADR 0007, Tranche 3 AC 6).The IAM grant is scoped to that one secret, on the api-handler role only — no
worker signs a cookie.
One secret with four fields, three of which are not secret. They are one
Discord application registration, owned by one person, and the
redirect_uriis re-pointed by hand at the domain cutover — a CloudFormation-managed value
would be restored by the next
cdk deploy, breaking sign-in silently sometime after the cutover appeared to work.
SecretsStackpublishes the name andnothing else, the same rule it already states for the mTLS material.
The read is skipped entirely while the portal is closed. Reading it
unconditionally would fail Lambda init on a deployment where nobody has
created the secret yet — and one router serves every route group, so that
would take
/v1down to protect four routes that answer an empty404eitherway. With the portal open the stance inverts: a missing or malformed secret is
fatal, so it surfaces as
Init Errorsat deploy rather than as a503under asign-in button.
Two things verified rather than built
The gateway throttle and the CloudFront cookie forwarding this task requires
were already correct from [0184], which wrote them citing this task. Both
were checked by measurement rather than taken on trust — and both are now
guarded, because in each case the correct setting was chosen for a different
reason and is load-bearing here by coincidence, which is what a later tidy-up
breaks.
apiGatewayCacheEnabledflipped tofalsein a throwaway synth and thearrays diffed: the three portal entries survive,
rate=10, burst=40, cachingEnabled=false.ALL_VIEWER_EXCEPT_HOST_HEADERwas chosen forHostand happens to forwardcookies;
CACHING_DISABLEDwas chosen forx-api-keyand happens to satisfy"do not cache auth paths".
tools/scripts/verify-openapi-routes.mjsgains four assertions:the two managed-policy IDs,
IncludeCookies: falseon the access log, and asource scan pinning
...portalSettings/apiDocsSettingsto both arms ofif (cacheEnabled). That last one cannot be a template check — production runsthe cache on, so a synth only ever exercises one arm and the regression
produces a byte-identical template. Measured, not assumed.
Testing
87 automated tests, 65 new: 46 unit across the six new modules, 36 integration
(23 new in
tests/portal_auth.rs), 27 frontend (9 new). The integration suitedrives all four routes over HTTP against a mock Discord bound to loopback,
using the same
reqwestclient production uses — a trait-based fake would haveskipped the HTTP layer, which is where three of the requirements actually live
(secret in the body not the URL, the verifier matching its challenge, the scope
check).
Green:
cargo fmt --check,cargo check --workspace,cargo clippy -p prices-api --all-targets -D warnings,cargo test --workspace,cargo build --features lambda,nx run-many -t lint typecheck build test,nx format:check --all,make -C infra synth-production,openapi:lint,openapi:verify-routes,openapi:verify-servers,verify-lambda-assets.sh.An adversarial review pass found no High or Medium defect. Its top hypothesis —
that API Gateway REST v1 would drop the second of the callback's two
Set-Cookieheaders and silently lose the session — was checked againstlambda_http1.2.1 and does not hold: it sends everything throughmultiValueHeadersand leavesheadersdeliberately empty. Secrets wereprobed with canary values at
RUST_LOG=info,debugandtrace, includingreqwest/hyperinternals: zero hits. Eight Low findings are recorded in thetask file.
Manual verification
The live Discord round-trip has been verified by Adam against a Discord
application he registered, running locally with the flag on: sign-in completes
and the page renders his Discord username and ID. That also settles the
question [0156] left open — redirect-URI matching behaved as the runbook
assumes.
docs/runbooks/portal-oauth-deploy-prep.mdis new: registration, secretprovisioning, local verification, and a five-step ordering for the
custom-domain cutover with the failure mode of inverting it spelled out
(Discord refuses on its own error page, so nothing reaches our logs).
Not in scope
Everything that turns an identity into an entitlement is [0189]:
guilds.members.read, the guild membership call,pending, the snowflakeaccount-age minimum. Key issuance is [0187]. This PR adds no store — [0190]
decides whether a registry is needed at all.
Ordering note for whoever merges
Safe to merge on its own: production stays
PORTAL_ENABLED=falseand theseroutes answer an empty
404.Do not flip the flag before [0205] deploys. The four sign-in routes sit at
depth 3 (
auth/login), and the mapping currently in production is [0184]'sintermediate
{proxy}+{proxy}/{sub}pair, which answers403 Missing Authentication Tokenat that depth. This branch carries the committed{proxy+}; [0205] is what ships it. The session-through-CloudFront criterionis the one thing that cannot be closed until then.