Robustness audit: input validation, atomic mutations, consistency fixes#13
Open
DennisAlund wants to merge 9 commits into
Open
Robustness audit: input validation, atomic mutations, consistency fixes#13DennisAlund wants to merge 9 commits into
DennisAlund wants to merge 9 commits into
Conversation
…ffic The MCP get_link_qr tool appended a bare ?qr parameter, but the redirect handler only maps utm_medium=qr to link_mode "qr". Scans of MCP-issued QR codes were recorded as plain link clicks. Align the URL shape with the REST QR endpoint (api/qr.ts).
- Whitelist theme (oddbit, dark, light) and lang (en, id, sv) in updateAppSettings; unknown or non-string values now return 400 instead of persisting silently. - Cap API key titles at 120 characters. - getAppSettings clamps a corrupted or out-of-bounds stored slug_default_length to the hardcoded default instead of returning NaN or an unusable length to the settings page and link creation.
D1 batches are transactional; sequential statements are not. A failure mid-sequence could leave a link with no primary slug (setPrimary, disable, remove, addCustom) or orphan slug and click rows (delete). Each mutation now runs its statements in one batch. setPrimary also verifies the slug belongs to the link before clearing primary flags. Clearing first and matching nothing afterwards stranded the link without any primary slug.
- createLink and updateLink now reject non-string labels and non-integer or negative expires_at values. The admin API path parses raw JSON without a zod schema, so a string expires_at reached D1 verbatim and broke the numeric expiry comparison in the redirect handler. - A corrupted stored slug_default_length no longer blocks link creation; the service falls back to the hardcoded default. Explicit caller input still returns 400. - addCustomSlugToLink surfaces a UNIQUE violation from a concurrent insert as the same 409 the pre-check produces, instead of a 500.
listBundles resolves the caller's bot and self-referrer filter preferences but getBundle passed undefined, so the bundle card and the MCP get_bundle tool reported different click totals than the bundles list for the same caller and range.
…input - With ACCESS_AUD configured, unauthenticated requests to /_/admin/api/* now get a 401 JSON response instead of a 302 redirect to the landing page. fetch() callers follow redirects and received HTML where they expected JSON. - getPageData clamps theme and lang to the known sets before rendering. Cookie values are caller-controlled and stored settings may predate validation.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 33e2804 | Jun 11 2026, 09:46 AM |
There was a problem hiding this comment.
Pull request overview
Robustness-focused changes to harden request validation, ensure analytics consistency, and make multi-statement database mutations transactional so partial failures don’t corrupt link/slug state.
Changes:
- Added service-layer validation for admin-originated link fields (
label,expires_at) and strengthened settings validation (theme/lang allowlists, API key title length, safe fallback for corruptedslug_default_length). - Made slug/link repository multi-statement mutations transactional via D1 batches; improved behavior under slug-collision races (409 instead of 500).
- Aligned MCP QR generation with REST QR tracking (
utm_medium=qr) and ensured bundle summaries apply viewer filter preferences consistently.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/services/link-management.ts | Adds input/type validation for admin JSON paths; handles slug UNIQUE races consistently. |
| src/services/bundle-management.ts | Applies viewer click filters to getBundle summaries to match listBundles. |
| src/services/admin-management.ts | Validates/clamps settings inputs (theme/lang/title length) and safely handles corrupted stored slug length. |
| src/mcp/server.ts | Updates MCP QR URL tracking parameter to utm_medium=qr for correct analytics attribution. |
| src/index.tsx | Returns 401 JSON for unauthenticated admin API calls under Access; clamps theme/lang cookie/settings before render. |
| src/db/slug-repository.ts | Uses transactional D1 batches for multi-step slug mutations; adds membership guard for primary changes. |
| src/db/link-repository.ts | Makes link deletion atomic via D1 batch to avoid orphan rows on partial failure. |
| src/constants.ts | Introduces theme allowlist/default and API key title max length constant. |
| src/tests/service/link-service.test.ts | Adds tests for field type validation, corrupted slug length fallback, and UNIQUE race handling. |
| src/tests/service/bundle-service.test.ts | Verifies getBundle summary matches list view under click filters. |
| src/tests/service/admin-service.test.ts | Adds tests for slug length fallback, theme/lang validation, and API key title length. |
| src/tests/repository/slug-repository.test.ts | Adds tests ensuring setPrimary doesn’t clear primaries for non-member/nonexistent slugs. |
| src/tests/repository/link-repository.test.ts | Adds deletion test ensuring no orphan link/slug rows remain. |
| src/tests/handler/mcp.test.ts | Tests MCP QR output contains utm_medium=qr. |
| src/tests/handler/api-misc.test.ts | Tests admin API returns 401 JSON (not redirect) when unauthenticated under Access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
70
to
83
| static async setPrimary(db: D1Database, linkId: number, slug: string): Promise<void> { | ||
| await db.prepare("UPDATE slugs SET is_primary = 0 WHERE link_id = ?").bind(linkId).run(); | ||
| await db.prepare("UPDATE slugs SET is_primary = 1 WHERE slug = ? AND link_id = ?").bind(slug, linkId).run(); | ||
| // Verify membership first: clearing primaries and then matching nothing | ||
| // would leave the link without any primary slug. | ||
| const member = await db | ||
| .prepare("SELECT 1 FROM slugs WHERE slug = ? AND link_id = ?") | ||
| .bind(slug, linkId) | ||
| .first(); | ||
| if (!member) return; | ||
|
|
||
| await db.batch([ | ||
| db.prepare("UPDATE slugs SET is_primary = 0 WHERE link_id = ?").bind(linkId), | ||
| db.prepare("UPDATE slugs SET is_primary = 1 WHERE slug = ? AND link_id = ?").bind(slug, linkId), | ||
| ]); | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed in b627c2a. The SELECT did leave a check-then-act window outside the batch. setPrimary is now a single conditional UPDATE: SET is_primary = CASE WHEN slug = ? THEN 1 ELSE 0 END guarded by an EXISTS on the same link/slug pair, so the membership check and the handover execute atomically. A non-member or deleted slug matches zero rows and leaves primary flags untouched. Covered by the existing setPrimary tests (set/clear, foreign slug no-op, nonexistent slug no-op); full suite passes.
The membership SELECT ran outside the transactional batch, so a slug deleted between the check and the batch could still clear every primary flag without setting a new one. A single UPDATE with an EXISTS guard performs the check and the handover atomically.
Comment on lines
+25
to
+30
| function validateExpiresAt(expiresAt: unknown): string | null { | ||
| if (expiresAt === undefined || expiresAt === null) return null; | ||
| if (typeof expiresAt !== "number" || !Number.isInteger(expiresAt) || expiresAt < 0) { | ||
| return "expires_at must be a nonnegative integer Unix timestamp"; | ||
| } | ||
| return null; |
Member
Author
There was a problem hiding this comment.
Fixed in 33e2804, taking option (b). Option (a) would tighten the service validator past the public zod schema (nonnegative), and changing that schema is a spec change that stales all three SDK hashes, so the redirect check now honors the documented contract instead: null means no expiry, any stored number is a real timestamp. redirect.ts and the two admin-page checks (links.tsx expired filter, link-detail.tsx expired badge and expiry control guard) are null-aware (!= null), so expires_at = 0 404s as expired since epoch. Regression test added in expiration-flow.test.ts (fails on the old truthy check); full suite passes.
The Link schema documents null as "no expiry" and accepts any nonnegative integer, but the redirect handler and the admin pages used truthy checks, so a stored 0 (epoch) never expired and the detail page hid its expiry control. All three checks are now null-aware. The JSX guard on the detail page also no longer renders a stray "0".
Comment on lines
+114
to
121
| // A corrupted or out-of-bounds stored value must not poison link creation | ||
| // or the settings page; fall back to the hardcoded default instead. | ||
| const parsedSlugLength = parseInt(slugLength ?? String(DEFAULT_SLUG_LENGTH), 10); | ||
| return ok({ | ||
| slug_default_length: parseInt(slugLength ?? String(DEFAULT_SLUG_LENGTH), 10), | ||
| slug_default_length: validateSlugLength(parsedSlugLength) === null ? parsedSlugLength : DEFAULT_SLUG_LENGTH, | ||
| theme: theme ?? null, | ||
| lang: lang ?? null, | ||
| default_range: isValidRange(defaultRange) ? defaultRange : DEFAULT_TIMELINE_RANGE, |
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.
Summary
Full-application robustness audit. Six focused fixes, no new features, no public API surface change (OpenAPI spec and SDK hashes untouched). Every fix landed test-first; the suite grew from 901 to 922 tests, all passing.
Fixes
get_link_qrtool appended a bare?qrparameter, but the redirect handler only recognizesutm_medium=qr. Scans recorded as plain link clicks. Now aligned with the REST QR endpoint.themeandlangnow validate against the known sets (oddbit|dark|light,en|id|sv), API key titles cap at 120 characters, and a corrupted storedslug_default_lengthfalls back to the default instead of returning NaN or blocking link creation.SlugRepository.setPrimary/disable/remove/addCustomandLinkRepository.deleteran sequential statements; a mid-sequence failure could strand a link with no primary slug or leave orphan slug and click rows. All now run in transactional D1 batches.setPrimaryalso verifies slug membership first; previously a mismatched slug cleared every primary flag on the link and set none.expires_atwas stored verbatim and broke the numeric expiry comparison in the redirect handler.createLink/updateLinknow reject non-string labels and non-integer or negative expiries at the service layer.getBundleignored the viewer's bot/self-referrer filter preferences whilelistBundlesapplied them, so the bundle card andget_bundleMCP tool showed different numbers than the list. Now consistent./_/admin/api/*requests without a session were 302-redirected to the landing page; fetch() callers followed the redirect and received HTML. They now get 401 JSON. Page routes keep the redirect. Theme/lang cookie values are also clamped to known sets before rendering.Verification
yarn test: 50 files, 922 tests passingnpx tsc --noEmit: cleansrc/api/router.ts,src/api/schemas.ts, or resource sub-apps: spec hash and SDKs unaffected🤖 Generated with Claude Code