fix(share-backend): publish-data hardening — revoke idempotency, hidden_turns cap#369
Merged
Merged
Conversation
…s/hidden_turns caps
Two follow-ups in the publish-data path, surfaced in the post-launch
audit.
revoke/[id] not idempotent against an already-revoked row:
The ownership SELECT had no revoked_at IS NULL clause. Re-revoking
a tombstoned share would overwrite the original revoked_at with a
newer timestamp, corrupting the audit trail AND shifting the row
outside the 7-day orphan-sweep window — leaving R2 artefacts
undeleted long after the share went 410.
Fix is fully idempotent: SELECT revoked_at FROM published_shares
WHERE id=? AND user_id=?. If not owned → 404. If owned but already
revoked → 200 with no DB/KV/R2/audit write (preserves the original
revoked_at). If owned and live → revoke as before. This shape is
important on the renderer side: the cross-device case (user revokes
on web, retries on desktop with a stale local cache) used to throw
`revoke 404` through the IPC handler; now it returns clean and the
desktop UI converges silently.
Array bounds on turns / turn_order / hidden_turns:
All three previously capped at 500 — far too tight against real-
world distribution (p99 ≈ 1,740, max observed ≈ 4,565 on actual
power-user dbs; 9% of sessions exceed 500). At the same time
hidden_turns had no cap at all, so a crafted payload could ship a
multi-million-entry array and bloat zod validation memory before
the cross-reference refine ran.
Set all three to .max(20_000): ~4× the largest real session as a
legitimate-use headroom, while still acting as a sanity guard
before MAX_SNAPSHOT_BYTES (2MB) would have rejected the payload at
the body-cap stage. The body cap remains the real defense; this is
the validator-CPU bound.
Tests:
- revoke: 200 idempotent on already-revoked + assert original
revoked_at preserved
- tests/_helpers/fakes.ts: new matcher for SELECT revoked_at FROM
published_shares so the regression exercises the production
query path
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
graydawnc
force-pushed
the
feat/share-backend-publish-data
branch
from
June 8, 2026 08:03
99c2ffa to
98129ca
Compare
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
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.
What
Two follow-ups in the publish-data path, surfaced in the post-launch audit.
Idempotent revoke
The ownership SELECT had no
revoked_at IS NULLclause. Re-revoking a tombstoned share would overwrite the originalrevoked_atwith a newer timestamp:Plus a renderer-side consequence: the IPC handler does
if (!r.ok) throw. The cross-device case (revoked on web, retried on desktop with stale local cache) used to toastrevoke 404for what was actually a successful no-op.stateDiagram-v2 [*] --> Live: publish Live --> Revoked: POST /api/revoke/:id<br/>200 + revoked_at=T0 Revoked --> Revoked: POST /api/revoke/:id (retry)<br/>200, no write<br/>(revoked_at stays T0) Live --> [*]: 410 Gone (reader path) Revoked --> [*]: 410 Gone (reader path)<br/>orphan-sweep at T0 + 7d state Revoked { [*] --> AuditTrailPreserved }Fix shape:
404 NOT_FOUND200 { ok: true }(no D1/KV/R2/audit write)The renderer's existing
if (!r.ok) throwconverges cleanly without any client-side change.Array bounds on turns / turn_order / hidden_turns
All three previously capped at 500. Measured against real-world session-length distribution on actual power-user dbs:
→ 9.2% of real sessions exceed 500 turns. 500 was far too tight; the cap was rejecting legitimate publishes.
At the same time
hidden_turnshad NO cap at all — a crafted payload could ship a multi-million-entry array and bloat zod validation memory before the cross-reference refine ran.Fix:
turns,turn_order,hidden_turns→.max(20_000)MAX_SNAPSHOT_BYTES(2 MB) would have rejected the payload at the body-cap stageVerification
200on already-revoked + assert originalrevoked_atpreservedtests/_helpers/fakes.ts: new matcher forSELECT revoked_at FROM published_sharesso the regression exercises the production query pathpnpm --filter @spool/share-backend test→ 175/175 green.Risk
Pure correctness improvement on both fronts. The 200-idempotent shape matches what most callers already expect from an unpublish action. Raising the turns cap from 500 → 20,000 unblocks ~9% of real sessions that previously failed validation; the body-bytes ceiling (2 MB) is unchanged.
Submitted by @graydawnc.