🔒 Fix fallback of allowedOrigin to localhost in production#381
🔒 Fix fallback of allowedOrigin to localhost in production#381is0692vs wants to merge 3 commits into
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughSummary by CodeRabbit
WalkthroughGETハンドラ内でallowedOriginの決定方法を変更した。APP_URL環境変数が未設定の場合、NODE_ENVがproductionであれば500エラーで処理を中断し、それ以外の環境ではlocalhost URLにフォールバックするよう分岐処理を追加した。 ChangesallowedOrigin決定ロジックの修正
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves environment variable validation in the card API route by returning a 500 error in production if the APP_URL environment variable is not configured. The reviewer suggested trimming APP_URL to prevent issues with accidental leading or trailing whitespace.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const url = new URL(request.url); | ||
| const options = parseCardQueryParams(url.searchParams); | ||
| const allowedOrigin = process.env.APP_URL || "http://localhost:3000"; | ||
| let allowedOrigin = process.env.APP_URL; |
There was a problem hiding this comment.
Trimming the APP_URL environment variable prevents potential issues caused by accidental leading or trailing whitespace in configuration files, which could otherwise result in malformed font URLs or failed origin validation checks.
| let allowedOrigin = process.env.APP_URL; | |
| let allowedOrigin = process.env.APP_URL?.trim(); |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/app/api/card/`[username]/route.ts:
- Around line 22-23: In the route handler for card rendering, replace the
plain-text production 500 response with the same SVG error fallback used by the
other error paths via renderErrorCardResponse, so the response format stays
consistent for embedded <img> usage. Also remove the internal APP_URL detail
from the message by using a generic server configuration error string, and make
sure the 500 response includes the same Cache-Control behavior as the rest of
the image route; if renderErrorCardResponse needs a fontUrl, update its call or
signature in a way that works without exposing APP_URL.
- Around line 20-26: Add a test for the production-only early return in the card
route when APP_URL is missing. Update the route logic in route.ts’s
allowedOrigin handling only if needed, and cover the branch where
process.env.NODE_ENV is "production" and process.env.APP_URL is unset so the
handler returns a 500 Response with the server configuration error message. Use
the existing route handler symbols in src/app/api/card/[username]/route.ts and
mirror the style of route.test.ts to assert this fallback path directly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7787d9d2-18dd-4d18-a60c-2911d96f7591
📒 Files selected for processing (1)
src/app/api/card/[username]/route.ts
| let allowedOrigin = process.env.APP_URL; | ||
| if (!allowedOrigin) { | ||
| if (process.env.NODE_ENV === "production") { | ||
| return new Response("Server configuration error: APP_URL environment variable is not set", { status: 500 }); | ||
| } | ||
| allowedOrigin = "http://localhost:3000"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: route.test.ts で allowedOrigin フォールバックロジックのテストを確認
# テストファイルの内容を確認
cat -n src/app/api/card/[username]/route.test.tsRepository: Hiroki-org/github-user-summary
Length of output: 6312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# route.ts の分岐と、関連テストの有無を確認
cat -n src/app/api/card/[username]/route.ts
printf '\n---\n'
rg -n "APP_URL|localhost:3000|Server configuration error: APP_URL" src/app/api/card -S
printf '\n---\n'
rg -n "NODE_ENV|production|allowedOrigin|fontUrl" src/app/api/card/[username] -SRepository: Hiroki-org/github-user-summary
Length of output: 4442
NODE_ENV=production の 500 パスを追加してください。
route.test.ts では localhost フォールバック側は踏めていますが、APP_URL 未設定かつ production 時の早期 500 リターンは未カバーです。
🤖 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/app/api/card/`[username]/route.ts around lines 20 - 26, Add a test for
the production-only early return in the card route when APP_URL is missing.
Update the route logic in route.ts’s allowedOrigin handling only if needed, and
cover the branch where process.env.NODE_ENV is "production" and
process.env.APP_URL is unset so the handler returns a 500 Response with the
server configuration error message. Use the existing route handler symbols in
src/app/api/card/[username]/route.ts and mirror the style of route.test.ts to
assert this fallback path directly.
| if (process.env.NODE_ENV === "production") { | ||
| return new Response("Server configuration error: APP_URL environment variable is not set", { status: 500 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
500レスポンスに情報漏洩、フォーマット不整合、キャッシュヘッダー欠落の3つの問題があります。
-
情報漏洩: エラーメッセージ
"Server configuration error: APP_URL environment variable is not set"が内部環境変数名APP_URLを露出しています。"Server configuration error"のような汎用メッセージを使用してください。 -
レスポンスフォーマット不整合: このエンドポイントは画像（SVGカード）を返すルートですが、500レスポンスはプレーンテキストです。他のすべてのエラーパス（行33, 47, 65）は
renderErrorCardResponseでSVGを返しています。<img>タグで埋め込まれている場合、プレーンテキストの500は表示を破壊します。 -
キャッシュヘッダー欠落: 500レスポンスに
Cache-Controlヘッダーが設定されていません。CDNやブラウザがこのエラーをキャッシュする可能性があります。As per coding guidelines、src/app/api/card/**/*.{ts,tsx}では「Image routes should preserve cache headers and predictable fallback behavior」が求められています。
renderErrorCardResponse を使用する場合、fontUrl が allowedOrigin に依存するため、fontUrl なしで呼び出すかフォールバック値を使用する必要があります。renderErrorCardResponse のシグネチャを確認してください。
🔧 提案する修正
if (process.env.NODE_ENV === "production") {
- return new Response("Server configuration error: APP_URL environment variable is not set", { status: 500 });
+ return renderErrorCardResponse({
+ message: "Server configuration error",
+ options,
+ status: 500,
+ cacheControl: ERROR_CACHE,
+ });
}※ renderErrorCardResponse が fontUrl なしで動作するか確認してください。fontUrl が必須の場合は、デフォルト値の追加またはシグネチャの調整が必要です。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.NODE_ENV === "production") { | |
| return new Response("Server configuration error: APP_URL environment variable is not set", { status: 500 }); | |
| if (process.env.NODE_ENV === "production") { | |
| return renderErrorCardResponse({ | |
| message: "Server configuration error", | |
| options, | |
| status: 500, | |
| cacheControl: ERROR_CACHE, | |
| }); |
🤖 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/app/api/card/`[username]/route.ts around lines 22 - 23, In the route
handler for card rendering, replace the plain-text production 500 response with
the same SVG error fallback used by the other error paths via
renderErrorCardResponse, so the response format stays consistent for embedded
<img> usage. Also remove the internal APP_URL detail from the message by using a
generic server configuration error string, and make sure the 500 response
includes the same Cache-Control behavior as the rest of the image route; if
renderErrorCardResponse needs a fontUrl, update its call or signature in a way
that works without exposing APP_URL.
Source: Coding guidelines
1 participant
🎯 What: The vulnerability fixed
In
src/app/api/card/[username]/route.ts, if theAPP_URLenvironment variable is not explicitly set, theallowedOriginfalls back tohttp://localhost:3000.This insecure fallback might cause fonts or other internal fetch requests to hit a local developer endpoint instead of the intended service in production environments. Crucially, this variable is used via
renderCardResponseas the trustedallowedOrigininisTrustedFontUrl(invalidators.ts), potentially meaning that the production environment would implicitly trustlocalhost:3000URLs when it shouldn't.🛡️ Solution: How the fix addresses the vulnerability
The code now properly verifies the presence of the
APP_URLenvironment variable. IfAPP_URLis empty and the environment isproduction, the server now explicitly returns a500 Server configuration error. In non-production environments, it safely falls back tohttp://localhost:3000.PR created automatically by Jules for task 9766932802489647517 started by @is0692vs
Greptile Summary
本PRは、
APP_URL環境変数が未設定の場合にproduction環境でhttp://localhost:3000にフォールバックしてしまうセキュリティ上の問題を修正しています。修正後は、production環境かつAPP_URLが未設定の場合にrenderErrorCardResponseを通じて500エラーを返し、非production環境では引き続きlocalhost:3000へのフォールバックを維持します。route.ts:allowedOriginの決定ロジックを変更し、production環境での安全なエラーハンドリングを追加。エラーメッセージは汎用的な "Server configuration error" を使用しており、内部の環境変数名は露出しない設計になっています。route.test.ts:APP_URLが未設定かつNODE_ENV=productionの場合にrenderErrorCardResponseが正しく呼ばれることを検証する新しいテストケースを追加。Confidence Score: 5/5
本番環境での誤ったlocalhostフォールバックを防ぐ小規模な修正で、安全にマージできます。
変更範囲はごく小さく、本番環境での誤ったフォールバックを正確に防いでいます。エラーメッセージは汎用的で内部情報を露出しておらず、レスポンス形式も統一されています。テストが新しいコードパスを網羅しており、既存の動作への影響もありません。
特に注意が必要なファイルはありません。
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[GET /api/card/:username] --> B{APP_URL is set?} B -- "Yes" --> C[allowedOrigin = APP_URL] B -- "No" --> D{NODE_ENV === 'production'?} D -- "Yes" --> E[renderErrorCardResponse\nstatus: 500\n'Server configuration error'] D -- "No" --> F[allowedOrigin = 'http://localhost:3000'] C --> G[fontUrl = allowedOrigin + /fonts/...] F --> G G --> H{Rate limit exceeded?} H -- "Yes" --> I[renderErrorCardResponse 429] H -- "No" --> J[fetchCardData] J -- "null" --> K[renderErrorCardResponse 404] J -- "data" --> L[renderCardResponse 200] J -- "error" --> M[renderErrorCardResponse 503]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[GET /api/card/:username] --> B{APP_URL is set?} B -- "Yes" --> C[allowedOrigin = APP_URL] B -- "No" --> D{NODE_ENV === 'production'?} D -- "Yes" --> E[renderErrorCardResponse\nstatus: 500\n'Server configuration error'] D -- "No" --> F[allowedOrigin = 'http://localhost:3000'] C --> G[fontUrl = allowedOrigin + /fonts/...] F --> G G --> H{Rate limit exceeded?} H -- "Yes" --> I[renderErrorCardResponse 429] H -- "No" --> J[fetchCardData] J -- "null" --> K[renderErrorCardResponse 404] J -- "data" --> L[renderCardResponse 200] J -- "error" --> M[renderErrorCardResponse 503]Reviews (2): Last reviewed commit: "Fix APP_URL environment variable fallbac..." | Re-trigger Greptile