fix(control-ui): share gateway token scope across loopback hosts#43254
fix(control-ui): share gateway token scope across loopback hosts#43254ssdiwu wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes a real UX pain point in the Control UI by normalising loopback host aliases ( Key observations:
Confidence Score: 4/5
Last reviewed commit: 894ba94 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| host === "::1" || | ||
| host === "[::1]" || |
There was a problem hiding this comment.
"::1" check is unreachable dead code
Per the WHATWG URL Living Standard, URL.hostname for an IPv6 address is serialized with surrounding brackets — e.g. new URL("ws://[::1]:18789").hostname returns "[::1]", not "::1". This means the host === "::1" branch can never be entered for a validly-formed URL; only host === "[::1]" (line 51) actually fires.
Keeping the dead branch is harmless in practice (the [::1] check correctly handles IPv6 loopback), but it will mislead future readers about which check is doing the work and may create a false sense of coverage.
| host === "::1" || | |
| host === "[::1]" || | |
| host === "::1" || | |
| host === "[::1]" || |
Consider removing the "::1" alternative and leaving only "[::1]", or adding an explanatory comment clarifying the WHATWG URL hostname serialization behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/storage.ts
Line: 50-51
Comment:
**`"::1"` check is unreachable dead code**
Per the WHATWG URL Living Standard, `URL.hostname` for an IPv6 address is serialized *with* surrounding brackets — e.g. `new URL("ws://[::1]:18789").hostname` returns `"[::1]"`, not `"::1"`. This means the `host === "::1"` branch can never be entered for a validly-formed URL; only `host === "[::1]"` (line 51) actually fires.
Keeping the dead branch is harmless in practice (the `[::1]` check correctly handles IPv6 loopback), but it will mislead future readers about which check is doing the work and may create a false sense of coverage.
```suggestion
host === "::1" ||
host === "[::1]" ||
```
Consider removing the `"::1"` alternative and leaving only `"[::1]"`, or adding an explanatory comment clarifying the WHATWG URL hostname serialization behaviour.
How can I resolve this? If you propose a fix, please make it concise.| it("reuses a session token across loopback host aliases", async () => { | ||
| setTestLocation({ | ||
| protocol: "http:", | ||
| host: "127.0.0.1:18789", | ||
| pathname: "/", | ||
| }); | ||
|
|
||
| const { loadSettings, saveSettings } = await import("./storage.ts"); | ||
| saveSettings({ | ||
| gatewayUrl: "ws://127.0.0.1:18789", | ||
| token: "loopback-token", | ||
| sessionKey: "main", | ||
| lastActiveSessionKey: "main", | ||
| theme: "system", | ||
| chatFocusMode: false, | ||
| chatShowThinking: true, | ||
| splitRatio: 0.6, | ||
| navCollapsed: false, | ||
| navGroupsCollapsed: {}, | ||
| }); | ||
|
|
||
| localStorage.setItem( | ||
| "openclaw.control.settings.v1", | ||
| JSON.stringify({ | ||
| gatewayUrl: "ws://localhost:18789", | ||
| sessionKey: "main", | ||
| lastActiveSessionKey: "main", | ||
| theme: "system", | ||
| chatFocusMode: false, | ||
| chatShowThinking: true, | ||
| splitRatio: 0.6, | ||
| navCollapsed: false, | ||
| navGroupsCollapsed: {}, | ||
| }), | ||
| ); | ||
|
|
||
| expect(loadSettings()).toMatchObject({ | ||
| gatewayUrl: "ws://localhost:18789", | ||
| token: "loopback-token", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing IPv6 loopback test coverage
The new tests verify 127.0.0.1 ↔ localhost alias sharing and port isolation, but there is no test confirming that ws://[::1]:18789 is normalised to the same scope as ws://127.0.0.1:18789. Given that the PR description explicitly lists [::1] as a handled variant and there is a dedicated branch for it in normalizeGatewayTokenScope, a parallel test case would ensure that branch is exercised and that the IPv6 path is not accidentally regressed in future refactors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/storage.node.test.ts
Line: 210-250
Comment:
**Missing IPv6 loopback test coverage**
The new tests verify `127.0.0.1` ↔ `localhost` alias sharing and port isolation, but there is no test confirming that `ws://[::1]:18789` is normalised to the same scope as `ws://127.0.0.1:18789`. Given that the PR description explicitly lists `[::1]` as a handled variant and there is a dedicated branch for it in `normalizeGatewayTokenScope`, a parallel test case would ensure that branch is exercised and that the IPv6 path is not accidentally regressed in future refactors.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 894ba946e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| host === "127.0.0.1" || | ||
| host === "::1" || | ||
| host === "[::1]" || | ||
| host.startsWith("127."); |
There was a problem hiding this comment.
127.* hostnames as loopback literals
The new loopback check in normalizeGatewayTokenScope uses host.startsWith("127."), which matches regular DNS hostnames like 127.example.com and 127.0.0.1.nip.io in addition to numeric IP literals. In those cases the code rewrites the host to 127.0.0.1[:port], so non-loopback gateways can collide with local loopback token scopes and load/save the wrong session token. Restrict this logic to actual IPv4 loopback literals (127/8 numeric addresses) rather than a string prefix.
Useful? React with 👍 / 👎.
|
Follow-up pushed: removed the redundant "::1" branch and added a [::1] <-> 127.0.0.1 loopback scope test. |
|
CI note: the current |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Keep this PR open. Current main still scopes Control UI token and session-selection storage by the exact parsed gateway URL, so the localhost/127.0.0.1/[::1] loopback sharing this PR proposes has not been implemented. The PR targets the right storage surface, but the current patch needs a focused security/correctness revision for 127.* DNS hostnames and a rebase to the current UiSettings shape before it is merge-ready. Best possible solution: Keep the PR open and request a focused revision in ui/src/ui/storage.ts: normalize only localhost plus actual numeric IPv4 loopback literals and bracketed IPv6 loopback, preserve protocol/port/path boundaries, add positive localhost/127/[::1] same-port sharing tests, add different-port and DNS-hostname negative tests, update fixtures for the current UiSettings fields, then run the targeted Control UI storage test before maintainer review/merge. What I checked:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against bdfb408ce6ee. |
1 participant
Summary
Fix a Control UI auth footgun where gateway tokens are scoped by exact
gatewayUrl, causing same-machine loopback aliases to fragment auth state.Today these are treated as different token scopes even though they refer to the same local gateway:
ws://127.0.0.1:18789ws://localhost:18789ws://::1:18789/[::1]variantsBecause the Control UI stores the token in
sessionStorageand keys it by exactgatewayUrl, switching between loopback aliases can produce:unauthorized: gateway token missingWhat changed
ui/src/ui/storage.ts, normalize loopback hosts to a shared token scope:localhost127.0.0.1127.*::1[::1]127.0.0.1andlocalhostreuse the same token scopeWhy this is safe
sessionStorage; this PR does not relax persistence or broaden auth beyond loopback alias normalization.Related
Verification
pnpm exec vitest run --config /tmp/openclaw-vitest-storage-only.config.tsThe temporary config used: