fix(doctor): warn when OPENCLAW_GATEWAY_TOKEN env overrides gateway.auth.token config#74433
Conversation
Greptile SummaryThis PR adds a warning in Confidence Score: 4/5Safe to merge; the logic is correct and well-scoped, with only a missing test for the audit finding. Only P2 findings — a missing test for the new src/security/audit-gateway-config.ts — needs a test in audit-gateway.test.ts for the new Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/security/audit-gateway-config.ts
Line: 119-132
Comment:
**Missing test coverage for new audit finding**
The new `gateway.env_token_overrides_config` finding in `collectGatewayConfigFindings` has no corresponding test. The existing `audit-gateway.test.ts` file already has helpers (`hasFinding`, `hasFindingWithSeverity`, `withEnvAsync`) that make it straightforward to add cases — one where both `OPENCLAW_GATEWAY_TOKEN` env and `sourceConfig.gateway.auth.token` are set (expecting the finding), and one where only the env token is set (expecting no finding). Without these tests the audit path could regress silently.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(doctor): warn when OPENCLAW_GATEWAY_..." | Re-trigger Greptile |
| if (envTokenConfigured && tokenConfiguredFromConfig) { | ||
| findings.push({ | ||
| checkId: "gateway.env_token_overrides_config", | ||
| severity: "warn", | ||
| title: "OPENCLAW_GATEWAY_TOKEN overrides gateway.auth.token for CLI commands", | ||
| detail: | ||
| "Both OPENCLAW_GATEWAY_TOKEN (environment) and gateway.auth.token (config) are set. " + | ||
| "CLI commands (status, call, probe) use env-first precedence, but the gateway server uses config-first. " + | ||
| "If the two values differ, CLI commands will fail to authenticate with the gateway.", | ||
| remediation: | ||
| "Remove OPENCLAW_GATEWAY_TOKEN from ~/.openclaw/.env if gateway.auth.token is the intended source, " + | ||
| "or remove gateway.auth.token from config if the env var is intentional.", | ||
| }); | ||
| } |
There was a problem hiding this comment.
The new gateway.env_token_overrides_config finding in collectGatewayConfigFindings has no corresponding test. The existing audit-gateway.test.ts file already has helpers (hasFinding, hasFindingWithSeverity, withEnvAsync) that make it straightforward to add cases — one where both OPENCLAW_GATEWAY_TOKEN env and sourceConfig.gateway.auth.token are set (expecting the finding), and one where only the env token is set (expecting no finding). Without these tests the audit path could regress silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/audit-gateway-config.ts
Line: 119-132
Comment:
**Missing test coverage for new audit finding**
The new `gateway.env_token_overrides_config` finding in `collectGatewayConfigFindings` has no corresponding test. The existing `audit-gateway.test.ts` file already has helpers (`hasFinding`, `hasFindingWithSeverity`, `withEnvAsync`) that make it straightforward to add cases — one where both `OPENCLAW_GATEWAY_TOKEN` env and `sourceConfig.gateway.auth.token` are set (expecting the finding), and one where only the env token is set (expecting no finding). Without these tests the audit path could regress silently.
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: passed. Summary Reproducibility: yes. Source inspection shows current main has the env-first CLI/config-first server precedence split and no conflict diagnostic; this read-only review did not run a live CLI scenario. Real behavior proof Next step before merge Security Review detailsBest possible solution: Land this diagnostic after exact-head CI and mergeability are green, then let the merged PR close #74271. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main has the env-first CLI/config-first server precedence split and no conflict diagnostic; this read-only review did not run a live CLI scenario. Is this the best way to solve the issue? Yes. Centralizing the redacted conflict helper and reusing it in doctor, status, and audit is the narrow maintainable approach, with guards for same-source SecretRefs and remote mode. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7d5ca3064a51. |
3bef7ca to
bdb5df8
Compare
bdb5df8 to
90b5b51
Compare
472bd6a to
9929dd0
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper automerge |
|
ClawSweeper 🐠 automerge status ClawSweeper finished this automerge repair pass without changing the branch. Executor outcome: no planned fix actions. Worker actions:
This pass stayed observational only. No branch push, replacement, merge, or re-review was started. fish notes: model gpt-5.5, reasoning high. Automerge progress:
|
Signed-off-by: sallyom <somalley@redhat.com>
9929dd0 to
b665be3
Compare
…uth.token config (openclaw#74433) * fix(doctor): warn when OPENCLAW_GATEWAY_TOKEN env overrides gateway.auth.token config (openclaw#74271) * fix(doctor): narrow gateway token source warning * test(status): type env secret provider fixture * fix(doctor): scope gateway token conflict warning to local mode Signed-off-by: sallyom <somalley@redhat.com> --------- Signed-off-by: sallyom <somalley@redhat.com> Co-authored-by: sallyom <somalley@redhat.com>
Summary
Fixes #74271
When both
OPENCLAW_GATEWAY_TOKEN(environment) andgateway.auth.token(config) are set, CLI commands (status, call, probe) use env-first precedence while the gateway server uses config-first. If the values differ, CLI commands silently fail to authenticate.Changes
noteSecurityWarnings()when both env and config tokens are set, explaining the precedence divergenceSecurityAuditFindingwithcheckId: "gateway.env_token_overrides_config"inauditGatewayConfigforopenclaw security audit --deep