Skip to content

[codex] Harden provider node URL validation - #5760

Merged
diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.43from
nguyenxvotanminh3:codex/security-provider-node-cors
Jul 2, 2026
Merged

diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.43from
nguyenxvotanminh3:codex/security-provider-node-cors

Conversation

@nguyenxvotanminh3

Copy link
Copy Markdown
Contributor

Summary

  • Validate custom provider-node base URLs during create/update with the existing outbound URL guard. Private/local provider targets are blocked by default, while the explicit OMNIROUTE_ALLOW_PRIVATE_PROVIDER_URLS=true opt-in still works.
  • Tighten token-auth CORS fallback so unlisted public web origins are not reflected by default; loopback local browser/Electron origins and explicit allowlists still work.
  • Add regression coverage for blocked provider URLs, opt-in private URLs, update safety, and token API CORS behavior.

Validation

  • node --import tsx/esm --test tests/unit/custom-headers-provider-nodes.test.ts tests/unit/cc-compatible-provider.test.ts tests/unit/provider-nodes-route.test.ts tests/unit/authz/pipeline.test.ts tests/unit/cors/origins.test.ts
  • npm run typecheck:core
  • npx eslint src/app/api/provider-nodes/urlGuard.ts src/app/api/provider-nodes/route.ts src/app/api/provider-nodes/[id]/route.ts src/server/cors/origins.ts tests/unit/provider-nodes-route.test.ts tests/unit/cors/origins.test.ts tests/unit/authz/pipeline.test.ts
  • git diff --check
  • Commit/push hooks: docs sync, explicit-any budget, tracked artifacts

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@nguyenxvotanminh3
nguyenxvotanminh3 marked this pull request as ready for review July 1, 2026 04:31
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@diegosouzapw diegosouzapw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the security-hardening work, @nguyenxvotanminh3 — the direction is right, but there are a few blocking issues (verified against the code). Requesting changes:

🔴 1. Wrong guard function — breaks local providers by default. urlGuard.ts calls getProviderOutboundGuard(), which is arePrivateProviderUrlsAllowed() ? "none" : "public-only" — so with the default (private URLs OFF) it returns public-only and blocks http://127.0.0.1:11434 (Ollama / LM Studio / llama.cpp) with a 400. That contradicts OmniRoute's local-first stance. Use getProviderValidationGuard() instead: its default is block-metadata (allows LAN/loopback, blocks only cloud-metadata like 169.254.169.254). The new test provider nodes route blocks private ... base URLs by default asserts the broken 400, so please flip it to assert 200 for 127.0.0.1:11434 by default and 400 only for a cloud-metadata URL (or when OMNIROUTE_ALLOW_LOCAL_PROVIDER_URLS=false).

🔴 2. CORS loopback check has a bypass. isLoopbackBrowserOrigin() uses host.startsWith("127."), which matches attacker-registrable public hostnames — http://127.evil.com, http://127.0.0.1.nip.io — and echoes them into Access-Control-Allow-Origin. Please use an exact match (host === "127.0.0.1" || host === "::1" || host === "localhost" || host.endsWith(".localhost")) or validate the full 127.0.0.0/8 range numerically, not by string prefix.

🟠 3. Hard Rule #12 (error sanitization). For an OutboundUrlGuardError with code OUTBOUND_URL_INVALID, error.message echoes the raw user-supplied URL back in error.details[].message. Please substitute a fixed string (e.g. "Invalid provider base URL format") for that code path (the PROVIDER_URL_BLOCKED / CLOUD_METADATA_BLOCKED messages are already fixed strings and are fine).

🟠 4. CORS tightening is a breaking change + overlaps #5759. Narrowing /v1/* relaxForTokenAuth from echo-any-origin to loopback-only will CORS-block public-hosted browser API clients (these routes are token-authed via Authorization/x-api-key, which browsers never auto-attach — no CSRF risk, which is why the echo was intentional). If we want this, it needs a CHANGELOG entry + a docs note that operators must set CORS_ALLOWED_ORIGINS. It also overlaps the in-flight #5759 (CORS work for #5602) — let's reconcile the two before merging so we don't double-touch applyCorsHeaders.

Happy to help once these are addressed. The SSRF guard (with the correct function) and the exact-loopback CORS check are both good additions.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks for the hardening pass. Three blocking issues before this can land, plus a base + coordination note:

  1. [blocking — breaks local-first] Wrong guard. urlGuard.ts uses getProviderOutboundGuard(), which defaults to public-only and rejects 127.0.0.1 / localhost / RFC-1918. OmniRoute is local-first — Ollama (http://127.0.0.1:11434), LM Studio, and llama.cpp would all be blocked from registration by default. Use getProviderValidationGuard() instead (default block-metadata: allows LAN/loopback, blocks only cloud-metadata like 169.254.169.254). The test asserting 400 for http://127.0.0.1:11434/v1 by default should flip to 200; keep a 400 assertion for a cloud-metadata URL.
  2. [blocking — security] CORS bypass. isLoopbackBrowserOrigin() uses host.startsWith("127."), which also matches public registrable hostnames like 127.evil.com or 127.0.0.1.nip.io, echoing them into Access-Control-Allow-Origin. Use an exact match / numeric 127.0.0.0/8 range check.
  3. [blocking — Rule fix(ui): fix Select dropdown dark theme inconsistency #12] Error leak. In the catch block, the OUTBOUND_URL_INVALID path echoes error.message, which embeds the raw user-supplied URL. Substitute a static string for that error code.
  4. [coordination] This overlaps origins.ts / origins.test.ts with feat(dashboard): wildcard-CORS runtime warning + CORS security doc (#5602) #5759 (the CORS work for docs(api): CORS security guidance + dashboard warning when CORS_ALLOW_ALL=true is set #5602). Let's land feat(dashboard): wildcard-CORS runtime warning + CORS security doc (#5602) #5759 first and reconcile against it. Also, the relaxForTokenAuth echo→loopback-only change is user-visible and breaking, so it needs a CHANGELOG entry plus a note that operators must set CORS_ALLOWED_ORIGINS for public browser clients.
  5. [base] Retarget main → release/v3.8.43.

Leaving the PR open for the revision.

@nguyenxvotanminh3
nguyenxvotanminh3 force-pushed the codex/security-provider-node-cors branch from b50f8fb to 8c16e31 Compare July 1, 2026 08:28
@nguyenxvotanminh3 nguyenxvotanminh3 changed the title [codex] Harden provider node URLs and token CORS [codex] Harden provider node URL validation Jul 1, 2026
@nguyenxvotanminh3
nguyenxvotanminh3 changed the base branch from main to release/v3.8.43 July 1, 2026 08:28
@nguyenxvotanminh3

Copy link
Copy Markdown
Contributor Author

Thanks Diego, I updated the PR based on your review.

Changes made:

  • Switched provider-node create/update validation to getProviderValidationGuard() so local-first providers remain allowed by default (127.0.0.1, localhost, LAN/Ollama/LM Studio/llama.cpp), while cloud metadata URLs are still blocked.
  • Flipped/added tests so http://127.0.0.1:11434/v1 is allowed by default, local URLs are blocked only when OMNIROUTE_ALLOW_LOCAL_PROVIDER_URLS=false, and 169.254.169.254 is blocked by default.
  • Sanitized invalid provider base URL errors so OUTBOUND_URL_INVALID returns the fixed message Invalid provider base URL format instead of echoing user-supplied input.
  • Removed the CORS tightening from this PR to avoid overlapping with feat(dashboard): wildcard-CORS runtime warning + CORS security doc (#5602) #5759 and to keep the user-visible token-auth CORS behavior unchanged here.
  • Retargeted the PR from main to release/v3.8.43 and updated the title to match the narrower scope.

Verification performed locally:

  • node --import tsx/esm --test tests/unit/provider-nodes-route.test.ts tests/unit/cc-compatible-provider.test.ts tests/unit/provider-nodes-validate-modelid.test.ts tests/unit/provider-nodes-validate-docker-hint.test.ts tests/unit/cors/origins.test.ts tests/unit/authz/pipeline.test.ts tests/unit/webhook-ssrf-guard.test.ts tests/unit/webhook-metadata-guard-3269.test.ts tests/unit/webhook-private-optin-3269.test.ts — 128/128 passed.
  • npm run test:security — 23/23 passed.
  • npm run typecheck:core — passed.
  • Scoped eslint on changed provider-node files/tests — 0 errors.
  • git diff --check and pre-push hooks — passed.

I also ran a live local-first verification from this branch on an isolated data dir/port: created an OpenAI-compatible provider node pointing at a dummy local provider on 127.0.0.1, added a connection for it, and successfully called /v1/chat/completions with llive/local-agent, receiving local agent ok. Metadata and malformed provider URLs were blocked/sanitized as expected.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Maintainer note — validated release-green ✅

I merged this PR's latest revision onto the current release/v3.8.43 tip in an isolated worktree and ran the touched suites:

  • tests/unit/provider-nodes-route.test.ts + tests/unit/cc-compatible-provider.test.ts → 39 pass / 0 fail
  • typecheck:core → clean

The switch from getProviderOutboundGuard (default public-only) to getProviderValidationGuard (default block-metadata) is the right call: getProviderValidationGuard() returns block-metadata by default (areLocalProviderUrlsAllowed() is ON by default), so it still allows loopback/local provider URLs like http://127.0.0.1:11434/v1 (Ollama) while blocking SSRF to cloud metadata endpoints. Local-first is preserved and the earlier request-changes concern is addressed.

This is ready to merge; it's only being held right now by the active release freeze (#5826 — v3.8.43 reconciliation). It'll be merged once the freeze lifts.

@diegosouzapw
diegosouzapw dismissed their stale review July 2, 2026 00:52

Stale review — all round-6 findings were fully addressed in the force-pushed head (local-first guard via getProviderValidationGuard, CORS scope removed, error message sanitized with regression test). Verified in re-review 2026-07-01.

@diegosouzapw
diegosouzapw merged commit 19703cd into cheaper-inference:release/v3.8.43 Jul 2, 2026
1 check passed
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks @nguyenxvotanminh3! You addressed every point from the previous review — validation guard local-first by default, metadata endpoints blocked, sanitized errors with regression tests. Textbook response to feedback. Merged into release/v3.8.43. 🙌

@diegosouzapw diegosouzapw mentioned this pull request Jul 2, 2026
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
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