Skip to content

fix(setup): quote gateway bind, auth mode, and reload mode - #1482

Open
SebTardif wants to merge 3 commits into
openclaw:mainfrom
SebTardif:fix/f016-quote-gateway-config
Open

SebTardif wants to merge 3 commits into
openclaw:mainfrom
SebTardif:fix/f016-quote-gateway-config

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What Problem

BuildConfigCommands wrote gateway.bind, gateway.auth.mode, and gateway.reload.mode into a bash -c script with no quoting. Extra config values were already single-quoted. The script also contained $PATH, which wsl.exe expands before bash.

Why

A setup value with a semicolon, quote, or command substitution ran in the gateway distro.

User Impact

Those three fields are now POSIX single-quoted, and the configure script is sent with inputViaStdin true.

Evidence

Head 47e860bb02ea3e2c310716da33f40443b6f6cd17.

Production ConfigureGatewayStep.BuildConfigCommands built the configure script for bind loopback, auth mode token, and reload mode hybrid;$(id); echo PWNED. CommandRunner.RunInWslAsync sent that script to wsl.exe -d Ubuntu-24.04 -- bash -s (inputViaStdin: true). A shell function named openclaw printed each argument so the real CLI was not invoked and no gateway config was changed.

wsl.exe exited 0. gateway.reload.mode arrived as one argument, hybrid;$(id); echo PWNED. There was no second line that executed PWNED. gateway.bind was loopback. gateway.auth.mode was token. The auth token argument was the fake value proof-token-not-real.

On 2026-09-25 the same quoting was sent to the real binary /root/.openclaw/bin/openclaw, OpenClaw 2026.9.6 (eb377ac). gateway.reload.mode was unset. openclaw config set gateway.reload.mode with that value as one single-quoted argument exited 1. The output had no uid= line, and the key stayed unset. openclaw config set gateway.reload.mode hybrid then openclaw config get gateway.reload.mode returned hybrid. The key was unset again afterward. Windows http://127.0.0.1:18789/ still returned HTTP 200.

The production configure script was then piped to bash -s with that same binary on PATH. It ran openclaw plugins registry --refresh and the config set lines for mode, port 18789, bind loopback, auth mode token, the existing gateway token, reload mode hybrid, the node command allowlist, and device-pair public URL plus enabled. Exit 0. Stdout contained GATEWAY_CONFIGURED. Token length stayed 48. Read-back was mode local, port 18789, bind loopback, auth mode token, reload mode hybrid, public URL http://127.0.0.1:18789, device-pair enabled true. Reload mode and the other keys that had been unset were unset again afterward. Windows http://127.0.0.1:18789/ returned HTTP 200 after the gateway restart.

Required proof pools

  • windows-wsl-gateway-e2e: the production configure script reached GATEWAY_CONFIGURED through the real OpenClaw 2026.9.6 CLI. A full setup wizard was not run.

Validation

  • Head 47e860bb02ea3e2c310716da33f40443b6f6cd17.
  • dotnet build src/OpenClaw.SetupEngine/OpenClaw.SetupEngine.csproj -p:Platform=x64 succeeded.
  • Focused ConfigureGateway_ExecuteAsyncQuotesOrRejectsHostileReloadMode: 1 passed, 0 failed.
  • ./build.ps1 exited 0.
  • Shared tests: 4093 passed, 32 skipped, 0 failed, 4125 total.
  • Tray tests: 3059 passed, 5 failed, 0 skipped, 3064 total. The 5 failures are source-contract checks that expect CRLF snippets. This checkout has LF (0 CRLF in ConnectionPage.xaml.cs). They are not the quoting change.

Real behavior proof

  • Behavior or issue addressed: bind, auth mode, and reload mode are quoted and the configure script is piped to bash -s.
  • Real environment tested: Windows, Ubuntu-24.04 under WSL, head 47e860bb02ea3e2c310716da33f40443b6f6cd17.
  • Exact steps or command run after this patch: production BuildConfigCommands plus RunInWslAsync to wsl.exe -d Ubuntu-24.04 -- bash -s. Then the same configure script piped to bash -s with /root/.openclaw/bin/openclaw on PATH, using the existing gateway token. Read the keys back, then unset the ones that had been unset.
  • Evidence after fix: the hostile value was one argv element in the earlier trace. The real CLI rejected that value with exit 1. The full script exited 0 and printed GATEWAY_CONFIGURED. Token length stayed 48. Reload mode read back as hybrid during the run and was unset again afterward.
  • Observed result after fix: $(id) did not run, and the product configure script completed against the real CLI.
  • Not verified / blocked: a full setup wizard was not run. The five Tray source-contract failures on an LF checkout are the CRLF mismatch above.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added 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. labels Sep 23, 2026
@clawsweeper

clawsweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed October 7, 2026, 12:47 PM ET / 16:47 UTC (Revision 7).

ClawSweeper review

What this changes

The setup engine quotes Gateway bind, authentication mode, and reload mode as single shell arguments and adds regression coverage.

Merge readiness

⛔ Blocked before merge - 2 items remain

The fix remains necessary: main and the latest release still emit these values without quoting. No introduced correctness defect was found, but the declared setup E2E proof requirement remains unmet.

Priority: P2
Reviewed head: 0a46fc8b3a4984f6eeb16ca6ef7d35932cbebaed

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation is focused and appears correct, with useful live evidence but an outstanding scoped integration proof gate.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Recorded Windows/WSL transport and real Gateway CLI execution substantiate the unchanged quoted commands, including hostile-value rejection and successful configuration read-back. However, the declared repository setup E2E pool and prior fresh-setup rank-up remain unfulfilled. The patch changes no stored schema, keys, defaults, or valid-value semantics. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Recorded Windows/WSL transport and real Gateway CLI execution substantiate the unchanged quoted commands, including hostile-value rejection and successful configuration read-back. However, the declared repository setup E2E pool and prior fresh-setup rank-up remain unfulfilled. The patch changes no stored schema, keys, defaults, or valid-value semantics. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Verified introduced change: The pinned main-to-head delta changes three production interpolations to the existing POSIX quoting helper and adds two tests. Stdin transport is already present on main and is not introduced by this reviewed delta.
Current main still needs quoting: Main emits Bind, AuthMode, and ReloadMode directly into shell commands. Bind is allowlisted before execution; AuthMode and ReloadMode are not allowlisted here. ExtraConfig values already use the quoting helper.
Latest release check: The v2026.9.8 source also contains the three unquoted interpolations, so the central quoting fix is not already shipped.
Findings None None.
Security None None.

How this fits together

The Windows setup engine turns Gateway settings into a script executed inside WSL. That script configures the Gateway before subsequent startup and pairing steps.

flowchart TD
 A[Setup settings] --> B[Validate configuration]
 B --> C[Quote configuration values]
 C --> D[Send script through WSL stdin]
 D --> E[Gateway configuration CLI]
 E --> F[Startup and pairing]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Recorded Windows/WSL transport and real Gateway CLI execution substantiate the unchanged quoted commands, including hostile-value rejection and successful configuration read-back. However, the declared repository setup E2E pool and prior fresh-setup rank-up remain unfulfilled. The patch changes no stored schema, keys, defaults, or valid-value semantics. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Provide the declared current-head Windows/WSL setup E2E result and refresh the required validation results. Redact private information before posting evidence; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC production +3/-3, tests +105/-1 Production size is unchanged; regression coverage supports a focused shell-quoting repair.

Technical review

Best possible solution:

Keep the focused quoting repair using the existing POSIX helper, with successful product setup integration demonstrated before landing.

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

Yes, from source: a shell-metacharacter value in Gateway.AuthMode or Gateway.ReloadMode reaches an unquoted command on main. This review did not execute that failing path.

Is this the best way to solve the issue?

Yes. Reusing the canonical POSIX quoting helper preserves valid arguments and avoids a parallel escaping implementation; setup integration proof remains incomplete.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5e3fb40bcc89.

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Recorded Windows/WSL transport and real Gateway CLI execution substantiate the unchanged quoted commands, including hostile-value rejection and successful configuration read-back. However, the declared repository setup E2E pool and prior fresh-setup rank-up remain unfulfilled. The patch changes no stored schema, keys, defaults, or valid-value semantics. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.

Label justifications:

  • P2: This is a bounded setup hardening fix with source-backed value and no evidence of an urgent active regression.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Recorded Windows/WSL transport and real Gateway CLI execution substantiate the unchanged quoted commands, including hostile-value rejection and successful configuration read-back. However, the declared repository setup E2E pool and prior fresh-setup rank-up remain unfulfilled. The patch changes no stored schema, keys, defaults, or valid-value semantics. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Verified introduced change: The pinned main-to-head delta changes three production interpolations to the existing POSIX quoting helper and adds two tests. Stdin transport is already present on main and is not introduced by this reviewed delta. (src/OpenClaw.SetupEngine/ConfigureGatewayStep.cs:128, 0a46fc8b3a49)
  • Current main still needs quoting: Main emits Bind, AuthMode, and ReloadMode directly into shell commands. Bind is allowlisted before execution; AuthMode and ReloadMode are not allowlisted here. ExtraConfig values already use the quoting helper. (src/OpenClaw.SetupEngine/ConfigureGatewayStep.cs:128, 5e3fb40bcc89)
  • Latest release check: The v2026.9.8 source also contains the three unquoted interpolations, so the central quoting fix is not already shipped. (src/OpenClaw.SetupEngine/ConfigureGatewayStep.cs, 94006b3672a0)
  • Existing quoting contract: The shared helper wraps each value and escapes embedded single quotes using the POSIX close-escape-reopen sequence, preserving literal shell metacharacters without interpreting them. (src/OpenClaw.Shared/WslShellQuoting.cs:51, 0a46fc8b3a49)
  • Focused live proof and continuity: The captured PR body, sourceRevision 5cc0572f8785006d09b0611a964dee40b470bc221165458c6b27af34c692f827, records real Windows/Ubuntu WSL transport, literal hostile argv, real OpenClaw 2026.9.6 rejection without command substitution, and successful production-script configuration with read-back. The earlier proof-head ConfigureGatewayStep blob was retrieved through REST; the quoted commands remain identical, while the transport call differs only in formatting and its explanatory comment. The body explicitly says a full setup wizard was not run. (src/OpenClaw.SetupEngine/ConfigureGatewayStep.cs:93, 47e860bb02ea)
  • Applicable setup proof gate: The selected windows-wsl-gateway-e2e pool requires the real SetupAndConnectTests product path and a nonzero test count with its result artifact. The prior concrete rank-up request for a current-head fresh setup run remains applicable; focused script execution does not establish completion of this declared pool. (.github/proof-pools.json:359, 0a46fc8b3a49)

Likely related people:

  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • karkarl: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • bkudiess: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Attach a current-head, non-skipping windows-wsl-gateway-e2e setup result with nonzero test counts and redacted evidence; retain the existing hostile-value observations.

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 (6 earlier review cycles)
  • reviewed 2026-09-23T02:23:17.854Z sha 0e6d12f :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-23T20:05:32.880Z sha 47e860b :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-25T10:54:34.847Z sha 47e860b :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-25T11:16:33.524Z sha 47e860b :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-25T19:00:13.556Z sha 47e860b :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-25T20:13:26.037Z sha 47e860b :: blocked before merge. :: none

@karkarl

karkarl commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Global triage: NEEDS_HUMAN_TEST. Take confidence 72%; recommendation confidence 88%; effort small; risk medium.

Reviewed exact head 0e6d12f3ffa8. Quoting ownership and stdin transport match the architecture ledger and WSL argv guidance. Correct the description: bind is allowlisted before this builder, so reachable unquoted values are auth mode and reload mode, especially the unvalidated ExtraConfig reload override. Add an ExecuteAsync hostile-reload test. Remaining gate is a current-head windows-wsl-gateway-e2e setup run proving configuration succeeds over stdin. The reported wsl --version decode failure is unrelated.

ExtraConfig reload mode is the unquoted value that can still reach the command. ExecuteAsync now rejects or quotes a reload mode that contains shell metacharacters.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 23, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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. and removed 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 25, 2026
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed 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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. labels Oct 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants