fix(device-pairing): guard role normalization against non-string entries - #93504
Conversation
7ea2735 to
41fca21
Compare
|
@clawsweeper review — CI is green and the PR body has live, model-free before/after proof for the device-pairing role-normalization crash (completes the merged #90654 / #92178 sibling hardening on a path they missed). Happy to address any findings. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed June 22, 2026, 2:44 AM ET / 06:44 UTC. Summary PR surface: Source -10, Tests +52. Total +42 across 2 files. Reproducibility: yes. Current Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow shared-helper repair after maintainer clearance and a green or accepted merge-gate result, keeping approval preview normalization aligned with the existing infra pairing hardening. Do we have a high-confidence way to reproduce the issue? Yes. Current Is this the best way to solve the issue? Yes. Reusing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1bd85e3cc35b. Label changesLabel justifications:
Evidence reviewedPR surface: Source -10, Tests +52. Total +42 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
|
/clownfish automerge |
|
Clownfish is on the reef for this PR. 🐠 I tagged A maintainer can call |
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
|
I think the fix would involve adjusting auth. Would that work for your use case? |
|
Clownfish 🐠 reef automerge status This repair lap finished without changing the PR. Clownfish checked the reef and found no safe patch to push this time. Target: #93504 Worker actions:
Clownfish left the PR as-is: no push, no rebase, no replacement PR, no merge, and no fresh ClawSweeper pass. fish notes: model gpt-5.5, reasoning medium. |
41fca21 to
c422956
Compare
|
Clownfish 🐠 reef update Thanks for the work here. Clownfish got this branch swimming again without needing a replacement PR. Source PR: #93504 fish notes: model gpt-5.5, reasoning medium; reviewed against b5556373c9b6. |
6cbf45c to
b555637
Compare
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
b555637 to
92f4041
Compare
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
8c02489 to
a2a0bf0
Compare
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
normalizeRoleList in src/shared/device-pairing-access.ts called .trim() on every roles[] entry and the singular role without a typeof === "string" guard, so a malformed/legacy on-disk pairing record (roles/role loaded via blind-cast JSON in coercePairingStateRecord) threw "TypeError: role.trim is not a function" and crashed resolvePendingDeviceApprovalState -- and thus `openclaw devices list`, which calls it per pending request with no try/catch. Route each item through the shared non-string-safe normalizer normalizeUniqueSingleOrTrimmedStringList, mirroring the openclaw#90654/openclaw#92178 fix that already guarded the sibling mergeRoles/mergeScopes (src/infra/device-pairing.ts) and the in-file scopes path (normalizeDeviceAuthScopes). Non-string entries are dropped; valid roles are still trimmed, deduped, and sorted. Net -10 LOC. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a2a0bf0 to
091db93
Compare
|
Clownfish 🐠 reef automerge status This pass ended as a no-op: no narrow repair surfaced, so Clownfish left the branch untouched. Target: #93504 Worker actions:
Clownfish left the PR as-is: no push, no rebase, no replacement PR, no merge, and no fresh ClawSweeper pass. fish notes: model gpt-5.5, reasoning medium. |
…ies (openclaw#93504) normalizeRoleList in src/shared/device-pairing-access.ts called .trim() on every roles[] entry and the singular role without a typeof === "string" guard, so a malformed/legacy on-disk pairing record (roles/role loaded via blind-cast JSON in coercePairingStateRecord) threw "TypeError: role.trim is not a function" and crashed resolvePendingDeviceApprovalState -- and thus `openclaw devices list`, which calls it per pending request with no try/catch. Route each item through the shared non-string-safe normalizer normalizeUniqueSingleOrTrimmedStringList, mirroring the openclaw#90654/openclaw#92178 fix that already guarded the sibling mergeRoles/mergeScopes (src/infra/device-pairing.ts) and the in-file scopes path (normalizeDeviceAuthScopes). Non-string entries are dropped; valid roles are still trimmed, deduped, and sorted. Net -10 LOC. Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
normalizeRoleListinsrc/shared/device-pairing-access.tscalled.trim()on everyroles[]entry and the singularrolewithout atypeof === "string"guard. Theroles/rolefields are loaded from disk via a blind cast (coercePairingStateRecord,src/infra/pairing-files.ts), so a malformed/legacy pairing record with a non-string entry — e.g.roles: [123]orrole: 5— threwTypeError: role.trim is not a function, crashingresolvePendingDeviceApprovalState.openclaw devices listcalls that per pending request with no surrounding try/catch (src/cli/devices-cli.runtime.ts), so the whole table render blows up on one bad record.The scopes half of the same summary (
normalizeDeviceAuthScopes) is already guarded, and the merged #90654/#92178 work added the same guard to the siblingmergeRoles/mergeScopes/formatAuditListinsrc/infra/device-pairing.ts— but missed this path.This routes each item through the existing shared non-string-safe normalizer
normalizeUniqueSingleOrTrimmedStringList(the exact helpermergeRolesuses), so non-string entries are dropped instead of crashing while valid roles are still trimmed, deduped, and sorted. Net −10 LOC; all threenormalizeRoleListcallers (pending request, approved device, token roles) are covered by the single fix.Changes
src/shared/device-pairing-access.ts— routenormalizeRoleListthrough the guarded shared helpersrc/shared/device-pairing-access.test.ts— regression tests for non-stringroles/role/token.roleReal behavior proof
Behavior addressed:
normalizeRoleListcrashed (TypeError: role.trim is not a function) on a non-string role from a malformed pairing record, taking downresolvePendingDeviceApprovalState/openclaw devices list; it now drops non-string entries and classifies access normally.Real environment tested: a direct-call harness importing the real
src/shared/device-pairing-access.ts, run with the repo'stsx, Node 22.22.1, no model / provider / channel credentials and no network (the function is pure). The "before" column reverts only that one source file toorigin/main.Exact steps or command run after this patch:
Evidence after fix:
Observed result after fix: every non-string role entry that previously threw is now dropped, valid roles are trimmed/deduped/sorted (
["admin","operator"]), and the approval classification returns normally — no crash on any of the three role paths.What was not tested: end-to-end
openclaw devices listwith a hand-crafted on-disk pairing file holding a non-string role. The pure-function repro exercises the exact crash; reachability is confirmed by source (theroles/rolefields reachnormalizeRoleListunvalidated throughcoercePairingStateRecord's blind cast, and the CLI'sparseDevicePairingListonly doesArray.isArraychecks, never element-type checks).Additional checks
Four cases in
device-pairing-access.test.tscover non-stringroles[], singularrole, andtoken.role(exercising all three callers), alongside the pre-existing valid-role and revoked-token cases. Formatter, static analysis, and type checks all pass for the changed files. Mirrors the merged #90654/#92178 robustness fix; net −10 source LOC.