Restrict chat gateway target overrides#899
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe gateway base URL resolution logic in gatewayProxy.ts was refactored to accept an explicit nodeEnv parameter and introduces a new exported function, resolveGatewayBaseUrlForEnvironment, which fails closed to null in production when unconfigured. New Jest tests validate this behavior. ChangesGateway Base URL Resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client as Proxy Endpoint
participant Resolver as resolveGatewayBaseUrlForEnvironment
participant Env as process.env
Client->>Env: read AI_STATS_GATEWAY_URL, NODE_ENV
Client->>Resolver: resolve(configuredBaseUrl, requestedBaseUrl, nodeEnv)
alt production
Resolver-->>Client: normalized configuredBaseUrl or null
Client->>Client: if null, return gateway_not_configured
else development
Resolver-->>Client: requestedBaseUrl, configuredBaseUrl, or public fallback
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/app/api/chat/_shared/gatewayProxy.test.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the app’s absolute import alias here.
This test is under
apps/web, so prefer the@/alias instead of a relative import.Suggested change
-import { resolveGatewayBaseUrlForEnvironment } from "./gatewayProxy"; +import { resolveGatewayBaseUrlForEnvironment } from "`@/app/api/chat/_shared/gatewayProxy`";As per coding guidelines,
apps/web/**/*.{ts,tsx}: “Web app uses Next.js 15 App Router with TypeScript ES modules and absolute imports using@/prefix; Tailwind for styling.”🤖 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 `@apps/web/src/app/api/chat/_shared/gatewayProxy.test.ts` at line 1, The import in the gatewayProxy test should use the web app’s absolute alias instead of a relative path. Update the import in gatewayProxy.test.ts to reference resolveGatewayBaseUrlForEnvironment via the `@/` prefix, matching the apps/web import convention and keeping it consistent with the gatewayProxy module location.Source: Coding guidelines
4-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a production assertion for arbitrary requested targets.
The production test covers public and localhost overrides, but the main abuse case is an arbitrary external target. Add one deterministic fixture to lock that down too.
Suggested addition
expect( resolveGatewayBaseUrlForEnvironment({ configuredBaseUrl: "https://private-gateway.example.com", requestedBaseUrl: "http://127.0.0.1:8787/v1", nodeEnv: "production", }), ).toBe("https://private-gateway.example.com/v1"); + + expect( + resolveGatewayBaseUrlForEnvironment({ + configuredBaseUrl: "https://private-gateway.example.com", + requestedBaseUrl: "https://attacker.example.com/v1", + nodeEnv: "production", + }), + ).toBe("https://private-gateway.example.com/v1");As per coding guidelines,
**/*.test.{ts,tsx}: “Jest test files should cover new logic and data validations with deterministic fixtures.”🤖 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 `@apps/web/src/app/api/chat/_shared/gatewayProxy.test.ts` around lines 4 - 20, The production coverage in resolveGatewayBaseUrlForEnvironment is missing the arbitrary external-target case, so add one deterministic assertion in gatewayProxy.test.ts alongside the existing production checks. Reuse the same test and verify that when nodeEnv is production and requestedBaseUrl points to an external non-public target, the function still resolves to the configuredBaseUrl with the requested path preserved, matching the pattern already used for the public and localhost fixtures.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/web/src/app/api/chat/_shared/gatewayProxy.test.ts`:
- Line 1: The import in the gatewayProxy test should use the web app’s absolute
alias instead of a relative path. Update the import in gatewayProxy.test.ts to
reference resolveGatewayBaseUrlForEnvironment via the `@/` prefix, matching the
apps/web import convention and keeping it consistent with the gatewayProxy
module location.
- Around line 4-20: The production coverage in
resolveGatewayBaseUrlForEnvironment is missing the arbitrary external-target
case, so add one deterministic assertion in gatewayProxy.test.ts alongside the
existing production checks. Reuse the same test and verify that when nodeEnv is
production and requestedBaseUrl points to an external non-public target, the
function still resolves to the configuredBaseUrl with the requested path
preserved, matching the pattern already used for the public and localhost
fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee058a1a-99cb-4506-8781-f81c3a711638
📒 Files selected for processing (2)
apps/web/src/app/api/chat/_shared/gatewayProxy.test.tsapps/web/src/app/api/chat/_shared/gatewayProxy.ts
1 participant
Summary
AI_STATS_GATEWAY_URL.Validation
pnpm --filter @ai-stats/web test -- gatewayProxy.test.tspnpm --filter @ai-stats/web typecheckpnpm --filter @ai-stats/web exec eslint "src/app/api/chat/_shared/gatewayProxy.ts" "src/app/api/chat/_shared/gatewayProxy.test.ts"Note: full
pnpm --filter @ai-stats/web lintstill fails on existing repo-wide lint issues unrelated to this change.Created with Codex
Summary by CodeRabbit
Bug Fixes
Tests