Skip to content

fix(setup): reject an invalid Linux user before wizard WSL commands - #1488

Open
SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f026-invalid-linux-user
Open

SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f026-invalid-linux-user

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What Problem

--wizard-only does not run ConfigureWslInstanceStep, which was previously the only caller of WslConfig.IsValidLinuxUserName. The wizard path could therefore reach operator pairing and WSL reload commands with an invalid configured Linux user. The old username regex also accepted a single trailing newline because it used $ rather than an absolute end anchor.

What Changed

  • Reject an invalid WSL user before registry access, operator pairing, or any wizard WSL command.
  • Send reload-mode scripts through bash -s stdin so wsl.exe does not rewrite $PATH in argv.
  • Make Linux username validation null-safe and use absolute \A...\z anchors.
  • Cover hostile shell input, JSON null, and trailing-newline input.
  • Preserve contributor history. No force-push or history rewrite was used.

Required proof pools

  • windows-wsl-gateway-e2e: wizard-only WSL username handling and the valid-user product setup path changed.

Validation

PR head: f4006f0c957863948089b10e15e60e56aa516c83 (empty validation refresh commit on the contributor branch).

Locally integrated merge candidate: tree 3c7a7428510151d6f82553ac50fee2d8a4001ecb, combining the PR head with current main 60e978c594e582586c1e27e8624d1073294622cb, which includes #1507 (fix(setup): retry guarded restart intent contention).

  • Focused invalid-user plus restart-recovery tests: 65 passed, 0 failed.
  • ./build.ps1: passed.
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 4,145 passed, 33 skipped, 0 failed.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 3,180 passed, 0 failed.
  • dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore: 1,393 passed, 1 skipped, 0 failed.
  • Rubber-duck review: no blocking findings.
  • Integrated local exact setup-connect shard with task-owned process TEMP/TMP on D:\: 32 passed, 13 failed, 1 skipped. Both real setup fixtures passed WSL preflight, operator pairing, node pairing, and Gateway wizard completed; fix(setup): retry guarded restart intent contention #1507's restart-intent contention no longer failed setup. Later failures were host tray/MCP connection timeouts and disconnected-state fanout. Cleanup completed, no test distro remained, and the task-owned temp directory was deleted.

Real behavior proof

The real OpenClaw.SetupEngine.Program.Main wizard-only path was invoked with its production CommandRunner, not a fake runner.

Configured user Result WSL command records
bad$(id) FailedTerminal, exit 1 0
JSON null FailedTerminal, exit 1 0
openclaw\n FailedTerminal, exit 1 0

Each run stopped at run-wizard with the actionable Invalid WSL user diagnostic before registry loading, pairing, or WSL execution.

The integrated valid-user setup proof installed fresh WSL distros using the default openclaw user, completed operator and node pairing, completed the Gateway wizard, restored reload mode through stdin, exercised #1507's guarded restart-intent retry, and reached post-setup tray/MCP testing.

Publication blocker

Not merged yet. A clean two-parent merge with current main exists locally as commit 56504a32f2c0d7b1c0aac428962b22f450adfee0 (parents f4006f0c and 60e978c5; tree 3c7a7428). Direct push is rejected because this OAuth token lacks workflow scope and current main changes .github/workflows/ci.yml. GitHub's update-branch REST endpoint returns HTTP 422 merge conflict, while local git merge-tree --write-tree succeeds with no conflict.

A workflow-authorized maintainer must push that prepared merge commit to SebTardif's original fix/f026-invalid-linux-user branch, or perform the equivalent normal merge in GitHub's web interface. Then required PR CI can run against current main; merge only if it is green.

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: blocked before merge. Reviewed September 28, 2026, 3:51 PM ET / 19:51 UTC (Revision 11).

ClawSweeper review

What this changes

The branch validates the configured Linux username before setup-wizard pairing or WSL commands, sends reload scripts through WSL standard input, and adds invalid-input regression tests.

Merge readiness

⛔ Blocked before merge - 2 items remain

Keep this PR open. Current main handles the WSL stdin path but still lacks the wizard’s early invalid-user check. The proposed guard has direct behavior proof; the remaining blocker is integrating the head with current main and validating that result.

Priority: P2
Reviewed head: f4006f0c957863948089b10e15e60e56aa516c83

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Focused code and production-path proof support the fix, with integrated validation still needed after resolving current-main drift.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The captured PR body ties the changed wizard guard to production Program.Main runs that rejected invalid users before any WSL command, and reports valid-user WSL setup through Gateway wizard completion. The later tray/MCP shard failures and current-main conflict remain validation blockers. No stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured PR body ties the changed wizard guard to production Program.Main runs that rejected invalid users before any WSL command, and reports valid-user WSL setup through Gateway wizard completion. The later tray/MCP shard failures and current-main conflict remain validation blockers. No stored-data contract changes.
Evidence reviewed 9 items Introduced guard: The pinned PR delta checks the username before registry loading and adds guards to reload suspension and restoration.
Validation contract: The PR makes username matching null-safe and uses absolute end anchoring; the normal WSL configuration step already calls this validator.
Current-main gap: Current main uses stdin for the reload command, but its wizard enters registry loading without the proposed username guard.
Findings None None.
Security None None.

How this fits together

The setup engine reads a local setup configuration, runs the Gateway wizard, and issues commands to the managed WSL distro. Its result controls whether pairing and the rest of setup can proceed.

flowchart LR
  A[Setup configuration] --> B[Wizard-only setup]
  B --> C{Linux user valid?}
  C -->|No| D[Terminal diagnostic]
  C -->|Yes| E[Gateway pairing and WSL commands]
  E --> F[Setup result]
Loading

Before merge

  • Resolve merge risk (P1) - The pinned head conflicts with current main in the wizard runner. The prepared integration and its reported validation cover an older main revision, so required setup/connect CI and strict MXC validation remain unverified on a resolved current-main head.
  • Complete next step (P2) - Have a workflow-authorized maintainer resolve the conflict against current main, update the PR branch, and run required setup/connect CI including strict MXC on the resulting head.
Agent review details

Security

None.

Review metrics

None.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Land the shared username guard at wizard entry while retaining stdin-based WSL execution, with the resolved head proving both invalid-user rejection and valid-user setup.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Land the shared username guard at wizard entry while retaining stdin-based WSL execution, with the resolved head proving both invalid-user rejection and valid-user setup.

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

Yes. Current-main source shows that wizard-only setup skips the existing WSL-user validation step; the PR body also records production-path invalid-user runs with zero WSL commands after the change.

Is this the best way to solve the issue?

Yes. Reusing the existing username contract at the wizard boundary is a narrow repair, and stdin execution follows the repository’s documented WSL argument-handling pattern.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 373887bbe211.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: The PR repairs a specific setup validation gap with limited scope and direct behavior proof.
  • 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 captured PR body ties the changed wizard guard to production Program.Main runs that rejected invalid users before any WSL command, and reports valid-user WSL setup through Gateway wizard completion. The later tray/MCP shard failures and current-main conflict remain validation blockers. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured PR body ties the changed wizard guard to production Program.Main runs that rejected invalid users before any WSL command, and reports valid-user WSL setup through Gateway wizard completion. The later tray/MCP shard failures and current-main conflict remain validation blockers. No stored-data contract changes.

Evidence

What I checked:

Likely related people:

  • Scott Hanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Karen Lai: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Sebastien Tardif: 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.

  • Resolve the current-main conflict and report required setup/connect CI and strict MXC results for the resulting head.

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 (10 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-23T23:17:24.998Z sha f2f7c33 :: needs real behavior proof before merge. :: [P1] Validate the username before operator pairing
  • reviewed 2026-09-27T19:38:08.769Z sha f2979e3 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-28T17:56:34.820Z sha f2979e3 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-28T18:33:22.717Z sha 37f4448 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-28T18:45:14.134Z sha 37f4448 :: blocked before merge. :: none
  • reviewed 2026-09-28T18:58:12.498Z sha 37f4448 :: blocked before merge. :: none
  • reviewed 2026-09-28T19:27:40.063Z sha 37f4448 :: blocked before merge. :: none
  • reviewed 2026-09-28T19:44:01.859Z sha f4006f0 :: blocked before merge. :: none

@karkarl

karkarl commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Global triage: NEEDS_HUMAN_TEST. Take confidence 68%; recommendation confidence 90%; effort small; risk medium.

Reviewed exact head f2f7c33bf480. The injection is real because username is interpolated inside double quotes in the PATH script, so values such as bad$(id) execute substitution. Reusing the established Linux-user contract is correct, both WSL calls move to stdin, and default behavior is preserved. Remaining gate is windows-wsl-gateway-e2e: show terminal rejection for an invalid user and successful completion for the default user, then report closeout tests. A shared null-safe username helper is a useful non-blocking follow-up.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 23, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

Head f2f7c33 records this closeout in the PR body.

Invalid user: SetupWizard_RejectsInvalidLinuxUserBeforeAnyWslCommand passed for bad"user and bad$(id). The outcome is FailedTerminal, and the fake runner recorded no WSL call.

Default user openclaw: live OPENCLAW_RUN_E2E FullSetup_TrayConnects_OperatorAndNode created OpenClawE2E-782a01ca, paired the operator and the node, and logged Gateway wizard completed. The following openclaw gateway restart exited 1 with GATEWAY_RESTART_PREPARATION_REFUSED, then the fixture rolled the distro back. That restart refusal is after wizard completion.

Shared: 4093 passed, 0 failed, 32 skipped. Tray: 3059 passed, 5 failed. Those five source-contract failures are the same on 1488, 1489, 1490, and 1491.

The username check ran only when reload mode was suspended or
restored. Wizard startup with auto-approval reached the pairing
WSL command first. RunCoreAsync now rejects the same invalid user
before registry lookup, pairing, or any other wizard command.

Tests: SetupWizard_RejectsInvalidLinuxUserBeforeAnyWslCommand
passed, 2 of 2. ./build.ps1 exit 0. Shared 4093 passed, 32 skipped.
Tray 3059 passed. Five source-contract tests still fail on an LF
checkout because they expect CRLF snippets.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 28, 2026
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. label Sep 28, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 5ac1f11f-ab65-454b-b841-11c6cafe7381
@shanselman

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@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. and removed 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 28, 2026
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 28, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Maintainer landing assessment is complete at 37f4448b3669b6932964b3e26994d08bee1289b2. Implementation, direct zero-WSL-command proof, required unit/integration suites, and rubber-duck review are complete. Merge remains blocked by required setup-connect CI in the shared fixture after Gateway wizard completed: StateDatabaseCoordinatorContentionError plus GATEWAY_RESTART_PREPARATION_REFUSED: Cannot record restart intent. This is owned by #1507 (fix(setup): retry guarded restart intent contention). Active landing is paused until #1507 lands, then #1488 should rerun CI.

@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 28, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5ac1f11f-ab65-454b-b841-11c6cafe7381
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 28, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Current main including #1507 (fix(setup): retry guarded restart intent contention) is locally integrated and validated. Publication is blocked only by workflow authorization: direct push of the normal merge is rejected because this OAuth token lacks workflow scope, while GitHub update-branch returns HTTP 422 despite a clean local merge-tree. Prepared merge commit: 56504a32f2c0d7b1c0aac428962b22f450adfee0 (parents f4006f0c and 60e978c5, tree 3c7a7428). A workflow-authorized maintainer can push it with git push pr-SebTardif-openclaw-windows-node 56504a32f2c0d7b1c0aac428962b22f450adfee0:fix/f026-invalid-linux-user, then required CI should run. Active landing is paused until that handoff.

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. 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.

3 participants