Skip to content

fix(device-pairing): validate callerScopes against resolved token scopes on repair [AI]#72925

Merged
pgondhi987 merged 2 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-523
Apr 28, 2026
Merged

fix(device-pairing): validate callerScopes against resolved token scopes on repair [AI]#72925
pgondhi987 merged 2 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-523

Conversation

@pgondhi987
Copy link
Copy Markdown
Contributor

Summary

  • Problem: approveDevicePairing() checked callerScopes only against operator scopes explicitly in pending.scopes. A re-pair request with empty/omitted scopes skipped the guard entirely, while resolveApprovedTokenScopes() independently fell back to the existing token's scopes (e.g. operator.admin), minting a fresh token with inherited high-privilege scopes.
  • Why it matters: A device session holding only operator.pairing could re-pair with pending.scopes = [] and obtain a fresh operator.admin token — violating the scope containment contract documented in docs/channels/pairing.md.
  • What changed: Moved the callerScopes guard to run after resolveApprovedTokenScopes(), so it validates the effective (resolved + inherited) scopes that will actually be minted, not just the explicitly requested scopes. Introduced a two-pass loop: validate all roles first, write tokens only if all checks pass.
  • What did NOT change: Token inheritance behavior for callers who hold the necessary scopes is unchanged. No public API or protocol surface changed.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • This PR fixes a bug or regression

Root Cause (if applicable)

  • Root cause: The callerScopes guard filtered pending.scopes for operator-prefixed entries before validating caller authority. When pending.scopes was empty, requestedOperatorScopes was empty and the guard block was never entered. Meanwhile, resolveApprovedTokenScopes() used a separate fallback chain (existingToken.scopes → approvedScopes → existing.scopes) that could return operator.admin and other high-privilege scopes without any caller authorization check.
  • Missing detection / guardrail: No test asserted the rejection path for an empty-scope re-pair when the inherited scopes exceed the caller's authority.
  • Contributing context (if known): The guard and the token resolver were written to validate different things — the guard validated the request, the resolver resolved the effective output — creating a gap when the two diverged.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: src/infra/device-pairing.test.ts
  • Scenario the test should lock in: Device with operator.admin baseline, re-pair with empty scopes, approval caller holds only operator.pairing → result is { status: "forbidden", reason: "caller-missing-scope", scope: "operator.admin" } and the existing token is unchanged.
  • Why this is the smallest reliable guardrail: The unit test exercises the exact vulnerable code path in isolation without requiring gateway or network setup.
  • Existing test that already covers this (if any): None — the gap was untested.
  • If no new test is added, why not: N/A — test is added.

User-visible / Behavior Changes

Re-pair approval requests with empty scopes are now rejected with { status: "forbidden", reason: "caller-missing-scope" } when the caller's callerScopes do not cover the device's inherited operator scopes. Previously these were silently approved. Callers that already hold sufficient scopes to cover the inherited baseline are unaffected.

Diagram (if applicable)

Before:
[repair, scopes=[]] -> [callerScopes guard: skip (no requested scopes)] -> [token minted with operator.admin]

After:
[repair, scopes=[]] -> [resolveApprovedTokenScopes → ["operator.admin",...]] -> [callerScopes guard: check] -> [forbidden if caller lacks operator.admin]

Security Impact (required)

  • New permissions/capabilities? No
  • Secrets/tokens handling changed? Yes — token minting for operator role now requires caller authorization even for empty-scope re-pair requests
  • New/changed network calls? No
  • Command/tool execution surface changed? No
  • Data access scope changed? No
  • Risk + mitigation: The fix tightens an authorization check; it does not widen any surface. Callers who previously relied on the bypass (whether inadvertently or not) will receive a forbidden result. No legitimate caller should be affected since the guard was always intended to require caller authority for operator scope minting.

Repro + Verification

Environment

  • OS: Linux (CI)
  • Runtime/container: Node 22
  • Model/provider: N/A
  • Integration/channel (if any): N/A
  • Relevant config (redacted): default

Steps

  1. Set up a paired device with operator.admin scope.
  2. Issue a re-pair request for the same device with no scopes (pending.scopes = undefined).
  3. Call approveDevicePairing(requestId, { callerScopes: ["operator.pairing"] }).

Expected

  • Returns { status: "forbidden", reason: "caller-missing-scope", scope: "operator.admin" }.
  • Existing token value and scopes are unchanged.

Actual (before fix)

  • Returned { status: "approved" } and minted a fresh token with ["operator.admin", "operator.read", "operator.write"].

Evidence

  • Failing test/log before + passing after — new regression test in src/infra/device-pairing.test.ts ("rejects repair without requested scopes when caller cannot approve inherited token scopes") was written to reflect the vulnerable behavior and passes after the fix.

Human Verification (required)

AI-assisted PR — generated by OpenAI Codex with no human code modifications. Reviewed by Claude (claude-sonnet-4-6) prior to submission.

  • Verified scenarios: Rejection path (caller lacks operator.admin, repair with empty scopes); approval path (caller holds operator.admin, repair with empty scopes — token preserved correctly per existing test at line 593).
  • Edge cases checked: Two-pass loop ensures no partial token writes on forbidden result; caller-scopes-required path (no callerScopes provided at all) now also fires for inherited operator scopes.
  • What you did not verify: Live gateway RPC roundtrip; multi-role device approval scenarios beyond the unit test.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? Yes — returns forbidden where previously it returned approved for the vulnerable path. Callers with correct scopes are unaffected.
  • Config/env changes? No
  • Migration needed? No

Risks and Mitigations

  • Risk: Legitimate re-pair flows that relied on empty-scope inheritance without passing callerScopes will now fail.
    • Mitigation: The callerScopes parameter was always required for operator scope approval (per the existing caller-scopes-required guard). The only new enforcement is that it also applies when scopes are inherited rather than explicitly requested.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Apr 27, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 27, 2026

Greptile Summary

This PR fixes a scope-containment bypass in approveDevicePairing() where a re-pair request with empty/omitted scopes skipped the callerScopes guard entirely, allowing a low-privilege caller (operator.pairing) to inherit high-privilege scopes (operator.admin) from the existing token via resolveApprovedTokenScopes(). The fix moves the guard to run after scope resolution and introduces a two-pass loop to prevent partial token writes on a forbidden result.

Confidence Score: 5/5

Safe to merge — the fix correctly closes the scope-inheritance bypass without altering any legitimate approval path.

The logic change is minimal and targeted: the caller-scope guard is relocated after resolveApprovedTokenScopes() and the two-pass loop ensures atomicity. Both the regression test (forbidden path) and the existing passing test (caller holds sufficient scope) exercise the key branches. No new surface is exposed and no prior auth checks are removed.

No files require special attention.

Reviews (1): Last reviewed commit: "fix: address issue" | Re-trigger Greptile

@pgondhi987
Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@pgondhi987 pgondhi987 merged commit 189c91e into openclaw:main Apr 28, 2026
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026
…pes on repair [AI] (openclaw#72925)

* fix: address issue

* docs: add changelog entry for PR merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer Maintainer-authored PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant