Add i18n check, URL edge-case coverage, and lint fixes#69
Add i18n check, URL edge-case coverage, and lint fixes#69CodewithEvilxd wants to merge 4 commits intomrmps:mainfrom
Conversation
|
@CodewithEvilxd is attempting to deploy a commit to the Explainer Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds SSRF protection for OG article images by validating and clearing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OGRoute as OG Route
participant Validator as Validator (isPrivateIP / BLOCKED_HOSTNAMES)
participant Remote as Image Host
Client->>OGRoute: Request with articleImage URL
OGRoute->>Validator: Parse URL, check scheme, hostname, private/IP mapping
alt Invalid or blocked
Validator-->>OGRoute: blocked/invalid
OGRoute-->>Client: Render OG without external image
else Allowed
Validator-->>OGRoute: allowed
OGRoute->>Remote: (optional) fetch or reference image
Remote-->>OGRoute: image data / response
OGRoute-->>Client: Render OG with image
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Greptile SummaryThis PR strengthens SSRF protection by detecting sophisticated IPv4-mapped IPv6 bypass attempts and applies this validation to the OG image route. It also adds a new i18n validation script to prevent locale drift and improves test stability.
Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[User Input URL] --> B[normalizeUrl]
B --> C{Protocol Check}
C -->|Valid http/https| D{Hostname Check}
C -->|Invalid| E[Reject]
D --> F[isPrivateIP]
F --> G{IPv4?}
G -->|Yes| H[Check Private Ranges<br/>10.x, 172.16-31.x, 192.168.x<br/>127.x, 169.254.x, 0.x]
G -->|No| I{IPv6?}
I -->|Yes| J[Check IPv6 Ranges]
J --> K[Loopback ::1]
J --> L[Link-local fe80:]
J --> M[Unique local fc/fd]
J --> N[IPv4-mapped ::ffff:x.x.x.x]
J --> O[IPv4-mapped hex ::ffff:a00:1]
J --> P[Expanded IPv4-mapped<br/>0:0:0:0:0:ffff:x.x.x.x]
N --> Q[Recursively check<br/>mapped IPv4]
O --> Q
P --> Q
D --> R{In BLOCKED_HOSTNAMES?}
R -->|Yes| E
R -->|No| S{Private IP?}
H -->|Private| E
Q -->|Private| E
K --> E
L --> E
M --> E
S -->|No| T[Allow URL]
S -->|Yes| E
U[OG Route Image] --> V{articleImage exists?}
V -->|Yes| W[Parse URL]
W --> X{Protocol http/https?}
X -->|No| Y[Clear image]
X -->|Yes| Z[Check with isPrivateIP<br/>and BLOCKED_HOSTNAMES]
Z -->|Blocked| Y
Z -->|Safe| AA[Render image]
V -->|No| AB[No image]
Last reviewed commit: bd240f8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/routes/gravity.ts (1)
153-159:⚠️ Potential issue | 🟡 MinorStale comment:
_adProvideris not actually logged to ClickHouse.Line 158 states "adProvider from client is used only for ClickHouse logging", but
_adProvideris never passed totrackAdEvent(lines 172–192). The comment is misleading — either remove it or, if the intent is to log it, actually pass it as a field.Suggested comment fix
const { type, sessionId, hostname, brandName, adTitle, adText, clickUrl, impUrl, cta, favicon, deviceType, os, browser, adProvider: _adProvider } = body; // Derive provider from impUrl prefix only — never trust client-sent adProvider // for forwarding decisions (prevents spoofing to skip Gravity billing) const isZeroClick = impUrl?.startsWith("zeroclick://") ?? false; - // adProvider from client is used only for ClickHouse logging, not forwarding logic + // adProvider from client is accepted but intentionally unused const provider = isZeroClick ? "zeroclick" : "gravity";app/api/og/route.tsx (1)
124-142:⚠️ Potential issue | 🟡 MinorESLint suppression is correct —
next/imageis unsupported inImageResponse(Satori).However, there's an unaddressed SSRF vulnerability:
articleImageoriginates from unvalidated user input (?image=...query parameter or fetched article metadata) and Satori will fetch it server-side during image generation. An attacker could pass?image=http://169.254.169.254/latest/meta-data/or other private IP addresses. Apply the existingisPrivateIP()validation fromlib/validation/url.tstoarticleImagebefore rendering to prevent this attack vector.
🤖 Fix all issues with AI agents
In `@lib/validation/url.test.ts`:
- Around line 80-89: Add unit tests to cover the new IPv4-mapped hex-hextet and
expanded IPv6 parsing branches that currently lack coverage: add tests invoking
normalizeUrl with "[::ffff:a00:1]" (hex-hextet private) and "[::ffff:808:808]"
(hex-hextet public) and with the expanded form "[0:0:0:0:0:ffff:10.0.0.1]" to
assert the private addresses throw "Cannot access internal or private network
addresses." and the public hex-hextet returns the normalized URL; place these in
lib/validation/url.test.ts alongside the existing IPv4-mapped tests so the code
paths in url.ts (the hex-hextet branch and the expanded form branch) are
exercised.
🧹 Nitpick comments (3)
scripts/check-i18n.ts (2)
86-97: Static analysis:forEachcallbacks should not return a value.Biome flags lines 88, 92, and 96 because the arrow-function shorthand implicitly returns the result of
console.log(). Use block bodies to silence the lint errors.Proposed fix
if (missing.length) { console.log(` Missing (${missing.length})`); - missing.forEach((key) => console.log(` - ${key}`)); + missing.forEach((key) => { console.log(` - ${key}`); }); } if (extra.length) { console.log(` Extra (${extra.length})`); - extra.forEach((key) => console.log(` - ${key}`)); + extra.forEach((key) => { console.log(` - ${key}`); }); } if (typeMismatches.length) { console.log(` Type mismatches (${typeMismatches.length})`); - typeMismatches.forEach((key) => console.log(` - ${key}`)); + typeMismatches.forEach((key) => { console.log(` - ${key}`); }); }
60-66: Consider wrapping the base file read in a try/catch with a user-friendly error.If
messages/en.jsondoesn't exist (e.g., running from wrong directory),readFileSyncwill throw a rawENOENTerror. A small guard would improve DX.Proposed improvement
const projectRoot = process.cwd(); const messagesDir = join(projectRoot, "messages"); const baseFile = join(messagesDir, "en.json"); +import { existsSync } from "fs"; +if (!existsSync(baseFile)) { + console.error(`Base locale file not found: ${baseFile}`); + process.exit(1); +} + const baseJson = readJson(baseFile);lib/validation/url.ts (1)
103-110: Overly broadincludes("ffff:")match may false-positive on non-mapped IPv6.Any IPv6 address containing the substring
"ffff:"will enter this branch — e.g.2001:db8:0:0:0:ffff:192.168.1.1is a legitimate (non-mapped) IPv6 address whose embedded IPv4 tail happens to be private. This fails safely (over-blocks rather than under-blocks), but it could be tightened to only match the canonical mapped prefix pattern:Tighter match for expanded IPv4-mapped forms
- // Handle expanded IPv4-mapped IPv6 forms (e.g., 0:0:0:0:0:ffff:10.0.0.1) - if (lower.includes("ffff:")) { - const lastColon = lower.lastIndexOf(":"); - const tail = lastColon >= 0 ? lower.slice(lastColon + 1) : ""; - if (tail.includes(".") && /^\d{1,3}(?:\.\d{1,3}){3}$/.test(tail)) { - return isPrivateIP(tail); - } - } + // Handle expanded IPv4-mapped IPv6 forms (e.g., 0:0:0:0:0:ffff:10.0.0.1) + const expandedMapped = lower.match( + /^(?:0+:){5}(?:0*f{4}):(\d{1,3}(?:\.\d{1,3}){3})$/ + ); + if (expandedMapped) { + return isPrivateIP(expandedMapped[1]); + }
…verage - Add SSRF protection to OG image route by validating articleImage URLs - Export isPrivateIP function for use in OG route - Fix misleading comment in gravity.ts about adProvider usage - Fix forEach lint warnings in check-i18n.ts with explicit block bodies - Add existsSync check for base locale file in check-i18n.ts - Add missing hex-hextet public IPv6 test case
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/api/og/route.tsx`:
- Around line 70-85: The SSRF check in the articleImage validation only calls
isPrivateIP(imageHost) and misses internal non-numeric hostnames; update the
validation in route.tsx to also (1) ensure the URL.protocol is either "http:" or
"https:" and (2) check the imageUrl.hostname against the blocked hostnames list
(BLOCKED_HOSTNAMES) exported from url.ts (or import/export it if not yet
exported) and clear articleImage when the hostname matches; keep the existing
try/catch and numeric-IP check (isPrivateIP) and combine these checks before
accepting the image URL.
🧹 Nitpick comments (3)
scripts/check-i18n.ts (2)
1-17: WrapreadJsonin a try/catch for friendlier errors on malformed locale files.If a locale JSON file contains a syntax error, the script will crash with a raw
SyntaxErrorand stack trace instead of a clear, actionable message identifying which file is broken.♻️ Suggested improvement
function readJson(filePath: string): unknown { - const raw = readFileSync(filePath, "utf-8"); - return JSON.parse(raw); + const raw = readFileSync(filePath, "utf-8"); + try { + return JSON.parse(raw); + } catch (err) { + console.error(`Failed to parse JSON: ${filePath}`); + throw err; + } }
19-35:collectShapedoes not recurse into array elements — intentional?Arrays are recorded as a leaf
"array"type, so structural differences inside array items (e.g., an array of objects with translatable keys) won't be detected. If your locale files only use arrays for simple value lists this is fine; just calling it out in case nested-object arrays are expected.lib/validation/url.ts (1)
103-110: The"ffff:"substring check is intentionally broad — consider tightening.
lower.includes("ffff:")will match any IPv6 address containing that substring, not just IPv4-mapped forms. The inner dotted-decimal guard on Line 107 prevents most false positives, but a tighter prefix check (e.g., matching the full0:0:0:0:0:ffff:pattern or ending with:ffff:<ipv4>) would be more precise. This is non-urgent since over-blocking is safer for SSRF prevention.
- Export BLOCKED_HOSTNAMES from url.ts for reuse - Add protocol validation (only allow http/https) in OG image route - Check articleImage hostname against BLOCKED_HOSTNAMES - Prevents SSRF via internal DNS names like metadata.google.internal Addresses CodeRabbit feedback on incomplete SSRF protection
1 participant
What
check-i18nscript to verify locale key consistency.<img>lint warnings in OG + lightbox.adProvidervar in gravity route.Why
Testing
Summary by CodeRabbit
New Features
Improvements
Tests
Chores