Repository navigation
Policy: admit anonymous reads through a grant that opts in, rate-limited per realm and address - #6568
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7ee1353f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await this.#chargeAnonymous( | ||
| request, | ||
| requestContext, | ||
| await dispatch.handle(), |
There was a problem hiding this comment.
Because JavaScript evaluates await dispatch.handle() before calling #chargeAnonymous, every request from an address that has already exhausted its quota still runs the policy gate and fully assembles the card—including projections and link expansion—before the database rejects it and the response is discarded. An unauthenticated client can therefore issue unlimited expensive reads despite receiving 429s, defeating the rate limit's load-protection purpose; charge after confirming the grant but before executing/assembling the admitted read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. Admission now checks the address's remaining budget without counting (AnonymousRateLimiter.remaining) before the request runs. An address that has used up its window gets the 429 in place of running anything: no gate, no assembly. A successful read is still counted afterwards, so refusals cost nothing. The 429 depends only on the address, so it says nothing about the target. Pinned by "an address over its limit is turned away before anything it asks for runs, whatever it names", which also asserts that no predicate is evaluated.
| .get('/.*', SupportedMimeType.CardJson, this.getCard.bind(this), { | ||
| ...APPLIES_ARCHIVED_SEAL, | ||
| anonymous: ANONYMOUS_CARD_READ, |
There was a problem hiding this comment.
.json requests before redirecting
When the policy contains any anonymous read grant, this route annotation admits every unauthenticated card+json request before resolving its target. getCard then returns its unconditional .json-to-canonical 302 before invoking the operation gate, so requests such as /private-draft.json or /missing.json are redirected and even charged as admitted rather than receiving the same actor-required 401 as other ungranted or missing targets. The redirect needs to occur only after the target-specific anonymous grant has been checked, or this spelling must bypass anonymous admission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. A request is now counted only once it has served what it asked for: a card document or headers, or a file's bytes (AnonymousCaller.served). So the .json 302 is free and isn't recorded as an admission. The redirect stays ahead of the gate, as it is for a signed-in caller the policy judges. It is the same for a card that exists and one that doesn't, so it reveals nothing. Pinned by "a redirect, and a HEAD that finds nothing a grant opens, cost nothing".
| .get('/.*', SupportedMimeType.CardJson, this.getCard.bind(this), { | ||
| ...APPLIES_ARCHIVED_SEAL, | ||
| anonymous: ANONYMOUS_CARD_READ, |
There was a problem hiding this comment.
Only the GET route is connected to anonymous admission. A card+json HEAD still enters through the existing CONSUMES_COARSE_OUTCOME route; because HEAD initially passes the coarse ACL, headCard performs its own read probe and then calls #policyJudges, which always rejects a caller without authenticatedUser. Consequently an anonymous caller granted read receives only the realm-discovery headers from HEAD while GET of the same card returns its actual ETag/content headers, breaking the established GET/HEAD equivalence and conditional-cache probing for these public cards.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. headCard now admits a caller who isn't signed in through #admitsAnonymous with the card read's operations, just as the GET does. A granted card answers with its own headers and validator, and is counted. Anything a grant doesn't open still gets the discovery answer, free. Pinned by "a HEAD of a card a grant opens answers as its GET does, and is counted".
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Independent review of the anonymous read path. The posture looks right. Admission reads only the compiled summary and realm.json, and loads no target. The gate keeps only anonymous grants and skips actor() predicates for such a caller. A hidden card, a missing card and a target the gate refuses all come back as the same 401 on card+json and on the byte routes (refusalForNonReader → target-not-found → #authenticationRequired/#bytesNotThere). Both fast paths stay off for a declined caller (#readDeclined), so the document cache and the conditional 304 can't bypass the gate. The middleware strips both client headers in lowercase before anything reads them, and Requests are built from req.headers, not rawHeaders. A blocklist that won't parse and an address that can't be worked out both fail closed. A policy that won't compile, or a peek that throws, opens nothing. Card+json HEAD still gets the discovery answer. Signed-in callers only gain a cheap instanceof check.
Inline findings, highest first:
- A refused or missing byte-route HEAD, and the
.json302, are charged and recorded asadmitted. - In a realm whose policy opens nothing to anonymous callers, every unauthenticated request on the tagged routes now makes an extra index read.
- If the limiter's charge throws, the response's file stream leaks and the request answers 500.
- A doc comment on
#admitsDespiteCoarseRefusalnow contradicts the code. - Test gaps: forged
x-boxel-client-class, HEAD, stylesheet, archived, and the routes that must stay closed.
The fixture IPs are RFC 5737, and I saw no ticket or PR references in code comments.
| this.#recordAnonymous(request, caller, { outcome: 'refused' }); | ||
| return response; | ||
| } | ||
| if (!caller.charged || response.status >= 400) { |
There was a problem hiding this comment.
[Claude Code 🤖] Refusals are charged when they aren't 4xx: a byte-route HEAD, and the .json redirect. This only skips responses at or above 400. On an anonymous HEAD of a data file or card .json, #grantableBytesDispatch admits the caller from inside handle(). When the file is missing or the gate refuses it, loadModuleFromDisk → #bytesNotThere returns realmIdentityResponse, which is a 200. So the refused HEAD costs a unit, and the record says outcome: 'admitted' with a count. A card+json GET of foo.json (the 302 at the top of getCard, before any read runs) is also charged and recorded as admitted. Every path is treated the same way, so this isn't an oracle. But it breaks "refusals cost nothing", and the telemetry overstates admissions, which is what an operator reads to decide what to block.
Suggested fix: decide charging by whether the read actually served something, not by the status. For example, set a served flag on requestContext.anonymousCaller where the card+json document and the byte serve are produced, and charge only when it's set. Alternatively, leave HEAD and 3xx uncharged. A HEAD test for a missing or draft path that asserts the budget is untouched would pin this.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. Charging is decided by whether the read served something (AnonymousCaller.served, set where a card document or headers or a file's bytes go out), not by status. So a byte-route or card HEAD that finds nothing a grant opens, and the .json redirect, cost nothing. The HEADs are recorded as refused, and the redirect isn't recorded. Pinned by "a redirect, and a HEAD that finds nothing a grant opens, cost nothing".
| !this.#stale && | ||
| now() - this.#validatedAt < MAX_UNVALIDATED_MS; | ||
| if (!warm) { | ||
| let row = await this.#env.readCard(new URL(card)); |
There was a problem hiding this comment.
[Claude Code 🤖] A realm whose policy opens nothing to anonymous callers now makes an extra index read on every unauthenticated request. When #current isn't warm, every call does instanceSource(card), which is a boxel_index query. This method never calls get() when the card has no anonymous grant, so it never warms the cache itself. Only a signed-in declined caller, an explain or a byte route warms it, and only for MAX_UNVALIDATED_MS (5 s). So in a private realm with a policy, every unauthenticated GET of a card+json, a data file, a stylesheet or _capabilities pays one more DB read than it did before. That includes render traffic without credentials, which the doc comment above mentions, and it gives an unauthenticated flood a cheap way to add DB load. The test "a realm whose policy opens nothing … loading nothing" only checks policyLoads and predicateEvaluations, so it can't see this.
Suggested fix: keep the peek's answer the way #current is kept. Memoize { card, opensAnything }, clear it in indexMoved when the move reads from the card's realm, and bound it by the same MAX_UNVALIDATED_MS. Add a counter to stats so the test can assert that repeated unauthenticated reads don't each read the card.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. RealmPolicyCache now keeps the peek's answer per card for MAX_UNVALIDATED_MS. indexMoved drops it when the card's realm is indexed, and a generation counter stops a read that started before an index move from storing what it read. A peeks counter in stats makes the read countable.
| return response; | ||
| } | ||
| let { limit, limitFrom } = await this.getAnonymousAccess(); | ||
| let outcome = await this.#anonymousRateLimiter.charge({ |
There was a problem hiding this comment.
[Claude Code 🤖] A failed charge leaks the response stream and turns the read into a 500. DBAnonymousRateLimiter.charge runs an unguarded query. If it throws (a DB blip, pool exhaustion), the error leaves #chargeAnonymous and goes through internalHandle's catch, which rethrows anything that isn't an auth or seal error. By then dispatch.handle() has already built the response. For the byte serve that response holds an open nodeStream from serveLocalFile, and nothing destroys it, so each such request leaks a file handle, and every anonymous read fails while the limiter is down.
Suggested fix: wrap the charge in try/catch. On failure, destroy or cancel the body as the 429 branch does, and make the fail-closed (or fail-open) choice explicit, e.g. a 503 with Retry-After. Record it with its own outcome, so an outage shows up in boxel:operations.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. Both the pre-run budget check and the charge are guarded. A limiter failure fails closed with a 503 rate-limit-unavailable and Retry-After, and is recorded with outcome unavailable. Where the read has already built its response, the body is released first (discardBody, shared with the 429 path), so the file stream is closed.
| async #admitsDespiteCoarseRefusal( | ||
| request: Request, | ||
| requestContext: RequestContext, | ||
| anonymous?: AnonymousDispatch, |
There was a problem hiding this comment.
[Claude Code 🤖] Evergreen: this method's doc comment now contradicts the code. Its last bullet says a request that authenticated nobody "is told to authenticate, whatever its path names and whatever the policy holds". It also says the hashed stylesheet serve "serves whoever is admitted here" only as a consequence of signed-in admission. With anonymous passed through to #admitsAnonymous, a request that authenticated nobody can now be admitted on a tagged route. Please update the bullet to say so, e.g. "or, on a route tagged anonymous, the policy opens one of its operations to such callers and the realm doesn't refuse their address (see #admitsAnonymous)", so the next reader doesn't rely on the old rule.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. The bullet now says a caller who isn't signed in is admitted on a route that runs an operation the policy opens to such callers, from an address the realm doesn't refuse (see #admitsAnonymous), and that any other request that authenticated nobody is told to authenticate.
| assert.strictEqual(block?.clientIP, null); | ||
| }); | ||
|
|
||
| test('the address is the one the load balancer appended, not one the caller wrote', async function (assert) { |
There was a problem hiding this comment.
[Claude Code 🤖] Test gaps on the security edges. This test covers a forged X-Forwarded-For. Nothing tests the header that matters most. A caller who could send x-boxel-client-class: infra (or its own x-boxel-client-ip) would never be limited or blocked. The middleware strips it today, but no test pins that. Also missing:
- A HEAD of a data file or card
.json, both granted and refused/missing: the answers, plus the budget (see the charging comment). - A hashed
_scoped-css/GET for an anonymous caller: served, never charged. - An archived newsroom: an admitted anonymous caller gets the seal only where a grant admits it, and the 401 elsewhere.
- In the newsroom, where anonymous grants exist,
_search/_federated-search,_operations, card+html and a card+json PATCH/POST stay 401actor-requiredfor an anonymous caller, so an over-broad futureanonymous:tag gets caught.
Suggested: one test that sends x-boxel-client-class: infra and x-boxel-client-ip: <OTHER_VISITOR> from VISITOR under a limit of 1, and asserts the second read is 429 and the record's clientIP is VISITOR. Plus a closed-routes test that loops over the routes above.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7367fd602b. New tests:
x-boxel-client-class: infraplusx-boxel-client-ipfrom the caller: still counted, under the caller's own address.- HEADs of the card and byte routes, granted and refused or missing: answers and budget.
- The hashed
_scoped-css/stylesheet: served, never counted. - An archived newsroom: 403
archivedwhere a grant admits the card, and the active realm's 401 elsewhere. - card+html,
_search,_operations, and card+json POST, PATCH and DELETE stay 401 for an anonymous caller, with nothing admitted.
_federated-searchis left to the anonymous-search change, which opens it.
…s, admit anonymous card HEADs Checks the address's budget before a request runs, so a caller over the limit costs the realm one query. Charges only once a read has served a document, headers or bytes, so redirects and HEAD refusals are free. A limiter failure answers 503 rate-limit-unavailable and releases the response stream. The anonymous policy peek is memoized like the compiled policy. Card+json HEAD admits anonymous callers as GET does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Host Test Results 1 files ± 0 1 suites ±0 6m 45s ⏱️ - 1h 44m 15s Results for commit c107673. ± Comparison against earlier commit 7bb9988. Realm Server Test Results 1 files ±0 315 suites ±0 2h 0m 17s ⏱️ -53s Results for commit c107673. ± Comparison against earlier commit 7bb9988. |
richardhjtan
left a comment
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] One blocking finding, inline: _capabilities admits callers who aren't signed in without counting them, so it gets around the anonymous rate limit.
…t card+html with its own media type A capability check loads each pair's target and runs its grants, so a caller admitted without a session is charged one unit per pair against the realm's per-address budget before any is checked. A request that doesn't fit gets the 429 whole. Charging is shared with the served-read path through #countAnonymous. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // | ||
| // A policy no grant of which opts in to such callers is answered from the | ||
| // card as the index holds it, without compiling: the realm asks this of | ||
| // every request that authenticated nobody, including the ones its own | ||
| // renders make, and a realm whose policy opens nothing to such callers pays | ||
| // for no compile on their account. |
There was a problem hiding this comment.
[Claude Code 🤖] This comment is hard to parse, and two parts of it are off: "every request that authenticated nobody" is broader than what actually reaches anonymousAdmission (only requests the ACL rejects for missing credentials, on routes tagged for anonymous callers, in a realm with a policy), and the first paragraph's "read from the compiled policy and nothing else" is contradicted by this paragraph, which reads the raw card. It also leaves out the warm-compile shortcut. Proposed rewrite of the whole comment (lines 338–349), in plain words:
// Which operations this realm's policy lets unauthenticated callers run.
// Empty if none. Reading this is cheap: it never loads a card or runs a
// predicate. If the policy fails to compile, the answer is "none", so an
// unauthenticated caller gets the same 401 they'd get from any realm and
// can't tell that this one has a policy at all.
//
// Cost matters here because this runs for every request the ACL rejects
// for missing credentials, and the realm's own renders send such requests
// while it indexes. So before compiling anything, we read the raw policy
// card from the index and look for a grant with `anonymous: true`. If
// there isn't one, we return "none" without compiling. If a fresh compile
// is already cached, we skip that check and use it directly.There was a problem hiding this comment.
[Claude Code 🤖] Fixed in dec5b3154f. I took your rewrite, with one addition: the raw-card check's answer is remembered as long as a compile is kept, which is what stops every unauthenticated request in a policy-without-anonymous-grants realm from paying an index read.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] Proposal: extract the anonymous admission code out of
No index, loader, adapter, or routing. That's code that wants to be its own module. Suggested shape. An interface AnonymousAdmissionEnvironment {
realmURL: string;
log: Logger;
limiter: AnonymousRateLimiter;
hasPolicy(): Promise<boolean>;
openedOperations(): Promise<ReadonlySet<string>>;
access(): Promise<AnonymousAccessSettings>;
}What moves with it: the What stays in
The Why here rather than later. #6575 (anonymous search) and #6577 (anonymous writes) are stacked on this and will each add more of the same kind of code. On the extracted class they add methods to a ~300-line file; on the current shape they add a few hundred more lines to |
…dmission Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-anonymous-reads-through-a-grant-that-opts-in
|
[Claude Code 🤖] Done in The stacked PRs moved onto it in the same pass. The search entry points and the writes' counting modes, write dispatch constants,
|
…-anonymous-reads-through-a-grant-that-opts-in
…-anonymous-reads-through-a-grant-that-opts-in
The limiter counts in fixed windows, so requests either side of a boundary were counted apart and a test expecting a refusal saw an admission. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-anonymous-reads-through-a-grant-that-opts-in
The wait counts down by the second, so two refusals a moment apart could name different waits and fail a byte-for-byte comparison. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-anonymous-reads-through-a-grant-that-opts-in
3 participants
Important
Merge order (each must merge before the next):
Compile a policy grant's opt-in to callers who aren't signed in #6534: compile a grant's anonymous opt-in (merged)4a. Policy: let a grant on a declared operation admit a caller who isn't signed in #6579 (CS-13605): anonymous named operations, stacked on Policy: admit anonymous writes as a user the realm's config names #6577. boxel-skills#174's §12 describes it
4b. Policy: explain how a realm treats a caller who isn't signed in #6581 (CS-13609): explaining a visitor's refusal, stacked on Policy: let a grant on a declared operation admit a caller who isn't signed in #6579. boxel-skills#174's §10/§12 describe it
Add the judgment calls to the realm-policy-authoring skill boxel-skills#175(CS-13108): merged, and merged into boxel-skills#174. This section is §12, Refusals §13, its section §14 and the checklist §15.5a. Say that no policy grant reaches a path the realm ignores boxel-skills#178 (CS-13336): edits other parts of the same skill; lands before boxel-skills#174
7a. Say that a policy's rules are not secret from the writers of other realms boxel-skills#181 (CS-13398): a realm-policy-authoring §9 bullet, stacked on z-index handling for modals opening modals #179, merges right after it
Every PR in this order carries this callout. Whoever opens one of the PRs above tells the owners of the others, and each owner updates its own callout.
In service of CS-13521, which lets a realm's policy admit a caller who isn't signed in: an
OperationGranton a base operation opts in withanonymous: true, an anonymous write is made as a user the governed realm names, and the governed realm limits and blocks such callers by address. This PR switches that on for reads. Grants already compile with the opt-in, and the realm already parses itsanonymousRateLimitandanonymousBlocklistand works out each caller's address; this is the first change that admits anyone on them.What a caller who isn't signed in can now reach, when the realm's policy has an anonymous grant for it: a card+json GET or HEAD (
read), a card's or data file's stored bytes (readSource), the scoped stylesheets that go with markup it may see, and_capabilities. Search and writes stay closed to such callers and follow in their own changes.How a request gets in. Admission lives in
Realm#admitsAnonymous, beside the existing signed-in path in#admitsDespiteCoarseRefusal. It only considers a request the ACL refused for want of authentication, that carries noAuthorizationheader, whose route is tagged with the operations it would run (RouteOptions.anonymous), and in a realm that names a policy. It then reads the compiled policy's anonymous summary and the realm's ownrealm.json: whether any of the route's operations is opened to anonymous callers, then whether the caller's address is determined, the blocklist is valid, and the address isn't on it. It loads no target and evaluates no predicate, so a refused request costs what it always has. A realm whose policy has no anonymous grant doesn't compile its policy for this at all (RealmPolicyCache.anonymousAdmission()peeks at the raw card first), and answers exactly as before.Once admitted, the gate holds an anonymous caller to the grants that opted in, and treats a predicate that reads
actor()as not holding rather than throwing. Every refusal such a caller meets, including a target that doesn't exist, is the same 401actor-requireda caller no grant admitted gets, so a visitor can't tell a hidden card from a missing one.Counting. Each anonymous caller has a per-address budget in each realm, kept in the Postgres fixed-window table (
AnonymousRateLimiter). Admission first checks, without counting, whether the address has any budget left. An address that has used it up is answered 429 before anything runs, whatever it asked for. Otherwise a read is charged one unit once it has served a card document, card headers or a file's bytes. So a refusal, a missing target, or the.jsonredirect costs nothing. A capability check is charged one unit per pair it asks about, before any pair is checked, and a request that doesn't fit gets the 429 whole. Stylesheets are never charged. The 429 carriesRetry-Afterand a JSON:API errorcode: 'rate-limited'withmeta.retryAfterSeconds. If the count can't be read or updated, the caller gets a 503rate-limit-unavailablewithRetry-Afterrather than being let through uncounted. A request from the platform's own egress (x-boxel-client-class: infra, set by the client-address middleware fromBOXEL_INFRA_EGRESS_IPS) still needs a grant, but is never blocked or charged. A realm whose policy opens nothing to anonymous callers decides that from the policy card as the index holds it. That answer is kept, like the compiled policy, until the card's realm is indexed again.Telemetry. Each anonymous call that reaches this path emits an
anonymous-requestrecord onboxel:operationswith an outcome ofadmitted,infra,refused,blocked(withblockReason)rate-limitedorunavailable, plus the raw client IP, the key it is counted under, the limit and where it came from, the post-charge count, the cost where it is more than one, and the correlation id. Explain with no actor runs the gate as an anonymous caller when the policy opens the operation to one. A write is answered as its route answers it, withactor-required, whatever the policy opens.Deployment. Counting by address relies on
BOXEL_TRUSTED_PROXY_HOPS=1so the realm server reads the address the ALB appended rather than the ALB's own. cardstack/infra#752 sets it andBOXEL_INFRA_EGRESS_IPS. Without it the realm server counts every visitor as one address, which over-limits but never lets a caller choose its address.Tests:
realm-policy-anonymous-read-test.tscovers:actor()grants;X-Forwarded-For,x-boxel-client-classandx-boxel-client-ip;_search,_operations, card+json writes);The gate, capabilities, explain and source-read policy tests run unchanged.
🤖 Generated with Claude Code