Skip to content

feat: backend-worker#2

Merged
m6un merged 3 commits intomainfrom
agent/backend-worker
Mar 1, 2026
Merged

feat: backend-worker#2
m6un merged 3 commits intomainfrom
agent/backend-worker

Conversation

@m6un
Copy link
Copy Markdown
Owner

@m6un m6un commented Feb 28, 2026

Summary

Built by agent per spec: docs/exec-plans/active/backend-worker.md

Cloudflare Worker with POST /save and GET /{uuid}. TypeScript strict, Wrangler v3, full test coverage. Layer linter passes.

@m6un
Copy link
Copy Markdown
Owner Author

m6un commented Mar 1, 2026

@claude

@claude
Copy link
Copy Markdown

claude bot commented Mar 1, 2026

Claude finished @m6un's task in 1m 52s —— View job


PR Review

  • Gathering context
  • Reading changed files
  • Checking against spec and invariants
  • Writing review feedback

Overall the implementation is clean and well-structured. Spec compliance is solid — all acceptance criteria are met, the layer rule (KV only through store.ts) is respected, TTL is hardcoded at 3600, and index.ts is routing-only. Two issues worth fixing before merge, plus a few minor notes.


Issues

1. CORS headers missing on 404 responses — bug

GET /{uuid} error responses don't carry CORS headers, but POST /save error responses do. This inconsistency means an iOS URLSession or WKWebView cross-origin request that gets a 404 could be blocked depending on how CF handles it.

backend/src/handlers/get.ts:37:

return new Response("Not Found", { status: 404 });

backend/src/index.ts:34:

return new Response("Not Found", { status: 404 });

Both should add "Access-Control-Allow-Origin": "*" as the save handler does. Fix this →

2. Unescaped HTML content in <article> — potential XSS

backend/src/handlers/get.ts:22:

<article>${content}</article>

title is escaped via escapeHtml() but content is injected raw. If content is trusted HTML from the iOS client (which it is — parsed from WKWebView), this is intentional. But since the backend has no auth and POST /save is publicly reachable, any caller can inject arbitrary HTML/JS into the served page. Instapaper parses the HTML, so script tags may not execute there, but a served URL pointing at piper.workers.dev would execute scripts in any browser. Worth an explicit comment in the code acknowledging this is intentional, or adding a Content-Security-Policy header to block scripts on GET responses. Fix this →


Minor notes

Env interface is duplicated across index.ts, handlers/get.ts, and handlers/save.ts. Not a correctness issue but any future binding addition requires three edits. Consider a shared types.ts. Low priority.

savedAt is stored in KV but never read. The StoredContent interface includes savedAt, the store writes it, but handleGet never surfaces it. It's harmless metadata but worth knowing it's unused.

POST /save returns 200 on success. 201 Created would be more semantically accurate since a new resource is being created. The spec doesn't mandate it, so this is cosmetic.

Test UUID pattern is loose ([0-9a-f-]{36}). This matches any 36-character hex-and-dash string, not strictly a v4 UUID. For a test it's fine, but a tighter regex like /[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}/ would give higher confidence.


Spec / invariant compliance

Criterion Status
POST /save returns { url } with UUID
POST /save 400 on missing fields
GET /{uuid} returns clean HTML
GET /{uuid} 404 on unknown/expired
expirationTtl: 3600 hardcoded
index.ts routing only
All KV access via store.ts
Logs contain UUID/timestamp only
No auth
TTL not configurable

The two CORS and CSP issues are the only items I'd want resolved before merging. The rest is fine to land as-is or address in follow-up.

m6un and others added 2 commits March 1, 2026 17:01
…uid}

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@m6un m6un force-pushed the agent/backend-worker branch from e35ed71 to 979599d Compare March 1, 2026 11:31
Comment on lines +21 to +30
if (
typeof body !== "object" ||
body === null ||
!("title" in body) ||
!("content" in body) ||
typeof (body as Record<string, unknown>).title !== "string" ||
typeof (body as Record<string, unknown>).content !== "string" ||
((body as Record<string, unknown>).title as string).trim() === "" ||
((body as Record<string, unknown>).content as string).trim() === ""
) {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This long if loop is un-readable. Clean it up.

@m6un
Copy link
Copy Markdown
Owner Author

m6un commented Mar 1, 2026

@claude /fix-review 2

@claude
Copy link
Copy Markdown

claude bot commented Mar 1, 2026

Claude finished @m6un's task in 3m 3s —— View job


Review Comments Addressed

  • Gathering context
  • Reading review comments and current code
  • Fixed save.ts: extracted validation logic into isValidPayload() type guard — replaces the 8-condition if block with a clean, named function
  • Fixed get.ts: added Access-Control-Allow-Origin: * to 404 response; added Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline' to 200 response
  • Fixed index.ts: 404 fallback now uses the existing CORS_HEADERS constant
  • Verified changes look correct
  • Pushed to agent/backend-worker

Summary of changes

File Change
backend/src/handlers/save.ts Extracted isValidPayload() type guard — the 8-line if condition is now a named function. TypeScript now narrows the type after the guard, removing all the casts too.
backend/src/handlers/get.ts 404 now returns Access-Control-Allow-Origin: *. 200 response now includes Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline' to block script execution on the served page.
backend/src/index.ts Fallback 404 now spreads CORS_HEADERS (already defined in the file) instead of returning bare headers.

The store.ts comment (explaining the repository pattern) was already answered in a previous reply — no code change needed.
· Branch

… headers

Co-authored-by: Midhun Chandran <m6un@users.noreply.github.com>
@m6un m6un merged commit 729b791 into main Mar 1, 2026
3 checks passed
@m6un m6un deleted the agent/backend-worker branch March 1, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant