feat: bootstrap gatekeeper-kit leaf modules by ndisidore · Pull Request #392 · cloudflare/cloudflare-os

ndisidore · 2026-08-31T00:50:15Z

Adds @gadgets/gatekeeper-kit: fourteen standalone leaf modules that factor out the plumbing every gatekeeper re-implements around its service-specific parts. The package is a library and each module takes the narrowest KV surface it needs and works à la carte, with no dependency on the assembly layer that will sit on top.

docs/gatekeeper-kit.md carries the module-by-module rationale, the implementation plan, and future Layer 2 work for the assembly spec itself

Modules

Connect flow: nonce minting, constant-time comparison, and the initiation → OAuth handshake as atomic KV transitions
Connect pages: the shared close-window, expired-link, and error pages, with escaping and response headers fixed once
Credentials: identity-fenced storage, coalesced skew-aware refresh, and the consumer cache that reports a dead grant
Credential expiry: arm-fenced notify-once latch that survives eviction and retries after a failed callback
Observers: four verification strategies, a batched-access tracker, and the prepare → authorize → commit gate
Actions: two-tier journal, declarative apply/reject dispatch, serialized resolution, one post-apply write
Simulation: frozen views over pending records, replay that stops at the first unsupported step, provisional IDs that refuse a conflicting rebind
Cursors, cache, serial queue, HTTP errors: RpcTarget cursors with serialized paging, generation-keyed TTL cache, the FIFO gate both need, numeric 401/403/404 classification

github-actions · 2026-08-31T00:52:20Z

Preview: `pr392-nathan-gatekeeper-kit`

https://pr392-nathan-gatekeeper-kit-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

ask-bonk · 2026-08-31T21:34:50Z

Submitted 7 actionable inline findings.

github run

ask-bonk · 2026-09-01T12:00:51Z

Submitted 3 actionable inline findings. Tests were not run because pnpm is unavailable.

github run

ask-bonk · 2026-09-01T16:13:34Z

@ndisidore Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

ask-bonk · 2026-09-01T16:29:17Z

+
+/** A `Set-Cookie` value binding the OAuth callback to the browser that began the redirect. */
+export function oauthBrowserCookie(nonce: string): string {
+  return `${requireOAuthCookieName(nonce)}=1; Path=/; Max-Age=${OAUTH_COOKIE_MAX_AGE}; ${OAUTH_COOKIE_SECURITY}`;


High: This does not actually bind the callback to the initiating browser. The cookie name is derived from the OAuth nonce exposed as state, and its value is always 1, so a recipient of the copied provider URL can obtain the callback code/state and send it with the computable Cookie header. That links the recipient provider identity to the initiating Workshop account despite the protection described in the design doc. Persist an independent random cookie secret with the OAuth-stage record and verify that secret at callback time.

ask-bonk · 2026-09-01T16:29:17Z

+   */
+  constructor(kv: CacheKv, authorityId: string) {
+    this.#kv = kv;
+    this.#authority = authorityId;


High: Freezing the authority for the lifetime of this cache does not provide the claimed reconnect partitioning. If an account reconnects from principal A to B while the facet/session remains alive, line 58 can still serve an A entry; after a miss, a loader using current B credentials is instead stamped as A. The separate-instance test cannot exercise this. Make authority an input to each load, and include it in the in-flight-load key, or otherwise rotate the live cache when credential identity changes.

ask-bonk · 2026-09-01T16:29:17Z

+      // TypeScript cannot correlate a tagged union's payload with its definition, so the dispatcher
+      // is the one place that erases the payload type.
+      const definitionFor = (entry: TaggedAction<M>) =>
+        definitions[entry.kind] as ActionDefinition<unknown, Host> | undefined;


Medium: Indexing a normal object accepts inherited prototype names as definitions. A stored stale kind such as constructor resolves to Object; its inherited apply function can return successfully, after which this action is removed as applied without any provider handler running. toString has the same issue. Require an own property before returning the definition so these records take the intended unsupported-kind path.

ask-bonk · 2026-09-01T16:29:17Z

+      const state = this.#coerce(raw)?.state;
+      if (state === undefined) continue;
+      const id = this.#idFrom(key);
+      if (state === "staged" || state === "failed") {


Medium: A terminal apply failure still has a pending Workshop action until the user rejects it, but this pruning removes the only stable failure record for that action. A later approval becomes Unknown pending action, and rejection succeeds as if there were no diagnostic, potentially hiding an ActionApplyError warning about partial external effects. Failed records are user-clearable through reject, so count them toward the bounded unresolved set rather than deleting records the Workshop still exposes.

ask-bonk · 2026-09-01T16:29:17Z

+   */
+  describe(payload: Payload, host: Host): ActionPresentation | Promise<ActionPresentation>;
+  /** Provisional references this payload creates, for actions a later one can depend on. */
+  provides?(payload: Payload): Iterable<string>;


Medium: string itself satisfies Iterable<string>, so the natural and type-correct provides: payload => payload.ref is spread into individual characters at line 311. A dependency on ~12 is then not cascaded, while unrelated references sharing a character can be cascaded incorrectly. Exclude a bare string from this callback type or normalize it as one reference; the same applies to dependsOn.

ask-bonk · 2026-09-01T16:29:17Z

+ */
+export function createSimulationView<R extends SimulationRecord<unknown>, Target>(
+  records: readonly R[],
+  targets: (action: R["action"]) => Iterable<Target>,


Medium: When Target is string, a type-correct extractor such as action => action.target returns an iterable and this loop indexes each character instead of the target. forTarget("page-123") then silently omits its pending actions, making simulated reads stale. Require a collection type that excludes bare strings or explicitly treat a string result as one target.

ask-bonk · 2026-09-01T16:29:24Z

Submitted 6 actionable inline findings.

github run

New workspace library @gadgets/gatekeeper-kit holding the provider-independent pieces every gatekeeper hand-rolls today. This first cut lands the connect-flow leaves: nonce primitives, the two-stage handshake, the shared connect pages, the HTTP no-access classifier, the simulation substrate, and the expiry latch. The latch differs from its prior art: it is set only after the callback RPC resolves, so a crash mid-notify re-notifies later instead of silencing every future expiry, and concurrent callers share one in-flight notification. Tests run as two vitest projects: Node for pure logic, workerd for the modules that need crypto.subtle.timingSafeEqual.

Four strategies cover the taxonomy every gatekeeper picks from -- private, ACL, tracked-set, open -- behind one ObserverStrategy seam, plus ObservationGate, which folds a strategy's exclusions into the approval-queue authorization and promotes newly-revealed sets only once the overseer has agreed to hide them. ObserverTracker extends the version it came from in the two ways real gatekeepers need: the observer and observed-set key families are named separately (so a port keeps both of its existing prefixes), and the ACL oracle is batched -- one provider call per admission round rather than per set. A short or ragged oracle result denies. legacyObservedValue honors a stored `true` as "observed", and denyMessage names the failing set.

CredentialCoordinator owns the account DO's credential record: skew-aware refresh, concurrent callers coalesced onto one provider round-trip, and an identity fence so a reconnect or revoke landing mid-refresh is never clobbered by the older refresh's result. CredentialsExpiredError is the one signal that means the grant itself died; anything else propagates with credentials intact. CredentialSource is the facet/verifier side: a short instance-local cache over the account DO, and the single place a provider auth failure becomes an expiry notification.

ActionJournal owns the queued-action record: sequential ids, the staged -> pending lifecycle, and listPending() as the input createSimulationView expects. It is two-tier -- a retained applied record moves out of the pending prefix to a sibling one, so the pending scan stays bounded by genuinely pending records however many applied ones accumulate (the shape github already uses). Lookups check both tiers and never filter by state: the output gate commits the staged record before the submitAction RPC can leave, so a record still marked staged may already be pending for the overseer. Retiring the retained tier is consumer policy -- retention is unbounded and caps are per-vendor. defineActions turns per-kind apply/reject handlers into the overseer's callbacks. An apply returns its apply-time artifacts, which the kit persists in one write together with the state transition, so handlers never touch the journal mid-apply and there is exactly one writer. An apply that throws leaves the record for a retry and reports "failed" -- a partial provider effect is when caches are most stale. afterResolve fires once per resolution because every gatekeeper invalidates caches after one and the big ones repeat it per branch, where a forgotten branch is a silent stale read. Revert is deliberately absent. Reject's variance lives inside a handler body, which dispatch absorbs; revert's variance lives in record lifecycle, which it cannot -- five gatekeepers have five incompatible revert/retention behaviours, so revert belongs at the facet seam as ordinary consumer TypeScript. Apply is at-least-once: the provider call can succeed and the process crash before the journal write, and the overseer's retry re-applies. No gatekeeper solves this today; the kit documents it rather than pretending otherwise. SerialTaskQueue and displayReason come from the applyActionsThrough work, so the batch contract can layer onto this later without reshaping the journal.

KvTtlCache holds stable provider metadata in the facet's own storage, keyed within a generation the caller bumps when an applied action may have invalidated everything at once -- cheaper and more complete than tracking which entries a write touched.

StreamingCursor fetches provider pages lazily, overlays simulation onto each item, and merges simulation-only items at their sort position, so a resource with a long history returns its first page without reading all of them. Pages emptied by the filter keep it fetching rather than reporting the end early. Both extend RpcTarget undecorated; a consumer subclasses and decorates when it wants validated cursor calls.

Deployable discovery keys on wrangler.jsonc alone (readDeployablePackages), not on the package name, so gatekeeper-kit is already invisible to the release pipeline, the dev server and preview configs. AGENTS.md said otherwise -- "Each gatekeeper runs as a separate Cloudflare Worker" -- which misleads a reader into treating the kit as deployable, or into adding a wrangler.jsonc to make it "work". Doing that would make it deployable and, because workerKind classifies by prefix, a gatekeeper: shortName "kit", a routed BASE_URL, and -- since it is absent from NO_DEFAULT_CRED_INPUTS -- a wizard demanding CLIENT_ID and CLIENT_SECRET before anyone could install the instance. CI already caught that, but only as three "missing fixture bundle" failures that name the symptom rather than the cause, so pin the invariant where it can say so directly. The internal repo's gatekeeper-shared is the same shape, so this documents an existing convention rather than inventing one.

Records the design behind the Layer 1 modules that just landed, in the docs/ directory alongside the other design docs. The plan drove the implementation, so committing it puts the reasoning next to the code rather than leaving it in a scratch file. Reconciled against the shipped signatures before committing, since a tracked doc reads as authoritative where an untracked one does not: * 4.6 credentials: the upgrade() contract returns { credentials, legacyKeys } and reads only -- the coordinator performs the deletes. Documents the write order in commit()/clear() and why the fence goes first. * 4.7 observers: aclObservers takes hasAccess() (answering, and only a literal true admits) rather than a throwing verify(); adds the canonicalSetId, maxTrackedSets and concurrency options, and observerIds() on the tracker and strategy. * 4.9 simulation: isProvisional is a constructor option, not a method. Corrects the binding key to `${namespace}prov:${id}`. * 4.10 cache: the API is cached(key, ttlMs, load); there is no public get/put pair, and the generation fence is internal rather than the caller's obligation. Also adds a status header (Layer 1 landed, Layer 2 still proposal) and the 4.8 key-layout tables a port needs: the counter convention is next-unused in 9 of 12 gatekeepers but last-issued in github, linear and spotify, where adopting the existing counter key would re-issue the last ID; and the retained tier's derived prefix matches no gatekeeper's existing keys.

ask-bonk · 2026-09-01T16:58:26Z

+  try {
+    await queue.submitAction(id, description);
+  } catch (error) {
+    journal.rollbackSubmission(id);


Medium: An RPC rejection does not prove that submitAction() failed before committing. If the Overseer stores the action and its response is then lost, this rollback deletes the gatekeeper record while the Workshop retains a pending action. Approval subsequently fails with Unknown pending action, and a retry submits a different ID. Preserve/reconcile the staged record on an ambiguous RPC failure rather than assuming rejection means non-delivery.

ask-bonk · 2026-09-01T16:58:26Z

+      return {
+        submit: async (queue, kind, payload) => {
+          const definition = definitions[kind];
+          const { title, description, implementsRevert } = await definition.describe(payload, host);


Medium: The payload is described before the journal snapshots it. Because describe() may await and Payload is mutable, it can derive text from one value while the caller or hook mutates that same object before stageAction() stores it, leaving the approver text different from what apply() sends. Snapshot the payload first and describe the exact stored snapshot, as the API contract promises.

ask-bonk · 2026-09-01T16:58:26Z

+    const current = this.#connected();
+    const expiresAt = this.#options.expiresAt?.(current);
+    const skew = this.#options.refreshSkewMs ?? ACCESS_TOKEN_SAFETY_MS;
+    if (expiresAt === undefined || Date.now() < expiresAt - skew) return current;


Medium: expiresAt() is documented to return a finite epoch, but that is not enforced here. Provider JSON such as 1e400 parses to Infinity; this comparison then treats the access token as permanently fresh, so the refresh grant is never used and eventual auth failure can incorrectly retire a recoverable connection. Reject a non-finite expiry instead of treating it as non-expiring (undefined already represents that case).

ask-bonk · 2026-09-01T16:58:26Z

+    const nonceKey = `${OBSERVER_NONCE_PREFIX}${id}`;
+    const nonce = generateNonce();
+    // Both writes before the first await, so no read can observe the attempt without its nonce.
+    kv.put(attemptKey, verifier);


Medium: This admission attempt can become permanent if the activation dies after the write, or if this write succeeds and the nonce write throws. The Overseer has not persisted observerId yet, so its next open generates a different ID and cannot rotate/remove this orphan. Orphans are enumerated as active observers and count toward maxObservers; enough interrupted admissions permanently block new collaborators. Make attempts reclaimable rather than relying on a retry of the same ID.

ask-bonk · 2026-09-01T16:58:27Z

+  response: Response, maxBytes: number = MAX_RESPONSE_BYTES,
+): Promise<string> {
+  const advertised = Number(response.headers.get("content-length"));
+  if (Number.isFinite(advertised) && advertised > maxBytes) {


Medium: maxBytes is never validated. Passing NaN or Infinity makes both this check and the running-total check false for every body size, silently disabling the memory cap that this helper is meant to enforce. Reject non-finite/invalid limits (the package already has requirePositiveInt) before reading the response.

ask-bonk · 2026-09-01T16:58:42Z

Submitted 5 actionable inline findings.

github run

github-actions Bot added delivery Changes to CI or release delivery gatekeeper Changes to a gatekeeper integration labels Aug 31, 2026