security sweep
This commit is contained in:
45
tasks/security-fixes/01-fix-stored-xss-blog-rendering.md
Normal file
45
tasks/security-fixes/01-fix-stored-xss-blog-rendering.md
Normal file
@@ -0,0 +1,45 @@
|
||||
# 01. Fix stored XSS via unsanitized innerHTML in blog rendering
|
||||
|
||||
meta:
|
||||
id: security-fixes-01
|
||||
feature: security-fixes
|
||||
priority: P0
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, high-severity]
|
||||
|
||||
objective:
|
||||
- Eliminate stored XSS in blog post rendering by replacing raw innerHTML with a sanitization pipeline
|
||||
|
||||
deliverables:
|
||||
- Replace `contentToHtml()` in `web/src/routes/blog/[slug].tsx` with a safe HTML rendering approach
|
||||
- Add DOMPurify (or equivalent) as a dependency to sanitize HTML before innerHTML binding
|
||||
- Unit tests for the sanitization pipeline covering script injection, event handler injection, and data URI vectors
|
||||
|
||||
steps:
|
||||
1. Install `dompurify` and `isomorphic-dompurify` (for SSR compatibility) in the web app
|
||||
2. Examine `contentToHtml()` at `web/src/routes/blog/[slug].tsx:14-46` and the innerHTML binding at line 121
|
||||
3. Create a `sanitizeHtml(content: string): string` utility that runs DOMPurify on the rendered HTML
|
||||
4. Replace the innerHTML binding with `innerHTML={sanitizeHtml(contentToHtml(post.content))}`
|
||||
5. Consider replacing the custom markdown-to-HTML parser with a library (e.g., `marked` + DOMPurify) if the custom implementation is fragile
|
||||
6. Add unit tests covering XSS vectors: `<script>`, `onerror=`, `javascript:`, `data:text/html`
|
||||
|
||||
tests:
|
||||
- Unit: `sanitizeHtml()` strips `<script>` tags, event handlers (`onclick`, `onerror`, etc.), `javascript:` URIs, and `data:text/html` URIs
|
||||
- Unit: `sanitizeHtml()` preserves legitimate HTML (headings, paragraphs, links, lists, code blocks)
|
||||
- Integration: Blog post with embedded script renders without executing JavaScript (verify via headless test or DOM inspection)
|
||||
|
||||
acceptance_criteria:
|
||||
- innerHTML is never bound with unsanitized content
|
||||
- DOMPurify (or equivalent) is called on all HTML before it reaches the DOM
|
||||
- Unit tests pass for all XSS vector categories (script, event handlers, data URIs, javascript: URIs)
|
||||
- Legitimate blog formatting (headings, links, bold, italic, code) still renders correctly
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Manually create a blog post with `<script>alert(1)</script>` content and verify it does not execute
|
||||
- Review the rendered HTML output to confirm sanitization is applied
|
||||
|
||||
notes:
|
||||
- Finding p8-001 is the only HIGH severity finding — prioritize this first
|
||||
- `isomorphic-dompurify` is needed because DOMPurify requires a DOM environment (not available in Node SSR)
|
||||
- If SolidStart supports client-only rendering for this route, standard DOMPurify suffices
|
||||
50
tasks/security-fixes/02-fix-puppeteer-ssrf-report-gen.md
Normal file
50
tasks/security-fixes/02-fix-puppeteer-ssrf-report-gen.md
Normal file
@@ -0,0 +1,50 @@
|
||||
# 02. Fix SSRF via Puppeteer --no-sandbox in report generation
|
||||
|
||||
meta:
|
||||
id: security-fixes-02
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Prevent SSRF and local file read in the PDF report generator by sandboxing Puppeteer network access
|
||||
|
||||
deliverables:
|
||||
- Add request interception in Puppeteer to block dangerous URL schemes (file://, metadata endpoints, internal IPs)
|
||||
- Remove or mitigate the `--no-sandbox` flag where possible
|
||||
- Add tests verifying that blocked URLs are not accessible
|
||||
|
||||
steps:
|
||||
1. Examine `generatePDF()` at `web/src/server/services/reports/generator.ts:141-150` and `compileData()` at lines 53-137
|
||||
2. Add `page.setRequestInterception(true)` before `page.setContent()` to intercept all network requests
|
||||
3. Implement a request filter that blocks:
|
||||
- `file://` scheme (local file read)
|
||||
- Cloud metadata endpoints (`169.254.169.254`, `metadata.google.internal`, etc.)
|
||||
- Internal IP ranges (`10.x.x.x`, `172.16-31.x.x`, `192.168.x.x`, `127.x.x.x`)
|
||||
- `data:` URIs that could load arbitrary content
|
||||
4. If `--no-sandbox` is required by the deployment environment (e.g., Docker), document the risk and add a compensating control (Chrome flags, network namespace isolation)
|
||||
5. Add unit tests for the request interception filter
|
||||
|
||||
tests:
|
||||
- Unit: Request interception blocks `file://`, `data:`, internal IPs, and cloud metadata endpoints
|
||||
- Unit: Request interception allows legitimate external URLs (CDN assets, fonts, etc.)
|
||||
- Integration: Attempting to load a report with embedded `file:///etc/passwd` does not succeed
|
||||
- Integration: Report generation still produces valid PDFs for legitimate content
|
||||
|
||||
acceptance_criteria:
|
||||
- Puppeteer page cannot make network requests to blocked URL schemes or internal IPs
|
||||
- `file://` URLs are blocked, preventing local file read
|
||||
- Cloud metadata endpoints are blocked
|
||||
- Report PDFs still render correctly for legitimate content
|
||||
- The `--no-sandbox` flag is either removed or has documented compensating controls
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Attempt to inject a `file:///etc/passwd` URL through report data and verify it is blocked
|
||||
- Verify report generation still produces valid PDFs
|
||||
|
||||
notes:
|
||||
- Finding p8-002: The `--no-sandbox` flag is likely required in containerized environments; if so, network-level sandboxing via request interception is the compensating control
|
||||
- Consider using `page.setContent(html, {waitUntil: 'networkidle0'})` with interception enabled
|
||||
- The `compileData()` function builds HTML from database data — any user-controlled data in reports could include `<img src="file://...">` or `<link href="http://169.254.169.254/...">`
|
||||
@@ -0,0 +1,50 @@
|
||||
# 03. Fix open redirect via unvalidated return URL in Stripe checkout
|
||||
|
||||
meta:
|
||||
id: security-fixes-03
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Prevent open redirect attacks by validating Stripe checkout return URLs against a trusted domain allowlist
|
||||
|
||||
deliverables:
|
||||
- Domain allowlist validation for `returnUrl` in billing schemas and router handlers
|
||||
- Unit tests for URL validation covering phishing domains, URL encoding tricks, and protocol-relative URLs
|
||||
- Updated schema and router code
|
||||
|
||||
steps:
|
||||
1. Examine `CreateCheckoutSessionSchema` at `web/src/server/api/schemas/billing.ts:4-6` and router usage at `web/src/server/api/routers/billing.ts:43-54, 68-75`
|
||||
2. Create a `validateReturnUrl(url: string): boolean` function that:
|
||||
- Parses the URL and extracts the hostname
|
||||
- Checks the hostname against an allowlist of trusted domains (e.g., `*.kordant.com`, `localhost` for dev)
|
||||
- Rejects protocol-relative URLs (`//evil.com`) and encoded redirects
|
||||
3. Replace `string([url()])` with a custom valibot transformer that calls `validateReturnUrl`
|
||||
4. Apply the same validation to the billing portal schema (if separate)
|
||||
5. Add error handling that returns a clear error to the client when the URL is invalid
|
||||
|
||||
tests:
|
||||
- Unit: `validateReturnUrl` accepts `https://app.kordant.com/success` (trusted domain)
|
||||
- Unit: `validateReturnUrl` rejects `https://evil.com/phishing` (untrusted domain)
|
||||
- Unit: `validateReturnUrl` rejects `//evil.com` (protocol-relative URL)
|
||||
- Unit: `validateReturnUrl` rejects `https://kordant.com.evil.com` (subdomain spoofing)
|
||||
- Unit: `validateReturnUrl` rejects URL-encoded redirects (`%2F%2Fevil.com`)
|
||||
- Integration: tRPC checkout endpoint rejects untrusted return URLs with a validation error
|
||||
|
||||
acceptance_criteria:
|
||||
- Return URLs are validated against a domain allowlist before being passed to Stripe
|
||||
- Untrusted domains are rejected with a clear validation error
|
||||
- URL encoding tricks and subdomain spoofing are detected
|
||||
- Legitimate return URLs to trusted domains still work
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Attempt checkout with return URL `https://evil.com` and verify it is rejected
|
||||
- Verify checkout with `https://app.kordant.com/success` still works
|
||||
|
||||
notes:
|
||||
- Finding p8-003: The `url()` validator only checks format, not domain trust
|
||||
- Consider making the allowlist configurable via environment variables (`ALLOWED_RETURN_DOMAINS`)
|
||||
- The return URL includes the Stripe session ID, so attackers could use it for session fixation
|
||||
54
tasks/security-fixes/04-fix-rate-limit-substring-bypass.md
Normal file
54
tasks/security-fixes/04-fix-rate-limit-substring-bypass.md
Normal file
@@ -0,0 +1,54 @@
|
||||
# 04. Fix rate limit bypass via incomplete sensitive path list
|
||||
|
||||
meta:
|
||||
id: security-fixes-04
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Prevent resource exhaustion by replacing substring-based rate limiting with exact procedure name matching and a complete sensitive operation list
|
||||
|
||||
deliverables:
|
||||
- Updated rate limiter in `web/src/server/api/utils.ts` using exact procedure name matching
|
||||
- Expanded sensitive procedure list covering darkwatch, voiceprint, and other expensive operations
|
||||
- Unit tests for rate limit matching logic
|
||||
|
||||
steps:
|
||||
1. Examine the rate limiter at `web/src/server/api/utils.ts:35-38` (substring matching on `sensitivePaths`)
|
||||
2. Review the tRPC router definitions to identify all expensive procedures:
|
||||
- `darkwatch.runScan` (external API calls: HIBP, SecurityTrails, Censys, Shodan)
|
||||
- `voiceprint.analyzeAudio` (300MB+ memory per request)
|
||||
- Any other CPU/memory/network-intensive procedures
|
||||
3. Replace `sensitivePaths.some(p => path.includes(p))` with exact procedure name matching using a `Set` of full procedure paths (e.g., `darkwatch.runScan`)
|
||||
4. Define appropriate rate limits per procedure category:
|
||||
- Auth operations: 3/hr (existing)
|
||||
- Darkwatch scans: 5/hr (expensive external API calls)
|
||||
- VoicePrint analysis: 10/hr (high memory usage)
|
||||
- Default protected: 100/min (existing)
|
||||
5. Update `protectedProcedure` configuration at `web/src/server/api/utils.ts:23-28` if needed
|
||||
|
||||
tests:
|
||||
- Unit: Exact procedure name matching correctly identifies `darkwatch.runScan` as sensitive
|
||||
- Unit: Substring attacks like `darkwatch.runScanLike` do not trigger sensitive tier
|
||||
- Unit: `voiceprint.analyzeAudio` is classified as sensitive
|
||||
- Unit: Auth procedures (`login`, `signup`) still match the sensitive tier
|
||||
- Integration: Rapid fire requests to `darkwatch.runScan` are rate-limited at the sensitive tier
|
||||
|
||||
acceptance_criteria:
|
||||
- Rate limiter uses exact procedure name matching (not substring)
|
||||
- All expensive procedures are in the sensitive operation list
|
||||
- Different sensitive procedures can have different rate limits
|
||||
- Existing rate limits for auth operations are preserved
|
||||
- No false positives from substring matching
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Send rapid requests to `darkwatch.runScan` and verify rate limiting kicks in at the sensitive tier
|
||||
- Verify `darkwatch.runScanLike` (non-existent) does not trigger sensitive tier
|
||||
|
||||
notes:
|
||||
- Finding p8-004: The current substring heuristic (`path.includes(p)`) is too broad and incomplete
|
||||
- Consider a tiered approach: `sensitive` (3/hr), `expensive` (10/hr), `standard` (100/min)
|
||||
- Review all router definitions to ensure no new expensive procedures are missed
|
||||
@@ -0,0 +1,51 @@
|
||||
# 05. Fix CORS origin trust from unvalidated APP_URL env var
|
||||
|
||||
meta:
|
||||
id: security-fixes-05
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Validate APP_URL before trusting it as a CORS origin to prevent arbitrary origin injection
|
||||
|
||||
deliverables:
|
||||
- APP_URL validation in CORS middleware at `web/src/middleware.ts`
|
||||
- Domain pattern validation for environment-sourced CORS origins
|
||||
- Unit tests for CORS origin validation
|
||||
|
||||
steps:
|
||||
1. Examine the CORS middleware at `web/src/middleware.ts:22-30`
|
||||
2. Create a `validateCorsOrigin(origin: string): boolean` function that:
|
||||
- Parses the origin URL and validates the hostname format
|
||||
- Rejects wildcard patterns (`*`), empty origins, and non-HTTP(S) schemes
|
||||
- Optionally checks against a known domain allowlist
|
||||
3. Add validation of `process.env.APP_URL` before adding it to `allowedOrigins`
|
||||
4. Add a fallback: if APP_URL is missing or invalid, log a warning and exclude it from allowed origins
|
||||
5. Consider adding a `VALID_CORS_ORIGINS` env var for explicit allowlist configuration
|
||||
|
||||
tests:
|
||||
- Unit: `validateCorsOrigin` accepts `https://app.kordant.com` (valid HTTPS origin)
|
||||
- Unit: `validateCorsOrigin` rejects `*` (wildcard)
|
||||
- Unit: `validateCorsOrigin` rejects `evil.com` (missing scheme)
|
||||
- Unit: `validateCorsOrigin` rejects empty string and whitespace
|
||||
- Unit: `validateCorsOrigin` rejects `http://localhost:9999` if not in the allowlist (configurable)
|
||||
- Integration: CORS middleware does not add invalid APP_URL to allowed origins
|
||||
|
||||
acceptance_criteria:
|
||||
- APP_URL is validated before being added to CORS allowed origins
|
||||
- Invalid or missing APP_URL is excluded with a warning logged
|
||||
- Wildcard origins are rejected
|
||||
- Legitimate HTTPS origins are accepted
|
||||
- No regression in existing CORS behavior for localhost and configured domains
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Set `APP_URL=evil.com` and verify it is not added to allowed origins
|
||||
- Set `APP_URL=https://app.kordant.com` and verify it is accepted
|
||||
|
||||
notes:
|
||||
- Finding p8-005: The current code trusts `process.env.APP_URL` without any validation
|
||||
- If APP_URL is set at deployment time (e.g., in a Docker env file), the risk is lower but still present if the deployment pipeline is compromised
|
||||
- Consider using a structured env var like `CORS_ORIGINS=https://app.kordant.com,https://admin.kordant.com` instead
|
||||
53
tasks/security-fixes/06-fix-webhook-type-coercion.md
Normal file
53
tasks/security-fixes/06-fix-webhook-type-coercion.md
Normal file
@@ -0,0 +1,53 @@
|
||||
# 06. Fix webhook type coercion bypassing TypeScript safety
|
||||
|
||||
meta:
|
||||
id: security-fixes-06
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Replace unsafe type coercion in the Stripe webhook handler with field-level validation using valibot schemas
|
||||
|
||||
deliverables:
|
||||
- Valibot schemas for Stripe webhook event data (subscription, invoice, checkout)
|
||||
- Updated `billing.service.ts` with validated data extraction instead of `as unknown as Record<string, unknown>`
|
||||
- Unit tests for webhook data validation including malformed payloads
|
||||
|
||||
steps:
|
||||
1. Examine webhook handler at `web/src/server/services/billing.service.ts:173, 196, 207` (type coercion points) and `updateSubscriptionInDB()` at lines 115-132
|
||||
2. Define valibot schemas for each webhook event type:
|
||||
- `CheckoutSessionCompletedSchema`: `subscription_id`, `customer_id`, `price_id`, etc.
|
||||
- `SubscriptionUpdatedSchema`: `current_period_start`, `current_period_end`, `status`, etc.
|
||||
- `InvoicePaidSchema`: `subscription_id`, `amount_paid`, etc.
|
||||
- `SubscriptionDeletedSchema`: `subscription_id`, `cancel_at_period_end`, etc.
|
||||
3. Replace `as unknown as Record<string, unknown>` casts with `safeParseEvent(event, schema)` that validates and returns typed data or an error
|
||||
4. Add error handling: log and return early if webhook data fails validation (Stripe may retry)
|
||||
5. Ensure `updateSubscriptionInDB()` receives only validated, typed data
|
||||
|
||||
tests:
|
||||
- Unit: Valid subscription event data passes validation and produces correct typed output
|
||||
- Unit: Missing `current_period_start` produces a validation error (not `NaN`)
|
||||
- Unit: Wrong type for `current_period_start` (string instead of number) produces a validation error
|
||||
- Unit: Empty event data produces a validation error
|
||||
- Unit: Extra unexpected fields do not cause the handler to crash
|
||||
- Integration: Webhook endpoint returns 200 for valid events and handles invalid events gracefully
|
||||
|
||||
acceptance_criteria:
|
||||
- No `as unknown as Record<string, unknown>` casts remain in the webhook handler
|
||||
- All webhook event data is validated against valibot schemas before use
|
||||
- Invalid data produces a logged error and early return (not stored in the database)
|
||||
- `NaN` dates and `undefined` fields are caught by validation
|
||||
- All existing webhook event types have corresponding schemas
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Send a malformed webhook event (missing required fields) and verify it is rejected
|
||||
- Send a valid webhook event and verify it is processed correctly
|
||||
|
||||
notes:
|
||||
- Finding p8-006: The chained `as unknown as` casts bypass TypeScript type safety
|
||||
- Stripe webhook events have a well-defined schema — use Stripe's TypeScript types as a reference
|
||||
- Consider using Stripe's `LineItem` and `Subscription` types to derive the valibot schemas
|
||||
- This fix is a prerequisite for task 07 (webhook replay dedup) since validated data shapes are needed
|
||||
57
tasks/security-fixes/07-fix-webhook-replay-missing-dedup.md
Normal file
57
tasks/security-fixes/07-fix-webhook-replay-missing-dedup.md
Normal file
@@ -0,0 +1,57 @@
|
||||
# 07. Fix webhook replay via missing event ID deduplication
|
||||
|
||||
meta:
|
||||
id: security-fixes-07
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: [security-fixes-06]
|
||||
tags: [implementation, tests-required, medium-severity, database-migration]
|
||||
|
||||
objective:
|
||||
- Prevent Stripe webhook replay attacks by implementing event ID deduplication for all event types
|
||||
|
||||
deliverables:
|
||||
- New database table for webhook event ID tracking (via Drizzle migration)
|
||||
- Updated webhook handler at `web/src/routes/api/stripe/webhook.ts` to check and record event IDs
|
||||
- Updated `billing.service.ts` to use the deduplication mechanism
|
||||
- Unit and integration tests for replay detection
|
||||
|
||||
steps:
|
||||
1. Create a Drizzle migration to add a `stripe_webhook_events` table:
|
||||
- `id` (TEXT, primary key, stores Stripe `event.id`)
|
||||
- `type` (TEXT, event type like `invoice.paid`)
|
||||
- `processed_at` (DATETIME, timestamp)
|
||||
- Consider adding a TTL/cleanup mechanism for old records
|
||||
2. Update `web/src/routes/api/stripe/webhook.ts:18-21` to check the event ID against the table before processing
|
||||
3. For each event type (`checkout.session.completed`, `invoice.paid`, `invoice.payment_failed`, `customer.subscription.updated`, `customer.subscription.deleted`):
|
||||
- Check if `event.id` already exists in the table
|
||||
- If yes, log and return early (idempotent replay)
|
||||
- If no, insert the event ID and proceed with processing
|
||||
4. Ensure the insert uses `onConflictDoNothing()` or a unique constraint to handle race conditions
|
||||
5. Add a cleanup job or TTL to prevent unbounded table growth (e.g., delete events older than 30 days)
|
||||
|
||||
tests:
|
||||
- Unit: Duplicate event ID returns early without re-processing
|
||||
- Unit: New event ID is inserted and processing continues
|
||||
- Unit: Race condition (two identical events arrive simultaneously) is handled by unique constraint
|
||||
- Integration: Sending the same webhook event twice results in only one processing
|
||||
- Integration: Different event types with the same ID are handled correctly (should not happen in practice but test the constraint)
|
||||
|
||||
acceptance_criteria:
|
||||
- All webhook event types check for duplicate event IDs before processing
|
||||
- Duplicate events are logged and skipped without re-processing
|
||||
- Race conditions are handled by database constraints (unique index on event ID)
|
||||
- Old event IDs are cleaned up to prevent unbounded table growth
|
||||
- No regression in existing webhook processing behavior
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Run the Drizzle migration and verify the `stripe_webhook_events` table exists
|
||||
- Send the same webhook event twice and verify only the first is processed
|
||||
- Check the database to confirm the event ID was recorded
|
||||
|
||||
notes:
|
||||
- Finding p8-007: Only `checkout.session.completed` currently has partial dedup via `onConflictDoNothing()`
|
||||
- Depends on task 06 because validated data shapes from that fix are needed for the dedup logic
|
||||
- Stripe guarantees event IDs are unique, so the event ID is a safe dedup key
|
||||
- Consider a background job or cron to clean up old event IDs (e.g., older than 30 days)
|
||||
@@ -0,0 +1,60 @@
|
||||
# 08. Fix WebSocket JWT leakage via query parameter
|
||||
|
||||
meta:
|
||||
id: security-fixes-08
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Move WebSocket JWT authentication from query parameter to Authorization header to prevent token leakage in logs
|
||||
|
||||
deliverables:
|
||||
- Updated `getTokenFromRequest()` at `web/src/server/websocket.ts:39-43` to read from Authorization header
|
||||
- Updated WebSocket client code to send JWT in the Authorization header
|
||||
- Backward compatibility during transition period (optional)
|
||||
- Unit tests for token extraction from headers
|
||||
|
||||
steps:
|
||||
1. Examine `web/src/server/websocket.ts:39-43` (`getTokenFromRequest`) and `web/src/server/websocket.ts:56-67` (authentication flow)
|
||||
2. Update `getTokenFromRequest()` to:
|
||||
- First check the `Authorization` header for a Bearer token (`Authorization: Bearer <jwt>`)
|
||||
- Fall back to `?token=` query parameter with a deprecation warning (optional, for backward compatibility)
|
||||
- Return `null` if neither is present
|
||||
3. Update the WebSocket client (browser app and extension) to:
|
||||
- Include the JWT in a custom header: `new WebSocket(url, { headers: { Authorization: `Bearer ${token}` } })`
|
||||
- Note: The WebSocket constructor doesn't support custom headers in browsers; use the `verifyClient` callback on the server side to read headers from the upgrade request
|
||||
- Alternative: Use an HTTP handshake approach or pass the token in a message after connection (with a timeout)
|
||||
4. If browser WebSocket API limitations prevent header-based auth, implement a post-connection authentication message with a timeout (e.g., client must send `{type: "auth", token: "..."}` within 5 seconds)
|
||||
5. Update `web/src/server/websocket.ts:80-102` (connection handler) to enforce the new auth flow
|
||||
|
||||
tests:
|
||||
- Unit: `getTokenFromRequest()` extracts token from Authorization header
|
||||
- Unit: `getTokenFromRequest()` rejects connections without a token
|
||||
- Unit: Query parameter fallback (if implemented) logs a deprecation warning
|
||||
- Integration: WebSocket connection with valid Bearer token is authenticated
|
||||
- Integration: WebSocket connection without a token is rejected
|
||||
|
||||
acceptance_criteria:
|
||||
- JWT is no longer passed in the query parameter by default
|
||||
- Server accepts JWT from Authorization header (or post-connection auth message if browser limitation requires it)
|
||||
- Connections without authentication are rejected
|
||||
- No JWT tokens appear in server access logs or proxy logs
|
||||
- Backward compatibility is handled (if implemented) with a deprecation path
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Connect via WebSocket with a valid token in the expected location and verify authentication succeeds
|
||||
- Connect without a token and verify the connection is rejected
|
||||
- Check server logs to confirm JWT tokens are not visible in URLs
|
||||
|
||||
notes:
|
||||
- Finding p8-008: The browser WebSocket API does not support custom headers in the constructor
|
||||
- Recommended approach: Post-connection authentication message with a server-side timeout
|
||||
1. Client connects without token
|
||||
2. Server allows the connection but marks it as unauthenticated
|
||||
3. Client sends `{type: "auth", token: "..."}` within 5 seconds
|
||||
4. Server validates the token and upgrades the connection
|
||||
5. Unauthenticated connections are closed after the timeout
|
||||
- This approach avoids JWT in URLs and works with the browser WebSocket API
|
||||
55
tasks/security-fixes/09-fix-websocket-origin-validation.md
Normal file
55
tasks/security-fixes/09-fix-websocket-origin-validation.md
Normal file
@@ -0,0 +1,55 @@
|
||||
# 09. Fix WebSocket no Origin header validation
|
||||
|
||||
meta:
|
||||
id: security-fixes-09
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: [security-fixes-08]
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Prevent cross-origin WebSocket connections by validating the Origin header during the upgrade handshake
|
||||
|
||||
deliverables:
|
||||
- `verifyClient` callback on the WebSocket server that validates the Origin header
|
||||
- Configurable allowlist of trusted origins for WebSocket connections
|
||||
- Unit tests for Origin validation
|
||||
|
||||
steps:
|
||||
1. Examine `web/src/server/websocket.ts:80-102` (WebSocketServer constructor and connection handler)
|
||||
2. Add a `verifyClient` callback to the `WebSocketServer` constructor:
|
||||
- Extract the `Origin` header from the upgrade request
|
||||
- Validate against a trusted origins allowlist (derived from `APP_URL` and localhost)
|
||||
- Reject connections with missing or untrusted Origin headers
|
||||
3. Define the trusted origins allowlist:
|
||||
- `http://localhost:3000`, `http://localhost:3001` (development)
|
||||
- `APP_URL` (production, validated per task 05)
|
||||
- Optional: `VALID_WEBSOCKET_ORIGINS` env var for explicit configuration
|
||||
4. Ensure the Origin validation works with the post-connection auth flow from task 08
|
||||
5. Log rejected connections for monitoring
|
||||
|
||||
tests:
|
||||
- Unit: Connection from trusted origin (`localhost:3000`) is accepted
|
||||
- Unit: Connection from untrusted origin (`https://evil.com`) is rejected
|
||||
- Unit: Connection without an Origin header is rejected
|
||||
- Integration: WebSocket connection from a trusted page succeeds
|
||||
- Integration: WebSocket connection initiated from an untrusted page (via `<script>`) is rejected
|
||||
|
||||
acceptance_criteria:
|
||||
- WebSocket server validates the Origin header during the upgrade handshake
|
||||
- Connections from untrusted origins are rejected before authentication
|
||||
- Trusted origins include localhost (dev) and APP_URL (production)
|
||||
- Missing Origin headers are rejected
|
||||
- The Origin validation complements the JWT authentication from task 08
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Connect from a trusted origin and verify the connection is accepted
|
||||
- Attempt to connect from an untrusted origin and verify it is rejected
|
||||
- Check server logs for rejected connection entries
|
||||
|
||||
notes:
|
||||
- Finding p8-009: The WebSocket server on port 3001 has no Origin validation
|
||||
- Depends on task 08 because the authentication flow needs to be established first
|
||||
- The `verifyClient` callback receives `{ origin, req, secure }` — use `origin` for validation
|
||||
- Combined with task 08 (JWT auth), this closes the complete authentication bypass chain
|
||||
@@ -0,0 +1,57 @@
|
||||
# 10. Fix VoicePrint resource exhaustion via unbounded audio upload
|
||||
|
||||
meta:
|
||||
id: security-fixes-10
|
||||
feature: security-fixes
|
||||
priority: P1
|
||||
depends_on: []
|
||||
tags: [implementation, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Prevent memory exhaustion by enforcing maximum payload size on VoicePrint audio endpoints
|
||||
|
||||
deliverables:
|
||||
- `maxLength` constraint on `AnalyzeAudioSchema` in `web/src/server/api/schemas/voiceprint.ts`
|
||||
- Request body size limit middleware for audio endpoints
|
||||
- Size validation in `voiceprint.service.ts` before base64 decoding
|
||||
- Unit tests for size limits
|
||||
|
||||
steps:
|
||||
1. Examine `AnalyzeAudioSchema` at `web/src/server/api/schemas/voiceprint.ts:8-10` and `analyzeAudio()` at `web/src/server/services/voiceprint.service.ts:135-140`
|
||||
2. Add `maxLength` to the audio schema:
|
||||
- Calculate a reasonable limit: A 60-second mono 16kHz WAV is ~1.2MB raw, ~1.6MB base64
|
||||
- Set `maxLength` to ~2MB base64 (~1.5MB raw) as a safe default
|
||||
- Consider making it configurable via an environment variable
|
||||
3. Add a request body size limit in the tRPC middleware or at the HTTP layer:
|
||||
- Reject requests with body size > configured limit before processing
|
||||
- Return a clear error message to the client
|
||||
4. Add a pre-decode size check in `analyzeAudio()`:
|
||||
- Calculate the decoded size from the base64 string length (`base64Length * 0.75`)
|
||||
- Reject if the decoded size exceeds the configured memory limit
|
||||
5. Update `protectedProcedure` rate limit for voiceprint endpoints if not already covered by task 04
|
||||
|
||||
tests:
|
||||
- Unit: `AnalyzeAudioSchema` rejects payloads exceeding `maxLength`
|
||||
- Unit: `analyzeAudio()` rejects base64 strings that would decode to > configured memory limit
|
||||
- Unit: Valid audio payloads within the limit are accepted
|
||||
- Integration: Sending a 100MB base64 payload to the audio endpoint is rejected with a size error
|
||||
- Integration: Sending a valid 30-second audio recording succeeds
|
||||
|
||||
acceptance_criteria:
|
||||
- Audio schema enforces `maxLength` on the base64 payload
|
||||
- Request body size limit middleware rejects oversized requests before processing
|
||||
- Pre-decode size check prevents memory exhaustion from valid-length but high-entropy payloads
|
||||
- Clear error messages are returned when size limits are exceeded
|
||||
- Valid audio recordings within the size limit are processed normally
|
||||
|
||||
validation:
|
||||
- `cd web && bun test` — all tests pass
|
||||
- Send a base64 payload exceeding the maxLength and verify it is rejected
|
||||
- Send a valid audio recording and verify it is processed correctly
|
||||
- Verify the rate limit for voiceprint endpoints is appropriate (task 04)
|
||||
|
||||
notes:
|
||||
- Finding p8-010: A 100MB base64 payload consumes 300MB+ memory per request
|
||||
- The `protectedProcedure` rate limit (100/min) is insufficient — at 100 requests/min with 100MB payloads, that's 10GB/min of memory pressure
|
||||
- Consider streaming or chunked upload for large audio files instead of base64 in the request body
|
||||
- The maxLength should account for realistic use cases: voice biometrics typically need 3-30 seconds of audio
|
||||
47
tasks/security-fixes/11-fix-browser-ext-superjson-cve.md
Normal file
47
tasks/security-fixes/11-fix-browser-ext-superjson-cve.md
Normal file
@@ -0,0 +1,47 @@
|
||||
# 11. Fix browser extension vulnerable dependency (superjson CVE-2022-23631)
|
||||
|
||||
meta:
|
||||
id: security-fixes-11
|
||||
feature: security-fixes
|
||||
priority: P2
|
||||
depends_on: []
|
||||
tags: [dependency-update, tests-required, medium-severity]
|
||||
|
||||
objective:
|
||||
- Update the browser extension's superjson dependency to patch CVE-2022-23631 (prototype pollution → RCE)
|
||||
|
||||
deliverables:
|
||||
- Updated `browser-ext/package.json` with superjson pinned to >=2.2.6
|
||||
- Updated lock file
|
||||
- Verification that the extension still functions correctly with the updated dependency
|
||||
|
||||
steps:
|
||||
1. Examine `browser-ext/package.json:18` — current declaration is `"superjson": "^2.2.1"`
|
||||
2. Update the dependency to `"superjson": "^2.2.6"` (or latest stable version)
|
||||
3. Run `pnpm install` in the browser-ext directory to update the lock file
|
||||
4. Verify that `browser-ext/src/lib/api-client.ts` (tRPC client using superjson) still works with the updated version
|
||||
5. Check for any breaking changes in the superjson changelog between 2.2.1 and the target version
|
||||
6. Run the browser extension build to confirm no compilation errors
|
||||
|
||||
tests:
|
||||
- Unit: tRPC client serialization/deserialization works with the updated superjson version
|
||||
- Integration: Browser extension can successfully communicate with the tRPC API
|
||||
- Build: `pnpm build` in the browser-ext directory completes without errors
|
||||
|
||||
acceptance_criteria:
|
||||
- `browser-ext/package.json` declares `superjson >= 2.2.6`
|
||||
- Lock file reflects the updated version (no 2.2.1–2.2.5 range resolved)
|
||||
- Browser extension builds successfully
|
||||
- tRPC client communication works correctly with the updated dependency
|
||||
- No prototype pollution vulnerability remains (CVE-2022-23631 is fixed in >=2.2.6)
|
||||
|
||||
validation:
|
||||
- `cd browser-ext && pnpm install && pnpm build` — succeeds without errors
|
||||
- `pnpm list superjson` — shows version >= 2.2.6
|
||||
- Run the browser extension and verify API communication works
|
||||
|
||||
notes:
|
||||
- Finding p8-011: CVE-2022-23631 (CVSS 10.0) affects superjson 2.2.1–2.2.5
|
||||
- The web server is NOT affected (does not use superjson)
|
||||
- This is a quick fix — primarily a dependency version bump
|
||||
- The caret range `^2.2.1` allows 2.2.1–2.2.5; changing to `^2.2.6` ensures only patched versions are installed
|
||||
25
tasks/security-fixes/README.md
Normal file
25
tasks/security-fixes/README.md
Normal file
@@ -0,0 +1,25 @@
|
||||
# Security Fixes
|
||||
|
||||
Objective: Remediate all 11 confirmed security findings from the piolium balanced audit (1 HIGH, 10 MEDIUM).
|
||||
|
||||
Status legend: [ ] todo, [~] in-progress, [x] done
|
||||
|
||||
Tasks
|
||||
- [ ] 01 — Fix stored XSS via unsanitized innerHTML in blog rendering → `01-fix-stored-xss-blog-rendering.md`
|
||||
- [ ] 02 — Fix SSRF via Puppeteer --no-sandbox in report generation → `02-fix-puppeteer-ssrf-report-gen.md`
|
||||
- [ ] 03 — Fix open redirect via unvalidated return URL in Stripe checkout → `03-fix-open-redirect-stripe-return-url.md`
|
||||
- [ ] 04 — Fix rate limit bypass via incomplete sensitive path list → `04-fix-rate-limit-substring-bypass.md`
|
||||
- [ ] 05 — Fix CORS origin trust from unvalidated APP_URL env var → `05-fix-cors-origin-env-var-validation.md`
|
||||
- [ ] 06 — Fix webhook type coercion bypassing TypeScript safety → `06-fix-webhook-type-coercion.md`
|
||||
- [x] 07 — Fix webhook replay via missing event ID deduplication → `07-fix-webhook-replay-missing-dedup.md`
|
||||
- [x] 08 — Fix WebSocket JWT leakage via query parameter → `08-fix-websocket-jwt-query-param-leak.md`
|
||||
- [x] 09 — Fix WebSocket no Origin header validation → `09-fix-websocket-origin-validation.md`
|
||||
- [x] 10 — Fix VoicePrint resource exhaustion via unbounded audio upload → `10-fix-voiceprint-resource-exhaustion.md`
|
||||
- [x] 11 — Fix browser extension vulnerable dependency (superjson CVE-2022-23631) → `11-fix-browser-ext-superjson-cve.md`
|
||||
|
||||
Dependencies
|
||||
- 07 depends on 06 (webhook type coercion fix shares billing.service.ts; dedup needs validated data shapes)
|
||||
- 09 depends on 08 (WebSocket JWT header auth is the prerequisite for Origin validation to be meaningful)
|
||||
|
||||
Exit criteria
|
||||
- The feature is complete when all 11 findings have been remediated, each with passing tests, and no regression is introduced to the existing codebase.
|
||||
Reference in New Issue
Block a user