Skip to content

Harden draft publishing surface: secure rendering, PIN flow, lifecycle controls, and abuse limits - #3

Merged
dkritarth merged 4 commits into
masterfrom
copilot/fix-xss-pin-leak-unbounded-storage-rate-limiting
Jul 19, 2026
Merged

dkritarth merged 4 commits into
masterfrom
copilot/fix-xss-pin-leak-unbounded-storage-rate-limiting

Conversation

Copilot AI commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor

FreeFlow’s worker/CLI publish path had multiple security and robustness gaps: draft HTML was served with weak browser protections, PINs leaked via URL/plaintext storage, and uploads lacked lifecycle/abuse controls. This change tightens draft access semantics and adds bounded, authenticated operations for safer self-hosted publishing.

  • Worker response hardening

    • Added strict draft response headers:
      • Content-Security-Policy (default-src 'none', script-src 'none', frame-ancestors 'none', etc.)
      • X-Frame-Options: DENY
      • X-Content-Type-Options: nosniff
      • Referrer-Policy: no-referrer
  • PIN flow + secret handling

    • Removed PIN unlock via ?key=... query parameter.
    • Added unlock via POST /d/:id form submission and x-draft-pin header.
    • New uploads store PIN as salted SHA-256 (pinHash + pinSalt) instead of plaintext.
    • Kept legacy plaintext PIN verification path for backward compatibility with previously stored drafts.
  • Draft lifecycle controls

    • Added optional upload TTL (ttlSeconds) with bounds validation and KV expirationTtl.
    • Added authenticated DELETE /d/:id endpoint (same bearer auth as upload) to remove drafts on demand.
  • Abuse/robustness controls

    • Enforced explicit upload payload cap (2 MiB) with 413 response on overflow.
    • Added per-IP KV-backed /upload rate limiting (configurable via UPLOAD_RATE_LIMIT_PER_MINUTE, default 30/min).
    • Increased draft ID entropy from 8 to 16 hex chars.
    • Replaced silent top-level 500s with logged error IDs and structured JSON error response.
  • CLI guardrail

    • Added local preflight file-size check (2 MiB) before reading/posting HTML to fail early with a clear message.
// New worker unlock flow (no PIN in URL)
if ((request.method === "GET" || request.method === "POST") && url.pathname.startsWith("/d/")) {
  return resolveAndRenderDraft(request, env);
}

Copilot AI changed the title [WIP] Harden worker: fix XSS, PIN leak, unbounded storage, no rate limiting Harden draft publishing surface: secure rendering, PIN flow, lifecycle controls, and abuse limits Jul 19, 2026
Copilot AI requested a review from dkritarth July 19, 2026 07:31
@dkritarth

Copy link
Copy Markdown
Owner

@copilot please address the following before merge:

Spec gaps (issue #2):

  1. Size check in handleUpload runs after await request.json() fully parses the body — a 2MB+ payload is still buffered/parsed before the 413 rejection. Check size earlier (e.g. Content-Length header, or check byte length before/while parsing) so oversized payloads are rejected without full parse.
  2. CSP's style-src 'unsafe-inline' contradicts the issue's explicit ask for "script/style restrictive." Tighten this unless there's a concrete reason inline styles must be allowed.
  3. Rate limiting is keyed by x-forwarded-for first, cf-connecting-ip as fallback — x-forwarded-for is attacker-controlled and spoofable. Use cf-connecting-ip first (trusted, set by Cloudflare) and drop or deprioritize x-forwarded-for.

Architecture:
4. Rate-limit counters (rl:<ip>:<bucket>) are stored in the same HTML_DRAFTS KV namespace as drafts. Mixing data domains in one namespace is fragile (e.g. a GET /d/rl:1.2.3.4:12345 request tries to serve the counter as a draft). Use a separate KV namespace/binding for rate limiting, or prefix clearly and document the shared namespace is intentional.

Minor cleanup:
5. secureHtmlHeaders(overrides = {}) — no call site passes overrides. Drop the unused parameter until something needs it.
6. pinHash/pinSalt (plus the legacy plaintext pin fallback) travel together across createPinPayload/verifyDraftPin/resolveAndRenderDraft — consider bundling into one small helper/type instead of three loose fields.

Please push fixes to this PR branch.

Copilot AI commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please address the following before merge:

Spec gaps (issue #2):...

Addressed in commit 71aac47.

Implemented the requested fixes:

  • Added early upload body rejection before JSON parsing using Content-Length precheck + bounded body reader.
  • Tightened CSP (style-src 'none') and removed inline styles from worker-rendered lock/not-found pages.
  • Rate limiting now prioritizes cf-connecting-ip over x-forwarded-for.
  • Added dedicated rate-limit KV support via UPLOAD_RATE_LIMITS binding, with prefixed fallback and draft-id validation guardrails to prevent /d/... serving counter keys.
  • Removed unused secureHtmlHeaders override parameter.
  • Bundled PIN storage/verification via pin object ({ hash, salt }) and centralized helper while keeping legacy compatibility (pinHash/pinSalt and plaintext pin).

@dkritarth

Copy link
Copy Markdown
Owner

@copilot good progress — all 6 points from the last review are fixed (early size check, tightened CSP, correct IP precedence, bundled PIN fields, dropped unused param, separated rate-limit store).

One new, more serious issue found by checking the CSP against this repo's own flagship template:

CSP breaks the product's core use case. DRAFT_SECURITY_HEADERS now sets script-src 'none' and style-src 'none' on every served draft. But templates/plan-template.html — the example template this whole product is built around — loads Tailwind via <script src="https://cdn.jsdelivr.net/npm/@tailwindcss/browser@4">, Font Awesome via <link rel="stylesheet" href="...">, and Lucide via another <script src>. Under the new CSP:

  • The Tailwind script won't execute → no styling at all.
  • The Font Awesome <link> stylesheet is blocked by style-src 'none'.
  • The Lucide icon script won't execute.

So the hardening fix, as written, renders every published draft that uses CDN-loaded CSS/JS (which is the expected case for "AI-generated HTML plans/mockups" per the README) completely unstyled and non-interactive. XSS protection at the cost of breaking the product's stated purpose isn't a fix, it's a tradeoff that needs a decision, not a silent default.

Options to reconcile:

  1. Allow specific trusted CDN origins in script-src/style-src (e.g. cdn.jsdelivr.net, cdnjs.cloudflare.com, unpkg.com) rather than blocking all scripts/styles outright — this still blocks inline <script> injection (the actual XSS vector) while letting legitimate CDN assets load.
  2. Keep script-src 'none' (fully static output only) but document this as an intentional product constraint, and update templates/plan-template.html + README to stop relying on CDN scripts, since they'll never execute once published.

Please pick one and implement it — silently shipping a CSP that breaks the example template isn't acceptable to merge as-is.

Also: getRateLimitStore falls back to env.HTML_DRAFTS when UPLOAD_RATE_LIMITS isn't bound — but wrangler.toml.example wasn't updated to add that binding, so on a real deploy following the docs, rate-limit keys still land in the same KV namespace as drafts (the exact issue this abstraction was meant to fix). Please add the UPLOAD_RATE_LIMITS binding to wrangler.toml.example and mention it in the deploy docs, or drop the fallback and require the binding explicitly.

…binding

The prior script-src/style-src 'none' policy blocked the repo's own
templates/plan-template.html, which loads Tailwind, Font Awesome, and
Lucide from CDN — every published draft using it rendered unstyled and
non-interactive. Inline <script> (the actual XSS vector) stays blocked;
a fixed CDN allowlist (jsdelivr, cdnjs, unpkg) covers legitimate assets.

Also documents the UPLOAD_RATE_LIMITS KV binding in wrangler.toml.example
so rate-limit counters land in their own namespace instead of silently
falling back to HTML_DRAFTS on a fresh deploy.
@dkritarth

Copy link
Copy Markdown
Owner

Copilot's last push (71aac47) fixed all 6 points from the first review round: early size-check before JSON parse, cf-connecting-ip checked before spoofable x-forwarded-for, PIN fields bundled into one object, unused overrides param dropped, and rate-limit store separated behind getRateLimitStore.

Two blockers remained from the second round — fixed directly in 4567c75:

  1. CSP broke the product. script-src 'none' / style-src 'none' blocked templates/plan-template.html's CDN-loaded Tailwind/Font Awesome/Lucide — every published draft using them would render unstyled and non-interactive. Replaced with a fixed CDN allowlist (jsdelivr, cdnjs, unpkg) for script-src/style-src/font-src/connect-src. Inline <script> — the actual XSS vector — stays blocked; style-src keeps 'unsafe-inline' since Tailwind's CDN build injects its generated CSS as an inline <style> tag at runtime (inline CSS can't execute script, so this is a narrower, deliberate tradeoff vs. allowing inline script).
  2. Rate-limit KV binding was undocumented. getRateLimitStore preferred env.UPLOAD_RATE_LIMITS but wrangler.toml.example never declared it, so a fresh deploy following the docs would still silently mix rate-limit counters into the HTML_DRAFTS namespace — the exact bug the abstraction was meant to fix. Added the binding to wrangler.toml.example and the README's Quick Start KV-creation steps.

All 8 findings from issue #2 are now addressed and verified against the repo's own template. Merging.

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.

Harden worker: fix XSS, PIN leak, unbounded storage, no rate limiting

2 participants