feat: server-side save-skill API for Chrome extension#66
Conversation
Chrome Web Store rejected the content script approach since it doesn't inject on pre-existing tabs. Move webpage-to-skill conversion from client-side to a server-side API route that uses Turndown + weighted tag detection, eliminating the need for content scripts entirely. - Add POST /api/save-skill route with SSRF protection and rate limiting - Rewrite extension to call API with tab URL instead of injecting scripts - Remove content script, scripting permission, and turndown dependency - Add name field validation and YAML-escape source URLs - Bump extension manifest to v1.18.0
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request migrates the skill generation functionality from client-side (browser extension) to server-side. A new API endpoint in the docs site handles URL-to-skill conversion with rate limiting, content extraction, and tag detection, while the extension is simplified to send URLs to the API and download the resulting skill files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Extension as Browser Extension<br/>(background)
participant API as API Server<br/>(save-skill)
participant ExternalURL as External URL/<br/>GitHub
participant Download as Download/<br/>Local Storage
User->>Extension: Click "Save as Skill"<br/>(send URL)
Extension->>Extension: Validate URL<br/>(reject restricted pages)
Extension->>API: POST /api/save-skill<br/>{url, name?}
API->>API: Check rate limit<br/>(per IP, 60s window)
alt Rate limit exceeded
API-->>Extension: 429 + rate-limit headers
Extension-->>User: Show error
else Within limit
API->>API: Validate & normalize input
API->>ExternalURL: Fetch content<br/>(URL with timeout)
ExternalURL-->>API: HTML/text response
API->>API: Extract content<br/>(HTML→Markdown via Turndown,<br/>detect tags from headings/code/keywords)
API->>API: Generate YAML front matter<br/>(name, description, tags)
API-->>Extension: Return<br/>{skillMd, name, tags, description}
Extension->>Download: Create Markdown blob<br/>& trigger download
Download-->>User: Skill file saved
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false; | ||
| if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false; | ||
| if (bare.startsWith('ff')) return false; |
There was a problem hiding this comment.
🔴 SSRF IPv6 prefix checks incorrectly block legitimate domain names
The isAllowedUrl function at docs/fumadocs/src/app/api/save-skill/route.ts:41-43 uses bare.startsWith('fc'), bare.startsWith('fd'), and bare.startsWith('ff') to block IPv6 ULA (fc00::/7) and multicast (ff00::/8) addresses. However, the bare variable is the hostname, which can be a regular domain name—not just an IP literal. This causes legitimate domains to be blocked.
Affected domains and root cause
The variable bare is derived from parsed.hostname.toLowerCase() (line 32-33), which for regular URLs is just the domain name (e.g., ffmpeg.org, fdroid.org). The checks:
bare.startsWith('fc')— blocks any domain starting with "fc" (though common ones likefacebook.comare not affected since the hostname isfacebook.comnotfc...)bare.startsWith('fd')— blocks domains likefdroid.orgbare.startsWith('ff')— blocks domains likeffmpeg.org,fflogs.com
Contrast this with the fe80: check on line 41 which correctly includes the colon, ensuring only IPv6 link-local addresses match. The fc, fd, and ff checks are missing the colon, so they match domain names instead of only IPv6 addresses.
Impact: Users attempting to save skills from any website whose hostname starts with fc, fd, or ff will receive a "URL not allowed" 403 error.
| if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false; | |
| if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false; | |
| if (bare.startsWith('ff')) return false; | |
| if (bare.startsWith('fe80:') || bare.startsWith('fc00:') || bare.startsWith('fd')) return false; | |
| if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false; | |
| if (/^ff[0-9a-f]{2}:/.test(bare)) return false; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { NextRequest, NextResponse } from 'next/server'; | ||
| import TurndownService from 'turndown'; | ||
|
|
||
| const rateLimitMap = new Map<string, { count: number; resetTime: number }>(); |
There was a problem hiding this comment.
🔴 Unbounded memory growth in rateLimitMap — expired entries are never purged
The module-level rateLimitMap at docs/fumadocs/src/app/api/save-skill/route.ts:4 stores a rate-limit entry per unique IP address. Entries are only overwritten when the same IP makes a new request after its window expires (line 12-13), but entries from IPs that never return are never removed.
Root Cause
The checkRateLimit function updates or creates entries in rateLimitMap on each call, but there is no cleanup mechanism (no periodic sweep, no TTL eviction, no size cap). In a production deployment behind a CDN or proxy, the x-forwarded-for header yields a unique key per client IP. Over time—especially under moderate traffic—the map accumulates entries indefinitely.
const rateLimitMap = new Map<string, { count: number; resetTime: number }>();Since this is a Next.js API route running in a long-lived server process (or a serverless function with warm instances), each unique IP that ever hits the endpoint leaves a permanent entry in memory.
Impact: Gradual memory leak proportional to the number of unique client IPs. Under sustained traffic this can eventually cause OOM or degraded performance.
Prompt for agents
Add a cleanup mechanism to the rateLimitMap in docs/fumadocs/src/app/api/save-skill/route.ts. One approach: at the beginning of checkRateLimit(), periodically (e.g. every 100 calls or every 60 seconds) iterate the map and delete entries whose resetTime is in the past. Alternatively, add a MAX_MAP_SIZE constant (e.g. 10000) and if the map exceeds it, sweep all expired entries. Another option is to switch to an LRU cache library with TTL support. The key change should be in the checkRateLimit function around lines 8-23.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@docs/fumadocs/src/app/api/save-skill/route.ts`:
- Around line 27-48: isAllowedUrl currently validates only the hostname string
which is vulnerable to DNS rebinding; update the flow so that before calling
fetch (the call around line 83) you perform a DNS resolution of the provided
hostname (e.g., using dns.promises.lookup or resolve to get all IPs) and run the
same IP-blocklist checks against each resolved address (including IPv4-mapped
IPv6 forms) in addition to the existing hostname checks in isAllowedUrl; also
explicitly add cloud metadata hostnames like "metadata.google.internal" and the
literal address "169.254.169.254" to the BLOCKED_HOSTS set; alternatively, you
may replace the fetch with an agent/library (e.g., ssrf-req-stream or a custom
DNS-resolving agent) that enforces IP validation before connecting.
- Around line 78-101: extractFromUrl currently calls response.text() (and
fetchGitHubContent does the same) which can OOM on large responses; add a size
guard: define a MAX_BODY_BYTES constant, check
response.headers.get('content-length') and throw if it exceeds the limit, and if
no or chunked length is returned, consume response.body via a ReadableStream
reader and accumulate chunks up to MAX_BODY_BYTES, aborting and throwing if the
limit is exceeded; replace response.text() usage with this bounded-stream result
and apply the same streaming/Content-Length guard inside fetchGitHubContent to
protect both code paths.
- Around line 4-23: The in-memory rate limiter leaks because expired entries in
rateLimitMap are never removed and is ineffective in serverless; update the
checkRateLimit function to evict stale entries on each call (iterate keys or
check specific key and remove when resetTime < now) using the existing
RATE_LIMIT_WINDOW_MS constant to compute expirations, and/or add a periodic
cleanup routine that prunes entries from rateLimitMap; additionally, replace or
back this logic with an external store (e.g., Upstash/Redis with a
sliding-window algorithm) when deploying to serverless to ensure global,
persistent rate limiting.
In `@packages/extension/src/background.ts`:
- Around line 73-90: callSaveApi currently calls response.json() unguarded which
will throw on non-JSON error pages; update callSaveApi to first check
response.ok and only parse JSON when appropriate, and for non-ok responses call
response.text() (or try/catch response.json()) to capture the raw body and the
HTTP status, then return an ErrorResponse containing the status and body;
likewise wrap the successful-path JSON.parse in try/catch so malformed JSON
returns a clear ErrorResponse. Reference: callSaveApi, SaveResponse,
ErrorResponse, API_URL.
- Around line 92-112: The front-matter "source" field in buildSelectionSkill is
inserting the raw url into skillMd which can break YAML; update
buildSelectionSkill to pass the url through yamlEscape (or import/implement
yamlEscape if missing) when constructing the `skillMd` string so the `source:
${...}` line is YAML-safe (preserve the existing conditional that omits the line
when url is falsy); reference the function name buildSelectionSkill, the skillMd
variable and the `source:` front-matter key when making the change.
- Line 3: The API_URL constant in background.ts is misspelled; change the value
of API_URL (const API_URL) from 'https://agenstskills.com/api/save-skill' to the
correct domain 'https://agentskills.com/api/save-skill' so save-skill requests
target the proper endpoint; update the string literal for API_URL accordingly.
🧹 Nitpick comments (4)
packages/extension/src/popup.ts (1)
39-46:titlein the SAVE_PAGE payload is sent but never consumed.The popup sends
titlein the SAVE_PAGE message (line 43), butbackground.ts(line 50) only destructures{ url, name }—titleis ignored since the server extracts the title itself. Consider removingtitlefrom theSAVE_PAGEpayload type and here to avoid confusion.docs/fumadocs/src/app/api/save-skill/route.ts (2)
172-177: Static analysis ReDoS warning is a false positive here.
TECH_KEYWORDSis a hardcodedSetof simple alphanumeric strings, so constructingnew RegExp(\\b${keyword}\b`)` from them is safe. No action needed.However, creating a new
RegExpper keyword per request is slightly wasteful. You could pre-compile these at module level:Optional: pre-compile keyword patterns
+const TECH_KEYWORD_PATTERNS = new Map( + [...TECH_KEYWORDS].map((kw) => [kw, new RegExp(`\\b${kw}\\b`, 'i')] as const), +); + // In detectTags: - for (const keyword of TECH_KEYWORDS) { - if (new RegExp(`\\b${keyword}\\b`, 'i').test(lower)) { - addTag(counts, keyword, 1); - } + for (const [keyword, pattern] of TECH_KEYWORD_PATTERNS) { + if (pattern.test(lower)) { + addTag(counts, keyword, 1); + } }
206-215: Rate limit key fromx-forwarded-foris easily spoofable.Line 207 trusts the first value of
x-forwarded-for, which any client can set. On platforms like Vercel, the reliable client IP header is typically platform-specific (e.g.,x-real-iporx-vercel-forwarded-for). If this runs behind a trusted reverse proxy, ensure the proxy strips/overwritesx-forwarded-for.packages/extension/src/background.ts (1)
123-133: Blob URL revocation relies on the download callback firing.The
chrome.downloads.downloadcallback may not fire if the download is blocked or fails silently, leaking the blob URL. This is a minor concern in a service worker context (short-lived), but worth noting.
| const rateLimitMap = new Map<string, { count: number; resetTime: number }>(); | ||
| const RATE_LIMIT_WINDOW_MS = 60 * 1000; | ||
| const RATE_LIMIT_MAX_REQUESTS = 10; | ||
|
|
||
| function checkRateLimit(key: string): { allowed: boolean; remaining: number } { | ||
| const now = Date.now(); | ||
| const entry = rateLimitMap.get(key); | ||
|
|
||
| if (!entry || now > entry.resetTime) { | ||
| rateLimitMap.set(key, { count: 1, resetTime: now + RATE_LIMIT_WINDOW_MS }); | ||
| return { allowed: true, remaining: RATE_LIMIT_MAX_REQUESTS - 1 }; | ||
| } | ||
|
|
||
| if (entry.count >= RATE_LIMIT_MAX_REQUESTS) { | ||
| return { allowed: false, remaining: 0 }; | ||
| } | ||
|
|
||
| entry.count++; | ||
| return { allowed: true, remaining: RATE_LIMIT_MAX_REQUESTS - entry.count }; | ||
| } |
There was a problem hiding this comment.
In-memory rate limit map leaks indefinitely and is ineffective in serverless deployments.
Two problems:
- Memory leak: Expired entries are never evicted. Under sustained traffic the map grows without bound.
- Serverless: Each cold-start instance gets a fresh map, so the rate limit is trivially bypassed on platforms like Vercel.
For a quick fix on (1), add periodic cleanup or evict stale entries inside checkRateLimit. For (2), consider an external store (e.g., Upstash Redis with sliding-window) if real rate limiting is required.
Minimal fix for the memory leak
function checkRateLimit(key: string): { allowed: boolean; remaining: number } {
const now = Date.now();
const entry = rateLimitMap.get(key);
+ // Prune expired entries periodically
+ if (rateLimitMap.size > 1000) {
+ for (const [k, v] of rateLimitMap) {
+ if (now > v.resetTime) rateLimitMap.delete(k);
+ }
+ }
+
if (!entry || now > entry.resetTime) {🤖 Prompt for AI Agents
In `@docs/fumadocs/src/app/api/save-skill/route.ts` around lines 4 - 23, The
in-memory rate limiter leaks because expired entries in rateLimitMap are never
removed and is ineffective in serverless; update the checkRateLimit function to
evict stale entries on each call (iterate keys or check specific key and remove
when resetTime < now) using the existing RATE_LIMIT_WINDOW_MS constant to
compute expirations, and/or add a periodic cleanup routine that prunes entries
from rateLimitMap; additionally, replace or back this logic with an external
store (e.g., Upstash/Redis with a sliding-window algorithm) when deploying to
serverless to ensure global, persistent rate limiting.
| function isAllowedUrl(url: string): boolean { | ||
| try { | ||
| const parsed = new URL(url); | ||
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false; | ||
|
|
||
| const hostname = parsed.hostname.toLowerCase(); | ||
| const bare = hostname.replace(/^\[|\]$/g, ''); | ||
|
|
||
| if (BLOCKED_HOSTS.has(hostname) || BLOCKED_HOSTS.has(bare)) return false; | ||
| if (bare.startsWith('::ffff:')) return isAllowedUrl(`http://${bare.slice(7)}`); | ||
| if (/^127\./.test(bare) || /^0\./.test(bare)) return false; | ||
| if (bare.startsWith('10.') || bare.startsWith('192.168.')) return false; | ||
| if (/^172\.(1[6-9]|2\d|3[01])\./.test(bare)) return false; | ||
| if (bare.startsWith('169.254.')) return false; | ||
| if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false; | ||
| if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false; | ||
| if (bare.startsWith('ff')) return false; | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
SSRF protection is vulnerable to DNS rebinding.
isAllowedUrl validates the hostname string but the actual fetch on line 83 resolves DNS independently. An attacker can register a domain that first resolves to a public IP (passing validation) and then rebinds to 127.0.0.1 (or a private IP) at fetch time.
Mitigations:
- Resolve the hostname to an IP before fetching and validate the resolved IP against the same blocklist.
- Alternatively, use a library like
ssrf-req-streamor configure a custom DNS resolver/agent that rejects private IPs.
Also, metadata.google.internal and cloud metadata endpoints (e.g., 169.254.169.254) should be explicitly blocked by hostname in addition to the IP range check on line 40.
🤖 Prompt for AI Agents
In `@docs/fumadocs/src/app/api/save-skill/route.ts` around lines 27 - 48,
isAllowedUrl currently validates only the hostname string which is vulnerable to
DNS rebinding; update the flow so that before calling fetch (the call around
line 83) you perform a DNS resolution of the provided hostname (e.g., using
dns.promises.lookup or resolve to get all IPs) and run the same IP-blocklist
checks against each resolved address (including IPv4-mapped IPv6 forms) in
addition to the existing hostname checks in isAllowedUrl; also explicitly add
cloud metadata hostnames like "metadata.google.internal" and the literal address
"169.254.169.254" to the BLOCKED_HOSTS set; alternatively, you may replace the
fetch with an agent/library (e.g., ssrf-req-stream or a custom DNS-resolving
agent) that enforces IP validation before connecting.
| 'testing', 'ci', 'cd', 'git', 'webpack', 'vite', 'tailwind', 'css', | ||
| 'ai', 'ml', 'llm', 'mcp', 'agent', 'prompt', 'rag', 'embedding', | ||
| ]); | ||
| const API_URL = 'https://agenstskills.com/api/save-skill'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for "agenstskills" occurrences
rg -n "agenstskills" --type-list | head -20Repository: rohitg00/skillkit
Length of output: 450
🏁 Script executed:
# Search for domain references to understand the context better
rg -n "https://" packages/extension/src/background.tsRepository: rohitg00/skillkit
Length of output: 121
🏁 Script executed:
# Search for any configuration files that might contain API endpoints
fd -e json -e yaml -e config | head -20 | xargs rg -l "agenstskills|agentskills" 2>/dev/null || trueRepository: rohitg00/skillkit
Length of output: 84
🏁 Script executed:
# Check git history to see if this was intentional or changed
cd packages/extension/src && git log --oneline -n 20 background.ts 2>/dev/null | head -10 || echo "Git not available or no history"Repository: rohitg00/skillkit
Length of output: 119
🏁 Script executed:
# Search for "agentskills" (without typo) to see if correct variant exists elsewhere
rg -n "agentskills"Repository: rohitg00/skillkit
Length of output: 2277
🏁 Script executed:
# Check marketplace/skills.json for API endpoint references
cat marketplace/skills.json | head -50Repository: rohitg00/skillkit
Length of output: 1334
🏁 Script executed:
# Search for other API endpoint references or domain patterns
rg -n "api/save-skill|API_URL|https://" packages/extension/src/Repository: rohitg00/skillkit
Length of output: 240
🏁 Script executed:
# Look for documentation or README files
fd -e md | xargs grep -l "agentskills\|API\|api/save-skill" 2>/dev/null | head -10Repository: rohitg00/skillkit
Length of output: 582
🏁 Script executed:
# Check if there's any environment configuration
fd -e env -e example | head -20Repository: rohitg00/skillkit
Length of output: 43
Typo in API URL domain — agenstskills.com should be agentskills.com.
The domain name is misspelled. The correct domain is documented in skills/find-skills/SKILL.md and referenced throughout marketplace/skills.json. The current typo will cause all save-skill API requests to fail.
🤖 Prompt for AI Agents
In `@packages/extension/src/background.ts` at line 3, The API_URL constant in
background.ts is misspelled; change the value of API_URL (const API_URL) from
'https://agenstskills.com/api/save-skill' to the correct domain
'https://agentskills.com/api/save-skill' so save-skill requests target the
proper endpoint; update the string literal for API_URL accordingly.
| async function callSaveApi(url: string, name?: string): Promise<SaveResponse | ErrorResponse> { | ||
| const response = await fetch(API_URL, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ url, name }), | ||
| }); | ||
|
|
||
| function generateAndDownload(input: GenerateInput): SaveResponse | ErrorResponse { | ||
| try { | ||
| const name = slugify(input.name || input.title || titleFromUrl(input.url)); | ||
| const tags = detectTags(input.url, input.content); | ||
| const description = makeDescription(input.content); | ||
| const savedAt = new Date().toISOString(); | ||
|
|
||
| const yamlTags = tags.length > 0 | ||
| ? `tags:\n${tags.map((t) => ` - ${t}`).join('\n')}\n` | ||
| : ''; | ||
|
|
||
| const skillMd = | ||
| `---\n` + | ||
| `name: ${name}\n` + | ||
| `description: ${yamlEscape(description)}\n` + | ||
| yamlTags + | ||
| `metadata:\n` + | ||
| (input.url ? ` source: ${input.url}\n` : '') + | ||
| ` savedAt: ${savedAt}\n` + | ||
| `---\n\n` + | ||
| input.content + '\n'; | ||
|
|
||
| const filename = `${name}/SKILL.md`; | ||
|
|
||
| const blob = new Blob([skillMd], { type: 'text/markdown' }); | ||
| const blobUrl = URL.createObjectURL(blob); | ||
|
|
||
| chrome.downloads.download({ | ||
| url: blobUrl, | ||
| filename: `skillkit-skills/${filename}`, | ||
| saveAs: false, | ||
| }, () => { | ||
| URL.revokeObjectURL(blobUrl); | ||
| }); | ||
|
|
||
| return { name, filename, skillMd, tags }; | ||
| } catch (err) { | ||
| return { error: err instanceof Error ? err.message : 'Generation failed' }; | ||
| const data = await response.json(); | ||
| if (!response.ok) { | ||
| return { error: data.error ?? `Server error: ${response.status}` }; | ||
| } | ||
| return { | ||
| name: data.name, | ||
| filename: `${data.name}/SKILL.md`, | ||
| skillMd: data.skillMd, | ||
| tags: data.tags ?? [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
response.json() will throw on non-JSON error responses (e.g., HTML 502/504 from proxy).
If the server or an intermediary returns an HTML error page, response.json() throws a SyntaxError. This is caught by the .catch() in the message listener (line 56) but silently swallows the actual HTTP status. Consider guarding:
Suggested fix
- const data = await response.json();
+ let data: Record<string, unknown>;
+ try {
+ data = await response.json();
+ } catch {
+ return { error: `Server error: ${response.status}` };
+ }📝 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.
| async function callSaveApi(url: string, name?: string): Promise<SaveResponse | ErrorResponse> { | |
| const response = await fetch(API_URL, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ url, name }), | |
| }); | |
| function generateAndDownload(input: GenerateInput): SaveResponse | ErrorResponse { | |
| try { | |
| const name = slugify(input.name || input.title || titleFromUrl(input.url)); | |
| const tags = detectTags(input.url, input.content); | |
| const description = makeDescription(input.content); | |
| const savedAt = new Date().toISOString(); | |
| const yamlTags = tags.length > 0 | |
| ? `tags:\n${tags.map((t) => ` - ${t}`).join('\n')}\n` | |
| : ''; | |
| const skillMd = | |
| `---\n` + | |
| `name: ${name}\n` + | |
| `description: ${yamlEscape(description)}\n` + | |
| yamlTags + | |
| `metadata:\n` + | |
| (input.url ? ` source: ${input.url}\n` : '') + | |
| ` savedAt: ${savedAt}\n` + | |
| `---\n\n` + | |
| input.content + '\n'; | |
| const filename = `${name}/SKILL.md`; | |
| const blob = new Blob([skillMd], { type: 'text/markdown' }); | |
| const blobUrl = URL.createObjectURL(blob); | |
| chrome.downloads.download({ | |
| url: blobUrl, | |
| filename: `skillkit-skills/${filename}`, | |
| saveAs: false, | |
| }, () => { | |
| URL.revokeObjectURL(blobUrl); | |
| }); | |
| return { name, filename, skillMd, tags }; | |
| } catch (err) { | |
| return { error: err instanceof Error ? err.message : 'Generation failed' }; | |
| const data = await response.json(); | |
| if (!response.ok) { | |
| return { error: data.error ?? `Server error: ${response.status}` }; | |
| } | |
| return { | |
| name: data.name, | |
| filename: `${data.name}/SKILL.md`, | |
| skillMd: data.skillMd, | |
| tags: data.tags ?? [], | |
| }; | |
| } | |
| async function callSaveApi(url: string, name?: string): Promise<SaveResponse | ErrorResponse> { | |
| const response = await fetch(API_URL, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ url, name }), | |
| }); | |
| let data: Record<string, unknown>; | |
| try { | |
| data = await response.json(); | |
| } catch { | |
| return { error: `Server error: ${response.status}` }; | |
| } | |
| if (!response.ok) { | |
| return { error: data.error ?? `Server error: ${response.status}` }; | |
| } | |
| return { | |
| name: data.name, | |
| filename: `${data.name}/SKILL.md`, | |
| skillMd: data.skillMd, | |
| tags: data.tags ?? [], | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/background.ts` around lines 73 - 90, callSaveApi
currently calls response.json() unguarded which will throw on non-JSON error
pages; update callSaveApi to first check response.ok and only parse JSON when
appropriate, and for non-ok responses call response.text() (or try/catch
response.json()) to capture the raw body and the HTTP status, then return an
ErrorResponse containing the status and body; likewise wrap the successful-path
JSON.parse in try/catch so malformed JSON returns a clear ErrorResponse.
Reference: callSaveApi, SaveResponse, ErrorResponse, API_URL.
Privacy policy previously stated "no external requests" which is now inaccurate. Updated to clearly disclose that the page URL is sent to agenstskills.com for processing, what data is NOT sent, and that nothing is stored server-side. Store listing updated to match: removed "no server" claims, added data flow explanation, permissions justifications, and data usage disclosure section for the Chrome Web Store Privacy tab.
- Fix SSRF IPv6 checks blocking legit domains (ffmpeg.org, fdroid.org) by requiring colon in fc00:/ff prefixes (Devin) - Add 5MB response body size limit to prevent OOM on large pages, applied to both extractFromUrl and fetchGitHubContent (CodeRabbit) - Guard response.json() in callSaveApi against non-JSON error pages from proxies returning HTML 502/504 (CodeRabbit) - YAML-escape source URL in buildSelectionSkill to match server-side handling of URLs with #, :, or query strings (CodeRabbit)
| } | ||
|
|
||
| const MAX_BODY_SIZE = 5 * 1024 * 1024; | ||
| const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT) }); |
There was a problem hiding this comment.
🔴 SSRF bypass via HTTP redirects — fetch follows redirects without re-validating the target URL
The isAllowedUrl check at route.ts:251 validates only the user-supplied URL, but the subsequent fetch call at route.ts:84 uses the default redirect policy (redirect: 'follow'). An attacker can supply a URL like https://attacker.com/redir that passes isAllowedUrl, but have the server respond with a 302 redirect to http://169.254.169.254/latest/meta-data/ (AWS instance metadata) or any other internal/blocked address.
Root Cause and Impact
The validation at line 251 calls isAllowedUrl(url) on the original URL only. When fetch(url, ...) is called at line 84, Node's fetch implementation follows redirects by default (up to 20 hops). The redirect target is never checked against isAllowedUrl.
// route.ts:84 — no redirect restriction
const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT) });Impact: A malicious user can read responses from internal services (cloud metadata endpoints, internal APIs on private networks) by pointing the server at an attacker-controlled redirect. This is a classic SSRF vector that defeats the hostname-based blocklist.
| const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT) }); | |
| const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT), redirect: 'error' }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
POST /api/save-skillAPI routeChanges
docs/fumadocs/src/app/api/save-skill/route.ts— API route with SSRF protection, rate limiting (10 req/min), Turndown HTML→Markdown, GitHub raw URL conversion, weighted tag detectionpackages/extension/src/background.ts— calls API instead of injecting content scripts; selection saves use Chrome's nativeselectionTextpackages/extension/src/popup.ts— readstab.url/tab.titledirectly, no content script messagingpackages/extension/src/types.ts— removedGET_PAGE_INFO,PAGE_INFO,PageContentmanifest.json— removedscriptingpermission +content_scriptsblock, bumped to v1.18.0packages/extension/src/content.ts— no longer neededturndowndependency from extension (moved to server)Architecture
Test plan
cd docs/fumadocs && pnpm dev→ curl the API endpoint with a URLpnpm --filter @skillkit/extension build), load unpacked in Chromechrome://extensions→ should show "Cannot save" error, not crashSummary by CodeRabbit
New Features
Changes