🔒 Security: Prevent SSRF vulnerability when fetching fonts - #562
Conversation
This commit removes the ability for the card API route to dynamically fetch fonts from an origin dictated by the `APP_URL` environment variable. Instead, the font data fetcher is hardcoded to use the default JSDelivr URL, completely neutralizing the SSRF vector. Risk: If an attacker exploited a misconfiguration to point `APP_URL` to an internal address, the edge worker could perform requests against internal services (SSRF) when fetching the font. Solution: Removed dynamic font fetching entirely and rely exclusively on the hardcoded `DEFAULT_FONT_URL`. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
👋 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. |
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR Summary by QodoPrevent SSRF by hardcoding font fetch to trusted CDN
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Unused validator import
|
| function getFontData(fontUrl?: string, allowedOrigin?: string): Promise<ArrayBuffer> { | ||
| const targetUrl = | ||
| fontUrl && isTrustedFontUrl(fontUrl, allowedOrigin) | ||
| ? fontUrl |
There was a problem hiding this comment.
1. Unused validator import 🐞 Bug ⚙ Maintainability
src/lib/cardRenderer.tsx still imports isTrustedFontUrl even though getFontData() no longer calls it after the refactor, leaving an unused import that can cause npm run lint to fail under the Next.js TypeScript ESLint config.
Agent Prompt
### Issue description
`src/lib/cardRenderer.tsx` no longer uses `isTrustedFontUrl` after `getFontData` was simplified to always fetch `DEFAULT_FONT_URL`, but the import remains. This creates an unused import that is typically flagged by the project’s ESLint setup.
### Issue Context
The PR removed `fontUrl`/`allowedOrigin` parameters and the conditional `isTrustedFontUrl(...)` selection logic; leaving the import behind is now dead code.
### Fix Focus Areas
- src/lib/cardRenderer.tsx[1-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ? fontUrl | ||
| : DEFAULT_FONT_URL; | ||
| function getFontData(): Promise<ArrayBuffer> { | ||
| const targetUrl = DEFAULT_FONT_URL; |
There was a problem hiding this comment.
JSDelivr に到達できない環境では、format=svg の通常描画が失敗した後、503 エラーカードも同じ DEFAULT_FONT_URL を再取得して失敗するため、クライアントには意図した 503 カードではなく未処理例外による 500 応答が返ります。
Knowledge Base Used: Card Data Pipeline
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/cardRenderer.tsx
Line: 21
Comment:
**SVG フォールバックも CDN 障害で失敗**
JSDelivr に到達できない環境では、`format=svg` の通常描画が失敗した後、503 エラーカードも同じ `DEFAULT_FONT_URL` を再取得して失敗するため、クライアントには意図した 503 カードではなく未処理例外による 500 応答が返ります。
**Knowledge Base Used:** [Card Data Pipeline](https://app.greptile.com/hiroki-org/-/custom-context/knowledge-base/hiroki-org/github-user-summary/-/docs/card-data-pipeline.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
🎯 What:
Fixed a Server-Side Request Forgery (SSRF) vulnerability caused by dynamically fetching fonts based on the
APP_URLenvironment variable.If an attacker exploited a misconfiguration in
APP_URLto point to an internal network address (or used a DNS rebinding attack if validation bypassed), the edge worker could be coerced into making arbitrary internal requests, potentially accessing private or sensitive data.🛡️ Solution:
Removed dynamic font fetching entirely. The
getFontDatafunction now strictly uses the hardcodedDEFAULT_FONT_URL(JSDelivr). RemovedfontUrlandallowedOriginpropagation from the API route and thecardRendererfunction signatures. Updated associated tests.PR created automatically by Jules for task 18188617785768465598 started by @is0692vs
Greptile Summary
APP_URL 由来のフォント URL 伝播を廃止し、SVG レンダリング用フォントを固定の JSDelivr URL から取得することで SSRF 経路を除去しています。
Confidence Score: 4/5
SVG の外部フォント取得に失敗すると 503 フォールバックも同じ理由で失敗するため、この経路を修正してからマージすべきです。
SSRF の原因となった動的 URL は除去されていますが、すべての SVG 描画を単一の外部 CDN に固定したことで、その取得障害が通常描画だけでなく catch 節内のエラーカード描画にも伝播し、意図した 503 応答を返せません。
Files Needing Attention: src/lib/cardRenderer.tsx、src/app/api/card/[username]/route.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["GET /api/card/:username?format=svg"] --> B["renderCardResponse"] B --> C["getFontData"] C --> D["固定 JSDelivr URL"] D -->|成功| E["SVG 応答"] D -->|失敗| F["route の catch"] F --> G["renderErrorCardResponse"] G --> C D -->|再失敗| H["未処理例外 / 500"]Comments Outside Diff (1)
src/lib/cardRenderer.tsx, line 9 (link)getFontDataから URL 検証を削除したことでisTrustedFontUrlは参照されなくなっています。残したままだと静的解析の警告や lint エラーになり、現在も URL 検証が使われているとの誤解を招くため削除してください。Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "🔒 security fix: prevent SSRF via fontUr..." | Re-trigger Greptile
Context used: