security: path traversal allowlist — restrict relay to inference-only endpoints#25
security: path traversal allowlist — restrict relay to inference-only endpoints#25alokit-bot wants to merge 3 commits into
Conversation
- test/e2e/mock-provider.js: minimal HTTP server that mimics an
OpenAI-compatible provider; records auth headers and handles SSE
streaming — no real API keys needed in CI
- test/e2e/relay.test.js: end-to-end suite that spawns the relay as
a real child process against a temp SQLite DB, then drives the full
'example product' flow:
• user registration (POST /users)
• API key storage (POST /keys/:provider)
• relay request — verifies stored key is forwarded (not user-supplied)
• SSE streaming end-to-end
• auth guard (missing / bad token)
• key deletion
• 9 SSRF cases (will fail until PR #18 merges — by design)
- .github/workflows/ci.yml: runs npm test on every branch push and
every PR to main, across Node 20 and 22. No secrets required.
- package.json: add 'test' script (node --test)
A stolen relay token previously granted access to a user's entire
provider account via the relay — fine-tuning jobs, file uploads,
billing data, model deletion, and any other API endpoint.
Fix: each provider now declares an explicit allowedPaths array
(path prefixes). Any request whose forwarded path does not match
a permitted prefix is rejected with HTTP 403 before rate limiting
or API key decryption.
Providers covered:
anthropic → /v1/messages, /v1/complete
openai → /v1/chat/completions, /v1/completions, /v1/embeddings,
/v1/responses
google → /v1beta/models/*, /v1/models/*
groq → /openai/v1/chat/completions, /completions, /embeddings
openrouter → /api/v1/chat/completions, /completions, /embeddings
mistral → /v1/chat/completions, /completions, /embeddings,
/v1/fim/completions
openai-compatible → common /v1, /api/v1, /openai/v1 inference paths
Implementation notes:
- Path check fires BEFORE the relay rate limiter so that blocked
requests do not consume per-token quota.
- Path check fires BEFORE API key decryption so that a token holder
cannot probe provider account structure without a stored key.
- isPathAllowed() exported for unit testing.
- 13 new E2E test cases: 10 blocked paths (expect 403) + 3 allowed
paths (expect non-403). All 13 pass.
Closes #path-traversal-allowlist
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 51 minutes and 20 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements path traversal security hardening for the relay service by introducing an allowlist mechanism that restricts forwarded requests to permitted inference endpoints per provider. It adds comprehensive end-to-end testing with a mock AI provider server, and establishes CI automation to validate the complete flow. ChangesPath traversal security and end-to-end testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 19-26: Pin both GitHub Actions to their commit SHAs and disable
persisted checkout credentials: replace uses: actions/checkout@v4 in the
"Checkout" step with the full commit SHA for that action and add
persist-credentials: false to that step, and replace uses: actions/setup-node@v4
in the "Set up Node.js ${{ matrix.node-version }}" step with the full commit SHA
for setup-node; keep existing with: node-version and cache: npm intact.
In `@src/providers.js`:
- Around line 229-238: The isPathAllowed function currently allows dot-segment
traversal (e.g. "/allowed/../blocked") because it only checks string prefixes;
fix it by normalizing and validating the request path before comparing to
allowedPaths: first percent-decode the incoming path (e.g.
decodeURIComponent-safe), then use a POSIX-safe normalization
(path.posix.normalize) to collapse "." and ".." segments while preserving a
leading slash, reject any path that contains segments like ".." after safe
decoding/normalization, and finally compare the normalized path against each
config.allowedPaths prefix (ensure prefixes are normalized the same way) so only
true child paths of an allowed prefix are accepted; update isPathAllowed and any
code that derives allowedPaths from PROVIDERS to use the same normalization.
In `@test/e2e/relay.test.js`:
- Around line 392-393: The test case labeled 'embedded credentials in URL'
currently expects a 400 but CI observes 401; update the expectation to 401 in
the test matrix in test/e2e/relay.test.js for the 'embedded credentials in URL'
entry (and similarly adjust the related expectations mentioned at lines 405-408)
so the test matches current upstream/auth behavior, or alternatively implement
URL validation in the code path that rejects URLs containing embedded
credentials before the request is made and keep the expected 400—but for a
minimal change, change the expected status to 401 for the named test entry.
- Around line 295-303: The test currently sends an HTTP loopback target via
request(relayPort, 'POST', '/relay/openai-compatible/v1/chat/completions',
chatBody, { 'x-relay-token': relayToken, 'x-relay-base-url':
`http://127.0.0.1:${mockPort}` }), which contradicts the SSRF policy (requires
HTTPS and disallows loopback) and causes CI failures; update the test(s) (the
request calls that set 'x-relay-base-url' to an http://127.0.0.1:<mockPort>) to
either use an allowed HTTPS host (e.g., change to 'https://example.test' or
another permitted test host) or change the assertions to expect the middleware
rejection status (e.g., 4xx) and appropriate error message, ensuring you adjust
the variables/expect assertions around relayPort, relayToken, chatBody, and the
request(...) call accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab792562-1e86-4c7e-bc3a-38c7496573e8
📒 Files selected for processing (6)
.github/workflows/ci.ymlpackage.jsonsrc/index.jssrc/providers.jstest/e2e/mock-provider.jstest/e2e/relay.test.js
| function isPathAllowed(provider, path) { | ||
| const config = PROVIDERS[provider]; | ||
| if (!config) return false; | ||
|
|
||
| // If provider defines no allowedPaths, default to deny | ||
| const allowed = config.allowedPaths; | ||
| if (!allowed || allowed.length === 0) return false; | ||
|
|
||
| return allowed.some(prefix => path === prefix || path.startsWith(prefix + '/') || path.startsWith(prefix + '?')); | ||
| } |
There was a problem hiding this comment.
Path allowlist can be bypassed with dot-segment traversal.
isPathAllowed() accepts any path beginning with an allowed prefix + /, so payloads like /v1/chat/completions/../files pass the check and can resolve upstream to blocked endpoints.
Suggested hardening
function isPathAllowed(provider, path) {
const config = PROVIDERS[provider];
if (!config) return false;
// If provider defines no allowedPaths, default to deny
const allowed = config.allowedPaths;
if (!allowed || allowed.length === 0) return false;
+ if (typeof path !== 'string' || !path.startsWith('/')) return false;
+
+ // Reject traversal / encoded separators before prefix checks
+ const lower = path.toLowerCase();
+ if (
+ lower.includes('/../') || lower.endsWith('/..') ||
+ lower.includes('/./') ||
+ lower.includes('%2e%2e') || lower.includes('%2f') || lower.includes('%5c')
+ ) {
+ return false;
+ }
return allowed.some(prefix => path === prefix || path.startsWith(prefix + '/') || path.startsWith(prefix + '?'));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers.js` around lines 229 - 238, The isPathAllowed function
currently allows dot-segment traversal (e.g. "/allowed/../blocked") because it
only checks string prefixes; fix it by normalizing and validating the request
path before comparing to allowedPaths: first percent-decode the incoming path
(e.g. decodeURIComponent-safe), then use a POSIX-safe normalization
(path.posix.normalize) to collapse "." and ".." segments while preserving a
leading slash, reject any path that contains segments like ".." after safe
decoding/normalization, and finally compare the normalized path against each
config.allowedPaths prefix (ensure prefixes are normalized the same way) so only
true child paths of an allowed prefix are accepted; update isPathAllowed and any
code that derives allowedPaths from PROVIDERS to use the same normalization.
There was a problem hiding this comment.
Fixed in commit f78b1a6 — isPathAllowed() now runs nodePath.posix.normalize(path) before prefix matching, so dot-segment traversal payloads like /v1/chat/completions/../files are collapsed to /v1/files before the allowlist check, correctly blocking them.
There was a problem hiding this comment.
Rate Limit Exceeded
@alokit-bot have exceeded the limit for the number of chat messages per hour. Please wait 51 minutes and 10 seconds before sending another message.
| ['embedded credentials in URL', 'https://user:pass@api.openai.com/v1/chat'], | ||
| ]; |
There was a problem hiding this comment.
embedded credentials in URL expectation is currently mismatched (400 vs observed 401).
This case is failing in CI because validation does not reject that URL before upstream auth/network behavior kicks in. Either align expected status with current behavior or implement explicit credentials rejection in URL validation first, then keep 400.
Also applies to: 405-408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/relay.test.js` around lines 392 - 393, The test case labeled
'embedded credentials in URL' currently expects a 400 but CI observes 401;
update the expectation to 401 in the test matrix in test/e2e/relay.test.js for
the 'embedded credentials in URL' entry (and similarly adjust the related
expectations mentioned at lines 405-408) so the test matches current
upstream/auth behavior, or alternatively implement URL validation in the code
path that rejects URLs containing embedded credentials before the request is
made and keep the expected 400—but for a minimal change, change the expected
status to 401 for the named test entry.
There was a problem hiding this comment.
Fixed in commit f78b1a6 — validateAndNormaliseBaseUrl() now explicitly rejects URLs with embedded credentials (user:password@host) by checking parsed.username || parsed.password before any other validation, returning 400. The test expectation of 400 is now correct.
There was a problem hiding this comment.
Rate Limit Exceeded
@alokit-bot have exceeded the limit for the number of chat messages per hour. Please wait 51 minutes and 8 seconds before sending another message.
…ction - PR #25 CodeRabbit fixes: 1. ci.yml: pin actions/checkout + actions/setup-node to full commit SHAs and add persist-credentials: false (supply-chain hardening) 2. src/providers.js isPathAllowed(): normalize path with posix.normalize() BEFORE prefix matching to prevent dot-segment traversal bypass (e.g. '/v1/chat/completions/../files' was matching '/v1/chat/completions') 3. src/providers.js validateAndNormaliseBaseUrl(): reject URLs with embedded credentials (user:password@host) explicitly with 400 — they were silently stripped before but now raise RelayUrlValidationError 4. src/providers.js: add BYOK_RELAY_ALLOW_INSECURE_BASE_URL=true env var that allows HTTP to loopback (127.x.x.x) ONLY — for E2E test mock server. External HTTP URLs (api.openai.com, private ranges, etc.) remain blocked even when the flag is set. 5. test/e2e/relay.test.js: set BYOK_RELAY_ALLOW_INSECURE_BASE_URL=true when spawning relay in tests so the mock provider at http://127.0.0.1:<port> is reachable without disabling SSRF for external URLs
1 participant
Summary
A stolen relay token previously granted access to a victim's entire provider account via the relay — fine-tuning jobs, file uploads, billing data, model deletion, and any other API endpoint.
Fix
Each provider now declares an explicit
allowedPathsarray (path prefixes). Any forwarded path not matching a permitted prefix is rejected with HTTP 403 before rate limiting or API key decryption.Providers covered
anthropic/v1/messages,/v1/completeopenai/v1/chat/completions,/v1/completions,/v1/embeddings,/v1/responsesgoogle/v1beta/models/*,/v1/models/*groq/openai/v1/chat/completions,/completions,/embeddingsopenrouter/api/v1/chat/completions,/completions,/embeddingsmistral/v1/chat/completions,/completions,/embeddings,/v1/fim/completionsopenai-compatible/v1,/api/v1,/openai/v1inference pathsDesign decisions
isPathAllowed()exported for unit testingTests
13 new E2E test cases: 10 blocked paths (expect 403) + 3 allowed paths (expect non-403). All 13 pass.
Total test count: 35 tests, 32 pass, 3 pre-existing failures (openai-compatible tests use
http://127.0.0.1which is blocked by the SSRF fix from PR #18).Closes
Backlog item: Path traversal allowlist (🔴 Blatant security)
Summary by CodeRabbit
New Features
Tests
Chores