Conversation
- Strip accidental "Bearer " prefix from COPILOT_API_KEY/COPILOT_GITHUB_TOKEN in resolveCopilotAuthToken() and createCopilotAdapter() to prevent double-prefixed "Authorization: Bearer Bearer <key>" rejected by external providers (e.g. OpenRouter) with 400 "badly formatted" error - Extend upstream_auth_error log to also fire on HTTP 400 with BYOK hint - Add unit tests: Bearer-prefix stripping, BYOK getAuthHeaders format, 400/401 upstream_auth_error log events Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/f4fac661-5248-46ac-919a-4f77d41f89a0
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR hardens the api-proxy Copilot BYOK auth path by sanitizing accidentally-prefixed Bearer tokens before header injection, and improves operator diagnostics by treating upstream HTTP 400 as a likely auth-formatting problem.
Changes:
- Strip an accidental
Bearerprefix (case-insensitive, tolerating leading whitespace) fromCOPILOT_GITHUB_TOKEN/COPILOT_API_KEYbefore building Copilot auth headers. - Extend
upstream_auth_errorlogging to include upstream HTTP 400 with a more prescriptive message. - Add/extend Jest coverage for token stripping, BYOK header formatting, and upstream auth-error logging on 400/401 (and not on 200).
Show a summary per file
| File | Description |
|---|---|
| containers/api-proxy/providers/copilot.js | Sanitize env-derived Copilot tokens to prevent double-Bearer Authorization headers. |
| containers/api-proxy/server.js | Emit upstream_auth_error for upstream HTTP 400 in addition to 401/403, with a targeted diagnostic message. |
| containers/api-proxy/server.test.js | Add regression tests for Bearer-prefix stripping, BYOK header injection, and 400/401 auth-error logging behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| if (proxyRes.statusCode === 400 || proxyRes.statusCode === 401 || proxyRes.statusCode === 403) { | ||
| const message = proxyRes.statusCode === 400 | ||
| ? `Upstream returned 400 — possible malformed Authorization header; check that the API key does not include a "Bearer " prefix (BYOK mode)` | ||
| : `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and has not expired`; | ||
| logRequest('warn', 'upstream_auth_error', { | ||
| request_id: requestId, provider, status: proxyRes.statusCode, | ||
| upstream_host: targetHost, path: sanitizeForLog(req.url), | ||
| message: `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and has not expired`, | ||
| message, | ||
| }); |
| // These tests guard against the "badly formatted Authorization header" bug in | ||
| // BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) where the | ||
| // sidecar could produce "Authorization: Bearer Bearer <key>" if the key value | ||
| // already contained the "Bearer " prefix. They also verify that the header |
| function resolveCopilotAuthToken(env = process.env) { | ||
| const githubToken = (env.COPILOT_GITHUB_TOKEN || '').trim() || undefined; | ||
| const apiKey = (env.COPILOT_API_KEY || '').trim() || undefined; | ||
| return githubToken || apiKey; | ||
| // Strip any accidental "Bearer " prefix (with optional leading whitespace) before | ||
| // trimming, so that a value like "Bearer " (prefix only, no actual token) | ||
| // correctly reduces to undefined rather than "Bearer". | ||
| const strip = (v) => ((v || '').replace(/^\s*Bearer\s+/i, '').trim()) || undefined; | ||
| return strip(env.COPILOT_GITHUB_TOKEN) || strip(env.COPILOT_API_KEY); | ||
| } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address the review feedback |
This comment has been minimized.
This comment has been minimized.
- Extract stripBearerPrefix() as a named module-level helper in
copilot.js, reused by both resolveCopilotAuthToken() and
createCopilotAdapter() — eliminates the duplicated inline `strip`
function; export for direct unit testing
- Replace Copilot/BYOK-specific HTTP 400 message in proxyRequest with
a generic "check that the API key is valid and correctly formatted"
message that is accurate for all providers
- Fix server.test.js BYOK mode comment to reference COPILOT_API_KEY
(the sidecar's actual input) rather than COPILOT_PROVIDER_API_KEY
(the agent-side placeholder)
- Add dedicated describe('stripBearerPrefix') unit tests for the new
helper; simplify resolveCopilotAuthToken integration tests
Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/fcc4060e-94ff-474f-8254-d8b81acd6204
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed all three review points in 8a55685:
|
🔥 Smoke Test Results — PR #2521
Overall: FAIL (file test inconclusive — template vars not expanded) PR: fix(api-proxy): strip accidental "Bearer " prefix in BYOK Copilot auth token
|
🔥 Smoke Test: Copilot BYOK (Offline Mode)
Running in BYOK offline mode ( Overall: PASS — PR by
|
|
Smoke Test Results: Status: PASS
|
Chroot Smoke Test Results
Result: ❌ Not all versions matched — Python and Node.js versions differ between host and chroot environments.
|
|
PRs: fix: remove unused exports from public API surface; refactor: split docker-manager.ts into focused modules Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test Results — FAIL
Overall: FAIL —
|
In BYOK mode (
COPILOT_PROVIDER_API_KEY+COPILOT_PROVIDER_BASE_URL), ifCOPILOT_API_KEYwas set with an accidental"Bearer "prefix, the sidecar would forwardAuthorization: ****** <key>— rejected by external providers (e.g. OpenRouter) with400 Authorization header is badly formatted.Changes
containers/api-proxy/providers/copilot.jsresolveCopilotAuthToken: Strip `****** prefix (case-insensitive, tolerates leading whitespace) before returning the token, preventing double-prefixing downstream.createCopilotAdapter: Apply the same sanitization to the locally-scopedgithubToken/apiKeyused ingetAuthHeadersfor the/modelsspecial-case path.containers/api-proxy/server.jsproxyRequest: Extend theupstream_auth_errorlog event to include HTTP 400 (alongside 401/403). A 400 from an external BYOK provider is a strong signal of a malformed auth header; the new message calls out the"Bearer "prefix explicitly so operators can diagnose without exhausting retries.containers/api-proxy/server.test.jsdescribe('createCopilotAdapter — BYOK getAuthHeaders')suite covering correct header format, double-prefix prevention, and edge cases.resolveCopilotAuthTokentests forBearer-prefix stripping (case variants, prefix-only input).createProviderServertests assertingupstream_auth_errorfires on 400/401 but not 200.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
api.example.com/opt/hostedtoolcache/node/20.20.2/x64/bin/node node /home/REDACTED/work/gh-aw-firewall/gh-aw-firewall/containers/api-proxy/node_modules/.bin/jest --testPathPatterns=server.test --verbose(dns block)/opt/hostedtoolcache/node/20.20.2/x64/bin/node node /home/REDACTED/work/gh-aw-firewall/gh-aw-firewall/containers/api-proxy/node_modules/.bin/jest --testPathPatterns=server.test --verbose s/.bin/jest(dns block)/opt/hostedtoolcache/node/20.20.2/x64/bin/node /opt/hostedtoolcache/node/20.20.2/x64/bin/node /home/REDACTED/work/gh-aw-firewall/gh-aw-firewall/containers/api-proxy/node_modules/jest-worker/build/processChild.js(dns block)If you need me to access, download, or install something from one of these locations, you can either: