Weekly review batch: silent custom-slug failure, unsafe client-side interpolation, Python SDK InvalidURL escape, slug-repository invariant guard, unscannable QR codes, title-fetch SSRF guard

[DennisAlund]

Weekly defect-hunting review of the app, its API, and all three SDKs. Six parallel passes (API/DB/services, admin UI, MCP/auth, TypeScript SDK, Python SDK, Dart SDK), each finding verified against the actual code and a failing-before/passing-after regression test before being fixed.

Fixes

createLink()'s custom-slug attach toasted "Link created" whether it succeeded or failed (src/client.ts)

• Wrong: the "New Link" modal's custom-slug branch had if (!slugRes.ok) { toast(t('client.linkCreated')); } else { toast(t('client.linkCreated')); } — both branches identical.
• Impact: requesting a custom slug that's already taken (or otherwise rejected) failed silently. The user was told "Link created" and redirected to the link's detail page with no sign the slug they asked for never attached.
• Fix: extracted the step into attachCustomSlugAndGo(), which reads the failure body and toasts client.customError (or the server's message), matching doAddSlug's existing handling of the same endpoint.
• Test: drives createLink() against a stubbed api() that succeeds on the link-create call and fails on the slug-attach call; asserts the failure surfaces, the generic fallback fires when the body carries no message, and the success toast still fires when the slug attaches.

Client-side t() used unsafe string-based interpolation (src/client.ts)

• Wrong: val.replace(new RegExp(...), String(params[k])) — the plain-string form of .replace() treats $&, $`, $', $1 etc. in the replacement string as special patterns instead of literal text. The server-side createTranslateFn() in src/i18n/index.ts already guards against exactly this with a replacer function; the client-side reimplementation didn't.
• Impact: any $-containing free text passed as a param corrupts the shown string. Two real call sites: API key titles (client.confirmDeleteKey) and bundle names (client.bundles.confirmArchive/confirmDelete), both unrestricted free text. A title like Prod $$ key renders as Prod $ key.
• Fix: pass a replacer function, mirroring the server-side fix.
• Test: extracts t() from the generated script and asserts literal interpolation, mirroring the existing server-side regression test for the same bug class.

Python SDK: httpx.InvalidURL escaped as a raw exception instead of ShrtnrError (sdk/python/src/shrtnr/resources/{links,bundles,slugs}.py)

• Wrong: every resource's _request/_request_text (8 call sites, sync and async) caught only httpx.RequestError. httpx.InvalidURL (e.g. an unparsable port) is not a subclass of it, and is raised synchronously from request-building, before any I/O.
• Impact: a malformed base_url (bad port, bad IDNA host, etc.) raised a raw httpx.InvalidURL instead of the documented ShrtnrError(status=0, ...), breaking the "network failures wrap as ShrtnrError" contract in errors.py and the README.
• Fix: widened the caught exception tuple to (httpx.RequestError, httpx.InvalidURL) at all 8 sites.
• Test: one regression test per sync/async client, constructing a client with a malformed base_url and asserting ShrtnrError(status=0).
• Single-SDK change: this is an httpx-specific exception-hierarchy quirk (InvalidURL isn't related to RequestError). The TypeScript SDK's fetch-based error handling and the Dart SDK's http client don't have an equivalent split, so there's nothing to port.

SlugRepository.disable() could zero out a link's primary flag (src/db/slug-repository.ts)

• Wrong: a link has exactly one system-generated (is_custom = 0) slug for its whole lifetime. disable()'s primary-fallback query promotes by is_custom = 0, so disabling that one row has the query re-select the very row being demoted next, leaving the link with no primary slug at all.
• Impact: currently unreachable — the only caller (disableSlug in link-management.ts) already refuses to disable a non-custom slug before reaching the repository — but a landmine for any future caller (MCP tool, script, admin action) that calls the repository directly.
• Fix: added the same guard SlugRepository.remove() already has, so the repository enforces its own invariant rather than relying solely on the one caller.
• Test: calls SlugRepository.disable() directly on a link's only slug (bypassing the service-layer guard) and asserts it refuses, leaving the primary flag untouched.

QR encoder produced unscannable codes for versions 6-10 (src/qr.ts)

• Wrong: three stacked defects, each confirmed with a real decoder (jsQR). (1) Versions 6-10 must split codewords into 2 (v6-9) or 4 (v10) Reed-Solomon blocks and interleave them; the encoder read the per-block ECC count as the total and ran one RS pass over the whole payload. (2) Versions 7-10 must carry two 18-bit BCH-protected version-information blocks; the encoder never reserved or wrote them, so data modules landed where a reader looks for the version. (3) From version 7 on, alignment patterns also sit on the timing pattern's row and column; the "skip if reserved" test meant for the three finder overlaps also skipped those.
• Impact: any QR payload over 106 bytes (a short link with a long custom slug plus ?utm_medium=qr gets there) rendered a symbol no reader could decode. Versions 1-5 were unaffected.
• Fix: per-version block-count table with proper block splitting and interleaving, version-information blocks reserved and written for v7+, and an index-based finder-overlap skip for alignment patterns.
• Test: rasterizes each version's output and decodes it with jsQR (new dev dependency), asserts the version-info bits against ISO/IEC 18004 Table D.1, and checks the 271-byte capacity boundary. Before the fix versions 6-10 failed to decode; after it every payload length from 1 to 271 bytes round-trips.

fetchPageTitle fetched any user-supplied URL server-side with no restriction (src/title-fetch.ts)

• Wrong: autoLabelLink() fetched the link URL with redirect: "follow" and no host check, an SSRF pattern. A link at a loopback, RFC 1918, link-local or cloud-metadata address (or a public page redirecting to one) had the Worker fetch it and store the response's <title> where the link's owner can read it.
• Impact: bounded in practice by Cloudflare Workers' own egress isolation, but the code assumed that instead of enforcing it.
• Fix: new isPublicHttpUrl() admits only http(s) URLs whose host is a public name or public IP literal, rejecting localhost/.localhost/.internal/.local names and every reserved IPv4 and IPv6 range (IPv4-mapped and NAT64 literals judged by the embedded IPv4 address; alternate IPv4 spellings are normalized by the URL parser first). fetchPageTitle() fetches with redirect: "manual" and follows up to five hops itself, vetting each Location before requesting it. Hostnames are not resolved (Workers expose no DNS), so a public name pointing at a private address is left to the platform.
• Test: 27 blocked targets across schemes, names and IPv4/IPv6 ranges assert fetch is never called; redirect tests cover public-to-public, relative Location, redirect-to-private, redirect-to-file:, redirect loops and a missing Location.

Verification

• yarn test: 1360 tests, all green.
• yarn e2e: 15 failures reproduce identically (same test names, same count) on a clean main checkout in this sandbox — net::ERR_CONNECTION_RESET/browser-launch issues from the sandbox's network policy, unrelated to this diff. Verified via a side-by-side worktree run against origin/main.
• Python SDK: 104 tests, all green. ruff check/ruff format --check/mypy clean on changed files.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_017a43SYLJdvdDhMcgJF4zXM

[DennisAlund]

Flag for judgment: QR encoder uses wrong EC-codeword totals for versions 6-10, and never emits version-info blocks for 7-10 (src/qr.ts)

const eccL = [0, 7, 10, 15, 20, 26, 18, 20, 24, 30, 18];
const totalCodewords = [0, 26, 44, 70, 100, 134, 172, 196, 242, 292, 346];
const numEcc = eccL[ver];
const numData = totalCodewords[ver] - numEcc;

eccL is the standard "EC codewords per block, level L" table. The code uses it directly as the total EC codeword count without multiplying by the number of blocks (1 block for versions 1-5, but 2 for versions 6-9 and 4 for version 10). For versions 1-5 this happens to be correct (1 block), but for versions 6-10 the real EC totals should be 36, 40, 48, 60, 72; the code uses 18, 20, 24, 30, 18 instead. The encoder also never writes the two mandatory 6×3 version-information blocks the spec requires for versions 7-10.

Reachable: custom slugs are allowed up to 128 characters, so origin + slug + "?utm_medium=qr" easily exceeds the 106-byte version-5 boundary for a long custom slug, and the QR endpoint (src/api/qr.ts, src/mcp/server.ts) returns 200 with a normal-looking SVG that most real-world scanners will likely fail to decode.

Not fixing directly: a correct fix means implementing block splitting/interleaving per the spec plus version-info emission, which is a non-trivial rewrite of the hand-rolled encoder rather than a small, low-risk patch. Needs a decision on whether to fix the encoder properly, cap supported slug/URL length to stay within versions 1-5, or accept the current limitation.

---

Flag for judgment: unrestricted server-side fetch of user-supplied link URLs for auto-labeling (SSRF pattern) (src/title-fetch.ts, src/services/link-management.ts, src/api/links.ts)

createLink only validates parsed.protocol === "http:" || "https:"; it does not restrict host or IP. Any caller holding only the create API scope can create a link to http://127.0.0.1/... or another internal/link-local address, and the Worker fetches it server-side (following redirects) via fetchPageTitle, storing the resulting <title> as the link's label — visible via GET /links/:id.

This is a standard SSRF shape: no allow/deny policy exists in the codebase for the destination host. Cloudflare Workers' own network isolation (no direct route to RFC1918/loopback addresses unless the deployment adds a Tunnel or service binding) limits practical reach in most deployments, but nothing in this code enforces that — it depends entirely on platform topology.

Not fixing directly: needs a product decision on policy (block RFC1918/loopback/link-local resolution, whether to also block redirects into such ranges, whether this should apply to all outbound fetches or just this one) rather than a one-line fix.

---

Generated by Claude Code

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	shrtnr	bf4d709	Sep 12 2026, 08:35 AM

[DennisAlund]

Both items above are now addressed on this branch, each with its own regression test and CI green on the resulting head:

• QR encoder: fixed in 9bde31a (per-version block splitting/interleaving, version-info blocks for v7-10, alignment-pattern reservation), verified against a real decoder (jsQR).
• title-fetch SSRF: fixed in bf4d709 (isPublicHttpUrl() gate plus per-redirect-hop vetting).

Leaving this comment as the record of what the original findings were and how they were resolved.

---

Generated by Claude Code

[DennisAlund]

Code review: PR #59

Reviewed all 6 commits at bf4d709. 14 findings, 12 filed inline below. The two that follow could not be anchored: one file is untouched by this PR, the other line falls outside the diff hunks.

I verified the headline findings by running code rather than reading it: decoded QR payloads of every length 1 to 271 with jsQR, ran isPublicHttpUrl against 16 probe URLs, and checked Uri.parse against the real Dart toolchain.

Highest priority

1. SSRF guard, trailing-dot bypass (src/title-fetch.ts:45). http://metadata.google.internal./ passes.
2. SSRF guard, IPv6 gaps (src/title-fetch.ts:105). ::a.b.c.d and 2002::/16 wrap loopback and link-local addresses past the check.
3. Dart SDK parity (below). The defect this PR fixed in Python is still live in Dart.
4. The custom-slug toast is destroyed by the redirect that follows it (src/client.ts:280). The PR's headline fix does not reach the user.

Not anchorable, 1: Dart SDK carries the same InvalidURL escape, so the single-SDK rationale does not hold

sdk/dart/lib/src/base_client.dart:170. CLAUDE.md: "Any change to one SDK under sdk/ must be evaluated and applied to the others." The commit message argues there is nothing to port. There is.

_buildUri() calls Uri.parse('$_baseUrl$path') at lines 172, 177 and 181, outside any try. Both call sites (requestJson line 60, requestText line 131) invoke it before the try { streamed = await _httpClient.send(request); } catch (e) { throw ShrtnrError(0, ...) } block that starts at line 77.

Confirmed against dart 3.10.4, using the exact input the new Python test uses:

Uri.parse('https://example.com:notaport/links')
  -> FormatException: Invalid port (at character 21)

So ShrtnrClient(baseUrl: 'https://example.com:notaport', apiKey: k).links.list() throws a raw FormatException, breaking sdk/dart/README.md:142: "Network failures also throw ShrtnrError with status: 0."

Fix: wrap the _buildUri call in both requestJson and requestText. The TypeScript SDK is worth the same check before this merges.

Not anchorable, 2: non-ASCII QR payloads

Filed as a file-level comment on src/qr.ts, because the offending line 13 sits outside this PR's hunks.

Process gate

CLAUDE.md: "Any change under src/pages/, src/admin/, src/client.ts, src/styles.ts or src/index.tsx admin routes ends with yarn e2e green, not only yarn test. A code review or release that touches those paths is not complete until the e2e suite has run."

This PR changes src/client.ts and reports 15 e2e failures. Reproducing on main is not the same as green. The gap is load-bearing here: finding 4 above is exactly the class of defect the rule exists for, since a render test cannot see a toast that a navigation destroys. e2e/links.spec.ts also has no m-custom step at all, so the changed create path has no browser coverage.

Remaining findings, by weight

• src/title-fetch.ts:140: redirect loop gives each hop its own 5s timeout, raising the worst case to 30s of attacker-controlled waitUntil.
• src/db/slug-repository.ts:103: the guard blocks disable() instead of fixing the fallback query, and overloads the null return.
• src/qr.ts:236: GF(256) tables rebuilt per Reed-Solomon block, now 2 to 4 times per code.
• sdk/python/src/shrtnr/resources/links.py:55: the same _request body copy-pasted into 8 methods, with _base.py sitting right there.
• src/__tests__/unit/qr.test.ts:143: the version-information test recomputes the writer's own formula.
• src/__tests__/unit/client-custom-slug-toast.test.ts:16: extractTopLevelChunk now duplicated four times.
• sdk/python/tests/test_async_client.py:95: the new async test never closes its client.
• src/__tests__/unit/client-t-replacer.test.ts:11: em dash in a new comment.

🤖 Generated with Claude Code

[DennisAlund]

A trailing dot bypasses every name-based block in this guard.

The WHATWG URL parser keeps a fully qualified hostname verbatim, so new URL("http://metadata.google.internal./computeMetadata/v1/").hostname is "metadata.google.internal.". That misses host === "localhost", .endsWith(".localhost"), .endsWith(".internal") and .endsWith(".local") alike, while DNS resolves name. to the same address as name.

Running the shipped function, all four of these return true:

http://metadata.google.internal./computeMetadata/v1/
http://localhost./
http://db.localhost./
http://printer.local./

The cloud metadata endpoint the doc comment names is reachable through the guard.

Fix: strip one trailing . from host before the comparisons. Numeric literals need no change, the parser already normalizes http://127.0.0.1./ to 127.0.0.1.

[DennisAlund]

isReservedIPv6 misses IPv4-compatible (::a.b.c.d) and 6to4 (2002::/16) addresses.

Both forms carry an IPv4 address that the guard never judges, so private and loopback targets pass when written that way. Verified against the shipped function:

Input	Wraps	isPublicHttpUrl
http://[::127.0.0.1]/	127.0.0.1	true
http://[::a00:1]/	10.0.0.1	true
http://[2002:7f00:1::]/	127.0.0.1	true
http://[2002:a9fe:a9fe::]/	169.254.169.254	true

The parser normalizes [::127.0.0.1] to [::7f00:1], so the leadingZero branch on line 106 only matches :: and ::1, and g[5] is 0 rather than 0xffff. Neither special case fires and the function falls through to return false.

The new 27-entry blocked-target list covers ::ffff: and 64:ff9b:: but not ::/96 or 2002::/16.

Fix: when leadingZero && g[5] === 0 && (g[6] | g[7]) !== 0, judge groupsToIPv4(g[6], g[7]); add a g[0] === 0x2002 branch doing the same on g[1]/g[2].

[DennisAlund]

The per-hop timeout raises the worst case from 5s to 30s.

The previous single fetch(url, { redirect: "follow", signal: AbortSignal.timeout(5000) }) bounded the entire followed chain at 5 seconds. This loop builds a fresh signal on each of the 6 iterations (hop = 0..MAX_REDIRECTS), so each hop gets its own budget.

A server that answers every request with a 302 to a new public URL after 4.9s holds autoLabelLink's waitUntil for roughly 30 seconds. Every link creation goes through this path (src/api/links.ts:85 and :507), and the destination is user-supplied.

Fix: hoist one AbortSignal.timeout(5000), or a deadline computed once, outside the loop and pass it to every hop.

[DennisAlund]

The error branch navigates away, so the toast this PR adds never gets read.

toast() writes into the in-page #toast element with a 3000ms lifetime. Assigning window.location.href on the next microtask replaces the document and takes the toast with it. The user still lands on the detail page carrying only the auto-generated slug, with no durable sign that the slug they typed was rejected. That is the failure this PR set out to surface.

The three new vitest cases pass because they stub window as a plain object, so the navigation is never modelled.

doAddSlug, the handler this function was modelled on, deliberately does not navigate or close the modal on failure. Mirror that: on the error branch skip both closeModal() (line 275) and this redirect, and leave the user in the modal with the message visible.

[DennisAlund]

rsEncode rebuilds the GF(256) tables on every call, and block splitting now multiplies that.

Each call allocates new Uint8Array(512) plus new Uint8Array(256), runs 255 + 257 iterations to fill them, then builds the generator polynomial in O(numEcc²). interleaveBlocks now calls this once per block: 2 calls for versions 6 to 9 and 4 for version 10, where the old code made exactly 1. src/api/qr.ts serves this per request.

Fix: hoist exp, log and gfMul to module scope, and memoize the generator polynomial by numEcc.

[DennisAlund]

This fix is copy-pasted into 8 method bodies when _base.py already exists for exactly this.

LinksResource._request, LinksResource._request_text, AsyncLinksResource._request, AsyncLinksResource._request_text and the four Bundles/Slugs equivalents are now byte-identical bodies carrying byte-identical three-line comments. _base.py already holds _build_url, parse_json_response and parse_text_response, the shared pieces of this same request path.

The next httpx exception-hierarchy surprise, or a retry policy, a header, a timeout, has to land in 8 places again and can silently miss one.

Fix: move _request and _request_text into a _BaseResource / _AsyncBaseResource in _base.py and have the three resource classes inherit.

[DennisAlund]

This test asserts the writer against itself, so a placement error would still pass.

readVersionInfo recomputes a = size - 11 + (i % 3) and b = Math.floor(i / 3), byte for byte the formula makeQR uses to write the version blocks, then reads grid[b][a] and grid[a][b] exactly as the writer wrote them. Transpose the two blocks or shift them by a module and this assertion still passes. The new v7+ blocks are covered for value, not for position.

The scannability suite cannot close the gap either: jsQR derives the version from the symbol dimension and only falls back to the version bits, so it decodes fine even with wrong version information.

Fix: assert against literal module coordinates from ISO/IEC 18004 Figure 25, or compare against a known-good reference symbol.

[DennisAlund]

extractTopLevelChunk is now copied into four test files.

The same 12-line function, down to the /^(function |var |if |window\.|document\.)/ terminator heuristic, already lives at src/__tests__/unit/client-api-error-toasts.test.ts:59 and src/__tests__/unit/client-script-escaping.test.ts:13. This PR adds copies here and in client-t-replacer.test.ts:16.

The heuristic is fragile: it breaks the moment a helper in client.ts starts with an unindented const or let, and the repair would then have to land in four places.

Fix: extract it to a shared helper module under src/__tests__/.

[DennisAlund]

This test never closes its client, breaking the teardown discipline the rest of the file follows.

The module's client fixture (line 33) wraps its yield in try/finally: await c.aclose() precisely so the underlying httpx.AsyncClient connection pool is released. This test builds AsyncShrtnr(...) directly on line 100 and returns after the pytest.raises block, leaving the pool open for the remainder of the session. That risks an "unclosed client" ResourceWarning or an event-loop-closed error in CI.

Fix: wrap it in async with, or add await client.aclose().

The sync twin in test_client.py is worth the same check.

[DennisAlund]

Em dash in new comment text.

CLAUDE.md, Writing rules, applies to all produced material including comments: "No em dashes. Use colon, comma, or period."

This line reads // key title, a bundle name — both free text) corrupts the rendered string. A comma or a colon works here.

[DennisAlund]

makeQR treats input as Latin-1 and sizes the symbol by text.length, so non-ASCII payloads produce silently undecodable codes.

Line 13 runs data.push(text.charCodeAt(i)), which yields 233 for é (Latin-1, where UTF-8 needs 0xC3 0xA9) and 20320 for 你. For any code point above 255 the value then overflows the byte field: toBin(20320, 8) returns a 15-character string and desynchronizes the whole bit stream. The capacity check on line 17 compares against text.length rather than the encoded byte length, so the version is undersized too.

Decoding real output with jsQR:

• makeQR("https://s.ex/café") returns a version-1 grid that decodes to "".
• makeQR("https://s.ex/你好") returns a grid jsQR cannot read at all.

Neither returns null, so callers ship a broken image. This is reachable today through the MCP qr tool (src/mcp/server.ts:491), whose base_url is caller-supplied and validated only by z.string().url(), which accepts IDN hosts.

Fix: encode with TextEncoder and derive the version from the byte length.

(Filed against the file rather than a line, because line 13 falls outside this PR's hunks.)