Conversation
|
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. 📝 WalkthroughWalkthroughIntroduces a new API endpoint for saving web content as Skill artifacts and a Chrome extension that enables users to extract and save page content through context menus and a popup interface. The API validates input, extracts content, and generates Skill markdown with metadata. The extension manages content extraction, Skill generation, and file downloads across background, content, and popup scripts. Changes
Sequence DiagramsequenceDiagram
actor User
participant Popup as Popup Script
participant ContentScript as Content Script
participant Background as Background Script
participant API as API Server
User->>Popup: Opens extension popup
Popup->>ContentScript: GET_PAGE_INFO (request page content)
ContentScript->>ContentScript: Extract title, URL, markdown, selection
ContentScript->>Popup: PAGE_INFO (return extracted content)
Popup->>Popup: Populate title, URL, markdown fields
User->>Popup: Click "Save as Skill"
Popup->>Background: SAVE_PAGE (url, title, markdown, name?)
Background->>API: POST /save (url, title, markdown, name?)
API->>API: Extract content, generate Skill markdown, detect tags
API->>Background: { name, skillPath, skillMd, tags }
Background->>Background: Create blob, compute filename, trigger download
Background->>Popup: SaveResponse (name, filename, skillMd, tags)
Popup->>Popup: Display success with download path
Popup->>User: Show result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
rohitg00
force-pushed
the
feat/chrome-extension-save-as-skill
branch
from
6097a19 to
734e71f
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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(); | ||
| if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1' || hostname === '0.0.0.0') return false; | ||
| if (hostname.startsWith('10.') || hostname.startsWith('192.168.')) return false; | ||
| if (/^172\.(1[6-9]|2\d|3[01])\./.test(hostname)) return false; | ||
| if (hostname.startsWith('169.254.')) return false; | ||
| if (hostname.startsWith('fe80:') || hostname.startsWith('fc') || hostname.startsWith('fd')) return false; | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 SSRF bypass: IPv6-mapped IPv4 addresses (::ffff:127.0.0.1) bypass all private-IP checks
The SSRF check only tests hostname strings against IPv4 patterns (e.g., startsWith('10.'), startsWith('192.168.')) and a few IPv6 prefixes. It does not handle IPv6-mapped IPv4 addresses like ::ffff:127.0.0.1, ::ffff:10.0.0.1, ::ffff:192.168.1.1, or ::ffff:169.254.169.254, all of which bypass the filter and route to the corresponding private IPv4 address.
Root Cause and Impact
When a user submits http://[::ffff:127.0.0.1]/ as the URL, URL.hostname becomes [::ffff:7f00:1]. This doesn't match any of the existing string-based checks:
- Not equal to
127.0.0.1orlocalhost - Doesn't start with
10.,192.168.,169.254., etc. - Doesn't start with
fe80:,fc, orfd
But when Node.js fetch() (called in packages/core/src/save/extractor.ts:65) resolves this address, it connects to 127.0.0.1.
Similarly:
http://[::ffff:169.254.169.254]/→ reaches AWS metadata endpointhttp://[::ffff:10.0.0.1]/→ reaches internal 10.x networkhttp://[::ffff:192.168.1.1]/→ reaches internal 192.168.x network
Impact: Full SSRF bypass to any private IPv4 address via IPv6-mapped notation, including cloud metadata endpoints.
Prompt for agents
In packages/api/src/routes/save.ts isAllowedUrl function, after stripping brackets from the hostname, add a check to detect IPv6-mapped IPv4 addresses. These have the form ::ffff:x.x.x.x or ::ffff:HHHH:HHHH. Strip the '::ffff:' prefix if present, then check if the remaining part is a dotted IPv4 address and re-run all private IPv4 range checks against it. Also consider using a proper IP parsing library (like 'ipaddr.js') that can normalize all address formats and check range membership, rather than relying on string prefix matching.
Was this helpful? React with 👍 or 👎 to provide feedback.
rohitg00
force-pushed
the
feat/chrome-extension-save-as-skill
branch
from
734e71f to
a80e805
Compare
packages/api/src/routes/save.ts
Outdated
| if (hostname.startsWith('10.') || hostname.startsWith('192.168.')) return false; | ||
| if (/^172\.(1[6-9]|2\d|3[01])\./.test(hostname)) return false; | ||
| if (hostname.startsWith('169.254.')) return false; | ||
| if (hostname.startsWith('fe80:') || hostname.startsWith('fc') || hostname.startsWith('fd')) return false; |
There was a problem hiding this comment.
🔴 SSRF fc/fd prefix check blocks legitimate public domains (e.g. fcc.gov, fdic.gov)
The check hostname.startsWith('fc') || hostname.startsWith('fd') on line 22 is intended to block IPv6 Unique Local Addresses (fc00::/7), but it operates on the raw hostname string. This incorrectly blocks legitimate public domains like fcc.gov, fdic.gov, fd-example.com, etc.
Root Cause and Impact
The code at packages/api/src/routes/save.ts:22:
if (hostname.startsWith('fe80:') || hostname.startsWith('fc') || hostname.startsWith('fd')) return false;This string prefix check is far too broad for regular domain names. Any domain starting with "fc" or "fd" (e.g., fcc.gov, fdic.gov, facebook-dev.example.com) will be rejected as a private address. Meanwhile, the actual IPv6 ULA addresses it's meant to block ([fc00::1]) have brackets and don't match startsWith('fc') anyway (see BUG-0001).
Impact: Users cannot save skills from any URL whose hostname starts with "fc" or "fd", causing false rejections for legitimate public websites.
Prompt for agents
In packages/api/src/routes/save.ts line 22, the IPv6 ULA and link-local checks need to operate on the bare hostname (with brackets stripped) and should not match against regular domain names. Strip brackets from the hostname before checking IPv6 patterns. For example, extract the bare address with hostname.replace(/^\[|\]$/g, '') and only apply the fc/fd/fe80 checks to that bare value when it looks like an IPv6 address (contains a colon). This prevents false positives on domains like fcc.gov while still blocking [fc00::1].
Was this helpful? React with 👍 or 👎 to provide feedback.
| const result = generator.generate(content, { | ||
| name: body.name, | ||
| global: body.global ?? true, | ||
| }); |
There was a problem hiding this comment.
🔴 API /save endpoint writes files to server filesystem via SkillGenerator.generate()
The /save API endpoint calls generator.generate() which internally calls mkdirSync and writeFileSync to write SKILL.md files to the server's disk. Every POST request creates directories and files on the server.
Root Cause and Impact
At packages/api/src/routes/save.ts:59-62:
const result = generator.generate(content, {
name: body.name,
global: body.global ?? true,
});The SkillGenerator.generate() method (packages/core/src/save/skill-generator.ts:46-50) performs filesystem writes:
const outputDir = options.outputDir ?? this.defaultOutputDir(name, options.global);
mkdirSync(outputDir, { recursive: true });
const skillPath = join(outputDir, 'SKILL.md');
writeFileSync(skillPath, skillMd, 'utf-8');With global: true (the default), this writes to ~/.skillkit/skills/<name>/SKILL.md on the server. Since name comes from user input (after slugification), any unauthenticated caller can fill the server's disk with arbitrary files. The API appears to be intended as a stateless content-generation endpoint that returns JSON, but it has an unintended side effect of persisting files.
Impact: Unauthenticated disk writes on the server; potential disk-filling DoS; the server accumulates files from every request with no cleanup mechanism.
Prompt for agents
In packages/api/src/routes/save.ts, the SkillGenerator.generate() method writes files to the server filesystem which is not appropriate for an API endpoint. Instead, either: (1) refactor SkillGenerator to have a method that only generates the skillMd content without writing to disk, or (2) use ContentExtractor to get the content and then build the SKILL.md string directly in the route handler (similar to how background.ts does it), returning only the JSON response without any filesystem side effects.
Was this helpful? React with 👍 or 👎 to provide feedback.
rohitg00
force-pushed
the
feat/chrome-extension-save-as-skill
branch
from
a80e805 to
1006a04
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/api/src/routes/save.ts`:
- Around line 24-28: The private-range prefix checks on the hostname variable
bare are currently applied to raw hostnames and must only run when bare is a
literal IP; update the logic so you first detect whether bare is an IP (use a
canonical IP test such as Node's net.isIP or a robust IP regex) and only then
apply the existing checks (the ::ffff: handling, startsWith('10.'),
startsWith('192.168.'), the /^172\.(1[6-9]|2\d|3[01])\./ regex,
startsWith('169.254.'), startsWith('fe80:'/ 'fc'/ 'fd')). Ensure IPv4-mapped
IPv6 (::ffff:) handling remains correct and skip these prefix checks when bare
is a domain name (instead of blocking domains like fcbarcelona.com); consider
adding a comment about resolving hostnames later if DNS rebinding must be
addressed.
In `@packages/extension/src/background.ts`:
- Around line 69-108: The download callback in generateAndDownload revokes the
blobUrl immediately which can cancel Chrome's read; instead delay revocation (or
wait for chrome.downloads.onChanged to confirm the download has
started/completed) before calling URL.revokeObjectURL(blobUrl). Update the
callback passed to chrome.downloads.download in generateAndDownload to schedule
URL.revokeObjectURL(blobUrl) after a short timeout (e.g. ~500–1000ms) or hook
chrome.downloads.onChanged for the returned downloadId and revoke once the
download state advances, ensuring blobUrl is not revoked prematurely.
- Around line 80-89: The YAML frontmatter construction in the skillMd template
inserts input.url raw into the `source:` field which can produce invalid YAML
for URLs containing ":" or "#" (see variable skillMd and input.url); update that
interpolation to escape or quote the URL (reuse the existing yamlEscape function
or wrap the value in quotes) so the `source:` line becomes a safe YAML scalar,
e.g., call yamlEscape(input.url) or add explicit quoting when building skillMd.
In `@packages/extension/src/popup.ts`:
- Around line 77-101: The save function currently awaits
chrome.runtime.sendMessage which can reject and cause an unhandled promise; wrap
the call in a try/catch inside save to catch any thrown errors, ensure
setButtonsDisabled(false) is executed in both success and failure (use finally
or restore state in catch), and in the catch call setStatus('error',
errorMessage) and hide resultEl (same behavior as when response is missing or
contains error) so the popup doesn't crash; update handling of the local
response variable accordingly to keep the rest of save (setting resultPath,
setStatus('success')) unchanged when a valid response is returned.
🧹 Nitpick comments (6)
packages/extension/package.json (1)
8-9:devscript doesn't copy static assets todist/.
tsup --watchonly rebuilds TS entry points. The manifest, popup HTML/CSS, and icons won't be indist/until a fullbuildis run. This means developers must runbuildfirst, and any edits to static assets during watch mode won't be reflected.Consider using
tsup'sonSuccesshook intsup.config.tsor a small helper script to copy statics on each rebuild.packages/extension/src/manifest.json (2)
18-23: Consider programmatic injection instead of<all_urls>content script.Injecting a content script into every page the user visits adds overhead and broadens the extension's footprint. Since you already have
activeTabpermission, you could inject the content script on-demand viachrome.scripting.executeScript()in the background worker when the user triggers a save action. This would:
- Eliminate the per-page injection cost
- Remove the need for the broad
<all_urls>match pattern- Require adding the
"scripting"permission insteadThis is a common Manifest V3 best practice for extensions that only need page access on user interaction.
6-6: Remove the unusedstoragepermission.The
storagepermission is declared in the manifest butchrome.storageAPI is not used anywhere in the codebase. The "storage" references found are for custom application-level storage classes (InboxStorage, SentStorage, etc.), not the Chrome storage API. Remove this permission to keep the permission set minimal.packages/extension/src/popup.html (2)
5-5: Non-standard viewport meta value.
content="width=360"is non-standard for a viewport meta tag. Chrome extension popups size themselves based on content dimensions, so this meta tag has no practical effect. You can either remove it or use the conventionalwidth=device-width, initial-scale=1.0.
37-40: Consider addingaria-liveto the status region.Adding
role="status"andaria-live="polite"to the status div would allow screen readers to announce state changes (saving, success, error) automatically.♿ Proposed fix
- <div id="status" class="status" style="display:none"> + <div id="status" class="status" style="display:none" role="status" aria-live="polite">packages/extension/src/background.ts (1)
144-152:detectTagssubstring matching yields false positives for short keywords.
text.includes(kw)matches substrings — e.g.,"go"matches "google", "going", "cargo";"cd"matches "code";"ai"matches "plain", "maintain", etc. This degrades tag quality.Use word-boundary matching instead:
Proposed fix
function detectTags(url: string, content: string): string[] { const text = `${url} ${content}`.toLowerCase(); const found: string[] = []; for (const kw of TECH_KEYWORDS) { - if (text.includes(kw)) found.push(kw); + if (new RegExp(`\\b${kw}\\b`).test(text)) found.push(kw); if (found.length >= 10) break; } return found; }
| if (bare.startsWith('::ffff:')) return isAllowedUrl(`http://${bare.slice(7)}`); | ||
| 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; |
There was a problem hiding this comment.
Private-range checks match domain names, not just IPs — false positives and SSRF bypass risk.
The startsWith checks for private IP ranges are applied to the raw hostname, which can be a domain name. This causes two problems:
- False positives: Domains like
fda.gov,fcbarcelona.com, or10bis.co.ilare incorrectly blocked because they matchfd,fc, or10.prefixes. - SSRF bypass: An attacker can register a domain (e.g.,
evil.com) that resolves to a private IP, bypassing all these checks entirely (DNS rebinding).
Guard the prefix checks behind an IP-format test so they only apply to actual IP addresses:
Proposed fix
+ // Only apply IP-range checks to numeric/IP hostnames
+ const isIPv4 = /^\d{1,3}(\.\d{1,3}){3}$/.test(bare);
+ const isIPv6 = bare.includes(':');
+
- 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 (isIPv4) {
+ 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('0.')) return false;
+ }
+ if (isIPv6) {
+ if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false;
+ }Note: DNS rebinding (hostname resolving to a private IP after validation) remains an open concern — consider resolving the hostname and validating the resolved IP if the threat model warrants it.
🤖 Prompt for AI Agents
In `@packages/api/src/routes/save.ts` around lines 24 - 28, The private-range
prefix checks on the hostname variable bare are currently applied to raw
hostnames and must only run when bare is a literal IP; update the logic so you
first detect whether bare is an IP (use a canonical IP test such as Node's
net.isIP or a robust IP regex) and only then apply the existing checks (the
::ffff: handling, startsWith('10.'), startsWith('192.168.'), the
/^172\.(1[6-9]|2\d|3[01])\./ regex, startsWith('169.254.'), startsWith('fe80:'/
'fc'/ 'fd')). Ensure IPv4-mapped IPv6 (::ffff:) handling remains correct and
skip these prefix checks when bare is a domain name (instead of blocking domains
like fcbarcelona.com); consider adding a comment about resolving hostnames later
if DNS rebinding must be addressed.
| 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' }; | ||
| } | ||
| } |
There was a problem hiding this comment.
URL.revokeObjectURL in download callback may fire before Chrome reads the blob.
The chrome.downloads.download callback fires when the download is initiated, not completed. If Chrome hasn't finished reading the blob URL by then, revocation could cause a failed download. Adding a small delay is a common defensive pattern:
Proposed fix
chrome.downloads.download({
url: blobUrl,
filename: `skillkit-skills/${filename}`,
saveAs: false,
}, () => {
- URL.revokeObjectURL(blobUrl);
+ setTimeout(() => URL.revokeObjectURL(blobUrl), 1000);
});📝 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.
| 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' }; | |
| } | |
| } | |
| 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, | |
| }, () => { | |
| setTimeout(() => URL.revokeObjectURL(blobUrl), 1000); | |
| }); | |
| return { name, filename, skillMd, tags }; | |
| } catch (err) { | |
| return { error: err instanceof Error ? err.message : 'Generation failed' }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/background.ts` around lines 69 - 108, The download
callback in generateAndDownload revokes the blobUrl immediately which can cancel
Chrome's read; instead delay revocation (or wait for chrome.downloads.onChanged
to confirm the download has started/completed) before calling
URL.revokeObjectURL(blobUrl). Update the callback passed to
chrome.downloads.download in generateAndDownload to schedule
URL.revokeObjectURL(blobUrl) after a short timeout (e.g. ~500–1000ms) or hook
chrome.downloads.onChanged for the returned downloadId and revoke once the
download state advances, ensuring blobUrl is not revoked prematurely.
| 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'; |
There was a problem hiding this comment.
Unescaped URL in YAML frontmatter can produce invalid YAML.
The source URL (line 86) is interpolated raw into YAML. URLs commonly contain : and #, both YAML-significant characters. A URL like https://example.com/page#section will break YAML parsers.
Apply yamlEscape (or simply quote the value) for the source field:
Proposed fix
- (input.url ? ` source: ${input.url}\n` : '') +
+ (input.url ? ` source: ${yamlEscape(input.url)}\n` : '') +📝 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.
| 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 skillMd = | |
| `---\n` + | |
| `name: ${name}\n` + | |
| `description: ${yamlEscape(description)}\n` + | |
| yamlTags + | |
| `metadata:\n` + | |
| (input.url ? ` source: ${yamlEscape(input.url)}\n` : '') + | |
| ` savedAt: ${savedAt}\n` + | |
| `---\n\n` + | |
| input.content + '\n'; |
🤖 Prompt for AI Agents
In `@packages/extension/src/background.ts` around lines 80 - 89, The YAML
frontmatter construction in the skillMd template inserts input.url raw into the
`source:` field which can produce invalid YAML for URLs containing ":" or "#"
(see variable skillMd and input.url); update that interpolation to escape or
quote the URL (reuse the existing yamlEscape function or wrap the value in
quotes) so the `source:` line becomes a safe YAML scalar, e.g., call
yamlEscape(input.url) or add explicit quoting when building skillMd.
| async function save(message: ExtensionMessage) { | ||
| setStatus('saving', 'Saving...'); | ||
| setButtonsDisabled(true); | ||
|
|
||
| const response: SaveResponse | ErrorResponse | undefined = | ||
| await chrome.runtime.sendMessage(message); | ||
|
|
||
| setButtonsDisabled(false); | ||
|
|
||
| if (!response) { | ||
| setStatus('error', 'No response from background script'); | ||
| resultEl.style.display = 'none'; | ||
| return; | ||
| } | ||
|
|
||
| if ('error' in response) { | ||
| setStatus('error', response.error); | ||
| resultEl.style.display = 'none'; | ||
| return; | ||
| } | ||
|
|
||
| setStatus('success', `Saved "${response.name}"`); | ||
| resultPath.textContent = `Downloads/skillkit-skills/${response.filename}`; | ||
| resultEl.style.display = 'block'; | ||
| } |
There was a problem hiding this comment.
Unhandled rejection if chrome.runtime.sendMessage throws.
If the background service worker fails to respond (e.g., extension update mid-operation), sendMessage can reject. Wrap in try/catch to avoid an unhandled promise rejection crashing the popup.
Proposed fix
async function save(message: ExtensionMessage) {
setStatus('saving', 'Saving...');
setButtonsDisabled(true);
- const response: SaveResponse | ErrorResponse | undefined =
- await chrome.runtime.sendMessage(message);
+ let response: SaveResponse | ErrorResponse | undefined;
+ try {
+ response = await chrome.runtime.sendMessage(message);
+ } catch {
+ setButtonsDisabled(false);
+ setStatus('error', 'Extension communication failed');
+ resultEl.style.display = 'none';
+ return;
+ }
setButtonsDisabled(false);🤖 Prompt for AI Agents
In `@packages/extension/src/popup.ts` around lines 77 - 101, The save function
currently awaits chrome.runtime.sendMessage which can reject and cause an
unhandled promise; wrap the call in a try/catch inside save to catch any thrown
errors, ensure setButtonsDisabled(false) is executed in both success and failure
(use finally or restore state in catch), and in the catch call
setStatus('error', errorMessage) and hide resultEl (same behavior as when
response is missing or contains error) so the popup doesn't crash; update
handling of the local response variable accordingly to keep the rest of save
(setting resultPath, setStatus('success')) unchanged when a valid response is
returned.
| function detectTags(url: string, content: string): string[] { | ||
| const text = `${url} ${content}`.toLowerCase(); | ||
| const found: string[] = []; | ||
| for (const kw of TECH_KEYWORDS) { | ||
| if (text.includes(kw)) found.push(kw); | ||
| if (found.length >= 10) break; | ||
| } | ||
| return found; | ||
| } |
There was a problem hiding this comment.
🟡 Extension detectTags uses substring matching, producing pervasive false-positive tags
The extension's detectTags function uses text.includes(kw) for substring matching instead of word-boundary matching. Short keywords like go, ai, ml, cd, ci, rest, css, sql, git, and rag will match common English words, causing nearly every page to be tagged incorrectly.
Root Cause and Impact
At packages/extension/src/background.ts:148:
if (text.includes(kw)) found.push(kw);This matches substrings rather than whole words. For example:
"go"matches "google", "algorithm", "category""ai"matches "email", "maintain", "container""cd"matches "incdental" or any URL with "cd" in the path"ci"matches "recipe", "special""rest"matches "restaurant", "forest"
The core library's AutoTagger (packages/core/src/save/tagger.ts:87) correctly avoids this by using word-boundary regex: new RegExp(\\b${keyword}\b`, 'i')`. The extension should use the same approach.
Impact: The first 10 matching keywords fill the tag list with false positives on virtually any page, making the tag metadata unreliable and obscuring the genuinely relevant tags.
| function detectTags(url: string, content: string): string[] { | |
| const text = `${url} ${content}`.toLowerCase(); | |
| const found: string[] = []; | |
| for (const kw of TECH_KEYWORDS) { | |
| if (text.includes(kw)) found.push(kw); | |
| if (found.length >= 10) break; | |
| } | |
| return found; | |
| } | |
| function detectTags(url: string, content: string): string[] { | |
| const text = `${url} ${content}`.toLowerCase(); | |
| const found: string[] = []; | |
| for (const kw of TECH_KEYWORDS) { | |
| const re = new RegExp(`\\b${kw}\\b`); | |
| if (re.test(text)) found.push(kw); | |
| if (found.length >= 10) break; | |
| } | |
| return found; | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
rohitg00
force-pushed
the
feat/chrome-extension-save-as-skill
branch
from
1006a04 to
fc6783b
Compare
Fully browser-based — no local server required. Content script extracts
page content as markdown (Turndown), background generates SKILL.md with
frontmatter and auto-tags, and downloads via chrome.downloads API.
- Manifest V3 with contextMenus, activeTab, downloads permissions
- Content script: HTML-to-markdown via Turndown, extracts article/main
- Background: SKILL.md generation (slugify, YAML frontmatter, tag detection)
- Downloads to skillkit-skills/{name}/SKILL.md in browser Downloads folder
- Monochromatic popup UI matching website design system
- Context menu: "Save page as Skill" / "Save selection as Skill"
- POST /save API endpoint still available for CLI users
rohitg00
force-pushed
the
feat/chrome-extension-save-as-skill
branch
from
fc6783b to
34b8e65
Compare
1 participant
Summary
Fully browser-based Chrome extension — no local server required. Click the extension or right-click any page to save it as a SKILL.md.
Downloads/skillkit-skills/{name}/SKILL.md— copy to~/.skillkit/skills/to make available to all 44 agentsskillkit saveusers (with SSRF protection)Architecture
No localhost API needed. Everything happens in the browser.
Test plan
pnpm build— All 13 packages compile (extension bundles Turndown into 23KB IIFE)pnpm test— All 25 test tasks passpackages/extension/dist/as unpacked extension in Chromechrome://pages → graceful fallback (no content script)Summary by CodeRabbit
Release Notes