Skip to content

fix(node): trust gateway session attribution only - #1072

Merged
shanselman merged 7 commits into
mainfrom
stack/passport-wiring-windows-node
Aug 8, 2026
Merged

fix(node): trust gateway session attribution only#1072
shanselman merged 7 commits into
mainfrom
stack/passport-wiring-windows-node

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 30, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

The Windows node must accept session attribution only from the Gateway-stamped top-level node.invoke.request.payload.sessionKey. Caller-controlled nested command data such as args.sessionKey or params.sessionKey must never be able to forge that attribution.

Why This Change Was Made

Current Gateway runtime code sends sessionKey as an optional sibling of paramsJSON. Stable older Gateways omit it, and there is no supported feature-announcement request for this field.

This change therefore applies the contract per invocation:

  • Read only the optional top-level Gateway envelope field.
  • Treat an omitted, null, blank, or malformed field as unattributed.
  • Ignore nested sessionKey data from command arguments.
  • Preserve both event and legacy request transports.
  • Emit no unsupported post-handshake feature-publication request.

NodeInvokeRequest.SessionKey remains transport-owned through [JsonIgnore] plus an internal setter. Approval validation and system.run.prepare consume only that trusted value.

This branch was replayed onto main at 603886a81d3495c173802af864a5c71c3c880cae, preserving #1076's correlated node hello-ok response guard and immediate pre-credential handshake authorization.

User Impact

New Gateways can provide trusted session attribution. Older Gateways remain compatible by omission: their invocations execute without attribution instead of trusting nested caller data.

Validation

Exact head: b5a37b8427cecf5725544af450de210fe3246142.

  • git diff --check origin/main..HEAD: passed.
  • $env:OPENCLAW_REPO_ROOT = (Get-Location).Path; .\build.ps1: passed.
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 3,631 passed, 32 skipped.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj: 2,241 passed.
  • dotnet test .\tests\OpenClaw.WinNode.Cli.Tests\OpenClaw.WinNode.Cli.Tests.csproj: 126 passed.
  • Focused attribution/handshake/approval tests: 191 passed.
  • .\scripts\validate-mxc-e2e.ps1 without -AllowSkip: 15 passed, 0 skipped.
  • Rubber-duck review: no blocking or non-blocking findings.
  • python .\.agents\skills\autoreview\scripts\autoreview --mode branch --base origin/main --engine codex --model gpt-5.6-sol --thinking high: clean, no actionable findings, correctness confidence 0.92.

Real behavior proof

Strict Gateway-to-Windows-node MXC proof completed successfully on the current head:

Test Run Successful.
Total tests: 15
Passed: 15

The proof included real Gateway node.invoke paths for bound executable execution, sandboxed system.run, and denied writes to the isolated tray data directory.

An isolated current-head tray instance was launched in MCP-only mode with a session-scoped data directory. No real tray settings, Gateway credentials, or identities were read or modified.

winnode --list-tools
ToolCount=51
ProofTools=system.run,system.run.prepare,system.which

winnode --command system.which --params '{"bins":["git","node"]}'
git: resolved
node: resolved

winnode --command system.run.prepare --params '{"command":["where.exe","git.exe"]}'
plan.argv: ["where.exe","git.exe"]
plan.sessionKey: null

The isolated process was stopped and its generated token/data directory was deleted after proof collection.

The user's paired Gateway was not used because the strict disposable Gateway E2E proved the same real gateway-mediated invocation path without risking mutation of the user's saved profile or connection.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 7, 2026, 9:38 PM ET / August 8, 2026, 01:38 UTC.

ClawSweeper review

What this changes

The PR accepts session attribution only from Gateway invocation envelopes, ignores nested command session data, and adds transport and approval regression coverage.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

This PR remains necessary: current main still falls back to caller-controlled nested session data. The patch cleanly narrows that trust boundary, but merge should wait for direct current-head Gateway attribution proof.

Priority: P1
Reviewed head: b5a37b8427cecf5725544af450de210fe3246142

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused patch and regression coverage are strong, but direct real-path attribution evidence is still needed.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 7 items Current main still needs the fix: Current main extracts a session key from the envelope and then falls back to nested command arguments, leaving the reported trust-boundary problem present.
Patch removes the fallback at the transport boundary: The PR reads only a nonblank string session key from the Gateway envelope for both event and request invocation transports.
Approval pipeline receives transport-owned attribution: The validator carries NodeInvokeRequest.SessionKey into the validated request, which reaches the approval presentation context; nested args.sessionKey is no longer read there.
Findings None None.
Security None None.

How this fits together

The Windows node decodes Gateway invocation envelopes, passes attribution into exec approval planning, and executes node capabilities. This PR protects the envelope-to-command boundary so caller arguments cannot select an approval session.

flowchart LR
  Gateway[Gateway invocation envelope] --> Decode[Windows node decoding]
  Decode --> Trust[Trusted session attribution]
  Decode --> Args[Caller command arguments]
  Trust --> Approval[Approval and run planning]
  Args --> Capability[Capability execution]
  Approval --> Capability
  Capability --> Response[Gateway response]
Loading

Before merge

  • Resolve merge risk (P1) - The posted MXC result proves the general Gateway-to-node path but does not directly show a stamped envelope session key winning over conflicting nested data on the exact head.
  • Resolve merge risk (P1) - Gateway and node must continue to agree that request params and event payload are trusted envelope fields while omissions remain unattributed.
  • Complete next step (P2) - This MEMBER-authored PR needs maintainer review of the direct current-head attribution proof before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +19/-25, docs +1, tests +211/-11 The production trust-boundary reduction is accompanied by substantial targeted transport and approval regression coverage.

Merge-risk options

Maintainer options:

  1. Add direct attribution proof (recommended)
    Post a redacted current-head Gateway trace showing the envelope session key reaches approval planning while a conflicting nested value is ignored.
  2. Accept focused-test evidence
    Maintainers may merge on the focused transport tests and existing MXC path proof while accepting the remaining cross-version contract risk.

Technical review

Best possible solution:

Keep a single transport-owned attribution source, then attach a redacted exact-head Gateway trace proving envelope precedence and nested-value rejection before merge.

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

Yes. Send either supported Gateway envelope with conflicting envelope and nested session keys; current source and focused tests establish the expected trusted value.

Is this the best way to solve the issue?

Yes. Reading attribution only at the decoded Gateway envelope is the narrowest maintainable ownership boundary, provided both supported envelope shapes remain contractually trusted.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P1: The PR changes a trust boundary used by Windows-node command approval and execution.
  • merge-risk: 🚨 compatibility: Older and newer Gateway invocation envelopes must retain interoperable attribution behavior.
  • merge-risk: 🚨 security-boundary: Nested caller-controlled data must not influence approval-session attribution.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

What I checked:

Likely related people:

  • shanselman: Authored the final contract-alignment and attribution-proof commits on this PR. (role: recent area contributor; confidence: high; commits: 79c190d0367d, b5a37b8427ce; files: src/OpenClaw.Shared/WindowsNodeClient.cs, tests/OpenClaw.Shared.Tests/WindowsNodeClientTests.cs)
  • bkudiess: Authored the latest merged main work affecting the Windows node gateway-recovery path. (role: recent area contributor; confidence: high; commits: 603886a81d34, bc210e048aa9; files: src/OpenClaw.Shared/WindowsNodeClient.cs, tests/OpenClaw.Shared.Tests/WindowsNodeClientTests.cs)
  • vincentkoc: Has prior merged Windows node connection and pairing history in addition to the original attribution patch. (role: prior Windows-node contributor; confidence: medium; commits: 66683a33e770, 64b650cb3313; files: src/OpenClaw.Shared/WindowsNodeClient.cs)

Rank-up moves

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

  • Post a redacted exact-head Gateway invocation trace that shows envelope attribution is used and nested attribution is ignored.

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 (23 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-02T16:41:29.757Z sha 4a137f6 :: needs changes before merge. :: [P1] Accept legacy unknown-method response variants | [P1] Preserve trusted request-envelope session attribution | [P1] Dispatch cancellation before attribution negotiation
  • reviewed 2026-08-02T17:57:45.239Z sha 4a137f6 :: found issues before merge. :: [P1] Accept legacy unknown-method response variants | [P1] Preserve trusted request-envelope session attribution | [P1] Dispatch cancellation before attribution negotiation
  • reviewed 2026-08-06T14:24:05.839Z sha 44e8c78 :: needs changes before merge. :: [P1] Fail closed while session-envelope negotiation is pending
  • reviewed 2026-08-06T14:31:34.868Z sha 9d921ea :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T14:40:37.104Z sha 9c197de :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T14:47:31.870Z sha 9c197de :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T14:56:09.395Z sha 9c197de :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T23:31:44.508Z sha 8738d67 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 30, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Adversarial maintainer review found blocking protocol regressions, so this draft should not be marked ready yet:

  • Legacy negotiation only recognizes an object-shaped, exact-case INVALID_REQUEST error, while this gateway commonly returns a flat string such as unknown method: ...; the compatibility fallback is effectively unreachable.
  • The documented request transport's envelope-level params.sessionKey is gateway-stamped and should remain trusted. Only caller-controlled args.sessionKey should be ignored.
  • Request cancellation is delayed behind feature negotiation even though request invokes execute immediately, making cancellation late or ineffective; generation changes can also drop a cancel response entirely.

Please accept both string/object unknown-method errors case-insensitively, preserve trusted request-envelope attribution, and dispatch/respond to cancellation without waiting on negotiation. Current merge confidence is 35%.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 31, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 2, 2026
@vincentkoc
vincentkoc force-pushed the stack/passport-wiring-windows-node branch from 4a137f6 to 44e8c78 Compare August 6, 2026 14:19
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 6, 2026
@vincentkoc

Copy link
Copy Markdown
Member Author

Exact-head native Windows CI is terminal green at 9c197dec58bcf2656655c4dd5ea168dab2e0e234. ClawSweeper reports no findings and no security concern on that head.

The current authoring environment has no Windows/Parallels/MXC runtime lane, so the real Gateway trace remains an explicit residual compatibility risk in the PR body. I am marking the PR ready for human review, not representing that unavailable proof as completed.

@vincentkoc
vincentkoc marked this pull request as ready for review August 6, 2026 14:50
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 6, 2026
vincentkoc and others added 7 commits August 7, 2026 18:13
Remove the unsupported protocol feature publication and treat top-level Gateway envelope attribution as optional on each invocation. Preserve older Gateway compatibility by leaving unattributed calls unset while rejecting nested forgery attempts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@shanselman
shanselman force-pushed the stack/passport-wiring-windows-node branch from 8738d67 to b5a37b8 Compare August 8, 2026 01:33
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 8, 2026
@shanselman

shanselman commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Maintainer acceptance for exact head b5a37b8427cecf5725544af450de210fe3246142: I am accepting the focused transport evidence for this narrow trust-boundary change. Both supported Gateway envelope shapes are covered; top-level attribution wins over conflicting nested data, and omitted, null, blank, or malformed envelope attribution remains unset. The strict disposable Gateway MXC lane passed 15/15 and proves the real Gateway-to-node execution path. Rubber-duck and structured autoreview found no actionable defect; autoreview confidence is 0.92. No direct redacted Gateway session-attribution trace is being claimed.

@shanselman
shanselman merged commit 2a4ec81 into main Aug 8, 2026
25 checks passed
@shanselman
shanselman deleted the stack/passport-wiring-windows-node branch August 8, 2026 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

2 participants