feat(beasties): add remote option to download stylesheets
#223
Conversation
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Beasties
participant getCssAsset
participant RemoteServer
Client->>Beasties: Process HTML with <link href>
Beasties->>getCssAsset: getCssAsset(href, options)
alt href is remote
alt options.remote == true
getCssAsset->>RemoteServer: fetch(normaliseProtocol(href))
alt HTTP 2xx
RemoteServer-->>getCssAsset: CSS text
getCssAsset-->>Beasties: return CSS text (inlined)
else non-2xx or fetch error
getCssAsset->>getCssAsset: log warning
getCssAsset-->>Beasties: return undefined (leave href)
end
else options.remote == false
getCssAsset-->>Beasties: return undefined (leave href)
end
else href is local
getCssAsset-->>Beasties: read/process local CSS
end
Beasties-->>Client: Return processed HTML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ 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 |
![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
🧹 Nitpick comments (1)
packages/beasties/test/beasties.test.ts (1)
408-435: Consider restoring the mocked fetch.The test correctly validates remote stylesheet fetching. However, the
globalThis.fetchmock could potentially affect other tests if not properly isolated.Consider using vitest's cleanup utilities:
it('fetches remote stylesheets when remote: true', async () => { const beasties = new Beasties({ reduceInlineStyles: false, remote: true, }) // Mock fetch + const originalFetch = globalThis.fetch const mockFetch = vi.fn().mockResolvedValue({ ok: true, text: async () => 'h1 { color: blue; } h2.unused { color: red; }', }) globalThis.fetch = mockFetch as any const result = await beasties.process(trim` <html> <head> <link rel="stylesheet" href="https://example.com/style.css"> </head> <body> <h1>Hello World!</h1> </body> </html> `) expect(mockFetch).toHaveBeenCalledWith('https://example.com/style.css') expect(result).toContain('<style>h1{color:blue}</style>') expect(result).toContain('https://example.com/style.css') + + // Restore original fetch + globalThis.fetch = originalFetch })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/beasties/README.md(1 hunks)packages/beasties/src/index.d.ts(1 hunks)packages/beasties/src/index.ts(2 hunks)packages/beasties/src/types.ts(1 hunks)packages/beasties/test/beasties.test.ts(1 hunks)
🔇 Additional comments (5)
packages/beasties/src/index.d.ts (1)
53-53: LGTM!The type definition for the
remoteoption is correctly declared and follows the existing pattern for optional boolean properties.packages/beasties/src/types.ts (1)
43-46: LGTM!The documentation is clear and accurately describes the feature, including all supported URL formats and the secure default value.
packages/beasties/test/beasties.test.ts (1)
387-406: LGTM!The test correctly validates that remote stylesheets are ignored by default, ensuring the secure-by-default behavior.
packages/beasties/README.md (1)
123-123: LGTM!The documentation clearly describes the
remoteoption and is consistent with the type definitions.packages/beasties/src/index.ts (1)
274-278: LGTM!The addition of query parameter and fragment stripping before checking the file extension is a solid improvement that allows CSS files with query strings or hashes to be properly processed.
| // Handle remote stylesheets | ||
| const isRemote = /^https?:\/\//.test(href) || href.startsWith('//') | ||
| if (isRemote) { | ||
| if (this.options.remote === true) { | ||
| try { | ||
| // Normalize protocol-relative URLs | ||
| const absoluteUrl = href.startsWith('//') ? `https:${href}` : href | ||
|
|
||
| const response = await fetch(absoluteUrl) | ||
|
|
||
| if (!response.ok) { | ||
| this.logger.warn?.(`Failed to fetch ${absoluteUrl} (${response.status})`) | ||
| return undefined | ||
| } | ||
|
|
||
| return await response.text() | ||
| } | ||
| catch (error) { | ||
| this.logger.warn?.(`Error fetching ${href}: ${(error as Error).message}`) | ||
| return undefined | ||
| } | ||
| } | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
Document the security implications of enabling remote stylesheet fetching.
The implementation correctly handles remote stylesheets with proper error handling. However, when remote: true is enabled with untrusted HTML input, this could allow Server-Side Request Forgery (SSRF) attacks by making requests to internal network resources.
Consider adding a security note to the documentation:
In packages/beasties/README.md, add a security section:
### Security Considerations
**Remote Stylesheet Fetching**
The `remote` option enables fetching stylesheets from external URLs. Only enable this option if:
- The HTML input is from a trusted source
- You have validated the URLs in the HTML
- Your application is not exposed to SSRF attacks via internal network access
For untrusted HTML input, consider implementing URL validation or an allowlist of permitted domains.Alternatively, consider implementing URL validation directly in the code:
// Add validation before fetching
const url = new URL(absoluteUrl)
if (url.hostname === 'localhost' || url.hostname.startsWith('127.') || url.hostname.startsWith('192.168.')) {
this.logger.warn?.(`Blocked internal URL: ${absoluteUrl}`)
return undefined
}🤖 Prompt for AI Agents
In packages/beasties/src/index.ts around lines 184 to 207, the remote stylesheet
fetch path allows SSRF when remote: true; update the code to validate
absoluteUrl before fetching (parse with URL and block or warn+return for
internal hosts/IPs such as localhost, 127.*, 10.*, 172.16-31.*, 192.168.* and
optionally support a configurable allowlist of safe hostnames), and log blocked
URLs; additionally, add a Security Considerations section to
packages/beasties/README.md documenting the risks of enabling remote:true,
recommending only using it for trusted HTML, validating URLs or using an
allowlist, and describing the behavior when a URL is blocked.
danielroe
changed the title
remote option to download remote stylesheets
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 85.78% 86.43% +0.64%
==========================================
Files 8 8
Lines 1280 1297 +17
Branches 331 340 +9
==========================================
+ Hits 1098 1121 +23
+ Misses 182 176 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielroe
changed the title
remote option to download remote stylesheetsremote option to download remote stylesheets
danielroe
changed the title
remote option to download remote stylesheetsremote option to download stylesheets
3 participants
This allows usage in environments where you only have the HTML and none of the assets