Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens gateway API authentication by removing the implicit “localhost/loopback is authenticated when WEB_API_TOKEN is unset” behavior, and adds regression tests to ensure unauthenticated requests are rejected and token-based auth paths behave as expected.
Changes:
- Remove loopback-address fallback from
hasApiAuth(); API access now requiresWEB_API_TOKEN,GATEWAY_API_TOKEN, and/or allowed query-token auth. - Update the gateway HTTP server test harness to default to a gateway token and explicitly mark unauthenticated requests in relevant cases.
- Add/adjust regression tests for loopback requests, empty bearer tokens, and terminal websocket upgrades with explicit request auth.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/gateway/gateway-http-server.ts |
Removes loopback-based API auth fallback by updating hasApiAuth() logic. |
tests/gateway-http-server.test.ts |
Updates request helper/default token mocking and adds/updates auth regression coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const authHeader = req.headers.authorization || ''; | ||
| const gatewayTokenMatch = | ||
| Boolean(GATEWAY_API_TOKEN) && authHeader === `Bearer ${GATEWAY_API_TOKEN}`; | ||
| if (opts?.allowQueryToken && url && hasQueryToken(url)) return true; | ||
|
|
||
| if (!WEB_API_TOKEN) { | ||
| return gatewayTokenMatch || isLoopbackAddress(req.socket.remoteAddress); | ||
| } | ||
| if (authHeader === `Bearer ${WEB_API_TOKEN}`) return true; | ||
| if (WEB_API_TOKEN && authHeader === `Bearer ${WEB_API_TOKEN}`) return true; | ||
| return gatewayTokenMatch; |
There was a problem hiding this comment.
With loopback fallback removed, external docs that say “if WEB_API_TOKEN is unset, loopback requests can omit Authorization” are now outdated (e.g. docs/reference/commands.md mentions this). Please update the relevant docs alongside this behavior change so users don’t rely on the old localhost-bypass guidance.
| return Object.assign(Readable.from(chunks), { | ||
| method: params.method || 'GET', | ||
| url: params.url, | ||
| headers: params.headers || {}, | ||
| headers: { | ||
| ...(params.authenticated === false | ||
| ? {} | ||
| : { authorization: `Bearer ${DEFAULT_TEST_GATEWAY_API_TOKEN}` }), | ||
| ...(params.headers || {}), | ||
| }, |
There was a problem hiding this comment.
makeRequest() now injects an Authorization header by default, which can silently change the meaning of many tests (e.g., tests meant to exercise query-token auth or “no-auth” paths will actually be Bearer-authenticated unless authenticated: false is set). Consider making the default be no Authorization header (and requiring tests to opt-in), or replace authenticated with an explicit auth/authToken option to avoid accidental coverage gaps.
| remoteAddress: '203.0.113.10', | ||
| headers: { 'x-forwarded-for': '127.0.0.1' }, | ||
| authenticated: false, | ||
| }); |
There was a problem hiding this comment.
The new loopback-auth regression test includes an x-forwarded-for: 127.0.0.1 header but leaves the socket remoteAddress at its default (also loopback). If the intent is to cover “forwarded loopback headers don’t grant auth”, set remoteAddress to a non-loopback value and keep x-forwarded-for loopback so the test actually exercises that scenario.
| }); | |
| }); | |
| Object.defineProperty(req.socket, 'remoteAddress', { | |
| value: '203.0.113.10', | |
| configurable: true, | |
| }); |
Summary
Root Cause
hasApiAuth treated the raw socket peer as authenticated when WEB_API_TOKEN was unset. That made local peers bypass bearer-token auth, and it was especially risky when the gateway was deployed behind a proxy or local process boundaries were compromised.
Validation