From 3c30300b754efeabc0589715e7a61c38a7050472 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Thu, 9 Jul 2026 01:35:04 +0700 Subject: [PATCH] fix(web): sanitizeHref hardening + API path-segment encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security findings from cora scan (web/): - utils.sanitizeHref: reject backslash (browser normalizes `\`→`/`, so `/\\evil.com` became protocol-relative `//evil.com` off-site redirect) and strip ASCII tab/newline/CR before checking (browser ignores them, allowing scheme smuggling like `java\tscript:`) - api/client: encodeURIComponent every path segment (slug/id/postId/ provider) to prevent path traversal via `..` reaching unintended routes (e.g. `/api/projects/../auth/me`) - board/[slug]: move `initialized` declaration above onMount (TDZ smell — used at line 84, declared at 179); simplify 'v' vote shortcut to a single auth guard - tests: sanitizeHref backslash + tab-smuggling cases --- web/src/lib/api/client.ts | 44 +++++++++++++----------- web/src/lib/utils.test.ts | 16 +++++++++ web/src/lib/utils.ts | 13 ++++++- web/src/routes/board/[slug]/+page.svelte | 11 +++--- 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 7c0ebad..51623b6 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -20,6 +20,10 @@ import type { const BASE = ''; +/** Encode a single path segment so a caller-supplied slug/id can never inject + * `..` or `/` and traverse to a different route (e.g. `/api/projects/../auth/me`). */ +const seg = (s: string) => encodeURIComponent(s); + class ApiError extends Error { constructor( public status: number, @@ -66,7 +70,7 @@ export const api = { getCurrentUser: () => request>('/auth/me').then((r) => r.data), login: (provider: string) => { - window.location.href = `${BASE}/auth/${provider}/login`; + window.location.href = `${BASE}/auth/${seg(provider)}/login`; }, logout: () => @@ -76,7 +80,7 @@ export const api = { listProjects: () => request<{ data: Project[] }>('/api/projects').then((r) => r.data), getProject: (slug: string) => - request>(`/api/projects/${slug}`).then((r) => r.data), + request>(`/api/projects/${seg(slug)}`).then((r) => r.data), createProject: (body: { name: string; slug?: string; description?: string }) => request>('/api/projects', { @@ -85,13 +89,13 @@ export const api = { }).then((r) => r.data), updateProject: (slug: string, body: { name?: string; description?: string }) => - request>(`/api/projects/${slug}`, { + request>(`/api/projects/${seg(slug)}`, { method: 'PATCH', body: JSON.stringify(body), }).then((r) => r.data), deleteProject: (slug: string) => - request(`/api/projects/${slug}`, { method: 'DELETE' }), + request(`/api/projects/${seg(slug)}`, { method: 'DELETE' }), // Posts listPosts: (slug: string, params?: { @@ -111,14 +115,14 @@ export const api = { if (params?.per_page !== undefined) query.set('per_page', String(params.per_page)); const qs = query.toString(); return request>( - `/api/projects/${slug}/posts${qs ? `?${qs}` : ''}`, + `/api/projects/${seg(slug)}/posts${qs ? `?${qs}` : ''}`, ); }, /** Fetch the public roadmap (posts grouped by lifecycle status). Public endpoint. */ getRoadmap: (slug: string, limit?: number) => { const qs = limit !== undefined ? `?limit=${limit}` : ''; - return request>(`/api/projects/${slug}/roadmap${qs}`).then((r) => r.data); + return request>(`/api/projects/${seg(slug)}/roadmap${qs}`).then((r) => r.data); }, /** Fetch the changelog (done posts, newest ship first). Public, paginated. */ @@ -128,62 +132,62 @@ export const api = { if (params?.per_page !== undefined) query.set('per_page', String(params.per_page)); if (params?.since) query.set('since', params.since); const qs = query.toString(); - return request>(`/api/projects/${slug}/changelog${qs ? `?${qs}` : ''}`); + return request>(`/api/projects/${seg(slug)}/changelog${qs ? `?${qs}` : ''}`); }, getPost: (id: string) => - request>(`/api/posts/${id}`).then((r) => r.data), + request>(`/api/posts/${seg(id)}`).then((r) => r.data), createPost: (slug: string, body: { title: string; description?: string; category?: PostCategory }) => - request>(`/api/projects/${slug}/posts`, { + request>(`/api/projects/${seg(slug)}/posts`, { method: 'POST', body: JSON.stringify(body), }).then((r) => r.data), updatePostStatus: (id: string, status: PostStatus) => - request>(`/api/posts/${id}`, { + request>(`/api/posts/${seg(id)}`, { method: 'PATCH', body: JSON.stringify({ status }), }).then((r) => r.data), updatePostCategory: (id: string, category: PostCategory) => - request>(`/api/posts/${id}`, { + request>(`/api/posts/${seg(id)}`, { method: 'PATCH', body: JSON.stringify({ category }), }).then((r) => r.data), - deletePost: (id: string) => request(`/api/posts/${id}`, { method: 'DELETE' }), + deletePost: (id: string) => request(`/api/posts/${seg(id)}`, { method: 'DELETE' }), // Votes toggleVote: (id: string) => - request>(`/api/posts/${id}/vote`, { + request>(`/api/posts/${seg(id)}/vote`, { method: 'POST', }).then((r) => r.data), checkVoted: (id: string) => - request>(`/api/posts/${id}/vote`).then((r) => r.data), + request>(`/api/posts/${seg(id)}/vote`).then((r) => r.data), // Comments listComments: (postId: string) => - request<{ data: Comment[] }>(`/api/posts/${postId}/comments`).then((r) => r.data), + request<{ data: Comment[] }>(`/api/posts/${seg(postId)}/comments`).then((r) => r.data), createComment: (postId: string, body: { content: string; parent_id?: string }) => - request>(`/api/posts/${postId}/comments`, { + request>(`/api/posts/${seg(postId)}/comments`, { method: 'POST', body: JSON.stringify(body), }).then((r) => r.data), deleteComment: (id: string) => - request(`/api/comments/${id}`, { method: 'DELETE' }), + request(`/api/comments/${seg(id)}`, { method: 'DELETE' }), // Attachments listAttachments: (postId: string) => - request(`/api/posts/${postId}/attachments`).then((r) => r.data), + request(`/api/posts/${seg(postId)}/attachments`).then((r) => r.data), uploadAttachment: async (postId: string, file: File) => { const formData = new FormData(); formData.append('file', file); - const res = await fetch(`/api/posts/${postId}/attachments`, { + const res = await fetch(`/api/posts/${seg(postId)}/attachments`, { method: 'POST', body: formData, credentials: 'include', @@ -196,7 +200,7 @@ export const api = { }, deleteAttachment: (id: string) => - request(`/api/attachments/${id}`, { method: 'DELETE' }), + request(`/api/attachments/${seg(id)}`, { method: 'DELETE' }), }; export { ApiError }; diff --git a/web/src/lib/utils.test.ts b/web/src/lib/utils.test.ts index 7499423..a99042a 100644 --- a/web/src/lib/utils.test.ts +++ b/web/src/lib/utils.test.ts @@ -102,4 +102,20 @@ describe('sanitizeHref', () => { it('rejects non-URL garbage', () => { expect(sanitizeHref('not a url at all')).toBeUndefined(); }); + + it('rejects backslash-based open-redirect bypass', () => { + // Browsers normalize `\` to `/`, so `/\\evil.com` would resolve to the + // protocol-relative `//evil.com` and navigate off-site. + expect(sanitizeHref('/\\evil.com')).toBeUndefined(); + expect(sanitizeHref('\\\\evil.com')).toBeUndefined(); + expect(sanitizeHref('/foo\\bar')).toBeUndefined(); + }); + + it('strips ASCII tab/newline/CR before checking (smuggling bypass)', () => { + // Browsers ignore embedded tab/newline/CR in URLs; without stripping, + // `java\tscript:` could slip a dangerous scheme past the check. + expect(sanitizeHref('java\tscript:alert(1)')).toBeUndefined(); + expect(sanitizeHref('java\nscript:alert(1)')).toBeUndefined(); + expect(sanitizeHref(' \t https://ok.com')).toBe('https://ok.com'); + }); }); diff --git a/web/src/lib/utils.ts b/web/src/lib/utils.ts index 22adabb..14ec720 100644 --- a/web/src/lib/utils.ts +++ b/web/src/lib/utils.ts @@ -82,9 +82,20 @@ const SAFE_URL_PROTOCOLS = new Set(['http:', 'https:', 'mailto:', 'tel:']); export function sanitizeHref(href: string | undefined | null): string | undefined { if (href == null || href === '') return undefined; - const trimmed = href.trim(); + // Browsers strip ASCII tab/newline/CR from URLs *before* parsing, so an + // attacker can smuggle a scheme past naive string checks (e.g. + // `java\tscript:`). Remove them up front so our check sees what the + // browser will actually execute. + const stripped = href.replace(/[\t\n\r]/g, ''); + const trimmed = stripped.trim(); if (trimmed === '') return undefined; + // Browsers normalize backslash to forward slash in URL contexts. That means + // a value that looks relative — `/\\evil.com` — becomes the + // protocol-relative `//evil.com` and navigates off-site. Reject any + // backslash outright; no legitimate in-app href needs one. + if (trimmed.includes('\\')) return undefined; + // Relative URLs are always safe (path, query, hash). // Note: protocol-relative `//host` is NOT relative — it inherits the // document protocol — so we reject it explicitly. diff --git a/web/src/routes/board/[slug]/+page.svelte b/web/src/routes/board/[slug]/+page.svelte index 0cd65b6..1050b62 100644 --- a/web/src/routes/board/[slug]/+page.svelte +++ b/web/src/routes/board/[slug]/+page.svelte @@ -26,6 +26,10 @@ let searchQuery = $state(''); let showForm = $state(false); let authed = $state(false); + // Set true once the initial onMount load finishes; gates the filter $effect + // so it doesn't fire before the first board fetch completes. Declared up + // here (before onMount) to avoid a forward reference / TDZ smell. + let initialized = $state(false); // Keyboard-shortcut focus state. Tracks the currently-focused post card // so j/k navigation and Enter/v shortcuts have a target. -1 = none focused. @@ -128,9 +132,9 @@ break; } case 'v': { - if (!authed || authRequired === false) { - if (!authed) return; - } + // Voting always requires auth. `authRequired` is irrelevant + // here (there's no anonymous vote path), so just guard on auth. + if (!authed) return; const post = posts[focusedPostIndex]; if (post) toggleVote(post); break; @@ -176,7 +180,6 @@ // `loadBoard()` is safe to call before `project` is set; it sets `project` // on first resolve. We also guard against running before mount completes // via the `initialized` flag set at the end of onMount. - let initialized = $state(false); // Reload board when filters or slug change. // Search is debounced to avoid excessive API calls while typing. let searchTimer: ReturnType | null = null;