fix: address review feedback on web bookmarks

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYiUtZMQeA5RHggQw73wto
This commit is contained in:
2026-09-06 18:05:11 +02:00
parent 14d4588a18
commit 9eae936c58
3 changed files with 47 additions and 12 deletions

View File

@@ -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 (
<article className="group border-b border-border px-4 py-3 transition-colors last:border-b-0 hover:bg-muted/40">
<div className="flex items-start justify-between gap-3">
<a
href={url}
target="_blank"
rel="noopener noreferrer"
className="flex min-w-0 items-center gap-1.5 text-[14px] font-medium hover:underline"
>
<span className="truncate">{title}</span>
<ExternalLink className="size-3.5 shrink-0 text-muted-foreground" aria-hidden />
</a>
{url ? (
<a
href={url}
target="_blank"
rel="noopener noreferrer"
className="flex min-w-0 items-center gap-1.5 text-[14px] font-medium hover:underline"
>
<span className="truncate">{title}</span>
<ExternalLink className="size-3.5 shrink-0 text-muted-foreground" aria-hidden />
</a>
) : (
<span className="truncate text-[14px] font-medium text-muted-foreground" title="Not a safe URL to open">
{title}
</span>
)}
<span className="shrink-0 text-xs text-muted-foreground">{relativeTime(event.created_at)}</span>
</div>

View File

@@ -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'));
});
});

View File

@@ -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`. */