diff options
| author | Adam Malczewski <[email protected]> | 2026-06-01 09:50:30 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-06-01 09:50:30 +0900 |
| commit | 1870d0b86e797077b2a86c39d93fd34004b39e40 (patch) | |
| tree | e1da8ed73fbec1483a5f5a2e263fbdae6a9f5f9f /packages | |
| parent | 29bdd00f946d75671bea7c4b534d32197e5b4b55 (diff) | |
| download | dispatch-1870d0b86e797077b2a86c39d93fd34004b39e40.tar.gz dispatch-1870d0b86e797077b2a86c39d93fd34004b39e40.zip | |
fix(notifications): address Gemini review — tighten validation, sanitize Click, support Basic auth, non-optimistic UI clear
Acted on 4 of 6 findings from the gemini-3-flash-preview second-opinion
review (the other 2 were verified-wrong or judged not worth the
complexity — see HANDOFF.md).
core/src/notifications/ntfy.ts:
- validateTopicUrl now enforces ntfy's actual topic-name constraints:
exactly one path segment, 1–64 chars, charset [A-Za-z0-9_-]. Prevents
users from saving topic URLs that look fine but silently 404 at
publish time (cf. binwiederhier/ntfy#1451 for the 64-char limit and
binwiederhier/ntfy's topic-name regex for the charset).
- Click header now passes through sanitizeHeader, closing the same
CRLF-injection vector that Title/Tags already had.
- Authorization header construction now factors through a small
buildAuthHeaderValue helper: a value that already starts with a scheme
token ("Bearer xyz", "Basic dXNlcjpwYXNz") is used verbatim, so users
of private ntfy servers that want Basic auth can paste the full header
value. Bare tokens still get the "Bearer " prefix automatically.
frontend/SettingsPanel.svelte:
- clearNtfyAuthToken() was optimistic: it flipped hasAuthToken=false
locally before awaiting the network call. If the request failed the
UI lied about server state, and worse — a subsequent Save() with
authToken:undefined would silently re-arm the original token. Now
awaits the response, surfaces failures via the existing ntfySaveError
banner, and only mutates local state on success. Adds a
ntfyClearingToken loading flag so the button disables + spins during
the request.
Tests: +6 in ntfy.test.ts (multi-segment rejection, charset rejection,
length boundary, 64-char acceptance, Basic auth pass-through, Click
sanitization). All 442 tests pass; biome clean; svelte-check clean;
manual ntfy.sh end-to-end re-verified.
Diffstat (limited to 'packages')
| -rw-r--r-- | packages/core/src/notifications/ntfy.ts | 56 | ||||
| -rw-r--r-- | packages/core/tests/notifications/ntfy.test.ts | 45 | ||||
| -rw-r--r-- | packages/frontend/src/lib/components/SettingsPanel.svelte | 57 |
3 files changed, 130 insertions, 28 deletions
diff --git a/packages/core/src/notifications/ntfy.ts b/packages/core/src/notifications/ntfy.ts index 07ce33b..1f5101c 100644 --- a/packages/core/src/notifications/ntfy.ts +++ b/packages/core/src/notifications/ntfy.ts @@ -28,9 +28,17 @@ export type FetchLike = ( ) => Promise<{ ok: boolean; status: number; statusText?: string; text(): Promise<string> }>; /** - * Validate a ntfy topic URL. Accepts only `http(s)://host/topic` with a - * non-empty topic path. Returns `null` on success, a human-readable error - * string on failure. + * ntfy topic-name rules: 1–64 chars, ASCII alphanumerics + `-` and `_`. Sourced + * from the ntfy server (cf. binwiederhier/ntfy issue #1451 — longer names + * silently 404). Matching this client-side keeps users from saving topic URLs + * that look fine but only fail at publish time. + */ +const NTFY_TOPIC_RE = /^[A-Za-z0-9_-]{1,64}$/; + +/** + * Validate a ntfy topic URL. Accepts only `http(s)://host/topic` where + * `topic` is a single path segment of 1–64 chars matching `[A-Za-z0-9_-]`. + * Returns `null` on success, a human-readable error string on failure. */ export function validateTopicUrl(topicUrl: string): string | null { const trimmed = topicUrl.trim(); @@ -44,9 +52,15 @@ export function validateTopicUrl(topicUrl: string): string | null { if (url.protocol !== "http:" && url.protocol !== "https:") { return "Topic URL must use http:// or https://"; } - // Path must be a non-empty topic (more than just "/") + // Path must be exactly one topic segment. const topic = url.pathname.replace(/^\/+|\/+$/g, ""); if (!topic) return "Topic URL must include a topic name (e.g. https://ntfy.sh/my-topic)"; + if (topic.includes("/")) { + return "Topic URL must point at a single topic (no extra path segments)"; + } + if (!NTFY_TOPIC_RE.test(topic)) { + return "Topic name must be 1–64 characters, letters/numbers/underscore/hyphen only"; + } return null; } @@ -79,17 +93,17 @@ export async function sendNtfy( } const headers: Record<string, string> = { - // ntfy treats the title/priority/tags/click headers as ASCII-only. Strip - // control chars from the title; the body is sent UTF-8 verbatim. + // ntfy is tolerant of non-ASCII in the Title header but many proxies + // aren't — sanitizeHeader strips CR/LF/control chars (injection guard) + // and leaves UTF-8 in place. Body is sent verbatim as UTF-8. Title: sanitizeHeader(event.title), Priority: String(priority), "Content-Type": "text/plain; charset=utf-8", }; if (tags.length > 0) headers.Tags = tags.map(sanitizeHeader).join(","); - if (event.clickUrl) headers.Click = event.clickUrl; - if (config.authToken && config.authToken.trim() !== "") { - headers.Authorization = `Bearer ${config.authToken.trim()}`; - } + if (event.clickUrl) headers.Click = sanitizeHeader(event.clickUrl); + const authValue = buildAuthHeaderValue(config.authToken); + if (authValue) headers.Authorization = authValue; // Per-request abort so a hung server doesn't pin a Bun worker forever. const controller = new AbortController(); @@ -119,9 +133,27 @@ export async function sendNtfy( } } +/** + * Build the `Authorization` header value from the user-configured token. + * + * - Empty/blank ⇒ no header. + * - Already starts with `Bearer ` / `Basic ` / etc. (RFC 7235 scheme + space) + * ⇒ used verbatim. Lets users paste a complete header for Basic auth or + * any other scheme their private ntfy server supports. + * - Otherwise ⇒ prefixed with `Bearer ` (the common case). + */ +function buildAuthHeaderValue(rawToken: string): string | null { + const trimmed = (rawToken ?? "").trim(); + if (!trimmed) return null; + if (/^[A-Za-z][A-Za-z0-9._~+/-]*\s+\S/.test(trimmed)) { + // Already includes a scheme token (e.g. "Bearer xyz", "Basic dXNlcjpw"). + return sanitizeHeader(trimmed); + } + return `Bearer ${sanitizeHeader(trimmed)}`; +} + function sanitizeHeader(value: string): string { - // Strip CR/LF (header injection guard) and trim. ntfy is tolerant of - // non-ASCII in titles, but we still drop control chars. + // Strip CR/LF (header injection guard) and other control chars, then trim. // biome-ignore lint/suspicious/noControlCharactersInRegex: intentional return value.replace(/[\r\n\u0000-\u001f]+/g, " ").trim(); } diff --git a/packages/core/tests/notifications/ntfy.test.ts b/packages/core/tests/notifications/ntfy.test.ts index 3fb1d51..c183847 100644 --- a/packages/core/tests/notifications/ntfy.test.ts +++ b/packages/core/tests/notifications/ntfy.test.ts @@ -61,6 +61,27 @@ describe("validateTopicUrl", () => { expect(validateTopicUrl("https://ntfy.sh")).toMatch(/topic/); expect(validateTopicUrl("https://ntfy.sh/")).toMatch(/topic/); }); + + it("rejects multi-segment paths (topic must be a single segment)", () => { + expect(validateTopicUrl("https://ntfy.sh/team/sub")).toMatch(/single topic/); + }); + + it("rejects topic names with disallowed characters", () => { + expect(validateTopicUrl("https://ntfy.sh/has.dot")).toMatch(/letters\/numbers/); + expect(validateTopicUrl("https://ntfy.sh/has space")).toMatch(/letters\/numbers/); + expect(validateTopicUrl("https://ntfy.sh/has%20enc")).toMatch(/letters\/numbers/); + }); + + it("rejects topic names longer than 64 chars", () => { + const long = "a".repeat(65); + expect(validateTopicUrl(`https://ntfy.sh/${long}`)).toMatch(/64/); + }); + + it("accepts 64-char topic names and underscore/hyphen mixes", () => { + const maxlen = "a".repeat(64); + expect(validateTopicUrl(`https://ntfy.sh/${maxlen}`)).toBeNull(); + expect(validateTopicUrl("https://ntfy.sh/My_Topic-123")).toBeNull(); + }); }); describe("sendNtfy", () => { @@ -91,13 +112,23 @@ describe("sendNtfy", () => { expect(init.headers.Tags).toBe("rotating_light"); }); - it("attaches Authorization header when authToken is set", async () => { + it("attaches Authorization header with Bearer prefix when authToken is a bare token", async () => { const fetchImpl = makeFetch(); await sendNtfy(makeConfig({ authToken: "tk_secret " }), makeEvent(), fetchImpl); const init = fetchImpl.mock.calls[0][1]; expect(init.headers.Authorization).toBe("Bearer tk_secret"); }); + it("passes a pre-prefixed Authorization value (Basic, custom schemes) through verbatim", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig({ authToken: "Basic dXNlcjpwYXNz" }), makeEvent(), fetchImpl); + expect(fetchImpl.mock.calls[0][1].headers.Authorization).toBe("Basic dXNlcjpwYXNz"); + + const fetchImpl2 = makeFetch(); + await sendNtfy(makeConfig({ authToken: "Bearer already_prefixed" }), makeEvent(), fetchImpl2); + expect(fetchImpl2.mock.calls[0][1].headers.Authorization).toBe("Bearer already_prefixed"); + }); + it("omits Authorization when authToken is blank", async () => { const fetchImpl = makeFetch(); await sendNtfy(makeConfig({ authToken: " " }), makeEvent(), fetchImpl); @@ -112,6 +143,18 @@ describe("sendNtfy", () => { expect(init.headers.Click).toBe("https://example.com/tab/abc"); }); + it("sanitizes Click header (CR/LF injection guard)", async () => { + const fetchImpl = makeFetch(); + await sendNtfy( + makeConfig(), + makeEvent({ clickUrl: "https://example.com/\r\nInjected: yes" }), + fetchImpl, + ); + const v = fetchImpl.mock.calls[0][1].headers.Click; + expect(v).not.toContain("\n"); + expect(v).not.toContain("\r"); + }); + it("appends short tab tag when tabId is set", async () => { const fetchImpl = makeFetch(); await sendNtfy( diff --git a/packages/frontend/src/lib/components/SettingsPanel.svelte b/packages/frontend/src/lib/components/SettingsPanel.svelte index 1d9ebf7..8031500 100644 --- a/packages/frontend/src/lib/components/SettingsPanel.svelte +++ b/packages/frontend/src/lib/components/SettingsPanel.svelte @@ -72,6 +72,7 @@ let ntfySaveOk = $state(false); let ntfyTesting = $state(false); let ntfyTestResult = $state<string | null>(null); let ntfyTestOk = $state(false); +let ntfyClearingToken = $state(false); function onChunkLimitChange(e: Event): void { const input = e.target as HTMLInputElement; @@ -212,22 +213,43 @@ async function sendNtfyTest(): Promise<void> { } } -function clearNtfyAuthToken(): void { +async function clearNtfyAuthToken(): Promise<void> { // `""` ⇒ explicit clear on save (vs. `undefined` which keeps existing). - ntfyAuthTokenInput = ""; - ntfy = { ...ntfy, hasAuthToken: false }; - // Send a save with explicit empty string to clear server-side. - void (async () => { - try { - await fetch(`${apiBase}/notifications`, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ authToken: "" }), - }); - } catch { - // ignore + // Optimistic local state on failure caused a real bug pre-review: UI showed + // the token cleared while the server still held it, then "Save" treated the + // blank input as "keep existing" and silently re-armed the old token. Await + // the response and only flip local state on success. + ntfyClearingToken = true; + ntfySaveError = null; + try { + const res = await fetch(`${apiBase}/notifications`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ authToken: "" }), + }); + const data = (await res.json().catch(() => ({}))) as { + config?: NtfyConfigView; + error?: string; + }; + if (!res.ok) { + ntfySaveError = data.error ?? `Clear failed (HTTP ${res.status})`; + return; } - })(); + ntfyAuthTokenInput = ""; + if (data.config) { + ntfy = { + ...DEFAULT_NTFY, + ...data.config, + events: { ...DEFAULT_NTFY.events, ...(data.config.events ?? {}) }, + }; + } else { + ntfy = { ...ntfy, hasAuthToken: false }; + } + } catch (e) { + ntfySaveError = e instanceof Error ? e.message : "Network error"; + } finally { + ntfyClearingToken = false; + } } async function toggleAutoExpand(): Promise<void> { @@ -421,9 +443,14 @@ $effect(() => { <button type="button" class="btn btn-xs btn-ghost btn-outline self-start" + disabled={ntfyClearingToken} onclick={clearNtfyAuthToken} > - Clear stored token + {#if ntfyClearingToken} + <span class="loading loading-spinner loading-xs"></span> + {:else} + Clear stored token + {/if} </button> {/if} </label> |
