Repository navigation
Conversation
A grant that opts a write in to callers who aren't signed in admits it only where its acting-user key names, in the governed realm's current realm.json config, a Matrix user who may write the realm; the write is made as that user. card+json POST/PATCH/DELETE and /_operations admit such callers. A write is counted once every entry is admitted and before anything is written, one unit per entry, and a batch that doesn't fit gets 429 with nothing written. Every refusal a non-reader would see as not found is the anonymous 401. Capability checks and explain judge such writes through the same acting-user check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mitted routes are recorded Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: 82b55d04e9
ℹ️ 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".
| request, | ||
| requestContext, | ||
| scope, | ||
| () => entries.length, |
There was a problem hiding this comment.
For an anonymous caller admitted to /_operations, an empty boxel:operations list is explicitly valid, and a query-target entry can also expand to zero entries. This passes 0 to #chargeCaller, but DBAnonymousRateLimiter.charge rejects costs below one, so the request is reported as 503 rate-limit-unavailable instead of returning the valid empty result. Skip beforeCommit when entries.length === 0, as #countAnonymous already does for zero-cost capability checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. #anonymousCommit's count returns without charging when the cost is below one, so an empty batch, or one whose many target finds nothing, is answered as it is for anyone and costs nothing. Pinned by "an empty batch is answered as it is for anyone, and costs nothing".
| let batch = this.#mintPosture(coarseDeclined); | ||
| let batch = { | ||
| ...this.#mintPosture(coarseDeclined), | ||
| ...this.#anonymousCommit(request, requestContext, scope, () => 1), |
There was a problem hiding this comment.
When an admitted anonymous PATCH is unchanged but its clean index document cannot be read, #patchAdmittedCard invokes its commit closure once normally and again with reserialize=true. Because this shared batch now contains beforeCommit, both invocations charge the visitor, so one PATCH consumes two rate-limit units and can even return 429 on its internal retry after the first unit was already spent. Make the charge callback one-shot for the request or omit it from the recovery commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. The count is memoized per request (counting ??= count()), so a PATCH that commits again to reserialize is still one write and one unit, and the second commit can't 429 after the first has landed.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Independent review of this PR's own diff (head 82b55d0), read against the worktree. The core gate path holds up. I couldn't find a way for a visitor to write through a grant that didn't opt in, or through one whose acting user doesn't resolve. actor()-reading grants never admit, card+source and _atomic stay ACL-only, and ids are minted on every anonymous create path. The acting user is resolved against the governed realm's config and ACL, including explain's draft core. beforeCommit sits after staging and every admit, and before commitStaged. #disclosableFailure and seenBy keep hidden and missing targets indistinguishable, including for a 429 raised after admission. Signed-in paths add no queries, and the new #cardWriteRefusal branches only match codes a signed-in caller never gets.
Findings inline:
- An envelope with zero entries makes the rate limiter throw, so the caller gets a 503 and the realm logs a warning.
- A capability check on a world-readable realm refuses every visitor write the routes would admit.
- Visitor index jobs share the acting user's writer lane and read-your-writes drain, which the
WriteOptionscomment says can't happen. - The PATCH reserialize retry charges a visitor twice.
- Acting-user resolution runs before any charge on refused writes, and can call Matrix.
- Test gaps.
| return { | ||
| actingUser: () => scope.actingUsers.admitted[0], | ||
| beforeCommit: async () => { | ||
| let counted = await this.#chargeCaller(request, caller, cost(), { |
There was a problem hiding this comment.
[Claude Code 🤖] A batch with no entries gets a 503 from the rate limiter. beforeCommit charges cost() with no cost < 1 guard (#countAnonymous has one). In the envelope, cost is entries.length, which is 0 for an empty boxel:operations list. The envelope parser documents that list as valid. It is also 0 for an expect: "many" query target whose anonymously granted search finds nothing. writes.length === 0 sends that batch to beforeCommit. Then DBAnonymousRateLimiter.charge throws on a cost below 1, #chargeCaller catches the throw as a count failure, and this returns rate-limit-unavailable. The visitor gets a 503 where a signed-in caller gets 200 []. The realm also logs a "could not count" warning and records an unavailable event for every such request.
Suggested fix: return early when cost() < 1 (count nothing and refuse nothing), and add a test for an empty batch and a zero-match many batch.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. A cost below one counts nothing and refuses nothing. Pinned by "an empty batch is answered as it is for anyone, and costs nothing".
| // what a grant opts in to them, made as a user who may write the realm. | ||
| if ( | ||
| (await this.#policyJudges(write.refusal, requestContext)) || | ||
| requestContext.anonymousCaller |
There was a problem hiding this comment.
[Claude Code 🤖] On a world-readable realm, this check refuses every visitor write the routes would admit. _capabilities is judged as a read. On a realm whose ACL gives * read, a visitor passes that check, so #admitsAnonymous never runs and requestContext.anonymousCaller is never set. The write probe then refuses with CoarseAuthenticationRequired, and #policyJudges is false because no user signed in. This branch is skipped, so every write pair comes back writesRefused: 'actor-required'.
The real POST/PATCH/DELETE and /_operations are judged as write. Their refusal makes the visitor an anonymous caller, and the gate admits them through the opted-in grant. A public site with a feedback form, which is the main use for this feature, would hide its submit button from visitors even though the submission succeeds.
Suggested fix: when write.refusal is CoarseAuthenticationRequired and nobody signed in, ask whether the policy opens any write to anonymous callers (the same summary #anonymousCallerFor reads), and judge the lane through the gate then. The current test realm isn't world-readable, so this needs a test on one that is.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. When the write lane's refusal is CoarseAuthenticationRequired and nobody signed in, #capabilityLanes asks #anonymousCallerFor with the anonymous-eligible write operations (ANONYMOUS_WRITE_CHECK, never counted). If the policy opens a write to such callers and the realm doesn't refuse the address, the gate judges the lane, acting-user check included. Pinned by "a realm anyone may read answers a capability check about a write it opens to such callers", which also posts the write on the same world-readable realm.
| // see `drainRequestersOwnIndexing`). | ||
| // reads. Absent for system-originated writes. A write by a caller who isn't | ||
| // signed in is initiated by the acting user it is made as (see | ||
| // `CommitBatchOptions.actingUser`), whose job no reader waits on: such a |
There was a problem hiding this comment.
[Claude Code 🤖] "Whose job no reader waits on" isn't true when the acting user is a real account. initiatingUser becomes the acting user (coordinator opts.actor ?? opts.actingUser?.()). The initiatedBy key does two things:
- It chooses the writer lane:
indexingWriterLaneandprerenderHtmlWriterLaneuse#user:<acting user>. - It drives
drainRequestersOwnIndexing, throughincrementalIndexingInitiatedBy(authenticatedUser).
So if the config names a person, such as the editor or owner, two things follow. Their own writes index in the same lane as every visitor submission. And each of their reads waits, up to the drain budget, for in-flight visitor jobs. A burst of visitors from many addresses then slows a real user's editing and reads.
Either correct the comment and state that the config should name a dedicated account, or keep the attribution in telemetry and give anonymous jobs a lane of their own, for example null or the owner lane, rather than the acting user's.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. The comment now states the contract: a visitor's write shares its acting user's writer lane, and that user's reads wait for it as for their own writes, so a realm's config names a dedicated account rather than a person who edits the realm. Attribution stays on the acting user, which is the point of naming one. The authoring skill's anonymous section (boxel-skills#174) is being updated to tell authors to configure a dedicated account.
| let batch = this.#mintPosture(coarseDeclined); | ||
| let batch = { | ||
| ...this.#mintPosture(coarseDeclined), | ||
| ...this.#anonymousCommit(request, requestContext, scope, () => 1), |
There was a problem hiding this comment.
[Claude Code 🤖] A visitor PATCH can be charged twice. beforeCommit charges on every commitBatch call that carries these batch options. #patchAdmittedCard calls commit() a second time, as commit(true), when the patch changed nothing and the index holds no document for the card. Each call reaches beforeCommit, so the visitor pays 2 units for one write. If the first charge used their last unit, they get a 429 after the first commit has already landed.
Suggested fix: make the charge run at most once per admission (memoize the promise in #anonymousCommit), or leave the batch options' beforeCommit off the reserialize call.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 611d92da3b. The count is memoized per admission, so the reserialize commit doesn't charge again.
| await fetchRealmPermissions(this.#dbAdapter, new URL(this.url)), | ||
| this.#matrixClient, | ||
| ); | ||
| return (await checker.can(user, 'write')) |
There was a problem hiding this comment.
[Claude Code 🤖] Each visitor write that matches an opted-in grant does this resolution before any charge. Each such request costs a realm_permissions SELECT and a config read. can() goes through effectiveRealmPermissions, which calls matrixClient.getProfile(user) whenever the ACL has a users row. Resolution runs in decide, before predicates are evaluated. A refused write costs nothing, so the only limit is the admission-time remaining() check, which refusals never use up. A visitor can repeat a write whose condition fails, without limit, and each repeat costs the realm a DB round trip and possibly a Matrix call.
This is low severity, but consider either of these:
- Check the user's write permission without the Matrix lookup when they are listed explicitly.
- Cache the resolution briefly, keyed by the config and permissions version.
There was a problem hiding this comment.
[Claude Code 🤖] Leaving this as is. The resolution is one config read and one realm_permissions SELECT per request, resolved once per key however many entries name it. That is the same per-request ACL read every signed-in write already pays in checkPermission, and the Matrix profile lookup is the existing RealmPermissionChecker behaviour for ACLs with a users row. A visitor who repeats a refused write is still turned away at admission once their address is out of budget, and the realm's blocklist covers an address that abuses refusals. A short-lived cache keyed on the config and ACL would add a staleness window to exactly the check that has to take effect on the next write.
| ); | ||
| } | ||
|
|
||
| test('a caller who is not signed in leaves feedback, written as the user the realm names', async function (assert) { |
There was a problem hiding this comment.
[Claude Code 🤖] Test gaps. These behaviors this PR relies on have no test:
- Minted ids. A visitor
POST(and an envelopecreate) carrying its ownidorlidgets a realm-minted id. The current assertion only checksstartsWith(NEWSROOM). - Side-loads. A visitor card+json create or update with
includedside-loads is refused, and nothing is written. - Index job attribution. The write's index job has
initiatedByset to the acting user, and the caller'sactorstays empty. - Envelope existence. An envelope
updateto a closed draft and to a missing one gives byte-identical 401s. Only card+json is covered. - Read-only batches. A visitor batch that only reads is counted once per entry, and gets a 429 when it doesn't fit.
- Empty batches. A visitor batch with no entries, or with a zero-match
expect: "many"target. This currently gives a 503. - 503. A write whose count can't be read gives
rate-limit-unavailablewithRetry-After, and nothing is written. - World-readable realm. A capability check on a world-readable realm with an anonymous write grant. This currently answers denied.
- Delete. A visitor
DELETEadmitted through an unconditional or a card-onlywheregrant. The only delete test is theactor()one.
There was a problem hiding this comment.
[Claude Code 🤖] Added in 611d92da3b:
- a client-chosen
lidreplaced by a realm-minted id; - a side-load refused, with nothing written;
- byte-identical envelope 401s for a closed and a missing draft;
- a read-only batch counted per entry, then 429;
- an empty batch costing nothing;
- a DELETE admitted through a card-only
wheregrant; - the world-readable capability check.
Not added:
- The 503 path. The test server has no hook to make the limiter fail mid-request; the path shares
anonymousCountRefusalwith the 429 the tests do cover. - The index job's
initiatedBy. It is asserted indirectly by theactingUserson the admitted record, which reads the sameActingUsers.admittedthe coordinator takesinitiatingUserfrom.
…dable capability checks about anonymous writes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…searches-through-a-query-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config # Conflicts: # packages/runtime-common/realm.ts
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] The design reads well: every acting-user failure fails closed, counting happens only once a batch is admitted and before anything is written, and the test module covers each bullet in the description. One structural request, not a blocker.
Adopt AnonymousAdmission for the new realm.ts methods. The base branch extracted anonymous admission and counting into that collaborator, and the realm already delegates to it in ten places. This branch merged the extraction in but added three private methods to realm.ts that do the same kind of work inline: #anonymousCommit, #actingUsersFor and #resolveActingUser. Together they are about seventy lines with one caller each, so the move is mechanical, and the anonymous-write tests are thorough enough to catch a slip. The inline comments say where each should go. #refusalSeenBy and the capability-check wiring are fine where they are: the first is about the coarse-declined posture for any non-reader, and the second is the join between the ACL outcome and the dispatch.
Since the collaborator lives on the base branch, which is still open, the move is best made once that branch settles rather than racing it.
| // the caller's budget once every entry is admitted and before anything is | ||
| // written, so a batch the gate refuses costs nothing, and one that doesn't | ||
| // fit is refused whole. Nothing for any other caller. | ||
| #anonymousCommit( |
There was a problem hiding this comment.
[Claude Code 🤖] This is entirely about counting an anonymous caller's batch, and its only dependency is the collaborator's own chargeCaller. It reads as a method on AnonymousAdmission, something like commitOptions(request, requestContext, scope, cost), returning the same Pick<CommitBatchOptions, 'actingUser' | 'beforeCommit'>. The two call sites would spread its result exactly as they do now.
There was a problem hiding this comment.
[Claude Code 🤖] Done in 77f23e5b83. The acting-user lookup is AnonymousAdmissionEnvironment.actingUser, a callback the realm supplies with its config and ACL reads. OperationPolicyAccess.actingUser points at the same function through AnonymousAdmission.resolveActingUser, so the gate, explain, capability checks and the collaborator all judge a key the same way. actingUsersFor and commitOptions are methods on AnonymousAdmission, and the two call sites spread commitOptions as they did before. Anonymous-write and capabilities tests: 43/43.
|
|
||
| // The acting users of a request a caller who isn't signed in sent, one set | ||
| // for the request, which every scope it builds shares. | ||
| #actingUsersFor(requestContext: RequestContext): ActingUsers | undefined { |
There was a problem hiding this comment.
[Claude Code 🤖] This lazily builds the request's one ActingUsers from a resolver, which is the same kind of per-request bookkeeping as callerFor. It belongs beside it on AnonymousAdmission, with the resolver coming from the environment (see the comment on #resolveActingUser).
There was a problem hiding this comment.
[Claude Code 🤖] Done in 77f23e5b83. The acting-user lookup is AnonymousAdmissionEnvironment.actingUser, a callback the realm supplies with its config and ACL reads. OperationPolicyAccess.actingUser points at the same function through AnonymousAdmission.resolveActingUser, so the gate, explain, capability checks and the collaborator all judge a key the same way. actingUsersFor and commitOptions are methods on AnonymousAdmission, and the two call sites spread commitOptions as they did before. Anonymous-write and capabilities tests: 43/43.
|
|
||
| // Who `key` names in this realm's current `realm.json` `config`, and | ||
| // whether that user may write the realm, as the realm's ACL says now. | ||
| async #resolveActingUser(key: string): Promise<ActingUserResolution> { |
There was a problem hiding this comment.
[Claude Code 🤖] This one should stay realm code, since it reads realm config, fetches the ACL and consults the permission checker, but as an environment callback rather than a private method. AnonymousAdmissionEnvironment already supplies hasPolicy, openedOperations and access that way; an actingUser(key) entry fits the same pattern, and OperationPolicyAccess.actingUser would point at the same function so the gate, explain and the collaborator all judge a key identically.
There was a problem hiding this comment.
[Claude Code 🤖] Done in 77f23e5b83. The acting-user lookup is AnonymousAdmissionEnvironment.actingUser, a callback the realm supplies with its config and ACL reads. OperationPolicyAccess.actingUser points at the same function through AnonymousAdmission.resolveActingUser, so the gate, explain, capability checks and the collaborator all judge a key the same way. actingUsersFor and commitOptions are methods on AnonymousAdmission, and the two call sites spread commitOptions as they did before. Anonymous-write and capabilities tests: 43/43.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Host Test Results 1 files ± 0 1 suites ±0 8m 16s ⏱️ - 1h 45m 0s Results for commit bfbdb49. ± Comparison against earlier commit 4e9eda5. Realm Server Test Results 1 files ±0 317 suites ±0 2h 5m 7s ⏱️ + 4m 28s Results for commit bfbdb49. ± Comparison against earlier commit 4e9eda5. |
…searches-through-a-query-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
…y-grant-that' into cs-13579-policy-admit-anonymous-writes-as-a-user-the-realms-config
2 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)Policy: admit anonymous reads through a grant that opts in, rate-limited per realm and address #6568(CS-13536): anonymous reads (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): merged, and merged into boxel-skills#1747a. 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. Reads and searches are already switched on. This PR switches on writes.Who a write is made as. A grant that opts a write in names
actingUser, a key in the governed realm'srealm.jsonconfig(compiled asCompiledOperationGrant.anonymous.actingUserKey).isMatrixUserId), and the user must holdwriteon the realm's ACL (RealmPermissionChecker).card-operations/acting-users.ts: oneActingUsersper request, shared by every scope the request builds. Each key is resolved once, and the request remembers who its admitted writes were made as and why any key failed (key-missing,not-a-matrix-id,no-write).OperationPolicyAccess.actingUser, so capability checks and explain judge a visitor's write by the same check. That includes a capability check on a realm anyone may read, whose ACL lets the visitor's check through: its write lane is judged by the gate wherever the policy opens a write to such callers.CommitBatchOptions.actingUser). It shares that user's writer lane, and that user's reads wait for it as for their own writes, so a realm's config should name a dedicated account rather than a person who edits the realm. The caller'sactorstays empty, soactor()in a program still refuses, and a grant whosewherereadsactor()never admits a visitor, even on a card naming its acting user.Where writes are admitted. card+json POST, PATCH and DELETE are tagged with their operations, and
/_operationswith any operation, so the realm admits a visitor to them exactly as it admits one to a read. card+source writes and_atomicstay ACL-only. Created cards get realm-minted ids, as they already do for any caller who can't read the realm. Every refusal a non-reader would see as not found, including a POST to a subdirectory that the destination check refuses, is the visitor's 401actor-required(#refusalSeenBy). The card-write routes now render that 401 and the count refusals with their codes.Counting. A visitor's write is counted only once the batch is admitted. The coordinator gains
beforeCommit, which runs after every entry is admitted and staged and before anything is written, and the realm counts there (#anonymousCommit)./_operationsbatch costs one unit per entry. A batch that only reads is counted once its reads have run.rate-limitedandRetry-After, and nothing is written. A count that can't be read gives 503rate-limit-unavailable.once-served(reads and search),by-handler(capability checks and writes), ornever(stylesheets).Telemetry. An admitted write's
anonymous-requestrecord carriesactingUsersand, for a batch,cost. A refusal whose grants failed on their acting user carriesactingUserFailures, so "why was my form's submission refused" points at the config key.Tests:
realm-policy-anonymous-write-test.tscovers:actor()grant never admitting a visitor;The anonymous read test moves to the new posture: an envelope create stays closed, an envelope read is now open, and explain judges a write whose acting user the realm doesn't name as denied. The card-write, capabilities, school-example, explain and gate policy tests run unchanged.
🤖 Generated with Claude Code