Code review fixes: ownership gaps, stored XSS, URL normalization, SDK parity#30
Open
DennisAlund wants to merge 6 commits into
Open
Code review fixes: ownership gaps, stored XSS, URL normalization, SDK parity#30DennisAlund wants to merge 6 commits into
DennisAlund wants to merge 6 commits into
Conversation
updateLink and setSlugPrimary were the only link/slug mutations that took no caller identity, so any create-scope API key or MCP session could repoint another owner's link to an arbitrary URL or reshuffle its primary slug. Because link disable is implemented as expires_at = now, the open update path also let a non-owner disable or re-enable a link, bypassing the owner-only guard that disableLink/enableLink/deleteLink enforce. Add an identity argument to both service functions and return 403 when created_by does not match, matching the sibling mutations. Thread the identity through the REST handlers, the admin routes, and the MCP update_link tool. Existing update tests pass the owning identity so their assertions are unchanged; new ownership tests cover the non-owner 403.
Both buttons interpolated user-controlled text into a single-quoted JS string inside an inline onclick. escHtml() deliberately does not escape the single quote (its own doc warns against this exact use), and the hand-rolled backslash escaping on the key title was defeated by a literal backslash. A link whose URL or an API key whose title contained a quote could break out of the string and run script in the admin origin. Carry the values on data-* attributes read by delegated click handlers, matching the existing data-copy-slug pattern, and refactor showDuplicateModal so the confirm button no longer interpolates the URL into a nested onclick. Regression tests cover a quote-bearing URL and the data-* delete action.
The regex /[/?#]+$/ stripped any trailing /, ?, or # unconditionally, so a slash that belonged to a query value or a hash-router fragment was silently removed. createLink and updateLink store the normalized string and serve it as the 301 Location, so a link to https://example.com/search?q=cats/ redirected to .../search?q=cats, and a hash-router target lost its trailing route segment. Only strip a redundant character: an empty trailing fragment, an empty trailing query, and trailing path slashes when no query or fragment follows. Add regression tests for the query-value, embedded-URL, and hash-router cases.
The QR endpoint sits behind a read-scoped bearer token, so a public directive lets a shared cache store the authenticated response and hand the link-id to slug mapping to unauthenticated clients. private gives the same browser-side caching without authorizing shared caches.
The API schema validates the QR size as an integer and the TypeScript SDK converted this parameter to a number in 1.0.2; the Dart SDK also takes an int. The Python SDK still annotated it as str, forcing callers to pre-stringify and diverging from the other two SDKs. Annotate size as int and stringify internally on both the sync and async resources. Bump the Python SDK to 1.1.1 with a changelog entry. Python-only change: it brings Python to the parity the other SDKs already have.
- Dart README: update() takes a Link/Bundle (2.0.0 signature), analytics ranges and bundle accents are enums not strings, and the exported model names are DateCount/SlugCount plus the enum types. The prior examples did not compile and named types that do not exist. - Dart BundleArchivedFilter: correct the member doc comments. The server maps true/1/only all to archived-only and all to include-archived; the old docs inverted this and activeOnly is flagged as a misnomer kept for wire compatibility. - Dart ShrtnrError.toString now matches the TS/Python message prefix it claims to mirror. - Python README: update() rows show the UNSET sentinel defaults and the clear-vs-unchanged semantics, so passing None no longer reads as a no-op. - CLAUDE.md: point at the real sdk-spec-drift job in ci.yml. - bump-sdk-version.sh: pub guidance no longer tells the user to push the tag immediately, matching the documented create-locally release rule.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 631d114 | Jul 03 2026, 06:54 AM |
There was a problem hiding this comment.
Pull request overview
This PR addresses several security and correctness issues in the Worker/admin UI and brings SDKs/docs back into parity with the API surface and semantics.
Changes:
- Add ownership enforcement for
updateLinkandsetSlugPrimary, threading caller identity through REST/admin/MCP entrypoints and extending ownership tests. - Eliminate stored-XSS vectors in admin UI actions by removing inline
onclickinterpolation of user-controlled values and switching todata-*+ delegated click handlers (with regression tests). - Fix URL normalization edge cases (query/fragment preservation), adjust QR endpoint caching to
private, and update Python/Dart SDK typing/docs plus release guidance/docs references.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/services/link-management.ts | Adds identity-based ownership checks to link update and primary-slug mutation. |
| src/pages/link-detail.tsx | Removes inline onclick URL interpolation for duplicate-link action. |
| src/pages/keys.tsx | Removes inline onclick title interpolation for key deletion action. |
| src/normalize-url.ts | Refines URL normalization to avoid corrupting query/fragment content. |
| src/mcp/server.ts | Threads MCP identity through updateLink calls. |
| src/index.tsx | Passes admin identity into link/slugs handlers for ownership enforcement. |
| src/client.ts | Implements delegated handlers for duplicate-link and key deletion; avoids inline JS-string interpolation. |
| src/api/slugs.ts | Adds identity parameter to handleSetPrimarySlug and forwards it to service. |
| src/api/qr.ts | Changes QR response caching from public to private for token-protected content. |
| src/api/links.ts | Threads API identity through updateLink (OpenAPI route + handler). |
| src/tests/unit/normalize-url.test.ts | Adds regression tests for query-value and hash-router trailing slashes. |
| src/tests/service/ownership.test.ts | Adds ownership coverage for update-link and set-primary-slug. |
| src/tests/service/link-service.test.ts | Updates service tests for new updateLink(..., identity) signature. |
| src/tests/page/link-detail-page.test.ts | Verifies duplicate-link markup no longer uses inline handler interpolation. |
| src/tests/page/keys-page.test.ts | Verifies key delete action uses data-* attributes (no inline onclick). |
| src/tests/handler/mcp.test.ts | Updates MCP service-layer test for new updateLink signature. |
| sdk/python/tests/test_client.py | Updates QR size argument to use int. |
| sdk/python/src/shrtnr/resources/links.py | Retypes links.qr(size) to int (sync/async) and stringifies in query. |
| sdk/python/README.md | Clarifies update() semantics using UNSET sentinel defaults and clear-vs-unchanged behavior. |
| sdk/python/pyproject.toml | Bumps Python SDK version to 1.1.1. |
| sdk/python/CHANGELOG.md | Adds 1.1.1 entry documenting links.qr(size: int) typing fix. |
| sdk/dart/README.md | Fixes README examples and signatures to match current SDK API/enums/types. |
| sdk/dart/lib/src/models.dart | Corrects BundleArchivedFilter doc comments to match server semantics. |
| sdk/dart/lib/src/errors.dart | Aligns ShrtnrError.toString() prefix with other SDKs. |
| scripts/bump-sdk-version.sh | Updates pub release guidance to avoid pushing tag prematurely. |
| CLAUDE.md | Fixes spec-drift CI gate reference to the sdk-spec-drift job in ci.yml. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+453
to
+456
| // Holds the URL to duplicate while the confirm modal is open. Keeping it in a | ||
| // closure variable avoids interpolating a user-controlled URL into an inline | ||
| // onclick JS-string, which esc() cannot make safe (it does not escape '). | ||
| var pendingDuplicateUrl = null; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A code-review pass surfaced two security bugs, a redirect-correctness bug, and several SDK parity/doc drifts. Each fix is a focused commit with tests where there is runtime surface to cover. The full Worker suite passes (1050 tests, 8 new),
tsc --noEmitis clean, and the OpenAPI spec hash is unchanged so thesdk-spec-driftgate stays green.Security
Missing ownership checks on link update and set-primary-slug (
src/services/link-management.ts)updateLinkandsetSlugPrimarywere the only link/slug mutations that took no caller identity, whiledisableLink/enableLink/deleteLink/removeSlug/disableSlug/enableSlugall return 403 whencreated_bydoes not match. Any create-scope API key or MCP session could repoint another owner's link to an arbitrary URL. Because link disable is implemented asexpires_at = now, the open update path also let a non-owner disable or re-enable a link, bypassing the owner-only guard the tests explicitly pin. Added anidentityargument to both service functions with a 403 on mismatch, threaded through the REST handlers, the admin routes, and the MCPupdate_linktool. New ownership tests cover the non-owner 403 for both operations.Stored XSS in the duplicate-link and delete-key buttons (
src/pages/link-detail.tsx,src/pages/keys.tsx,src/client.ts)Both buttons interpolated user-controlled text (a link URL, an API-key title) into a single-quoted JS string inside an inline
onclick.escHtml()deliberately does not escape the single quote (its own doc warns against exactly this), and the hand-rolled backslash escaping on the key title was defeated by a literal backslash. A link whose URL, or a key whose title, contained a quote could break out and run script in the admin origin. Moved both values ontodata-*attributes read by delegated click handlers (matching the existingdata-copy-slugpattern) and refactoredshowDuplicateModalso the confirm button no longer re-interpolates the URL. Regression tests cover a quote-bearing URL and thedata-*delete action.QR endpoint served with
publicCache-Control (src/api/qr.ts)The QR route sits behind a read-scoped bearer token, so
publiclet a shared cache store the authenticated response and serve the link-id to slug mapping to unauthenticated clients. Changed toprivate, max-age=86400, same browser caching without the shared-cache waiver.Correctness
normalizeUrlcorrupted query and fragment content (src/normalize-url.ts)The regex
/[/?#]+$/stripped any trailing/,?, or#unconditionally, so a slash inside a query value or a hash-router fragment was silently removed.createLink/updateLinkstore the normalized string and serve it as the 301Location, so a link tohttps://example.com/search?q=cats/redirected to.../search?q=cats, and a hash-router target lost its trailing route. Now only strips genuinely redundant characters: an empty trailing fragment, an empty trailing query, and trailing path slashes when no query or fragment follows. Regression tests added for the query-value, embedded-URL, and hash-router cases.SDK parity and docs
links.qr(size)typed asstr; the API schema validates an integer and the TS SDK converted it to a number in 1.0.2 (Dart already takes an int). Retyped tointwith internal stringify on both sync and async resources; bumped the Python SDK to 1.1.1 with a changelog entry.update(id, {...})signature dropped in 2.0.0, passed string literals whereTimelineRange/BundleAccentenums are required (examples did not compile), and namedDateClickCount/SlugClickCounttypes that do not exist. Corrected toupdate(link)/update(bundle), enum arguments, and the realDateCount/SlugCountplus the exported enum types.BundleArchivedFilterdoc comments inverted the server semantics (true/1/onlyall mean archived-only;allmeans include-archived). Corrected the comments and flaggedactiveOnlyas a misnomer kept for wire compatibility (a rename is breaking and left for the next major).ShrtnrError.toString()now uses the sameshrtnr API error (HTTP n)prefix as the TS/Python SDKs it claims to mirror.update()rows now show theUNSETsentinel defaults and the clear-vs-unchanged semantics, so passingNoneno longer reads as a no-op.sdk-spec-drift.yml; the gate is thesdk-spec-driftjob inci.yml.bump-sdk-version.shpub guidance no longer tells the user to push the tag immediately, matching the documented create-locally release rule.Notes / not addressed
A few findings were left out as they need a schema migration or a breaking change, better handled deliberately:
expires_at. Link disable overwritesexpires_atwithnowand enable sets it toNULL, so a disable/enable round-trip on a link with a real future expiry loses that expiry. Fixing it properly needs a separatedisabled_atcolumn (migration), so it is out of scope here.1y/allranges. Worth a dedicated fix with its own tests.BundleArchivedFilter.activeOnlyrename is a breaking SDK change; documented for now.🤖 Generated with Claude Code
Generated by Claude Code