Weekly defect review: 10 fixes across app, admin UI, and all three SDKs - #36
Conversation
normalizeUrl() stripped any trailing "?" unconditionally, even when the "?" was part of a URL fragment rather than an empty query string. https://example.com/#section? was silently corrupted to https://example.com/#section on link creation, contradicting the function's own documented guarantee to preserve fragment content. Only strip the trailing "?" when no "#" has opened a fragment.
validateCustomSlug() had no upper bound, unlike the public API's CustomSlugStringSchema (which the comment above it says this function is supposed to mirror). The admin route for adding a custom slug to a link (POST /_/admin/api/links/:id/slugs) calls this validator directly, not the zod schema, so an over-length slug that still matches the character regex was accepted, inserted into D1, and then made SlugCache.put() throw when writing a KV key over Cloudflare's 512-byte limit: the slug row exists in D1 but its redirect can never be cached or worked with, and the request that created it gets an uncaught 500 instead of a clean 400. Reject slugs over MAX_SLUG_LENGTH (128), matching the existing public API limit.
When the fetched content-type wasn't text/html or application/xhtml, fetchPageTitle returned null without cancelling res.body, unlike the success path a few lines below which cancels its reader once enough of the page has been read. Most real destination URLs are not HTML (APIs, PDFs, images, ...), and this fetch runs on every link creation that omits a label, so this leaked the response stream on the common path.
Two related holes in the admin panel's bundle editor, both exploitable by
any user who can create/edit a bundle (name/description/icon are
unrestricted free-text server-side, unlike slugs):
1. esc() escaped &, <, > via the DOM textContent/innerHTML round-trip but
never escaped ". Bundle name/description are placed inside a
double-quoted value="..." attribute (showEditBundleModal), so a bundle
named e.g. `x" onmouseover="alert(1)` broke out of the attribute and
injected an arbitrary one, firing without a click. Fixed by also
escaping " in esc()'s output.
2. renderIconPicker() interpolated the bundle's icon value directly into
an inline onclick="selectBundleIcon('...')" JS-string with no
escaping at all (esc() doesn't help there either: entities decode
before the script engine sees them, per the existing comment on
pendingDuplicateUrl covering the same class of bug for a different
field). An icon value containing a single quote broke out of the JS
string literal directly. Fixed by reading the icon from the already
present data-icon attribute at click time (this.dataset.icon) instead
of interpolating it into the handler's source.
Four slug-action error toasts (set primary, delete, disable, enable slug) fell back to a bare English 'Error' literal instead of a translated string, and the add-custom-slug modal's field label was hardcoded to 'Slug', both bypassing i18n unlike every sibling string in the same functions. Added client.setPrimaryError/deleteSlugError/disableSlugError/ enableSlugError and linkDetail.slugFieldLabel to en/id/sv and wired the call sites through t().
Only the default-fetch fallback path (resolveGlobalFetch()) was ever bound to globalThis. A fetch passed explicitly via ShrtnrClientConfig.fetch (the README's own example passes window.fetch for custom TLS configurations) was stored as a bare reference and invoked as this.fetchFn(...), which rebinds the receiver to the HttpClient instance. Real browsers brand-check fetch's receiver and reject that with 'Illegal invocation', reintroducing the exact bug fixed in ed90c25 for the default path. Function.prototype.bind on an already-bound function ignores the new receiver, so binding config.fetch too is a no-op for implementations that don't care about this.
Both READMEs' Models section omitted several types the SDKs actually export (BundleTopLink, BreakdownDimension, BreakdownPage, DateCount, SlugCount for TS; the same minus BreakdownPage/BreakdownDimension already listed, plus BundleTopLink/DateCount/SlugCount for Python), diverging from CLAUDE.md's README-parity rule. The Dart SDK's README already lists the complete set (per its own CHANGELOG 2.1.1 entry fixing the same gap there); this brings TS and Python's lists in line with it and with what sdk/typescript/src/index.ts and sdk/python/src/shrtnr/__init__.py actually export. Docs only, no code change.
parse_json_response() short-circuited to None for status_code == 204 OR an empty body, before the try/except around response.json() that 1.1.1 added specifically to catch 'a 2xx response whose body is not valid JSON' (e.g. an HTML error page or truncated body served with a 200). An empty body on a non-204 2xx is exactly that case, but never reached the try/except, so it returned None instead of raising. Every resource method then called SomeModel.from_dict(None), which raises a bare AttributeError instead of the documented ShrtnrError a caller following the README's except ShrtnrError pattern would catch. 204 still maps to None; only a non-204 2xx with an empty body now raises.
send() only returns headers/status; http.Response.fromStream(streamed)
is where the body is actually read off the socket. Only the send() call
was wrapped in try/catch, so a connection drop, reset, or timeout while
the body was still streaming threw a raw SocketException/ClientException
instead of the ShrtnrError the README documents ('Network failures also
throw ShrtnrError with status: 0'). The existing 'network error' test
only covers send() itself failing (MockClient's handler throwing before
any response), not a failure during body streaming after headers arrive,
so this was an untested gap in both requestJson and requestText.
doDeleteSlug()'s success path showed t('client.customAdded') ('Custom
slug added') after a DELETE succeeded, a copy-paste leftover from
doAddSlug(). Added client.slugDeleted ('Slug deleted') and swapped the
call site.
|
Items found during this review that need a developer call rather than a direct fix. None of these are in the diff. 1. SSRF in 2. 3. Unbounded N+1 fan-out in 4. Background poll silently resets breakdown-panel pagination ( 5. TS SDK's None of the code-quality nits from the diff-based pass I ran before this fanned-out review (a dead tooltip Generated by Claude Code |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | d02b118 | Aug 10 2026, 06:12 AM |
There was a problem hiding this comment.
Pull request overview
This PR batches a weekly defect sweep across the Cloudflare Workers app, admin UI client script, and all three SDKs, focusing on closing a stored-XSS vector, tightening correctness on edge cases, and enforcing SDK error contracts with regression tests.
Changes:
- Fixes several app-side correctness and security issues (stream draining in
fetchPageTitle, custom slug max length validation, and fragment-safe URL normalization, plus admin client script escaping and safer icon selection). - Hardens SDK behavior and contracts across TypeScript, Python, and Dart, with new regression tests.
- Updates i18n strings and SDK READMEs to match exported types.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/title-fetch.ts | Cancels non-HTML response bodies to avoid leaking streams. |
| src/slugs.ts | Enforces MAX_SLUG_LENGTH for custom slugs. |
| src/normalize-url.ts | Preserves ? when it is fragment content. |
| src/i18n/en.ts | Adds missing admin UI i18n keys for slug actions. |
| src/i18n/id.ts | Adds missing admin UI i18n keys for slug actions. |
| src/i18n/sv.ts | Adds missing admin UI i18n keys for slug actions. |
| src/client.ts | Improves escaping and avoids interpolating icon values into inline JS; routes some UI strings through t(). |
| src/tests/unit/title-fetch.test.ts | Adds regression test verifying non-HTML bodies are canceled. |
| src/tests/unit/slugs.test.ts | Adds regression tests for the 128-char custom slug limit. |
| src/tests/unit/normalize-url.test.ts | Adds regression test for fragment content ending in ?. |
| src/tests/unit/client-script-escaping.test.ts | Adds regression tests for admin client script escaping and icon picker injection prevention. |
| src/tests/unit/client-i18n-strings.test.ts | Adds regression tests preventing hardcoded slug-action UI strings from returning. |
| sdk/typescript/src/internal/http.ts | Binds caller-provided fetch to globalThis to avoid “Illegal invocation”. |
| sdk/typescript/tests/client.test.ts | Adds regression test for bound caller-supplied fetch. |
| sdk/typescript/README.md | Aligns documented exported key types with actual exports. |
| sdk/python/src/shrtnr/_base.py | Raises ShrtnrError on empty-body non-204 2xx responses. |
| sdk/python/tests/test_client.py | Adds regression test for empty-body 2xx error wrapping. |
| sdk/python/README.md | Aligns documented exported key types with actual exports. |
| sdk/dart/lib/src/base_client.dart | Wraps mid-stream body read failures as ShrtnrError(status: 0). |
| sdk/dart/test/client_test.dart | Adds regression test for mid-body connection drop mapping to ShrtnrError. |
Suppressed comments (3)
src/client.ts:491
- If the error response body is empty or non-JSON,
res.json()will reject and the user will get no toast plus an unhandled promise rejection. Add a.catch(...)fallback.
api('/links/' + linkId + '/slugs/' + slug, { method: 'DELETE' }).then(function(res) {
if (res.ok) { closeModal(); toast(t('client.slugDeleted')); window.location.reload(); }
else res.json().then(function(data) { toast(data.error || t('client.deleteSlugError'), 'error'); });
});
src/client.ts:505
- If the error response body is empty or non-JSON,
res.json()will reject and the user will get no toast plus an unhandled promise rejection. Add a.catch(...)fallback.
function doDisableSlug(linkId, slug) {
api('/links/' + linkId + '/slugs/' + slug + '/disable', { method: 'POST' }).then(function(res) {
if (res.ok) { closeModal(); window.location.reload(); }
else res.json().then(function(data) { toast(data.error || t('client.disableSlugError'), 'error'); });
});
src/client.ts:519
- If the error response body is empty or non-JSON,
res.json()will reject and the user will get no toast plus an unhandled promise rejection. Add a.catch(...)fallback.
function doEnableSlug(linkId, slug) {
api('/links/' + linkId + '/slugs/' + slug + '/enable', { method: 'POST' }).then(function(res) {
if (res.ok) { closeModal(); window.location.reload(); }
else res.json().then(function(data) { toast(data.error || t('client.enableSlugError'), 'error'); });
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The four slug-action handlers read `error` off the parsed error body, but nothing caught a res.json() rejection. An edge-level 502/524 serves an HTML page and a bare 500 can serve an empty body, so json() rejects, the toast never fires and the rejection goes unhandled: the click looks like it did nothing at all. Adds the same .catch() fallback the link-level disable/delete/enable handlers already carry, so the localized message shows either way. The regression test drives the real handlers from the generated script against empty and HTML bodies; before the fix it produced 8 unhandled rejections and no toasts.
The old wording ran the quote character straight into a colon ("but not ":"),
which reads as a two-character token rather than as "the double quote". Spell
out the character and what the round-trip does instead.
The slug-action fix left the same gap in seven sibling handlers: API key creation, add-slug, the inline label and expiry editors, and the bundle create/update/add-link paths all read `error` off a parsed error body with nothing catching a res.json() rejection. An HTML 502 or an empty 500 body silenced the toast and surfaced an unhandled rejection instead. All fourteen failure paths in the script now carry the fallback. The test covers each handler against empty and HTML bodies, and a guard test greps the generated script so a new unguarded path fails the suite rather than waiting for the next review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/client.ts:416
- The add-custom-slug modal still hardcodes a user-facing placeholder ("my-custom-slug"). Per CLAUDE.md's i18n rule, hints like placeholders should also go through t() and live in translation files so localized locales can change the UI copy.
'<div class="form-group"><label class="form-label">' + esc(t('linkDetail.slugFieldLabel')) + '</label><input class="form-input" id="m-new-slug" placeholder="my-custom-slug"></div>' +
Weekly defect-hunting review of the app, its API, and the TypeScript/Python/Dart SDKs. Eleven fixes, each with a regression test that fails before the fix and passes after. Full app suite (1124 tests), TS SDK suite, Python SDK suite, and Dart SDK suite are all green;
tsc --noEmitanddart analyzeare clean; spec hash is unchanged (./scripts/spec-hash.shmatches the recorded hash in all three SDK manifests).Security
Stored XSS in the admin bundle editor (
src/client.ts)esc()escaped&,<,>via a DOM textContent/innerHTML round-trip but never escaped". Bundle name/description are unrestricted free-text fields (no charset limit like slugs have) placed inside a double-quotedvalue="..."attribute, so a bundle named e.g.x" onmouseover="alert(1)broke out and injected an arbitrary attribute that fires without a click.renderIconPicker()interpolated the bundle's icon value directly into an inlineonclick="selectBundleIcon('...')"JS string with no escaping at all (entities decode before the script engine sees them, soesc()doesn't help there either).esc()now also escapes". The icon picker reads the icon from the already-presentdata-iconattribute at click time (this.dataset.icon) instead of interpolating it into the handler's source.src/__tests__/unit/client-script-escaping.test.tsextracts and evaluates the actual generated script against a minimal DOM stub, proving both the attribute-breakout and the onclick-injection are closed.Correctness
fetchPageTitleleaked the response stream on the common path (src/title-fetch.ts) — when content-type wasn't HTML (the common case: APIs, PDFs, images), it returnednullwithout drainingres.body, unlike the success path a few lines below. Fixed withawait res.body?.cancel().Custom slugs had no max-length check (
src/slugs.ts) — the admin route for adding a custom slug callsvalidateCustomSlug()directly, bypassing the public API's zod schema (which does enforce.max(MAX_SLUG_LENGTH), per the comment above it saying this function should mirror it). An over-length slug was inserted into D1 and then madeSlugCache.put()throw on a KV key over Cloudflare's 512-byte limit: a permanently broken slug and an uncaught 500 instead of a clean 400. Fixed by adding the same 128-char limit.normalizeUrlcorrupted URLs whose fragment ends in?(src/normalize-url.ts) — it stripped any trailing?unconditionally, even when it was part of a fragment's content rather than an empty query string, sohttps://example.com/#section?became.../#section. Fixed to only strip the trailing?when no#has opened a fragment.Admin actions went silent when a failure carried no JSON body (
src/client.ts) — eleven handlers readerroroff the parsed error body with nothing catching ares.json()rejection. An edge-level 502/524 serves an HTML page and a bare 500 can serve an empty body, sojson()rejected, the toast never fired and the rejection went unhandled: the click looked like it did nothing at all. Three sibling handlers (doDisableLink,doDeleteLink,doEnableLink) already carried the right fallback, so the fix propagates that pattern to all fourteen failure paths, covering the slug actions, API key creation, add-slug, the inline label and expiry editors, and the bundle create/update/add-link paths. The test drives each handler against empty and HTML bodies; a guard test greps the generated script so a new unguarded path fails the suite instead of waiting for the next review.TS SDK: a caller-supplied
fetchreintroduced a previously-fixed bug (sdk/typescript/src/internal/http.ts) — only the default-fetch fallback was bound toglobalThis. Afetchpassed viaShrtnrClientConfig.fetch(the README's own example passeswindow.fetch) was invoked asthis.fetchFn(...), which real browsers reject with "Illegal invocation" via their receiver brand check — the exact bug fixed ined90c25for the default path, just not for this one. Fixed by bindingconfig.fetchtoo (Function.prototype.bindon an already-bound function ignores the new receiver, so this is a no-op for implementations that don't care aboutthis).Python SDK: an empty-body 2xx response silently returned
None(sdk/python/src/shrtnr/_base.py) —parse_json_responseshort-circuited before the try/except that 1.1.1 added specifically to catch "a 2xx response whose body is not valid JSON." An empty body on a non-204 2xx is exactly that case but never reached it, so every resource method'sSomeModel.from_dict(None)raised a bareAttributeErrorinstead of the documentedShrtnrError. Fixed; 204 still maps toNone.Dart SDK: a mid-stream connection failure escaped the
ShrtnrErrorcontract (sdk/dart/lib/src/base_client.dart) —send()was wrapped in try/catch, buthttp.Response.fromStream(streamed)(where the body is actually read) was not, so a connection reset while streaming threw a raw exception instead of theShrtnrError(status: 0)the README promises for network failures. Fixed by wrapping both.Minor
t(); added the missing i18n keys (en/id/sv) and wired them up.doDeleteSlug()'s success toast said "Custom slug added" (copy-paste leftover fromdoAddSlug()) instead of a deletion message.BundleTopLink,BreakdownDimension,BreakdownPage,DateCount,SlugCount); brought in line with what the code exports and with the Dart README, which already lists the complete set.esc(). The old text ran the quote character straight into a colon ("but not":"), which reads as a two-character token rather than as the double quote it describes.Flagged for developer judgment (not fixed here)
See PR comment below — items that need a design/product call, change behavior beyond the reported bug, or where I couldn't write a clean regression test.
Generated by Claude Code