fix: bind Ollama to localhost with authenticated reverse proxy#679
Closed
fix: bind Ollama to localhost with authenticated reverse proxy#679
Conversation
Ollama has no built-in auth. Starting it on 0.0.0.0 exposed the inference endpoint to the entire local network (PSIRT bug 6002780, CVSS 7.5 High). Fix: bind Ollama to 127.0.0.1 and front it with a token-authenticated Node.js reverse proxy on 0.0.0.0:11435. A random per-instance Bearer token is generated at onboard time and stored in the OpenShell provider credential. The gateway (in a container) sends the token; external attackers without the token get 401. GET /api/tags is exempt for container health checks. Changes: - scripts/ollama-auth-proxy.js: new authenticated reverse proxy (~65 lines) - bin/lib/onboard.js: bind Ollama to 127.0.0.1, start proxy, use proxy port and random token for provider credential - bin/lib/local-inference.js: update provider URL and container reachability check to use proxy port 11435 - scripts/setup.sh: same changes for legacy setup path - test/local-inference.test.js: update expected URLs and messages
7 tests using a mock Ollama backend (no real Ollama needed): - Mock binds to 127.0.0.1 only - Proxy starts on 0.0.0.0 with random token - Unauthenticated requests get 401 - Wrong token gets 401 - Correct token proxies to backend - GET /api/tags health check exempt from auth - POST /api/tags still requires auth Runs as test-e2e-ollama-proxy job in pr.yaml, parallel with existing lint, test-unit, and test-e2e-sandbox jobs.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Ollama authenticated reverse proxy on port 11435, binds Ollama to 127.0.0.1:11434, spawns the proxy with a per-run token during onboarding/setup, updates provider wiring and tests to use the proxy, and adds a CI job and e2e test exercising the proxy. Changes
Sequence DiagramsequenceDiagram
participant Client as Application Client
participant Proxy as Ollama Auth Proxy (11435)
participant Ollama as Ollama Backend (127.0.0.1:11434)
Note over Client,Proxy: Authenticated inference request
Client->>Proxy: POST /v1/chat/completions\nAuthorization: Bearer TOKEN
Proxy->>Proxy: Validate Authorization header
alt valid token
Proxy->>Ollama: Forward request (strip auth/host headers)
Ollama->>Proxy: 200 OK (response body)
Proxy->>Client: 200 OK (proxied body)
else invalid/missing
Proxy->>Client: 401 Unauthorized
end
Note over Client,Proxy: Unauthenticated health check
Client->>Proxy: GET /api/tags
Proxy->>Proxy: Allow unauthenticated endpoint
Proxy->>Ollama: Forward GET /api/tags
Ollama->>Proxy: 200 OK (model list)
Proxy->>Client: 200 OK (model list)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Main switched to --credential "OPENAI_API_KEY" (env-lookup form from #675). We set the proxy token in the environment so openshell reads it via the env-name-only pattern while still using our random per-instance token instead of the static "ollama" dummy value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/e2e-ollama-proxy.sh (1)
95-102: Log messages reference hardcoded port 11435 but test uses$PROXY_PORT(19435).The pass/fail messages say "port 11435" but the actual test uses the variable
$PROXY_PORTwhich is set to 19435. Consider using the variable in log messages for consistency.✏️ Proposed fix
info "2. Verify proxy is listening on port $PROXY_PORT" if curl -sf --connect-timeout 2 http://127.0.0.1:$PROXY_PORT/api/tags > /dev/null 2>&1; then - pass "Proxy responding on port 11435" + pass "Proxy responding on port $PROXY_PORT" else - fail "Proxy not responding on port 11435" + fail "Proxy not responding on port $PROXY_PORT" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-ollama-proxy.sh` around lines 95 - 102, The pass/fail log messages in the test block use a hardcoded port "11435" while the curl check uses the variable $PROXY_PORT; update the messages to reference the variable instead (use $PROXY_PORT or ${PROXY_PORT}) so the output matches the actual port being tested — locate the test block that calls info "2. Verify proxy is listening on port $PROXY_PORT" and the pass "Proxy responding on port 11435"/fail "Proxy not responding on port 11435" lines and replace the hardcoded port with the variable.bin/lib/onboard.js (2)
287-296: Proxy startup errors are silently ignored.The
{ ignoreError: true }option means if the proxy fails to start (e.g., port 11435 is already in use), onboarding continues silently. Consider checking if the proxy is actually listening before proceeding, or at minimum removingignoreError: trueto surface startup failures.♻️ Proposed fix to verify proxy started
function startOllamaAuthProxy() { const crypto = require("crypto"); ollamaProxyToken = crypto.randomBytes(24).toString("hex"); run( `OLLAMA_PROXY_TOKEN=${shellQuote(ollamaProxyToken)} ` + `node "${SCRIPTS}/ollama-auth-proxy.js" > /dev/null 2>&1 &`, - { ignoreError: true }, + { ignoreError: false }, ); sleep(1); + // Verify proxy is listening + const check = runCapture("curl -sf http://127.0.0.1:11435/api/tags 2>/dev/null", { ignoreError: true }); + if (!check) { + console.error(" Ollama auth proxy failed to start on port 11435"); + process.exit(1); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 287 - 296, The startOllamaAuthProxy function currently swallows startup failures by passing { ignoreError: true } to run; remove that option and instead verify the proxy actually started (e.g., after launching node "ollama-auth-proxy.js" wait and attempt a TCP/HTTP connect to localhost:11435 or check the spawned process is alive) and only proceed if the check succeeds, otherwise log the error and exit; update references in startOllamaAuthProxy to drop ignoreError and add a short retry/timeout loop that attempts a connection to port 11435 before returning.
871-880: The fallback token"ollama"may mask issues.If
getOllamaProxyToken()returnsnull(proxy wasn't started), falling back to"ollama"silently proceeds with a predictable token. This could indicate a logic error in the flow that should fail explicitly rather than use a fallback.Consider either removing the fallback to surface the error, or logging a warning when the fallback is used.
♻️ Option: Fail explicitly if proxy token is missing
// Use the auth proxy URL (port 11435) instead of direct Ollama (11434). // The proxy validates a per-instance Bearer token before forwarding. - const proxyToken = getOllamaProxyToken() || "ollama"; + const proxyToken = getOllamaProxyToken(); + if (!proxyToken) { + console.error(" Ollama auth proxy token not set — was startOllamaAuthProxy() called?"); + process.exit(1); + } const proxyBaseUrl = `${HOST_GATEWAY_URL}:11435/v1`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 871 - 880, The code silently falls back to the literal "ollama" token when getOllamaProxyToken() is null; instead, call getOllamaProxyToken() once, detect a missing token and either throw or log a warning before proceeding, and only then build proxyToken and proxyBaseUrl for the run(...) invocation; update the block around proxyToken/ proxyBaseUrl and the run(...) call (refer to getOllamaProxyToken, proxyToken, proxyBaseUrl, HOST_GATEWAY_URL, and the openshell provider create/update command) so that a missing token does not get silently replaced by "ollama" (choose to remove the fallback and fail fast or emit a clear warning when using a fallback).scripts/ollama-auth-proxy.js (1)
45-57: Consider adding a timeout to the backend request.If Ollama becomes unresponsive, the proxy request will hang indefinitely. Adding a timeout prevents resource exhaustion from stalled connections.
⏱️ Proposed fix to add request timeout
const proxyReq = http.request( { hostname: "127.0.0.1", port: BACKEND_PORT, path: clientReq.url, method: clientReq.method, headers, + timeout: 300000, // 5 minutes }, (proxyRes) => { clientRes.writeHead(proxyRes.statusCode, proxyRes.headers); proxyRes.pipe(clientRes); }, ); + proxyReq.on("timeout", () => { + proxyReq.destroy(); + clientRes.writeHead(504, { "Content-Type": "text/plain" }); + clientRes.end("Gateway timeout"); + }); + proxyReq.on("error", (err) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ollama-auth-proxy.js` around lines 45 - 57, The backend http.request call (proxyReq created via http.request) has no timeout so requests to BACKEND_PORT can hang; add a timeout on proxyReq (e.g., proxyReq.setTimeout or socket 'timeout') and handle the timeout by aborting/ending the proxyReq and returning an appropriate error response (504 Gateway Timeout) to clientRes; also ensure you wire up proxyReq.on('error') and clientReq.on('aborted') to abort proxyReq to avoid leaked sockets—update handlers around http.request, proxyReq, clientReq, clientRes to implement these timeout/abort/error flows.scripts/setup.sh (1)
162-165: Consider killing any existing proxy process before starting a new one.If
setup.shis run multiple times, each run spawns a new proxy process without terminating the previous one. This could lead to multiple proxy instances or port conflicts on port 11435.♻️ Proposed fix to clean up existing proxy
# Start auth proxy so containers can reach Ollama through a token gate + pkill -f "ollama-auth-proxy.js" 2>/dev/null || true OLLAMA_PROXY_TOKEN="$(head -c 24 /dev/urandom | xxd -p)" OLLAMA_PROXY_TOKEN="$OLLAMA_PROXY_TOKEN" node "$SCRIPT_DIR/ollama-auth-proxy.js" > /dev/null 2>&1 & sleep 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 162 - 165, setup.sh currently starts a new auth proxy (OLLAMA_PROXY_TOKEN + node "ollama-auth-proxy.js") without stopping any existing instance, causing duplicate processes or port 11435 conflicts; update the startup sequence to detect and terminate any existing proxy before launching: search for processes matching the script name "ollama-auth-proxy.js" or listening on port 11435 (or use pkill -f "ollama-auth-proxy.js"/lsof to find the PID), gracefully kill/stop them (SIGTERM, fallback to SIGKILL), then proceed to set OLLAMA_PROXY_TOKEN and start node "ollama-auth-proxy.js" in the background as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ollama-auth-proxy.js`:
- Around line 30-38: The token check in the HTTP server request handler uses a
plain string comparison (auth !== `Bearer ${TOKEN}`) which is susceptible to
timing attacks; replace this with a timing-safe comparison using
crypto.timingSafeEqual: when not a health check, read
clientReq.headers.authorization into a variable, build the expected string
`Bearer ${TOKEN}`, convert both to Buffer (or use Buffer.from) and first ensure
both buffers are the same length (if lengths differ, perform timingSafeEqual
against a constant-length dummy buffer to avoid leaking length info), then call
crypto.timingSafeEqual(buffAuth, buffExpected) and reject with 401 if it returns
false; update the check in the request handler that references auth, TOKEN, and
isHealthCheck accordingly.
In `@test/e2e-ollama-proxy.sh`:
- Around line 84-93: Test 1 is checking the wrong port (11434) causing a false
pass; change the curl invocation URL in the "Verify Ollama is NOT on
0.0.0.0:$MOCK_PORT" block to use the MOCK_PORT variable instead of the hardcoded
11434 (update the curl line that currently points to
http://0.0.0.0:11434/api/tags to http://0.0.0.0:$MOCK_PORT/api/tags) so the test
actually verifies the mock backend's bind behavior; adjust the surrounding
informational message if necessary to reference $MOCK_PORT.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 287-296: The startOllamaAuthProxy function currently swallows
startup failures by passing { ignoreError: true } to run; remove that option and
instead verify the proxy actually started (e.g., after launching node
"ollama-auth-proxy.js" wait and attempt a TCP/HTTP connect to localhost:11435 or
check the spawned process is alive) and only proceed if the check succeeds,
otherwise log the error and exit; update references in startOllamaAuthProxy to
drop ignoreError and add a short retry/timeout loop that attempts a connection
to port 11435 before returning.
- Around line 871-880: The code silently falls back to the literal "ollama"
token when getOllamaProxyToken() is null; instead, call getOllamaProxyToken()
once, detect a missing token and either throw or log a warning before
proceeding, and only then build proxyToken and proxyBaseUrl for the run(...)
invocation; update the block around proxyToken/ proxyBaseUrl and the run(...)
call (refer to getOllamaProxyToken, proxyToken, proxyBaseUrl, HOST_GATEWAY_URL,
and the openshell provider create/update command) so that a missing token does
not get silently replaced by "ollama" (choose to remove the fallback and fail
fast or emit a clear warning when using a fallback).
In `@scripts/ollama-auth-proxy.js`:
- Around line 45-57: The backend http.request call (proxyReq created via
http.request) has no timeout so requests to BACKEND_PORT can hang; add a timeout
on proxyReq (e.g., proxyReq.setTimeout or socket 'timeout') and handle the
timeout by aborting/ending the proxyReq and returning an appropriate error
response (504 Gateway Timeout) to clientRes; also ensure you wire up
proxyReq.on('error') and clientReq.on('aborted') to abort proxyReq to avoid
leaked sockets—update handlers around http.request, proxyReq, clientReq,
clientRes to implement these timeout/abort/error flows.
In `@scripts/setup.sh`:
- Around line 162-165: setup.sh currently starts a new auth proxy
(OLLAMA_PROXY_TOKEN + node "ollama-auth-proxy.js") without stopping any existing
instance, causing duplicate processes or port 11435 conflicts; update the
startup sequence to detect and terminate any existing proxy before launching:
search for processes matching the script name "ollama-auth-proxy.js" or
listening on port 11435 (or use pkill -f "ollama-auth-proxy.js"/lsof to find the
PID), gracefully kill/stop them (SIGTERM, fallback to SIGKILL), then proceed to
set OLLAMA_PROXY_TOKEN and start node "ollama-auth-proxy.js" in the background
as before.
In `@test/e2e-ollama-proxy.sh`:
- Around line 95-102: The pass/fail log messages in the test block use a
hardcoded port "11435" while the curl check uses the variable $PROXY_PORT;
update the messages to reference the variable instead (use $PROXY_PORT or
${PROXY_PORT}) so the output matches the actual port being tested — locate the
test block that calls info "2. Verify proxy is listening on port $PROXY_PORT"
and the pass "Proxy responding on port 11435"/fail "Proxy not responding on port
11435" lines and replace the hardcoded port with the variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10ae1ad5-f9ef-4115-a26b-db934526a016
📒 Files selected for processing (7)
.github/workflows/pr.yamlbin/lib/local-inference.jsbin/lib/onboard.jsscripts/ollama-auth-proxy.jsscripts/setup.shtest/e2e-ollama-proxy.shtest/local-inference.test.js
cv
previously approved these changes
Mar 23, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
LGTM — addresses PSIRT bug 6002780 cleanly. The proxy is minimal, token generation uses crypto.randomBytes, health check exemption is correctly scoped to GET-only, and the E2E tests are self-contained (mock backend, no real Ollama).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 287-295: startOllamaAuthProxy currently unconditionally rotates
ollamaProxyToken and backgrounds a new ollama-auth-proxy.js, which can leave the
old proxy on :11435 serving the previous token; change the flow so you first
probe localhost:11435 to see if it's already bound by a running proxy (or
attempt a handshake to verify it serves the current token), and only generate a
new ollamaProxyToken and background a new proxy when either the port is free or
you can confirm the newly spawned process successfully binds/serves :11435;
after starting the child process (the one that runs ollama-auth-proxy.js) verify
ownership by attempting a connection/handshake to :11435 and only replace
ollamaProxyToken if that verification succeeds, otherwise kill the spawned
process and keep the existing token/daemon running.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a30172d7-1dde-4d4f-9dfc-17ec3cda96fd
📒 Files selected for processing (1)
bin/lib/onboard.js
Comment on lines
+287
to
+295
| function startOllamaAuthProxy() { | ||
| const crypto = require("crypto"); | ||
| ollamaProxyToken = crypto.randomBytes(24).toString("hex"); | ||
| run( | ||
| `OLLAMA_PROXY_TOKEN=${shellQuote(ollamaProxyToken)} ` + | ||
| `node "${SCRIPTS}/ollama-auth-proxy.js" > /dev/null 2>&1 &`, | ||
| { ignoreError: true }, | ||
| ); | ||
| sleep(1); |
Contributor
There was a problem hiding this comment.
Don't rotate the proxy token unless the new proxy actually owns :11435.
This helper always generates a new token and backgrounds a new proxy, but it never checks whether :11435 is already occupied or whether the child bound successfully. Since Step 1 only cleans up the OpenShell gateway, rerunning onboarding with Ollama can leave the old proxy serving the old token while ollama-local gets updated to the new one, which breaks later requests from the sandbox.
Suggested hardening
function startOllamaAuthProxy() {
const crypto = require("crypto");
- ollamaProxyToken = crypto.randomBytes(24).toString("hex");
+ if (runCapture("curl -sf http://127.0.0.1:11435/api/tags 2>/dev/null", { ignoreError: true })) {
+ console.error(" Ollama auth proxy is already running on port 11435. Stop it before rerunning onboard.");
+ process.exit(1);
+ }
+ const token = crypto.randomBytes(24).toString("hex");
run(
- `OLLAMA_PROXY_TOKEN=${shellQuote(ollamaProxyToken)} ` +
+ `OLLAMA_PROXY_TOKEN=${shellQuote(token)} ` +
`node "${SCRIPTS}/ollama-auth-proxy.js" > /dev/null 2>&1 &`,
{ ignoreError: true },
);
sleep(1);
+ if (!runCapture("curl -sf http://127.0.0.1:11435/api/tags 2>/dev/null", { ignoreError: true })) {
+ console.error(" Ollama auth proxy failed to start on port 11435.");
+ process.exit(1);
+ }
+ ollamaProxyToken = token;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 287 - 295, startOllamaAuthProxy currently
unconditionally rotates ollamaProxyToken and backgrounds a new
ollama-auth-proxy.js, which can leave the old proxy on :11435 serving the
previous token; change the flow so you first probe localhost:11435 to see if
it's already bound by a running proxy (or attempt a handshake to verify it
serves the current token), and only generate a new ollamaProxyToken and
background a new proxy when either the port is free or you can confirm the newly
spawned process successfully binds/serves :11435; after starting the child
process (the one that runs ollama-auth-proxy.js) verify ownership by attempting
a connection/handshake to :11435 and only replace ollamaProxyToken if that
verification succeeds, otherwise kill the spawned process and keep the existing
token/daemon running.
ericksoa
dismissed
cv’s stale review
New commits pushed (merge conflict resolution) — needs fresh review.
5 tasks
- Use crypto.timingSafeEqual for token comparison (prevents timing attacks) - Fix hardcoded port 11434 in E2E test 1 (should use $MOCK_PORT) - Kill stale proxy on :11435 before starting new one (prevents token mismatch on re-onboard) - Verify proxy is listening after start
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-ollama-proxy.sh (1)
84-93: Test 1 always passes regardless of curl outcome.The test prints an informational message when curl succeeds but unconditionally calls
passon line 93. This means the test cannot detect a failure if the mock backend were somehow reachable on0.0.0.0. The comments explain this is intentional for Linux where0.0.0.0may resolve to loopback, so the test is informational rather than a hard assertion.Consider making this explicit by using
infoinstead ofpass, or renaming to clarify this is a platform-specific check rather than a pass/fail assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-ollama-proxy.sh` around lines 84 - 93, The test currently always calls pass "Ollama bound to 127.0.0.1 only" regardless of the curl result, so replace that unconditional pass with either an info call or make the pass conditional; specifically update the block that uses curl, info, and pass (references: curl -sf --connect-timeout 2 http://0.0.0.0:$MOCK_PORT/api/tags, info, pass) so it logs a non-fatal informational message on platforms where 0.0.0.0 can resolve to loopback (use info "Ollama bound to 127.0.0.1 only") or only call pass when you can reliably assert failure (e.g., detect a non-loopback interface and only then fail), ensuring the intent is explicit and the test does not unconditionally pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e-ollama-proxy.sh`:
- Around line 95-102: The pass/fail messages are hardcoded to "port 11435" while
the check uses the PROXY_PORT variable; update the calls to pass and fail so
they reference the PROXY_PORT variable (e.g., pass "Proxy responding on port
$PROXY_PORT" and fail "Proxy not responding on port $PROXY_PORT") making sure to
use double quotes so the $PROXY_PORT expands; change the two message strings
associated with the curl check that uses PROXY_PORT accordingly.
---
Nitpick comments:
In `@test/e2e-ollama-proxy.sh`:
- Around line 84-93: The test currently always calls pass "Ollama bound to
127.0.0.1 only" regardless of the curl result, so replace that unconditional
pass with either an info call or make the pass conditional; specifically update
the block that uses curl, info, and pass (references: curl -sf --connect-timeout
2 http://0.0.0.0:$MOCK_PORT/api/tags, info, pass) so it logs a non-fatal
informational message on platforms where 0.0.0.0 can resolve to loopback (use
info "Ollama bound to 127.0.0.1 only") or only call pass when you can reliably
assert failure (e.g., detect a non-loopback interface and only then fail),
ensuring the intent is explicit and the test does not unconditionally pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fefebe87-5687-404a-8aac-8fae8c74cdd1
📒 Files selected for processing (3)
bin/lib/onboard.jsscripts/ollama-auth-proxy.jstest/e2e-ollama-proxy.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ollama-auth-proxy.js
Comment on lines
+95
to
+102
| # ── Test 2: Proxy is listening on 11435 ────────────────────────── | ||
|
|
||
| info "2. Verify proxy is listening on port $PROXY_PORT" | ||
| if curl -sf --connect-timeout 2 http://127.0.0.1:$PROXY_PORT/api/tags > /dev/null 2>&1; then | ||
| pass "Proxy responding on port 11435" | ||
| else | ||
| fail "Proxy not responding on port 11435" | ||
| fi |
Contributor
There was a problem hiding this comment.
Hardcoded port number in pass/fail messages doesn't match variable.
The test uses $PROXY_PORT (19435) but the messages on lines 99 and 101 hardcode "port 11435". This could confuse debugging output.
🐛 Proposed fix
info "2. Verify proxy is listening on port $PROXY_PORT"
if curl -sf --connect-timeout 2 http://127.0.0.1:$PROXY_PORT/api/tags > /dev/null 2>&1; then
- pass "Proxy responding on port 11435"
+ pass "Proxy responding on port $PROXY_PORT"
else
- fail "Proxy not responding on port 11435"
+ fail "Proxy not responding on port $PROXY_PORT"
fi📝 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
| # ── Test 2: Proxy is listening on 11435 ────────────────────────── | |
| info "2. Verify proxy is listening on port $PROXY_PORT" | |
| if curl -sf --connect-timeout 2 http://127.0.0.1:$PROXY_PORT/api/tags > /dev/null 2>&1; then | |
| pass "Proxy responding on port 11435" | |
| else | |
| fail "Proxy not responding on port 11435" | |
| fi | |
| # ── Test 2: Proxy is listening on 11435 ────────────────────────── | |
| info "2. Verify proxy is listening on port $PROXY_PORT" | |
| if curl -sf --connect-timeout 2 http://127.0.0.1:$PROXY_PORT/api/tags > /dev/null 2>&1; then | |
| pass "Proxy responding on port $PROXY_PORT" | |
| else | |
| fail "Proxy not responding on port $PROXY_PORT" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e-ollama-proxy.sh` around lines 95 - 102, The pass/fail messages are
hardcoded to "port 11435" while the check uses the PROXY_PORT variable; update
the calls to pass and fail so they reference the PROXY_PORT variable (e.g., pass
"Proxy responding on port $PROXY_PORT" and fail "Proxy not responding on port
$PROXY_PORT") making sure to use double quotes so the $PROXY_PORT expands;
change the two message strings associated with the curl check that uses
PROXY_PORT accordingly.
wscurran
added
security
- pr.yaml: keep both gateway-isolation and ollama-proxy CI jobs, bump to actions v6 - onboard.js: keep both proxy functions and ensureNamedCredential - onboard.js: adopt upsertProvider/runOpenshell API with proxy token - setup.sh: keep localhost binding with main's whitespace style
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
1134-1138: Consider failing on proxy startup failure.When the proxy doesn't start, the function logs a warning but continues. Downstream,
setupInferencewill configure the provider with the new token pointing at a non-responsive (or stale-token) proxy, causing all inference requests to fail with 401/502. Failing early here would surface the root cause immediately.Suggested hardening
// Verify proxy is actually listening before proceeding const probe = runCapture("curl -sf --connect-timeout 2 http://127.0.0.1:11435/api/tags 2>/dev/null", { ignoreError: true }); if (!probe) { - console.error(" Warning: Ollama auth proxy did not start on :11435"); + console.error(" Ollama auth proxy failed to start on :11435. Cannot proceed."); + process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1134 - 1138, The code currently logs a warning when the auth proxy probe (runCapture call assigned to probe) fails but continues, which can cause setupInference to configure a provider against a non-responsive/stale proxy; change the probe failure handling to abort early (throw or process.exit with non-zero code) when the curl probe to http://127.0.0.1:11435/api/tags fails so the onboarding stops instead of proceeding to setupInference, keeping the existing console.error message but adding an immediate exit to surface the root cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1989-1992: The current fallback token "ollama" masks missing proxy
tokens and causes 401s; replace the silent fallback by checking
getOllamaProxyToken() before calling upsertProvider and fail fast if it returns
null/undefined: in the block where proxyToken is assigned (calls to
getOllamaProxyToken and the subsequent upsertProvider("ollama-local", ...)), if
getOllamaProxyToken() returns falsy, log a clear error via processLogger.error
(or throw a descriptive Error) stating the Ollama proxy token is missing and
stop further execution (process.exit(1) or propagate the error) so you don't
register "ollama-local" with an incorrect token; alternatively, if you prefer
persistence, implement writing/reading the token to a known file under
~/.nemoclaw/ and read that via getOllamaProxyToken so upsertProvider receives a
stable token—choose one approach and apply it around
getOllamaProxyToken/startOllamaAuthProxy and the upsertProvider("ollama-local",
...) call.
In `@scripts/setup.sh`:
- Around line 167-171: Kill any existing proxy bound to port 11435 before
starting a new one, start ollama-auth-proxy.js with OLLAMA_PROXY_TOKEN but
capture and export its PID instead of backgrounding silently, then poll the
local URL (http://host.openshell.internal:11435/v1 or localhost:11435) for up to
a short timeout to verify the proxy is listening and responding (fail and print
an error if it never becomes available); reference the OLLAMA_PROXY_TOKEN
variable, the ollama-auth-proxy.js process, and OLLAMA_LOCAL_BASE_URL in your
changes and ensure stdout/stderr from the proxy is not discarded so failures are
visible.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1134-1138: The code currently logs a warning when the auth proxy
probe (runCapture call assigned to probe) fails but continues, which can cause
setupInference to configure a provider against a non-responsive/stale proxy;
change the probe failure handling to abort early (throw or process.exit with
non-zero code) when the curl probe to http://127.0.0.1:11435/api/tags fails so
the onboarding stops instead of proceeding to setupInference, keeping the
existing console.error message but adding an immediate exit to surface the root
cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9c4fcfe-778c-4b5c-bfb7-ee7274f9abc2
📒 Files selected for processing (5)
.github/workflows/pr.yamlbin/lib/local-inference.jsbin/lib/onboard.jsscripts/setup.shtest/local-inference.test.js
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/pr.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/local-inference.test.js
- bin/lib/local-inference.js
Comment on lines
+1989
to
+1992
| const proxyToken = getOllamaProxyToken() || "ollama"; | ||
| const proxyBaseUrl = `${HOST_GATEWAY_URL}:11435/v1`; | ||
| upsertProvider("ollama-local", "openai", "OPENAI_API_KEY", proxyBaseUrl, { | ||
| OPENAI_API_KEY: proxyToken, |
Contributor
There was a problem hiding this comment.
Fallback token "ollama" will cause 401s if proxy is running with a different token.
getOllamaProxyToken() returns null if startOllamaAuthProxy() was never called in this process (e.g., re-running setupInference in isolation or if the call order changes). The fallback "ollama" will not match the token the running proxy expects, silently breaking inference.
Consider either:
- Making the proxy token persistent (e.g., write to a file in
~/.nemoclaw/alongside credentials), or - Failing explicitly when the token is unavailable for
ollama-local.
Option 2: Fail explicitly
- const proxyToken = getOllamaProxyToken() || "ollama";
+ const proxyToken = getOllamaProxyToken();
+ if (!proxyToken) {
+ console.error(" Ollama proxy token is not set. Run the full onboard flow to initialize the proxy.");
+ process.exit(1);
+ }📝 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
| const proxyToken = getOllamaProxyToken() || "ollama"; | |
| const proxyBaseUrl = `${HOST_GATEWAY_URL}:11435/v1`; | |
| upsertProvider("ollama-local", "openai", "OPENAI_API_KEY", proxyBaseUrl, { | |
| OPENAI_API_KEY: proxyToken, | |
| const proxyToken = getOllamaProxyToken(); | |
| if (!proxyToken) { | |
| console.error(" Ollama proxy token is not set. Run the full onboard flow to initialize the proxy."); | |
| process.exit(1); | |
| } | |
| const proxyBaseUrl = `${HOST_GATEWAY_URL}:11435/v1`; | |
| upsertProvider("ollama-local", "openai", "OPENAI_API_KEY", proxyBaseUrl, { | |
| OPENAI_API_KEY: proxyToken, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1989 - 1992, The current fallback token
"ollama" masks missing proxy tokens and causes 401s; replace the silent fallback
by checking getOllamaProxyToken() before calling upsertProvider and fail fast if
it returns null/undefined: in the block where proxyToken is assigned (calls to
getOllamaProxyToken and the subsequent upsertProvider("ollama-local", ...)), if
getOllamaProxyToken() returns falsy, log a clear error via processLogger.error
(or throw a descriptive Error) stating the Ollama proxy token is missing and
stop further execution (process.exit(1) or propagate the error) so you don't
register "ollama-local" with an incorrect token; alternatively, if you prefer
persistence, implement writing/reading the token to a known file under
~/.nemoclaw/ and read that via getOllamaProxyToken so upsertProvider receives a
stable token—choose one approach and apply it around
getOllamaProxyToken/startOllamaAuthProxy and the upsertProvider("ollama-local",
...) call.
cv
approved these changes
Mar 25, 2026
2 tasks
fdzdev
approved these changes
Mar 25, 2026
Contributor
fdzdev
left a comment
fdzdev
left a comment
There was a problem hiding this comment.
Solid fix. Token comparison uses crypto.timingSafeEqual (no timing side
channel), health check exemption is correctly scoped to GET /api/tags only
(POST requires auth per test 7), and 24 random bytes is sufficient entropy.
E2E test suite is thorough, mock backend avoids needing real Ollama in CI.
Minor observation: lsof -ti :11435 | xargs kill in startOllamaAuthProxy()
will kill anything on that port, not just a stale proxy. Low risk for a dev
tool, but a PID file check would be more surgical.
Contributor
Author
|
Closing in favor of #1104, which merged the Ollama binding fix via a narrower WSL2-specific approach. The auth proxy layer from this PR can be revisited separately if needed. |
prekshivyas
added a commit
that referenced
this pull request
Apr 15, 2026
Ollama has no built-in auth and binding to 0.0.0.0 exposes it to the network (CWE-668, #1140). This adds an authenticated reverse proxy so Ollama stays on localhost while containers can still reach it. - Add scripts/ollama-auth-proxy.js — Node.js proxy on 0.0.0.0:11435 that validates a per-instance Bearer token before forwarding to Ollama on 127.0.0.1:11434. Health check (GET /api/tags) is exempt. Uses crypto.timingSafeEqual for timing-safe token comparison. - Bind Ollama to 127.0.0.1 instead of 0.0.0.0 during onboard - Start the auth proxy after Ollama, with stale proxy cleanup and startup verification - Route sandbox inference through proxy port (11435) with the generated token as the OpenAI API key credential - Gate macOS hint on process.platform === "darwin" - Add OLLAMA_PROXY_PORT (11435) to ports.ts - Add 7 e2e tests and CI job for the proxy - Update unit tests for new port and error messages Reimplements the approach from #679 (closed in favor of #1104) against the current TypeScript codebase, addressing CodeRabbit findings from the original PR (timing-safe comparison, stale proxy cleanup, startup verification). Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
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
6 participants
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
Ollama has no built-in authentication. During onboard, we start it on all interfaces so the OpenShell gateway (running in a container) can reach it. This change tightens the binding to localhost and adds an authenticated reverse proxy so containers can still reach it through a token gate.
127.0.0.1:11434instead of0.0.0.0:11434scripts/ollama-auth-proxy.js— lightweight Node.js reverse proxy on0.0.0.0:11435that validates a per-instance Bearer token before forwarding to OllamaGET /api/tagsfor container health checkssetup.shlegacy path with same patternTest plan
test/e2e-ollama-proxy.sh) — 7 tests using a mock Ollama backend, no real Ollama needed:test-e2e-ollama-proxyas parallel CI job inpr.yamlnemoclaw onboardwith Ollama provider, verify inference works through proxySummary by CodeRabbit
New Features
Tests
Chores