Skip to content

fix(worker): harden device token revalidation - #1206

Merged
steipete merged 2 commits into
mainfrom
codex/1200-hardening
Jul 29, 2026
Merged

fix(worker): harden device token revalidation#1206
steipete merged 2 commits into
mainfrom
codex/1200-hardening

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Summary

  • cache successful GitHub account and org/team revalidation for 60 seconds per exact device ID and verifier hash, with a 1,024-entry in-memory bound
  • keep durable device-token lookup, token/grant expiry, sealed-grant opening, revoked-user policy, allowed-org policy, and lease visibility checks on every request; successful revocation also evicts the matching cache entry
  • return pairing_reauth_required when the stored grant expires, cannot be opened, or GitHub rejects its credential, while device revocation remains device_token_invalid
  • document the accepted membership-removal window and threat model, and add regression coverage for cache hits/expiry, fail-closed refresh, revocation precedence, reauth, and cross-token isolation

Implements #1200.

Verification

npm ci --prefix worker
npm run format:check --prefix worker
npm run lint --prefix worker
npm run check --prefix worker
npm test --prefix worker
npm run build --prefix worker

Test Files  36 passed | 2 skipped (38)
Tests       1131 passed | 3 skipped (1134)
wrangler deploy --dry-run: success
gofmt -w $(git ls-files '*.go')
go vet ./...
scripts/check-docs.sh

go vet: pass
checked 238 markdown files: internal links ok
generated docs tests: 14 passed

The full Go race fan-out ran on Blacksmith Testbox. Every package outside the Testbox shared-filesystem permission fixture passed. The two affected packages were then rerun locally on the same commit with native filesystem semantics:

GOFLAGS=-p=2 go test -race ./internal/cli ./internal/providers/external

ok  github.com/openclaw/crabbox/internal/cli                 227.218s
ok  github.com/openclaw/crabbox/internal/providers/external   8.000s

The real CLI also built and ran:

go build -trimpath -o bin/crabbox ./cmd/crabbox
bin/crabbox --version

0.15.0

Local Wrangler HTTP proof

The Worker ran under local Wrangler and used a real active GitHub/org membership. All credentials remained off argv and were redacted from output.

github_identity=verified_active_org_member
pairing_exchange=200
device_request_1=200 observed_github_delta=1
device_request_2=200 observed_github_delta=0
device_revoke=204
revoked_device_request=401 error=device_token_invalid
expired_grant_device_request=401 error=pairing_reauth_required

Source-blind behavior validation passed all four contract clauses: repeated-request caching, immediate warm-token revocation, distinct expired-grant reauthentication, and credential-redacted evidence. Autoreview was clean with no accepted/actionable findings.

Security and configuration

The stored sealed OAuth credential model from device pairing is unchanged. No new secrets or configuration are required. The explicit tradeoff is that removed GitHub org/team membership can remain readable for at most about 60 seconds; lookup failures after cache expiry fail closed, and explicit device-token revocation remains immediate.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 29, 2026
@clawsweeper

clawsweeper Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed July 29, 2026, 1:11 PM ET / 17:11 UTC.

ClawSweeper review

What this changes

The Worker caches successful device-token GitHub account and organization/team revalidation for 60 seconds, distinguishes expired or unusable pairing grants from revoked device tokens, and adds documentation and regression coverage.

Merge readiness

⚠️ Ready for maintainer review - 6 items remain

Keep this PR open for an explicit maintainer decision on the authorization boundary: successful remote GitHub membership checks become reusable for 60 seconds, so a remote membership removal is not observed immediately even though local revocation and failed refreshes remain immediate/fail-closed.

Priority: P2
Reviewed head: 00badb61b0abe6a02b1ed65d522f8e0f142ef90c
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Strong real behavior evidence and targeted regression coverage support the implementation, while the intentional authorization-window change still needs maintainer acceptance.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The PR body supplies after-fix local Wrangler HTTP output against an active GitHub organization membership, covering cache reuse, revocation, and reauthentication with credentials redacted.
Patch quality 🐚 platinum hermit (4/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body supplies after-fix local Wrangler HTTP output against an active GitHub organization membership, covering cache reuse, revocation, and reauthentication with credentials redacted.
Evidence reviewed 5 items Authorization-boundary change: The PR adds a 60-second, 1,024-entry device-membership cache in the fleet coordinator while retaining durable device lookup and local policy checks for each request; this is the central security tradeoff requiring approval.
Fail-closed and reauthentication behavior: The auth change separates malformed authorization data from expired, unopened, or rejected stored OAuth credentials, mapping the latter to re-pairing rather than treating a revoked device token as the same failure.
Documented threat model: The pairing documentation explicitly records the bounded remote-membership removal window and says refresh failures are fail-closed while local revoked-user and allowed-organization policy remains current.
Findings 1 actionable finding [P3] Remove the release-owned changelog entry
Security Needs attention Approve the remote-membership staleness window: A successful GitHub account and organization/team check can be reused for 60 seconds, so a remote membership removal is not observed until cache expiry; local device revocation and local policy checks remain immediate.

How this fits together

Crabbox’s Worker authenticates device-token requests before exposing lease data and coordinator operations. It combines durable device-token state and local policy with GitHub account and membership checks, then authorizes the request to the fleet coordinator.

flowchart LR
  A[Device token request] --> B[Durable token and grant checks]
  B --> C[Local revoked-user and org policy]
  C --> D{Recent successful membership check?}
  D -->|Yes, under 60 seconds| E[Reuse bounded authorization result]
  D -->|No or expired| F[GitHub account and membership revalidation]
  F --> G[Authorize lease and coordinator access]
  E --> G
  F --> H[Reauth or deny response]
Loading

Decision needed

Question Recommendation
Should Crabbox accept a maximum 60-second delay in observing remote GitHub organization/team membership removal in exchange for reducing per-request GitHub revalidation and rate-limit amplification? Accept the 60-second bounded window: Approve the documented tradeoff, retaining immediate device and local-policy revocation plus fail-closed behavior on the next remote refresh.

Why: This PR intentionally changes when a remote authorization revocation becomes effective; source tests can establish the bounded behavior but cannot select the repository’s acceptable authorization window.

Before merge

  • Remove the release-owned changelog entry (P3) - Keep this behavior description in the PR body or commit message instead. CHANGELOG.md is release-owned, so a normal PR should not pre-author the unreleased release note.
  • Resolve security concern: Approve the remote-membership staleness window - A successful GitHub account and organization/team check can be reused for 60 seconds, so a remote membership removal is not observed until cache expiry; local device revocation and local policy checks remain immediate.
  • Resolve merge risk (P1) - A GitHub organization or team membership removal can remain authorized for up to 60 seconds after a successful validation; this is a deliberate authorization-boundary relaxation that green tests cannot approve on maintainers’ behalf.
  • Resolve merge risk (P1) - The branch also edits the release-owned CHANGELOG.md; that entry should be left to the release process rather than included in a normal feature/fix PR.
  • Complete next step (P2) - A human must accept or reject the intentional 60-second remote-membership authorization window; no safe automated repair can make that policy choice.

Findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:13
  • [medium] Approve the remote-membership staleness window — worker/src/fleet.ts:348
Agent review details

Security

Needs attention: The patch does not add a new secret path or third-party dependency, but it deliberately changes the remote authorization freshness boundary and needs explicit approval.

Review metrics

Metric Value Why it matters
Patch scope 6 files affected; 421 added, 32 removed The focused Worker/auth/docs/test surface makes the authorization-policy decision more important than broad implementation churn.
Authorization cache 60-second TTL; 1,024-entry bound These values define the maximum stated remote-membership staleness and in-memory cache scope.

Merge-risk options

Maintainer options:

  1. Approve the bounded authorization window (recommended)
    Accept the documented 60-second membership-removal delay with the PR’s immediate local revocation and fail-closed refresh safeguards.
  2. Preserve immediate remote removal
    Remove the successful-membership cache and keep a fresh GitHub account and membership check on every device request.
  3. Pause for security-policy direction
    Hold this PR until maintainers choose and document the acceptable remote-revocation latency for device tokens.

Technical review

Best possible solution:

Obtain explicit maintainer approval for the documented 60-second stale-membership window, retain the immediate local revocation and fail-closed refresh semantics, and remove the release-owned changelog entry before landing.

Do we have a high-confidence way to reproduce the issue?

Yes, for the proposed behavior: the PR provides a local Wrangler HTTP run showing one GitHub validation on the first request, a cache hit on the second, immediate device revocation, and expired-grant reauthentication. The policy question is not a failing reproduction; it is whether the 60-second authorization window is acceptable.

Is this the best way to solve the issue?

Unclear until maintainers choose the remote-revocation latency. The cache is narrowly scoped and documented, but making a positive remote authorization result reusable is a security-policy choice rather than a mechanically provable bug fix.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:13
    Keep this behavior description in the PR body or commit message instead. CHANGELOG.md is release-owned, so a normal PR should not pre-author the unreleased release note.
    Confidence: 0.96

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f7c915578a6c.

Labels

Label justifications:

  • P2: This is a bounded device-token authorization hardening change with an explicit security-policy tradeoff, not evidence of an active bypass or outage.
  • merge-risk: 🚨 security-boundary: Merging intentionally permits a successful remote GitHub membership check to remain effective for up to 60 seconds after remote membership changes.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body supplies after-fix local Wrangler HTTP output against an active GitHub organization membership, covering cache reuse, revocation, and reauthentication with credentials redacted.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix local Wrangler HTTP output against an active GitHub organization membership, covering cache reuse, revocation, and reauthentication with credentials redacted.

Evidence

Security concerns:

  • [medium] Approve the remote-membership staleness window — worker/src/fleet.ts:348
    A successful GitHub account and organization/team check can be reused for 60 seconds, so a remote membership removal is not observed until cache expiry; local device revocation and local policy checks remain immediate.
    Confidence: 0.99

What I checked:

  • Authorization-boundary change: The PR adds a 60-second, 1,024-entry device-membership cache in the fleet coordinator while retaining durable device lookup and local policy checks for each request; this is the central security tradeoff requiring approval. (worker/src/fleet.ts:348, 00badb61b0ab)
  • Fail-closed and reauthentication behavior: The auth change separates malformed authorization data from expired, unopened, or rejected stored OAuth credentials, mapping the latter to re-pairing rather than treating a revoked device token as the same failure. (worker/src/auth.ts:426, 00badb61b0ab)
  • Documented threat model: The pairing documentation explicitly records the bounded remote-membership removal window and says refresh failures are fail-closed while local revoked-user and allowed-organization policy remains current. (docs/features/device-pairing.md:70, 00badb61b0ab)
  • Behavior coverage and live proof: The PR adds cache-hit/expiry, revocation-precedence, reauthentication, and token-isolation coverage, and its body reports a local Wrangler run against an active GitHub organization membership with redacted credentials. (worker/test/pairing.test.ts:1, 00badb61b0ab)
  • Related canonical security work: This PR explicitly implements the still-open device-token security follow-up, which calls for bounded membership-check caching and a distinct re-pairing outcome before broad iOS-client use. (00badb61b0ab)

Likely related people:

  • steipete: Authored the linked device-token follow-up and both commits in this focused pairing and GitHub-membership hardening series; no broader current-main ownership trail was available in the supplied review evidence. (role: recent adjacent contributor; confidence: low; commits: cf9f8b74a441, 00badb61b0ab; files: worker/src/auth.ts, worker/src/fleet.ts, worker/src/github-membership.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain an explicit maintainer decision on the 60-second remote-membership removal window.
  • Remove the release-owned CHANGELOG.md entry.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-29T16:40:15.893Z sha cf9f8b7 :: needs maintainer review before merge. :: none

@steipete
steipete enabled auto-merge (squash) July 29, 2026 17:08
@steipete
steipete merged commit e31157d into main Jul 29, 2026
20 of 22 checks passed
@steipete
steipete deleted the codex/1200-hardening branch July 29, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant