Eligibility gate — Discord membership and minimum account age before a key is issued (lore 0189) - #230
Conversation
… key is issued
The issue path moves behind a fresh OAuth round-trip (action=issue in the
signed state): the callback checks Stellar Discord membership
(GET /users/@me/guilds/{guild}/member with the just-exchanged user token)
and the snowflake-derived account age against two operator-seeded SSM
parameters, and only then runs 0187's reconciler. Three outcomes, not two:
only Discord's own 10007/10004 on a 404 is 'not a member'; 401/403/429/5xx,
unrecognised shapes and an absent pending field refuse without accusation.
The /key route goes fully read-only — a session cookie alone can cause zero
control-plane writes — and answers no_key when there is nothing to reveal.
Scope becomes exactly 'identify guilds.members.read', compared as a set.
'Get my API key' becomes a top-level link into the issue round-trip; the key section fetches the (now read-only) reveal on mount and renders the five ?issue= landing states in the wording this task decides: not-a-member names the server and links discord.gg/stellardev, too-young renders the backend's wait_secs as a wait rather than a rejection, and could-not-verify is explicitly not an accusation. Landing params are one-shot — read once, stripped from the URL — which also closes 0186's O10 stale-banner item. The landing page states both prerequisites before the visitor authenticates.
…docs compute-stack passes PORTAL_GUILD_ID_PARAM and PORTAL_MIN_ACCOUNT_AGE_PARAM (names only, unconditional) and states the read grant explicitly beside the plan-id one. verify-openapi-routes gains check 7: the handler carries both names exactly, and no synthesized template may CREATE either parameter — a CDK-owned parameter would be restored by the next deploy, un-flipping production back to the test guild after 0179. The runbook gains §2a (the put-parameter seeding, the ownership split, the no-redeploy property) and its scope step is rewritten to the two-scope done state; the README gains the local procedure for the gate and its refusal states.
…asurements Step 0's tables stay empty with a dated deferral — the archived 0180 notes were found unmeasured and the prerequisites are operator-owned; the code is written to the documented safe rules with each unmeasured behaviour behind one reversible arm. Ticks the code-side acceptance criteria and names the two operator steps that remain.
All seven were verified against the code before acting; none was a false positive. The reconcile deadline is now derived from what is left of the invocation rather than reused from 0187, where the reconciliation was the whole request. With four network calls in front of it the worst case reached ~29s against a 15s Lambda, which answers an API Gateway 502 instead of ?issue=failed and can leave a key created but never attached. The authorize URL sends prompt=none. Three comments asserted that Discord does not re-prompt for consent on repeat authorisation and the parameter that makes that true was never sent, so every issue, every retry after a refusal and every future rework was a full consent screen — the cost the per-action model's own justification denied. ?issue=cancelled and ?issue=denied give the issue flow the pre-check pair sign-in has had since 0186. Without them a cancelled press landed on ?signin=cancelled, whose banner renders only in the signed-out branch an issue round-trip has by definition left: the visitor came back to an unchanged dashboard with nothing said. guild_id() and member_url now share one is_snowflake. Only the consumer checked the shape, so a guild name passed the cold-start probe and then refused every visitor as unknown, indefinitely. The task's parameter table named such a value, and is corrected with it. The key fetch runs once per load again — load() depended on an inline callback prop, so reporting the key re-fired the mount effect — and ?issue=ok no longer renders beside "you have no API key yet", a window that offered a second key to somebody who had just been given a first. Left alone deliberately: the wildcard apigateway:DELETE on /apikeys/*, which 0194's checklist already names verbatim, mitigation included.
Corrects two claims the round made stale — the redirect count and the test totals — and adds decisions 18 to 22 plus the two entries under Issues Encountered. The entry worth keeping is why a comment can be load-bearing and still describe code that was never written: three places asserted that Discord does not re-prompt for consent, and nothing sent prompt=none.
Neither was a criterion this slice's code could meet: both are manual prerequisites owned by the operator, and both remain recorded in Step 0's status note, in Future Work, and in the two runbooks they name. What is left is eleven criteria, all met, all asserted by tests.
karczuRF
left a comment
karczuRF
left a comment
There was a problem hiding this comment.
Code review of the eligibility gate. The security-relevant core is sound — I checked and cleared the scope set-comparison and member_url snowflake validation (path injection closed), classify_member_response's three-outcome table, the snowflake epoch math and wait_secs ceiling, the ISSUE_BUDGET/RECONCILE_FLOOR derivation against the real 15s apiHandler.timeoutSeconds, the read-only lookup vs. create-capable issue_for split (the reconciler is semantically unchanged; only KeyValue is dropped from Outcome), UsageCache being Arc-backed so the cloned handle in IssueDeps evicts the same cache, the Action::Issue state-token round-trip and mismatch refusal, the lambda/aws-mtls feature gating, and useOneShotParams.
Five findings below; the first is the significant one.
Two documented risks I did not file as inline bugs, but that are worth the team's eyes:
pending: None->Unknownmeans that if Discord's REST member object omitspending, every member is refused as "could not verify", indefinitely. Lore task 0180 item 2 is still unmeasured, so this is a fail-closed-for-100%-of-users path resting on unverified Discord semantics.prompt=noneis now on the sharedauthorize_url, so it also changes first-time sign-in behaviour, not just issuance.
Reviewed as the top of a two-PR stack on top of #227. Diff is correctly scoped by the branch target; if #227 takes review fixes, this needs a rebase before merge.
| @@ -382,6 +451,14 @@ async fn callback( | |||
| Err(error) => return refuse_discord("token exchange", error, drop_pending), | |||
There was a problem hiding this comment.
Discord failures on an action=issue callback bypass the designed ?issue= redirect.
They fall through to refuse_discord, producing a bare 502 JSON page worded "could not complete sign-in" with no link back — instead of the ?issue=unknown / ?issue=failed redirect this PR's whole three-outcome design is built around. The visitor pressed "issue a key", not "sign in", and lands on a dead end.
Same hole at issue.rs:262 (the identity read), and in login's unwired 503.
Most likely trigger on this PR specifically: UnexpectedScope, if the Developer Portal registration still carries only identify while the new prompt=none suppresses the re-consent that would otherwise have granted guilds.members.read. That combination turns a config lag into an unexplained 502 rather than the "could not verify" path you designed for exactly this.
| */ | ||
| function describeWait(waitSecs: string | null): string { | ||
| const parsed = | ||
| waitSecs && /^\d{1,7}$/.test(waitSecs) ? Number(waitSecs) : NaN; |
There was a problem hiding this comment.
describeWait rejects rather than clamps large waits, understating them drastically.
The /^\d{1,7}$/ guard rejects any wait_secs over 7 digits. So a min-account-age-minutes above roughly 16 weeks renders a months-long wait as "about a few minutes" — the most misleading possible failure direction for a value an operator sets via put-parameter without a redeploy.
Clamping to the top bucket would fail in the honest direction.
| key yet" is the page contradicting itself about the one fact the | ||
| visitor came for. The settling branch below says what is true. */} | ||
| {issue === 'ok' && view.state !== 'none' && ( | ||
| <p data-testid="issue-ok">Your key is ready.</p> |
There was a problem hiding this comment.
?issue=ok can render above a failed reveal.
The banner is guarded only against view.state === 'none', so an error state still shows "Your key is ready." directly above "Could not get your API key" — the same self-contradiction the none guard was written to avoid.
The guard wants to be against the states where the key is not on screen, not just none.
| {view.state === 'none' && issue === 'ok' && ( | ||
| <p data-testid="issue-ok-settling"> | ||
| Your key was created, and is taking a moment to appear.{' '} | ||
| <button type="button" onClick={load}> |
There was a problem hiding this comment.
The settling "Check again" button gives no feedback on a retry that still finds nothing.
It doesn't reset view to loading, so pressing it when the key still hasn't settled produces zero visible change — the user can't tell the press registered, and the natural read is that the button is broken.
Usage's Refresh (line 774) does the opposite and is the right model here.
| for (const file of templateFiles) { | ||
| const tpl = readJson(join(cdkOut, file), 'synthesized template'); | ||
| for (const [id, resource] of resourcesOfType(tpl, 'AWS::SSM::Parameter')) { | ||
| const name = resource.Properties?.Name; |
There was a problem hiding this comment.
Check 7(b) is evadable by a non-literal parameter name.
It only inspects Properties.Name when that value is a literal string. A CDK-created SSM parameter whose name synthesizes to an Fn::Join / Fn::Sub — which is what you get the moment anyone interpolates the env into it, i.e. the natural way someone would write /prices/${env}/discord-guild-id — slips straight past the "never CDK-owned" guard.
That matters more than a usual CI gap, because the guard exists precisely to stop a cdk deploy from silently restoring a committed value and un-flipping production back to the test guild after lore task 0179 step 4. Treating any non-string Name as a failure (or resolving the intrinsic) would close it.
2 participants
Eligibility gate — Discord membership and minimum account age (lore task 0189)
The whole of the epic's abuse story. Until this lands, lore task 0187 issues a key
to anyone holding a Discord account — acceptable on a dev distribution, and not
something that may reach production.
What it does
A key is issuable only by a member of the configured Discord guild whose account is
older than a configured minimum. Both facts are proved per action, by a fresh
OAuth round-trip, and neither is ever carried in the session (ADR 0010 §8) — a signed
"eligible" claim would date the verdict to sign-in time.
pending === false) + account ageThe last two rows are documented in
eligibility.rsbefore those slices exist, sothey inherit the table rather than re-deciding it.
The structural change
/keyis now fully read-only — no create, no attach, no delete.GETandPOSTboth reach the same reveal. The create-capable reconciler survives as
issue_for,reachable only from the
action=issuecallback.That makes "issue is unreachable with a session cookie alone" a property of the call
graph rather than a guard someone can remove: a session cookie can cause zero
control-plane writes. It also retires 0187's
SameSite=LaxGET-may-create argument.Costs accepted and documented at the module: a hand-deleted key answers
no_keyinstead of resurrecting, an unattached orphan reveals un-repaired, duplicates wait
for the next issue to converge. Each heals on one gated press.
Three outcomes, not two
Only Discord's own
10007/10004on a404reads as "not a member". A401,403,429,5xx, an unrecognised404body, or an absentpendingfield is unknown:refuse, but never claim non-membership. "Could not verify" is fixable by waiting;
"you are not a member" is an accusation the visitor can only disprove by joining a
server they may already be in.
?issue=failedis kept separate from?issue=unknownfor the same reason — one saysDiscord could not vouch for you, the other says you are fine and our key service was
not.
Two operator-seeded SSM parameters
/prices/{env}/discord-guild-idand/prices/{env}/min-account-age-minutes, resolvedper issuance through the Parameters and Secrets extension, so an operator's
put-parametertakes effect without a redeploy (the extension's ~5 min cache is theonly delay). Probed once at cold start, so a bad seed is an
Init Errorsevent withthe parameter named rather than a per-visitor refusal.
Nothing creates them. A CloudFormation-managed parameter is CDK-owned, so the next
cdk deploywould silently restore the committed value — un-flipping production backto the test guild after lore task 0179 step 4.
verify-openapi-routes.mjscheck 7refuses any synthesized template that would create either, and both halves of the
check are non-vacuity-tested.
Review round
A code review returned seven findings. Each was verified against the code before
acting; none was a false positive. Five were fixed directly, two after confirming no
future task owned them.
RECONCILE_DEADLINEwas sizedfor 0187, where the reconciliation was the request. With four network calls in
front of it the worst case reached ~29s against a 15s function — an API Gateway
502instead of?issue=failed, and possibly a key created but never attached.The deadline is now derived from what is left of the invocation, measured from
request entry, with a floor below which no reconciliation starts.
prompt=nonewas missing. Three comments asserted that Discord does notre-prompt for consent on repeat authorisation; the parameter that makes that true
was never sent. Every issue, every retry after a refusal and every future rework
was a full consent screen — the cost the per-action model's own justification
denied.
?issue=cancelledand?issue=denied. Not a sixth and seventh verdict: thefive states are outcomes of a completed check, these happen before one starts,
and sign-in has had exactly this pair since 0186. Issue had neither, so a cancelled
press landed on
?signin=cancelled— whose banner renders only in the signed-outbranch an issue round-trip has by definition left.
is_snowflake, shared. The seed was validated in one place and consumed inanother, and only the consumer checked the shape, so a guild name passed the
cold-start probe and then refused every visitor as
unknown, indefinitely. Thetask's own parameter table named such a value and is corrected with it.
GET /keyfired twice per load (measured), becauseloaddepended on aninline callback prop. And
?issue=okcould render beside "you have no API key yet"in
GetApiKeys's eventual-consistency window — offering a second key to somebodywho had just been given their first.
Deliberately not fixed here: the wildcard
apigateway:DELETEon/apikeys/*. It isa real weakness — "own" is enforced only in code — but lore task 0194's checklist
already names it verbatim, mitigation included, and fixing it here would remove the
audit's subject.
Still open, and owned by the operator
Every acceptance criterion this slice's code can meet is met and asserted by a test.
Two items that were previously carried in that list are not criteria for code at all
and have been dropped from it: Step 0's five measurements (lore task 0180 items 1–5)
and the consent-screen captures need a second guild with screening off and a second
non-member account — manual prerequisites owned by the operator. They stay recorded
in Step 0's status note, in the task's Future Work, and in the two runbooks that
carry the procedure. The archived result tables were checked and are empty
placeholders, so nothing was carried in and nothing was invented.
The code is written to the documented safe rules instead: only a confirmed
10007/10004reads as non-membership, everything else refuses without accusation,and an absent
pendingnever passes. Either measured outcome changes at most onematch arm each (Design Decisions 4-6 in the task).
Verified
cargo fmt --all --check·cargo clippy --workspace --all-targets(0 warnings inprices-api) ·
cargo test --workspace(553 passed, 0 failed) ·cargo check -p prices-api --features lambda(every local seam compiled out) ·nx run-many -t lint typecheck build test(65 frontend tests) ·nx format:check --all·make -C infra synth-production·openapi:lint·openapi:verify-routes(check 7 live) ·openapi:verify-servers🤖 Generated with Claude Code