Support trusted control UI origins - #295
Open
jamesachurchill wants to merge 2 commits into
Open
Conversation
Add a narrow, strictly-validated escape hatch for genuine cross-origin/ custom-origin Control UI deployments: operator-supplied origins in data/control-ui-origins.json (or CLAWBOX_CONTROL_UI_ORIGINS_FILE) are merged into the gateway's generated allowedOrigins and honored by the Next.js proxy's redirect-origin reflection, with exact scheme+host+port matching so a configured hostname can't be reflected across other schemes or ports. Same-origin .local/.ts.net/private access is unaffected and normally needs no entry.
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds configurable trusted Control UI origins through a JSON file. Python and TypeScript loaders validate and normalize entries, gateway startup merges them with defaults, and setup redirects use exact configured-origin matching. Documentation and unit tests cover configuration, validation, warnings, deduplication, and fallback behavior. ChangesTrusted Control UI origins
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant gateway-proxy
participant control-ui-origins
participant OriginsFile
Browser->>gateway-proxy: Request setup redirect with Host header
gateway-proxy->>control-ui-origins: Load configured origins
control-ui-origins->>OriginsFile: Read JSON origin array
OriginsFile-->>control-ui-origins: Return configured origins
control-ui-origins-->>gateway-proxy: Return normalized origins and warnings
gateway-proxy-->>Browser: Redirect to trusted origin or CANONICAL_ORIGIN
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦀 ClawReviewScuttled over to say hello and get you oriented 🦀 This PR adds a At a glance
Good to know
— ClawReview 🦀. I set the scene; CodeRabbit reviews the code; you decide. Conventions: docs. |
github-actions
Bot
added
area: docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/gateway-proxy.ts (1)
67-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRedirect with the normalized origin, not the raw
Hostheader.
isConfiguredOriginvalidates a normalized origin, but Line 86 builds theLocationURL from the rawhostHeader. The trust decision and the emitted URL come from two different values.The current code is not exploitable.
normalizeOriginrejects any composed value that carries a path, a query, a fragment, credentials, or*, and the WHATWG parser turns\and/inside the header into a path, which the path check atsrc/lib/control-ui-origins.tsLine 82 rejects. So a header that passes the check cannot carry extra structure.Returning the matched origin still removes the need for that reasoning and emits a canonical
Location. It also makes the port and case handling explicit.🔒️ Proposed refactor
-function isConfiguredOrigin(proto: string, hostHeader: string): boolean { - const { origin } = normalizeOrigin(`${proto}://${hostHeader}`); - return origin !== null && getConfiguredOrigins().has(origin); -} +function matchConfiguredOrigin(proto: string, hostHeader: string): string | null { + const { origin } = normalizeOrigin(`${proto}://${hostHeader}`); + return origin !== null && getConfiguredOrigins().has(origin) ? origin : null; +} export function redirectToSetup(request: NextRequest): NextResponse { const rawProto = request.headers.get("x-forwarded-proto"); const proto = rawProto ?.split(",") .map((t) => t.trim().toLowerCase()) .find((t) => ALLOWED_PROTOS.has(t)) ?? "http"; const hostHeader = request.headers.get("host"); const rawHost = hostHeader?.toLowerCase().replace(/:\d+$/, ""); - const reflectable = - !!rawHost && - (isReflectableHost(rawHost) || (!!hostHeader && isConfiguredOrigin(proto, hostHeader))); - if (reflectable) { - return NextResponse.redirect( - new URL(`${proto}://${hostHeader}/setup`), - 302 - ); - } + if (rawHost && isReflectableHost(rawHost)) { + return NextResponse.redirect(new URL(`${proto}://${hostHeader}/setup`), 302); + } + const configured = hostHeader ? matchConfiguredOrigin(proto, hostHeader) : null; + if (configured) { + return NextResponse.redirect(new URL(`${configured}/setup`), 302); + } return NextResponse.redirect(new URL(`${CANONICAL_ORIGIN}/setup`), 302); }Note: the test at
src/tests/unit/gateway-proxy-origins.test.tsLine 56 expectshttp://custom.example.com:8080/setup, which this refactor still produces.As per path instructions: "TypeScript server-side libraries. Review for proper error handling and type safety".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/gateway-proxy.ts` around lines 67 - 90, Update isConfiguredOrigin to return the normalized matched origin, or otherwise expose that normalized value to redirectToSetup, instead of returning only a boolean. In redirectToSetup, use that normalized origin to construct the /setup redirect whenever the origin is trusted, preserving the existing reflectable-host behavior and configured-origin port handling.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 252: Update the CLAWBOX_CONTROL_UI_ORIGINS_FILE row in README.md to
document the absolute default path
/home/clawbox/clawbox/data/control-ui-origins.json, matching the hard-coded
defaults used by the gateway and TypeScript loaders.
In `@scripts/gateway_origins.py`:
- Around line 122-134: The load_configured_origins file-read path must preserve
its never-raises contract for invalid UTF-8; add UnicodeDecodeError handling
after the OSError handler and return no origins with a warning. In
scripts/gateway-pre-start.sh, wrap resolve_origins_path and
load_configured_origins in a broad exception handler that warns to stderr and
exits successfully, allowing startup to continue with no extra origins.
- Around line 60-111: Align the gateway and proxy origin canonicalization
contracts: in scripts/gateway_origins.py, reject raw values containing
backslashes, tabs, CR, LF, percent signs, or non-ASCII characters before
urlsplit, and emit compressed IPv6 using
ipaddress.IPv6Address(hostname).compressed; src/lib/control-ui-origins.ts
requires no direct change. Add matching cases to
src/tests/unit/control-ui-origins.test.ts and
src/tests/unit/gateway-origins.test.ts covering these shared inputs and IPv6
normalization.
- Around line 126-140: Update the file-reading call in the surrounding
origin-loading function to omit the redundant default "r" mode while preserving
encoding. In the non-list JSON error return, parenthesize the adjacent f-strings
so their intentional concatenation is explicit without changing the resulting
message.
In `@scripts/gateway-pre-start.sh`:
- Line 95: Replace the global export of CLAWBOX_GATEWAY_ORIGINS_SCRIPT_DIR with
per-invocation environment scoping on the python3 heredoc call that consumes it,
while preserving the variable’s value for that call and preventing it from
reaching later openclaw or node processes.
- Around line 103-106: Update the import-exception handler in the gateway
pre-start script to write a clear warning to stderr before exiting successfully,
while preserving the non-blocking fallback behavior. Also update the
corresponding gateway-origins test assertion to expect the warning instead of an
empty stderr result.
In `@src/lib/control-ui-origins.ts`:
- Around line 137-152: Update the catch blocks in the control UI origins loading
and JSON parsing flow to handle caught values as unknown without using `as
Error`. Extract the message defensively so thrown non-Error values still produce
meaningful warning text, while preserving the existing warning prefixes and
fallback behavior.
In `@src/lib/gateway-proxy.ts`:
- Around line 56-65: Update getConfiguredOrigins and its cached state to refresh
the configured origins when the source file’s modification time changes, while
retaining the cache when the mtime is unchanged. Track the last observed mtime
alongside cachedConfiguredOrigins and reload through
loadConfiguredOriginsFromEnv when it changes; preserve warning emission and Set
construction for refreshed values.
In `@src/tests/unit/control-ui-origins.test.ts`:
- Around line 88-96: Update the normalizeOrigin tests to assert the warning text
for the out-of-range port and dotted-decimal host cases, confirming they
exercise the invalid-URL branch rather than port or IPv4 validation. Add parity
coverage for the backslash userinfo, embedded-newline hostname, and uncompressed
IPv6 inputs, asserting normalizeOrigin results and matching behavior in the
Python loader tests in gateway-origins.test.ts.
- Around line 189-191: Update the unreadable-path test around
loadConfiguredOrigins to create a temporary origins file, remove its read
permission with chmodSync, and assert the function does not throw. Skip this
test when running as root, and restore permissions and clean up the temporary
file afterward so the readFileSync error handler is actually exercised.
In `@src/tests/unit/gateway-origins.test.ts`:
- Around line 143-193: Update src/tests/unit/gateway-origins.test.ts lines 1,
143-193, and 295-385: import afterEach from vitest, add an afterEach cleanup
hook to both describe blocks, assign dir in the missing-file test, and remove
all inline rmSync calls so temporary directories are cleaned up even when
assertions fail.
In `@src/tests/unit/gateway-proxy-origins.test.ts`:
- Around line 125-154: Make the no-config test hermetic by setting ENV_VAR to a
nonexistent path under dir before importFresh(), rather than deleting it and
allowing the default device path. In the invalid-JSON test, spy on or otherwise
capture console.warn before importFresh(), then assert getConfiguredOrigins
emits a warning while preserving the existing host-reflection assertions.
- Around line 112-123: Extend the gateway proxy origin tests around the existing
“does not reflect” case to cover a configured IPv4 origin and a request using
the same address with a different port, asserting that the configured port is
not enforced by the current IPv4 matching path. Update the README’s exact-origin
matching claim to explicitly exclude IPv4 hosts, while preserving the existing
documented example and other matching guidance.
---
Outside diff comments:
In `@src/lib/gateway-proxy.ts`:
- Around line 67-90: Update isConfiguredOrigin to return the normalized matched
origin, or otherwise expose that normalized value to redirectToSetup, instead of
returning only a boolean. In redirectToSetup, use that normalized origin to
construct the /setup redirect whenever the origin is trusted, preserving the
existing reflectable-host behavior and configured-origin port handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac52d3bf-3b2a-47dd-ab61-b98af98d9689
📒 Files selected for processing (8)
README.mdscripts/gateway-pre-start.shscripts/gateway_origins.pysrc/lib/control-ui-origins.tssrc/lib/gateway-proxy.tssrc/tests/unit/control-ui-origins.test.tssrc/tests/unit/gateway-origins.test.tssrc/tests/unit/gateway-proxy-origins.test.ts
Comment on lines
+189
to
+191
| it("never throws, even on an unreadable path", () => { | ||
| expect(() => loadConfiguredOrigins("/root/definitely-not-readable/origins.json")).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This test does not reach the unreadable-file branch.
/root/definitely-not-readable/origins.json does not exist, so fs.existsSync at Line 130 returns false and the function returns early. The test duplicates the missing-file case at Line 144 and never exercises the readFileSync handler at Lines 137-142.
Create a real file, remove its read permission, and skip the test when the process runs as root.
💚 Proposed test that reaches the branch
- it("never throws, even on an unreadable path", () => {
- expect(() => loadConfiguredOrigins("/root/definitely-not-readable/origins.json")).not.toThrow();
- });
+ it.skipIf(typeof process.getuid === "function" && process.getuid() === 0)(
+ "returns a warning for an existing but unreadable file",
+ () => {
+ dir = mkdtempSync(path.join(tmpdir(), "control-ui-origins-"));
+ const file = path.join(dir, "origins.json");
+ writeFileSync(file, JSON.stringify(["http://a.example.com"]));
+ chmodSync(file, 0o000);
+ const result = loadConfiguredOrigins(file);
+ expect(result.origins).toEqual([]);
+ expect(result.warnings).toHaveLength(1);
+ },
+ );Import chmodSync from node:fs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("never throws, even on an unreadable path", () => { | |
| expect(() => loadConfiguredOrigins("/root/definitely-not-readable/origins.json")).not.toThrow(); | |
| }); | |
| it.skipIf(typeof process.getuid === "function" && process.getuid() === 0)( | |
| "returns a warning for an existing but unreadable file", | |
| () => { | |
| dir = mkdtempSync(path.join(tmpdir(), "control-ui-origins-")); | |
| const file = path.join(dir, "origins.json"); | |
| writeFileSync(file, JSON.stringify(["http://a.example.com"])); | |
| chmodSync(file, 0o000); | |
| const result = loadConfiguredOrigins(file); | |
| expect(result.origins).toEqual([]); | |
| expect(result.warnings).toHaveLength(1); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/unit/control-ui-origins.test.ts` around lines 189 - 191, Update the
unreadable-path test around loadConfiguredOrigins to create a temporary origins
file, remove its read permission with chmodSync, and assert the function does
not throw. Skip this test when running as root, and restore permissions and
clean up the temporary file afterward so the readFileSync error handler is
actually exercised.
Comment on lines
+125
to
+154
| it("retains existing ALLOWED_HOSTS/IP reflection when no origins are configured", async () => { | ||
| delete process.env[ENV_VAR]; | ||
| await importFresh(); | ||
|
|
||
| const request = createRequest("http://clawbox.local/", { host: "clawbox.local" }); | ||
| const response = gatewayProxy.redirectToSetup(request); | ||
|
|
||
| expect(response.headers.get("location")).toContain("clawbox.local"); | ||
|
|
||
| const ipRequest = createRequest("http://192.0.2.3/", { host: "192.0.2.3" }); | ||
| const ipResponse = gatewayProxy.redirectToSetup(ipRequest); | ||
| expect(ipResponse.headers.get("location")).toContain("192.0.2.3"); | ||
| }); | ||
|
|
||
| it("an invalid origins file yields no extras and does not affect existing host reflection", async () => { | ||
| const file = path.join(dir, "origins.json"); | ||
| writeFileSync(file, "{not json"); | ||
| process.env[ENV_VAR] = file; | ||
| await importFresh(); | ||
|
|
||
| const request = createRequest("http://clawbox.local/", { host: "clawbox.local" }); | ||
| const response = gatewayProxy.redirectToSetup(request); | ||
| expect(response.headers.get("location")).toContain("clawbox.local"); | ||
|
|
||
| const untrustedRequest = createRequest("http://untrusted.example.com/", { | ||
| host: "untrusted.example.com", | ||
| }); | ||
| const untrustedResponse = gatewayProxy.redirectToSetup(untrustedRequest); | ||
| expect(untrustedResponse.headers.get("location")).not.toContain("untrusted.example.com"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the no-config test hermetic, and assert the warning.
Two improvements apply:
- Line 126 deletes the env var, so
resolveOriginsPath()returns the absolute default/home/clawbox/clawbox/data/control-ui-origins.json. On a real device that file exists and can hold operator origins, so the test result depends on the machine. Point the env var at a non-existent path insidedirinstead. - The invalid-JSON test at Lines 139-154 does not check that the warning reaches the log.
getConfiguredOriginscallsconsole.warnatsrc/lib/gateway-proxy.tsLine 61. The PR objective requires clear reporting of invalid entries, so assert it.
💚 Proposed changes
it("retains existing ALLOWED_HOSTS/IP reflection when no origins are configured", async () => {
- delete process.env[ENV_VAR];
+ process.env[ENV_VAR] = path.join(dir, "absent.json");
await importFresh(); it("an invalid origins file yields no extras and does not affect existing host reflection", async () => {
const file = path.join(dir, "origins.json");
writeFileSync(file, "{not json");
process.env[ENV_VAR] = file;
await importFresh();
+ const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const request = createRequest("http://clawbox.local/", { host: "clawbox.local" });
const response = gatewayProxy.redirectToSetup(request);
expect(response.headers.get("location")).toContain("clawbox.local");
+ expect(warn).toHaveBeenCalledWith(expect.stringContaining("not valid JSON"));
+ warn.mockRestore();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/unit/gateway-proxy-origins.test.ts` around lines 125 - 154, Make
the no-config test hermetic by setting ENV_VAR to a nonexistent path under dir
before importFresh(), rather than deleting it and allowing the default device
path. In the invalid-JSON test, spy on or otherwise capture console.warn before
importFresh(), then assert getConfiguredOrigins emits a warning while preserving
the existing host-reflection assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
data/control-ui-origins.jsonallowlist for genuine cross-origin or custom-origin deploymentsWhy this remains necessary
Current OpenClaw already accepts ordinary same-origin private and MagicDNS access, so that broad part of #232 no longer needs a workaround. Genuine cross-origin deployments still require an explicit allowlist, and the pre-start script still replaces generated
allowedOriginson every boot.This replaces the approach in #255 rather than reviving it unchanged. The prior
controlUi.extraAllowedOriginskey is not part of the current strict OpenClaw schema, and #255 did not update the setup proxy's independent redirect-origin boundary.Security and update behavior
data/, so the updater's hard reset preserves itVerification
betabase: 1,567 tests passed across 128 filesmainhas the same affected source/dependency surface asbeta; the patch was also verified against its current v3.1.11 state during preparationThe repository-wide lint command still reports its existing unrelated beta errors; none are in the changed files.
Closes #232
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests