fix: bake CHAT_UI_URL into Docker build for remote dashboard access - #812
Conversation
setup.sh and onboard.js passed CHAT_UI_URL only as a runtime env var, but the gateway allowedOrigins are generated at build time and locked immutable (root:root 444 + Landlock). Remote browsers hit "origin not allowed" because the build always used the default localhost origin. Sed the ARG CHAT_UI_URL line in the build-context Dockerfile copy when the env var is set, so the build-time config generator writes the correct allowedOrigins. URL is validated before interpolation to prevent injection. Also forward CHAT_UI_URL through brev-setup.sh and warn on headless hosts when it is unset. Fixes #795, fixes #20
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree files are updated to address remote dashboard access for NemoClaw on Brev: documentation is added describing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/deployment/deploy-to-remote-gpu.md (2)
80-84: Multiple sentences on the same source line.This paragraph contains three sentences that should each be on their own line for diff readability.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."📝 Suggested reformat
-The NemoClaw dashboard validates the browser origin against an allowlist baked -into the sandbox image at build time. By default the allowlist only contains -`http://127.0.0.1:18789`. When accessing the dashboard from a remote browser -(for example through a Brev public URL or an SSH port-forward), set -`CHAT_UI_URL` to the origin the browser will use **before** running setup: +The NemoClaw dashboard validates the browser origin against an allowlist baked into the sandbox image at build time. +By default the allowlist only contains `http://127.0.0.1:18789`. +When accessing the dashboard from a remote browser (for example through a Brev public URL or an SSH port-forward), set `CHAT_UI_URL` to the origin the browser will use **before** running setup:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/deploy-to-remote-gpu.md` around lines 80 - 84, The paragraph in docs/deployment/deploy-to-remote-gpu.md packs three sentences into one line; split each sentence onto its own source line to follow "one sentence per line" guideline so diffs are readable. Locate the paragraph mentioning NemoClaw, the allowlist default `http://127.0.0.1:18789`, the remote browser access examples (Brev public URL / SSH port-forward), and the `CHAT_UI_URL` instruction, and break it into three separate lines—one for the allowlist/default origin, one for the remote access examples, and one for the instruction to set `CHAT_UI_URL` before running setup.
95-97: Multiple sentences on the same source line within admonition.The note contains two sentences that should each appear on their own line.
As per coding guidelines: "One sentence per line in source (makes diffs readable)."📝 Suggested reformat
:::{note} -On Brev, set `CHAT_UI_URL` in the launchable environment configuration so it is -available when the setup script builds the sandbox image. If `CHAT_UI_URL` is -not set on a headless host, `brev-setup.sh` prints a warning. +On Brev, set `CHAT_UI_URL` in the launchable environment configuration so it is available when the setup script builds the sandbox image. +If `CHAT_UI_URL` is not set on a headless host, `brev-setup.sh` prints a warning. :::🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/deploy-to-remote-gpu.md` around lines 95 - 97, The admonition text mentioning CHAT_UI_URL and brev-setup.sh contains multiple sentences on one source line; edit the note so each sentence is on its own source line (e.g., split "On Brev, set `CHAT_UI_URL`... image." and "If `CHAT_UI_URL` is not set on a headless host, `brev-setup.sh` prints a warning." into two separate lines) to follow the "one sentence per line" guideline and improve diff readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/deployment/deploy-to-remote-gpu.md`:
- Around line 80-84: The paragraph in docs/deployment/deploy-to-remote-gpu.md
packs three sentences into one line; split each sentence onto its own source
line to follow "one sentence per line" guideline so diffs are readable. Locate
the paragraph mentioning NemoClaw, the allowlist default
`http://127.0.0.1:18789`, the remote browser access examples (Brev public URL /
SSH port-forward), and the `CHAT_UI_URL` instruction, and break it into three
separate lines—one for the allowlist/default origin, one for the remote access
examples, and one for the instruction to set `CHAT_UI_URL` before running setup.
- Around line 95-97: The admonition text mentioning CHAT_UI_URL and
brev-setup.sh contains multiple sentences on one source line; edit the note so
each sentence is on its own source line (e.g., split "On Brev, set
`CHAT_UI_URL`... image." and "If `CHAT_UI_URL` is not set on a headless host,
`brev-setup.sh` prints a warning." into two separate lines) to follow the "one
sentence per line" guideline and improve diff readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7746d00e-fb3d-47b1-a245-375d688efdb2
📒 Files selected for processing (4)
bin/lib/onboard.jsdocs/deployment/deploy-to-remote-gpu.mdscripts/brev-setup.shscripts/setup.sh
cv
left a comment
There was a problem hiding this comment.
A few suggestions — nothing blocking, all optional.
| const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789'; | ||
| if (chatUiUrl !== 'http://127.0.0.1:18789') { |
There was a problem hiding this comment.
Nit: the default URL is repeated as a magic string here and must match the Dockerfile ARG default. If the Dockerfile default ever changes, this comparison silently breaks. Consider extracting a constant:
| const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789'; | |
| if (chatUiUrl !== 'http://127.0.0.1:18789') { | |
| const DEFAULT_CHAT_UI_URL = 'http://127.0.0.1:18789'; | |
| const chatUiUrl = process.env.CHAT_UI_URL || DEFAULT_CHAT_UI_URL; | |
| if (chatUiUrl !== DEFAULT_CHAT_UI_URL) { |
| // Ref: https://github.com/NVIDIA/NemoClaw/issues/795 | ||
| const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789'; | ||
| if (chatUiUrl !== 'http://127.0.0.1:18789') { | ||
| if (/^https?:\/\/[a-zA-Z0-9._:-]+(\/[a-zA-Z0-9._/~%-]*)?$/.test(chatUiUrl)) { |
There was a problem hiding this comment.
This regex is duplicated in scripts/setup.sh:198 (bash version). If one gets updated without the other, you get inconsistent validation depending on the entrypoint. Consider adding a comment in each location cross-referencing the other, e.g.:
// NOTE: keep in sync with scripts/setup.sh URL validation regex
| CHAT_UI_ENV="" | ||
| if [ -n "${CHAT_UI_URL:-}" ]; then | ||
| CHAT_UI_ENV="CHAT_UI_URL=$CHAT_UI_URL" | ||
| fi | ||
|
|
||
| # Capture full output to a temp file so we can filter for display but still | ||
| # detect failures. The raw log is kept on failure for debugging. | ||
| CREATE_LOG=$(mktemp /tmp/nemoclaw-create-XXXXXX.log) | ||
| set +e | ||
| # shellcheck disable=SC2086 # intentional word-split on CHAT_UI_ENV (validated URL, no spaces) | ||
| openshell sandbox create --from "$BUILD_CTX/Dockerfile" --name "$SANDBOX_NAME" \ | ||
| --provider nvidia-nim \ | ||
| -- env NVIDIA_API_KEY="$NVIDIA_API_KEY" >"$CREATE_LOG" 2>&1 | ||
| -- env NVIDIA_API_KEY="$NVIDIA_API_KEY" $CHAT_UI_ENV >"$CREATE_LOG" 2>&1 | ||
| CREATE_RC=$? |
There was a problem hiding this comment.
The word-splitting approach works but requires a shellcheck suppression and is fragile if the regex ever allows spaces. Consider using a bash array instead — more idiomatic and avoids the suppression entirely:
| CHAT_UI_ENV="" | |
| if [ -n "${CHAT_UI_URL:-}" ]; then | |
| CHAT_UI_ENV="CHAT_UI_URL=$CHAT_UI_URL" | |
| fi | |
| # Capture full output to a temp file so we can filter for display but still | |
| # detect failures. The raw log is kept on failure for debugging. | |
| CREATE_LOG=$(mktemp /tmp/nemoclaw-create-XXXXXX.log) | |
| set +e | |
| # shellcheck disable=SC2086 # intentional word-split on CHAT_UI_ENV (validated URL, no spaces) | |
| openshell sandbox create --from "$BUILD_CTX/Dockerfile" --name "$SANDBOX_NAME" \ | |
| --provider nvidia-nim \ | |
| -- env NVIDIA_API_KEY="$NVIDIA_API_KEY" >"$CREATE_LOG" 2>&1 | |
| -- env NVIDIA_API_KEY="$NVIDIA_API_KEY" $CHAT_UI_ENV >"$CREATE_LOG" 2>&1 | |
| CREATE_RC=$? | |
| # Build the runtime env args array. | |
| ENV_ARGS=(NVIDIA_API_KEY="$NVIDIA_API_KEY") | |
| if [ -n "${CHAT_UI_URL:-}" ]; then | |
| ENV_ARGS+=(CHAT_UI_URL="$CHAT_UI_URL") | |
| fi | |
| # Capture full output to a temp file so we can filter for display but still | |
| # detect failures. The raw log is kept on failure for debugging. | |
| CREATE_LOG=$(mktemp /tmp/nemoclaw-create-XXXXXX.log) | |
| set +e | |
| openshell sandbox create --from "$BUILD_CTX/Dockerfile" --name "$SANDBOX_NAME" \ | |
| --provider nvidia-nim \ | |
| -- env "${ENV_ARGS[@]}" >"$CREATE_LOG" 2>&1 |
Keep main's patchStagedDockerfile and formatEnvAssignment from #812, while preserving the security fix: strip NVIDIA_API_KEY from sandbox environment and delete from process.env after provider config storage.
|
+1 This fixes a usability issue for me. |
# Conflicts: # bin/lib/onboard.js # scripts/brev-setup.sh # scripts/setup.sh
…VIDIA#812) ## Summary - `setup.sh` and `onboard.js` passed `CHAT_UI_URL` only as a runtime env var, but the gateway `allowedOrigins` are generated at **build time** and locked immutable (`root:root 444` + Landlock). Remote browsers hit "origin not allowed" because the build always used the default localhost origin. - Sed the `ARG CHAT_UI_URL` line in the build-context Dockerfile copy when the env var is set, so the build-time config generator writes the correct `allowedOrigins`. URL is validated before interpolation to prevent injection. - Forward `CHAT_UI_URL` through `brev-setup.sh` and warn on headless hosts when it is unset. - Document `CHAT_UI_URL` requirement for remote deployments. Fixes NVIDIA#795, fixes NVIDIA#20 ## Test plan - [ ] Existing `security-c2-dockerfile-injection` tests pass (13/13) - [ ] Full test suite passes (306/307, 1 pre-existing flaky timeout) - [ ] URL validation rejects all injection vectors (tested: `|`, `$()`, backticks, `&`, `;`, spaces) - [ ] `setup.sh` without `CHAT_UI_URL` behaves identically to before (no regression) - [ ] `CHAT_UI_URL=https://openclaw0-test.brevlab.com ./scripts/setup.sh` bakes origin into build <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added instructions for remote dashboard access with `CHAT_UI_URL` configuration for Brev deployments * Clarified setup differences between SSH port-forwarding and public URL scenarios * **Improvements** * Setup script now logs `CHAT_UI_URL` configuration and warns about missing remote access settings on headless systems <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…VIDIA#812) ## Summary - `setup.sh` and `onboard.js` passed `CHAT_UI_URL` only as a runtime env var, but the gateway `allowedOrigins` are generated at **build time** and locked immutable (`root:root 444` + Landlock). Remote browsers hit "origin not allowed" because the build always used the default localhost origin. - Sed the `ARG CHAT_UI_URL` line in the build-context Dockerfile copy when the env var is set, so the build-time config generator writes the correct `allowedOrigins`. URL is validated before interpolation to prevent injection. - Forward `CHAT_UI_URL` through `brev-setup.sh` and warn on headless hosts when it is unset. - Document `CHAT_UI_URL` requirement for remote deployments. Fixes NVIDIA#795, fixes NVIDIA#20 ## Test plan - [ ] Existing `security-c2-dockerfile-injection` tests pass (13/13) - [ ] Full test suite passes (306/307, 1 pre-existing flaky timeout) - [ ] URL validation rejects all injection vectors (tested: `|`, `$()`, backticks, `&`, `;`, spaces) - [ ] `setup.sh` without `CHAT_UI_URL` behaves identically to before (no regression) - [ ] `CHAT_UI_URL=https://openclaw0-test.brevlab.com ./scripts/setup.sh` bakes origin into build <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added instructions for remote dashboard access with `CHAT_UI_URL` configuration for Brev deployments * Clarified setup differences between SSH port-forwarding and public URL scenarios * **Improvements** * Setup script now logs `CHAT_UI_URL` configuration and warns about missing remote access settings on headless systems <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…VIDIA#812) ## Summary - `setup.sh` and `onboard.js` passed `CHAT_UI_URL` only as a runtime env var, but the gateway `allowedOrigins` are generated at **build time** and locked immutable (`root:root 444` + Landlock). Remote browsers hit "origin not allowed" because the build always used the default localhost origin. - Sed the `ARG CHAT_UI_URL` line in the build-context Dockerfile copy when the env var is set, so the build-time config generator writes the correct `allowedOrigins`. URL is validated before interpolation to prevent injection. - Forward `CHAT_UI_URL` through `brev-setup.sh` and warn on headless hosts when it is unset. - Document `CHAT_UI_URL` requirement for remote deployments. Fixes NVIDIA#795, fixes NVIDIA#20 ## Test plan - [ ] Existing `security-c2-dockerfile-injection` tests pass (13/13) - [ ] Full test suite passes (306/307, 1 pre-existing flaky timeout) - [ ] URL validation rejects all injection vectors (tested: `|`, `$()`, backticks, `&`, `;`, spaces) - [ ] `setup.sh` without `CHAT_UI_URL` behaves identically to before (no regression) - [ ] `CHAT_UI_URL=https://openclaw0-test.brevlab.com ./scripts/setup.sh` bakes origin into build <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added instructions for remote dashboard access with `CHAT_UI_URL` configuration for Brev deployments * Clarified setup differences between SSH port-forwarding and public URL scenarios * **Improvements** * Setup script now logs `CHAT_UI_URL` configuration and warns about missing remote access settings on headless systems <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
…roxy access (#3002) ## Summary When `CHAT_UI_URL` is set to an HTTPS URL without an explicit port — as [documented](https://docs.nvidia.com/nemoclaw/latest/deployment/deploy-to-remote-gpu.html#remote-dashboard-access) for Brev deployments — onboard injects the internal dashboard port (`:18789`) into the origin via `onboard.ts:4002`. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy, Tailscale Funnel) serve on standard `:443`, so the browser sends origin `https://host` which doesn't match `https://host:18789` in `allowedOrigins` — causing "origin not allowed" on the dashboard. **Root cause:** `generate-openclaw-config.py` builds `allowedOrigins` from the port-overridden URL only. **Fix:** Also include the portless origin for non-loopback URLs. This is safe (same host) and covers both direct and reverse-proxy access. ## Prior art PR #812 and PR #2440 fixed the `CHAT_UI_URL` plumbing but the port-override gap remained. This PR closes the remaining gap from the #795 / #20 lineage. ## Changes | File | Change | |------|--------| | `scripts/generate-openclaw-config.py` | Add portless origin to `allowedOrigins` for non-loopback URLs when a port is present | | `test/generate-openclaw-config.test.ts` | Update existing test to expect portless origin; add new test for reverse-proxy case | ## Testing - **Unit tests:** 43/43 passing (`vitest run test/generate-openclaw-config.test.ts`) - **Manual verification:** On Brev GCP (`nemoclaw-gcp`, n2-standard-4), confirmed that adding the portless origin to `allowedOrigins` resolves the CORS error when accessing `https://brev-nc-xxx.brevlab.com/chat?session=main` ## What this does NOT fix - `onboard.ts:4002` still unconditionally overrides the port. A future improvement could skip the port override when the URL is HTTPS with no explicit port, but this config-level fix is sufficient and lower-risk. ## Related - Fixes #3000 - Ref: #795 (Brev dashboard inaccessible — closed) - Ref: #20 (Remote dashboard access — closed) - Ref: PR #812 (bake CHAT_UI_URL into Docker build — merged) - Ref: PR #2440 (inject NEMOCLAW_DASHBOARD_PORT — merged) Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved origin handling: when a configured UI URL includes an explicit port and is non-loopback, the app accepts both the full origin (with port) and a portless origin; IPv6 hostnames keep bracket formatting and duplicate entries are removed. Loopback behavior is unchanged. * **Tests** * Expanded tests to cover ported and portless origins, reverse-proxy scenarios, IPv6 behavior (including loopback) and resilience to malformed port values. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Summary
setup.shandonboard.jspassedCHAT_UI_URLonly as a runtime env var, but the gatewayallowedOriginsare generated at build time and locked immutable (root:root 444+ Landlock). Remote browsers hit "origin not allowed" because the build always used the default localhost origin.ARG CHAT_UI_URLline in the build-context Dockerfile copy when the env var is set, so the build-time config generator writes the correctallowedOrigins. URL is validated before interpolation to prevent injection.CHAT_UI_URLthroughbrev-setup.shand warn on headless hosts when it is unset.CHAT_UI_URLrequirement for remote deployments.Fixes #795, fixes #20
Test plan
security-c2-dockerfile-injectiontests pass (13/13)|,$(), backticks,&,;, spaces)setup.shwithoutCHAT_UI_URLbehaves identically to before (no regression)CHAT_UI_URL=https://openclaw0-test.brevlab.com ./scripts/setup.shbakes origin into buildSummary by CodeRabbit
Release Notes
Documentation
CHAT_UI_URLconfiguration for Brev deploymentsImprovements
CHAT_UI_URLconfiguration and warns about missing remote access settings on headless systems