From 9eae936c583b771f4abf1e0eb08d35b5ee6f35c1 Mon Sep 17 00:00:00 2001 From: highperfocused Date: Sun, 6 Sep 2026 18:05:11 +0200 Subject: [PATCH] fix: address review feedback on web bookmarks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: - bookmarkDTag() now matches the https scheme case-insensitively, so "HTTPS://…" and "https://…" collapse to the same d tag instead of creating duplicate bookmarks. - The form now accepts every scheme sanitizeUrl() allows (https, http, mailto, nostr) via a dedicated isBookmarkableUrl() check — not sanitizeUrl() itself, which resolves relative URLs against this app's own origin and would have "validated" a bare hostname like "example.com" as a link back into the app. - WebBookmarkRow no longer falls back to the raw unsanitized URL when sanitizeUrl() rejects it (e.g. a malicious "d" tag) — it renders plain text with no link instead of defeating the sanitization. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BYiUtZMQeA5RHggQw73wto --- src/apps/web-bookmarks/index.tsx | 47 +++++++++++++++++++++++-------- src/hooks/useWebBookmarks.test.ts | 6 ++++ src/hooks/useWebBookmarks.ts | 6 +++- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/apps/web-bookmarks/index.tsx b/src/apps/web-bookmarks/index.tsx index 6e9fcd2..96239ca 100644 --- a/src/apps/web-bookmarks/index.tsx +++ b/src/apps/web-bookmarks/index.tsx @@ -20,6 +20,21 @@ import { import { relativeTime, sanitizeUrl, tagValue } from '@/lib/nostrUtils'; import type { AppProps } from '@/os/types'; +/** + * Same protocol allowlist as sanitizeUrl(), but for an absolute bookmark URL + * rather than an href/src that may legitimately be relative to this app's + * own origin — sanitizeUrl() would resolve a bare "example.com" against + * `window.location.origin` and "validate" it as a link back into this app. + */ +const BOOKMARKABLE_SCHEMES = new Set(['https:', 'http:', 'mailto:', 'nostr:']); +function isBookmarkableUrl(value: string): boolean { + try { + return BOOKMARKABLE_SCHEMES.has(new URL(value).protocol); + } catch { + return false; + } +} + export default function WebBookmarksApp({ setTitle }: AppProps) { const { user } = useCurrentUser(); const [formOpen, setFormOpen] = useState(false); @@ -85,7 +100,7 @@ function NewBookmarkForm({ onDone }: { onDone: () => void }) { const { toast } = useToast(); const trimmedUrl = url.trim(); - const isValid = /^https?:\/\/.+/i.test(trimmedUrl); + const isValid = isBookmarkableUrl(trimmedUrl); const submit = async () => { if (!isValid) return; @@ -147,7 +162,11 @@ function WebBookmarkRow({ event }: { event: NostrEvent }) { const { toast } = useToast(); const dTag = tagValue(event, 'd') ?? ''; - const url = sanitizeUrl(bookmarkUrl(dTag)) ?? bookmarkUrl(dTag); + // sanitizeUrl() returning undefined means the reconstructed URL uses a + // protocol that could execute script (e.g. a malicious "d" tag) — in that + // case there is no safe href to link out to, full stop, not a fallback to + // the very value that just failed sanitization. + const url = sanitizeUrl(bookmarkUrl(dTag)); const title = tagValue(event, 'title') ?? dTag; const topics = webBookmarkTopics(event); @@ -167,15 +186,21 @@ function WebBookmarkRow({ event }: { event: NostrEvent }) { return (
- - {title} - - + {url ? ( + + {title} + + + ) : ( + + {title} + + )} {relativeTime(event.created_at)}
diff --git a/src/hooks/useWebBookmarks.test.ts b/src/hooks/useWebBookmarks.test.ts index cb08ace..a627250 100644 --- a/src/hooks/useWebBookmarks.test.ts +++ b/src/hooks/useWebBookmarks.test.ts @@ -20,4 +20,10 @@ describe('bookmarkDTag / bookmarkUrl', () => { const url = 'http://alice.blog/post'; expect(bookmarkUrl(bookmarkDTag(url))).toBe(url); }); + + it('strips the https scheme case-insensitively, so casing does not create duplicate bookmarks', () => { + expect(bookmarkDTag('HTTPS://alice.blog/post')).toBe('alice.blog/post'); + expect(bookmarkDTag('HtTpS://alice.blog/post')).toBe('alice.blog/post'); + expect(bookmarkDTag('HTTPS://alice.blog/post')).toBe(bookmarkDTag('https://alice.blog/post')); + }); }); diff --git a/src/hooks/useWebBookmarks.ts b/src/hooks/useWebBookmarks.ts index 9c0b4c4..08a5009 100644 --- a/src/hooks/useWebBookmarks.ts +++ b/src/hooks/useWebBookmarks.ts @@ -12,12 +12,16 @@ function queryKey(pubkey: string | undefined) { return ['nostr', 'web-bookmarks', pubkey ?? ''] as const; } +const HTTPS_SCHEME_RE = /^https:\/\//i; + /** * The `d` tag per NIP-B0: the URI with the `https://` scheme stripped (every * other scheme keeps its full form, so it round-trips through `bookmarkUrl`). + * The scheme match is case-insensitive so "HTTPS://" and "https://" collapse + * to the same `d` tag instead of creating duplicate bookmarks. */ export function bookmarkDTag(url: string): string { - return url.startsWith('https://') ? url.slice('https://'.length) : url; + return HTTPS_SCHEME_RE.test(url) ? url.slice('https://'.length) : url; } /** Reconstructs a clickable URL from a `d` tag written by `bookmarkDTag`. */