Skip to content

fix: bind Ollama to localhost with authenticated reverse proxy#679

Open
ericksoa wants to merge 5 commits intomainfrom
fix/ollama-localhost
Open

fix: bind Ollama to localhost with authenticated reverse proxy#679
ericksoa wants to merge 5 commits intomainfrom
fix/ollama-localhost

Conversation

@ericksoa
Copy link
Contributor

@ericksoa ericksoa commented Mar 23, 2026

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.

  • Bind Ollama to 127.0.0.1:11434 instead of 0.0.0.0:11434
  • Add scripts/ollama-auth-proxy.js — lightweight Node.js reverse proxy on 0.0.0.0:11435 that validates a per-instance Bearer token before forwarding to Ollama
  • Generate a random token at onboard time, store it in the OpenShell provider credential
  • Update provider base URL to use proxy port (11435)
  • Allow unauthenticated GET /api/tags for container health checks
  • Update setup.sh legacy path with same pattern

Test plan

  • 219/219 unit tests pass
  • New E2E test suite (test/e2e-ollama-proxy.sh) — 7 tests using a mock Ollama backend, no real Ollama needed:
    • Mock binds to localhost only
    • Proxy rejects unauthenticated requests (401)
    • Proxy rejects wrong tokens (401)
    • Correct token proxies successfully
    • Health check endpoint exempt from auth
    • POST to health check path still requires auth
  • Added test-e2e-ollama-proxy as parallel CI job in pr.yaml
  • Manual: nemoclaw onboard with Ollama provider, verify inference works through proxy

Summary by CodeRabbit

  • New Features

    • Added an authenticated local proxy for Ollama, enforcing bearer-token auth for inference while keeping an unauthenticated health endpoint.
  • Tests

    • Added end-to-end tests that validate proxy authentication, forwarding, and health-check behavior.
  • Chores

    • Added a CI job to run the new e2e proxy tests.
    • Updated local setup to run Ollama behind the auth proxy and use per-instance credentials.

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.
@coderabbitai
Copy link

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

Adds an Ollama authenticated reverse proxy on port 11435, reconfigures local Ollama to bind 127.0.0.1:11434, updates onboarding/setup to spawn the proxy with a per-instance token, adjusts provider wiring and tests to use the proxy, and adds a CI job running an e2e proxy test.

Changes

Cohort / File(s) Summary
CI Workflow
.github/workflows/pr.yaml
Added test-e2e-ollama-proxy job to run test/e2e-ollama-proxy.sh on ubuntu-latest with Node.js v22 and a 5-minute timeout.
Auth Proxy Script
scripts/ollama-auth-proxy.js
New Node.js HTTP reverse proxy requiring OLLAMA_PROXY_TOKEN, listens on OLLAMA_PROXY_PORT (default 11435), forwards to backend 127.0.0.1:11434, allows unauthenticated GET /api/tags, enforces Authorization: Bearer <TOKEN> for other endpoints.
Local Inference & Onboarding
bin/lib/local-inference.js, bin/lib/onboard.js, scripts/setup.sh
Switch local Ollama to 127.0.0.1:11434, start auth proxy with a generated token, update base URL to http://host.openshell.internal:11435/v1, and pass per-instance proxy token as provider credential. Updated messages and reachability checks.
Tests
test/local-inference.test.js, test/e2e-ollama-proxy.sh
Unit tests updated to expect proxy port 11435. Added test/e2e-ollama-proxy.sh that starts a mock backend and validates auth behavior (401 on missing/invalid token, proxying on valid token, unauthenticated GET /api/tags, etc.).

Sequence Diagram

sequenceDiagram
    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)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I brewed a token, small and bright,
Hid Ollama snug on loopback tight,
Proxy on one-one-four-three-five,
Guards the chats and keeps them alive,
Health checks hop in without a fight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the pull request: binding Ollama to localhost and adding an authenticated reverse proxy to control container access.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ollama-localhost

Comment @coderabbitai help to get the list of available commands and usage tips.

@ericksoa ericksoa requested a review from kjw3 March 23, 2026 01:05
@ericksoa ericksoa self-assigned this Mar 23, 2026
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.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_PORT which 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 removing ignoreError: true to 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() returns null (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.sh is 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

📥 Commits

Reviewing files that changed from the base of the PR and between ffa1283 and 96f248c.

📒 Files selected for processing (7)
  • .github/workflows/pr.yaml
  • bin/lib/local-inference.js
  • bin/lib/onboard.js
  • scripts/ollama-auth-proxy.js
  • scripts/setup.sh
  • test/e2e-ollama-proxy.sh
  • test/local-inference.test.js

cv
cv previously approved these changes Mar 23, 2026
Copy link
Contributor

@cv cv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 96f248c and a205395.

📒 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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 ericksoa marked this pull request as draft March 23, 2026 01:33
@ericksoa ericksoa marked this pull request as ready for review March 23, 2026 01:33
@ericksoa ericksoa dismissed cv’s stale review March 23, 2026 01:34

New commits pushed (merge conflict resolution) — needs fresh review.

- 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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pass on line 93. This means the test cannot detect a failure if the mock backend were somehow reachable on 0.0.0.0. The comments explain this is intentional for Linux where 0.0.0.0 may resolve to loopback, so the test is informational rather than a hard assertion.

Consider making this explicit by using info instead of pass, 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

📥 Commits

Reviewing files that changed from the base of the PR and between a205395 and d18684f.

📒 Files selected for processing (3)
  • bin/lib/onboard.js
  • scripts/ollama-auth-proxy.js
  • test/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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants