fix: harden proxy security fallbacks and passthrough guards #964
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| const PROVIDER_FAILURE_STATUSES = new Set([401, 402, 403, 408, 429, 451]); | ||
|
|
||
| export function shouldRecordProviderCircuitFailure(statusCode: number): boolean { | ||
| if (statusCode >= 500) { | ||
| return true; | ||
| } | ||
|
|
||
| return PROVIDER_FAILURE_STATUSES.has(statusCode); | ||
| } | ||
|
Comment on lines
+1
to
+9
There was a problem hiding this comment.
The PR already correctly excludes clear client-error codes (400, 404, 409, 413, 415, 422), and the same reasoning applies to 403. If the intent is specifically to trip the circuit on upstream key-revocation responses, consider adding a comment explaining the reasoning, or narrow the check to only treat 403 as a provider failure when there is additional context (e.g., an upstream error body) indicating a key-level access denial rather than a scope/permission mismatch. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/circuit-breaker-accounting.ts
Line: 1-9
Comment:
**HTTP 403 classification as a provider-side failure**
`403 Forbidden` is included in `PROVIDER_FAILURE_STATUSES`, but 403 is ambiguous in a proxy context: it is often client-driven (e.g., a client API key that lacks permission for the requested resource / model), not a signal that the upstream provider itself is unhealthy. Treating every 403 as a provider circuit failure could trigger unnecessary failovers when many clients send permission-constrained requests.
The PR already correctly excludes clear client-error codes (400, 404, 409, 413, 415, 422), and the same reasoning applies to 403. If the intent is specifically to trip the circuit on upstream key-revocation responses, consider adding a comment explaining the reasoning, or narrow the check to only treat 403 as a provider failure when there is additional context (e.g., an upstream error body) indicating a key-level access denial rather than a scope/permission mismatch.
How can I resolve this? If you propose a fix, please make it concise. |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { beforeEach, describe, expect, test, vi } from "vitest"; | ||
|
|
||
| vi.mock("server-only", () => ({})); | ||
|
|
||
| vi.mock("@/lib/logger", () => ({ | ||
| logger: { | ||
| warn: vi.fn(), | ||
| trace: vi.fn(), | ||
| info: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| const getRedisClientMock = vi.fn(); | ||
|
|
||
| vi.mock("@/lib/redis", () => ({ | ||
| getRedisClient: getRedisClientMock, | ||
| })); | ||
|
|
||
| vi.mock("@/app/v1/_lib/proxy/errors", () => ({ | ||
| sanitizeHeaders: vi.fn(() => "(empty)"), | ||
| sanitizeUrl: vi.fn((value: string) => value), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/session-tracker", () => ({ | ||
| SessionTracker: { | ||
| getActiveSessions: vi.fn(async () => []), | ||
| }, | ||
| })); | ||
|
|
||
| describe("SessionManager.getOrCreateSessionId fallback", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| test("without client session id, identical messages should not reuse the same derived session id even when Redis is ready", async () => { | ||
| const { SessionManager } = await import("@/lib/session-manager"); | ||
| const messages = [{ role: "user", content: "hello" }]; | ||
| const hashMapping = new Map<string, string>(); | ||
|
|
||
| const pipeline = { | ||
| setex: vi.fn((key: string, _ttl: number, value: string) => { | ||
| hashMapping.set(key, value); | ||
| return pipeline; | ||
| }), | ||
| exec: vi.fn(async () => []), | ||
| }; | ||
|
|
||
| getRedisClientMock.mockReturnValue({ | ||
| status: "ready", | ||
| get: vi.fn(async (key: string) => hashMapping.get(key) ?? null), | ||
| pipeline: vi.fn(() => pipeline), | ||
| }); | ||
|
|
||
| const first = await SessionManager.getOrCreateSessionId(11, messages, null); | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| const second = await SessionManager.getOrCreateSessionId(22, messages, null); | ||
|
|
||
| expect(first).toMatch(/^sess_/); | ||
| expect(second).toMatch(/^sess_/); | ||
| expect(first).not.toBe(second); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redactBillingHeaderAuditValueEvery call site already guards with
BILLING_HEADER_PATTERN.test(...)before invoking this function, so theif (!BILLING_HEADER_PATTERN.test(value))branch can never be reached in practice. Thereturn value.trim()path is unreachable dead code. The function could be simplified to:Or, alternatively, inline
REDACTED_BILLING_HEADER_VALUEdirectly at the two call sites and remove the helper altogether. As-is, the function gives the misleading impression that it might return the original value, which could cause confusion during future refactors.Prompt To Fix With AI