fix: prevent markdown link javascript URL bypass in description rendering - #271
fix: prevent markdown link javascript URL bypass in description rendering#271ikuradon wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
- Coverage 80.16% 80.16% -0.01%
==========================================
Files 235 235
Lines 12617 12622 +5
Branches 3476 3478 +2
==========================================
+ Hits 10115 10118 +3
- Misses 1221 1222 +1
- Partials 1281 1282 +1
|
Bundle ReportChanges will increase total bundle size by 4.53MB (9.85%) ⬆️
|
Greptile SummaryXSS リグレッションを修正する PR です。
Confidence Score: 4/5セキュリティ修正として有効で、XSS 遮断ロジックは堅牢です。相対 URL の挙動変更は意図的と思われますが、コメントとテストで明示されていないため確認が必要です。 アローリスト方式と new URL() 正規化の組み合わせは制御文字による難読化を確実に遮断しており、コアのセキュリティ修正は正しく実装されています。一方、相対 URL が従来は href として出力されていたのに対し、新コードではプレーンテキストになるという動作変更が文書化もテストもされていません。 src/shared/utils/html.ts の isSafeUrl 周辺（相対 URL の挙動変更の意図確認）と src/shared/utils/html.test.ts（相対 URL の新挙動を検証するテストの追加）に注目してください。 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["isSafeUrl(url)"] --> B{"url.trim() が空?"}
B -- Yes --> FAIL["return false"]
B -- No --> C{"スキーム正規表現\n一致?"}
C -- No --> FAIL
C -- Yes --> D["new URL(trimmed)"]
D -- "例外" --> FAIL
D -- "成功" --> E{"protocol が\nhttp: か https:?"}
E -- No --> FAIL
E -- Yes --> PASS["return true"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('blocks data: URLs', () => { | ||
| const result = renderMarkdown('[xss](data:text/html;base64,PHNjcmlwdD4=)'); | ||
| expect(result).not.toContain('href'); | ||
| }); | ||
|
|
||
| it('escapes HTML in link text', () => { |
There was a problem hiding this comment.
| it('blocks data: URLs', () => { | |
| const result = renderMarkdown('[xss](data:text/html;base64,PHNjcmlwdD4=)'); | |
| expect(result).not.toContain('href'); | |
| }); | |
| it('escapes HTML in link text', () => { | |
| it('blocks data: URLs', () => { | |
| const result = renderMarkdown('[xss](data:text/html;base64,PHNjcmlwdD4=)'); | |
| expect(result).not.toContain('href'); | |
| }); | |
| it('renders relative URLs as plain text (no href)', () => { | |
| expect(renderMarkdown('[link](/path)')).not.toContain('href'); | |
| expect(renderMarkdown('[anchor](#section)')).not.toContain('href'); | |
| expect(renderMarkdown('[rel](./foo)')).not.toContain('href'); | |
| }); | |
| it('escapes HTML in link text', () => { |
1 participant
Motivation
hrefwas interpreted asjavascript:after browser URL canonicalization (e.g.java\tscript:with embedded control characters).Description
isSafeUrlinsrc/shared/utils/html.tsto require an explicit scheme, canonicalize the URL withnew URL(...), and allow onlyhttp:andhttps:protocols.renderMarkdownbehavior (it still escapes raw HTML and constructs anchors), but now anchors are created only whenisSafeUrlreturns true.src/shared/utils/html.test.tsto cover obfuscatedjavascript:payloads anddata:URLs to prevent reintroducing the bypass.Testing
src/shared/utils/html.test.tsthat assertrenderMarkdowndoes not emithrefforjavascript:(including control-character-obfuscated variants) anddata:URLs.pnpm vitest src/shared/utils/html.test.ts, but the environment could not execute the suite because the repository requires Node>=24.0.0while the runtime provided Nodev20.20.2, so tests were not executed here.Codex Task