fix: 7 defects found in weekly review (bundle TOCTOU, redirect 500, Python/Dart SDK bugs)#29
Conversation
…delete
BundleService.deleteBundle discarded the boolean result of
BundleRepository.delete, always returning ok({deleted:true}) even when
no row was affected. updateBundle, archiveBundle, and unarchiveBundle
used non-null assertions on repository returns that can be null when the
row disappears between the existence check and the mutation (TOCTOU on a
concurrent DELETE). Each now checks the return value and returns
fail(404, "Bundle not found") on null/false.
Regression tests added in bundle-service.test.ts for all four paths,
using vi.spyOn to simulate the concurrent-delete race condition.
The redirect handler called new URL(entry.url) without a try/catch. A corrupted or legacy KV entry with a non-parseable URL string caused an uncaught TypeError, surfacing as a 500. Wrapped in try/catch and returns notFoundResponse() on parse failure. Regression test added to redirect-kv.test.ts to verify the handler returns 404 for a KV entry with url "not-a-valid-url".
_base.py: parse_json_response returned response.json() bare on 2xx. An HTML error page or truncated response on a 200 caused an unhandled JSONDecodeError. Wrapped in try/except; raises ShrtnrError(status) so callers see a typed error instead of a raw exception. links.py: LinksResource.qr() and AsyncLinksResource.qr() accepted size as str | None even though the API and all other SDKs treat it as int. Added int | None type and converts to str for the query param. Regression tests added: test_non_json_2xx_raises_shrtnr_error, test_links_qr_size_accepts_int, test_links_qr_size_omitted_when_none.
…ccent Bundle.fromJson and BundleWithSummary.fromJson cast json['accent'] as String (non-nullable). When a bundle was created before the accent field was introduced, or when the API omits the field, the cast threw a TypeError at runtime. Changed to (json['accent'] as String?) ?? 'orange' to match the server default. Regression tests added for both classes: accent absent from map and accent explicitly null, both expect BundleAccent.orange. Dart SDK is not installed in CI for this environment; tests were written and verified structurally but not executed locally.
|
Flagged (not fixed): JWT issuer not validated —
The fix is straightforward — add Recommended action: add Generated by Claude Code |
|
Flagged (not fixed): raw string interpolation in
// src/db/filters.ts ~line 47
`... WHERE click_ts >= ${sinceTs} ...`The value comes from a controlled enum translation so actual injection is low-probability, but a bound parameter would eliminate the risk with no downside. Worth a one-line fix when the file is next touched. Generated by Claude Code |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 2b46d69 | Jul 03 2026, 06:24 AM |
There was a problem hiding this comment.
Pull request overview
This pull request fixes several correctness issues across the Worker service layer and multiple SDKs, primarily around race-safety, defensive redirect handling, and stricter/accurate client parsing and typing.
Changes:
- Make bundle update/archive/unarchive/delete return
404when repository operations returnnull/false(concurrent deletion / TOCTOU hardening) and add Vitest regression coverage. - Prevent redirect handler crashes on malformed cached URLs by guarding
new URL()and returning404, with a regression test. - Improve SDK robustness and correctness: Python wraps invalid JSON on 2xx into
ShrtnrErrorand fixesqr(size)typing; Dart bundle model factories now tolerate missing/nullaccentwith a default and add tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/bundle-management.ts | Propagates null/false repository results as 404 for race-safe bundle mutations. |
| src/redirect.ts | Guards URL parsing to avoid uncaught exceptions and return 404 for malformed entries. |
| src/tests/service/bundle-service.test.ts | Adds regression tests for concurrent-delete / TOCTOU scenarios in bundle service methods. |
| src/tests/handler/redirect-kv.test.ts | Adds regression test ensuring malformed KV URLs don’t produce a 500. |
| sdk/python/src/shrtnr/_base.py | Wraps invalid JSON on successful responses into ShrtnrError for consistent error typing. |
| sdk/python/src/shrtnr/resources/links.py | Corrects qr(size) typing to int and ensures query encoding is stringified. |
| sdk/python/tests/test_client.py | Adds regression tests for Python JSON parsing behavior and qr(size) parameter handling. |
| sdk/dart/lib/src/models.dart | Defaults bundle accent when missing/null to prevent runtime cast errors. |
| sdk/dart/test/client_test.dart | Adds regression tests covering missing/null accent deserialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The BundleAccent enum group had a test named "Bundle.fromJson throws when accent is absent" that expected a TypeError. That test documented the bug, not the intended behavior. Updated to assert the correct outcome after the fix: accent defaults to BundleAccent.orange.
Weekly defect-hunting review of the app, API, and all three SDKs. Seven confirmed bugs fixed, each with a regression test. Two items flagged as PR comments for developer judgment (not committed).
All 1047 app tests pass. All 94 Python SDK tests pass. Dart SDK tests written but not verified locally (no
dartbinary in CI environment).Fix 1: Bundle service TOCTOU — null/false return values not checked
File:
src/services/bundle-management.tsdeleteBundlediscarded thebooleanreturned byBundleRepository.delete, always callingreturn ok({ deleted: true })even when no row was found.updateBundle,archiveBundle, andunarchiveBundleall used non-null assertions (!) on repository returns that becomenullwhen a concurrent DELETE races between the fetch and the write. Each now captures the return value and callsreturn fail(404, "Bundle not found")onnull/false.Impact: A request to delete/update/archive/unarchive a bundle that was concurrently deleted returns
okwith a stale or invented body instead of404. Under non-concurrent conditions the assertions are vacuously safe, but a simple race between two admin users triggers them.Regression tests: Four new tests in
src/__tests__/service/bundle-service.test.ts, each usingvi.spyOn(BundleRepository, "<method>").mockResolvedValueOnce(false/null)to simulate the race.Fix 2: Redirect handler — unguarded
new URL()crashes with 500File:
src/redirect.tsThe redirect handler called
new URL(entry.url)without atry/catch. A corrupted or legacy KV entry with a non-parseable URL string throws aTypeErrorthat was uncaught, surfacing as an unhandled 500. Wrapped intry/catch; returnsnotFoundResponse()on parse failure.Regression test:
redirect-kv.test.ts— "malformed URL in KV cache returns 404 instead of 500".Fix 3: Python SDK —
parse_json_responsedoes not wrapJSONDecodeErroron 2xxFile:
sdk/python/src/shrtnr/_base.pyparse_json_responsereturnedresponse.json()bare on success paths. An HTML error page or truncated body on a 200 (CDN edge, misconfigured proxy) raised a rawJSONDecodeErrorinstead of a typedShrtnrError. Wrapped intry/except; raisesShrtnrError(response.status_code, ...)so callers see a consistent exception type.Regression test:
test_non_json_2xx_raises_shrtnr_error— mocks a 200 returning<html>Bad Gateway</html>and assertsShrtnrErroris raised withstatus == 200.Fix 4: Python SDK —
LinksResource.qr()sizeparameter typedstrinstead ofintFile:
sdk/python/src/shrtnr/resources/links.pyBoth
LinksResource.qr()(sync, line 174) andAsyncLinksResource.qr()(async, line 330) declaredsize: str | None. The OpenAPI spec definessizeas an integer; the TypeScript and Dart SDKs both acceptint. Passing an integer worked accidentally (str conversion via query encoding), but type checkers flagged it and the docstring was wrong. Changed toint | None;str(size)applied when building the query param.Regression tests:
test_links_qr_size_accepts_intandtest_links_qr_size_omitted_when_none.Fix 5: Dart SDK —
Bundle.fromJson/BundleWithSummary.fromJsoncrash on nullaccentFile:
sdk/dart/lib/src/models.dartBoth factories cast
json['accent'] as String(non-nullable). When a bundle was created before the accent field existed, or when the API omits it (nullable in schema), the cast throws aTypeErrorat runtime. Changed to(json['accent'] as String?) ?? 'orange'to match the server-side default.Regression tests: Four new tests in
sdk/dart/test/client_test.dart—Bundle.fromJsonandBundleWithSummary.fromJson, each with accent absent and accent explicitly null, both expectBundleAccent.orange.Flagged items (not committed — need developer judgment)
These were noted during review but not fixed. Added as PR review comments on the relevant lines.
1. JWT issuer not validated (
src/access.ts) —jwtVerifyis called without anissueroption. A token signed by a different Cloudflare Access application (same account, differentaud) passes the audience check but should be rejected. Fix requires knowing the expected issuer URL for each deployment, so left for the developer.2.
slugClickCountSqlinterpolatessinceTsdirectly into SQL (src/db/filters.ts) — ThesinceTsvalue comes from a controlled enum translation so injection is low-probability, but it is a raw string interpolation. A parameterized query would eliminate the risk without loss of readability.Generated by Claude Code