Conversation
Semgrep Security ScanNo security issues found. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
PR Metrics
Updated Thu, 07 May 2026 17:00:40 GMT · run #1418 |
…from ending up in server logs
- inject DnsLookup into url-validation, proxy, and preview routes - inject SearchExaClient into search route via createApp deps - inject email senders into createAuth to capture OTPs per test - avoids mock.module leaking across test files
- store test PGlite instance on globalThis so it survives module reloads - close on process beforeExit instead of afterAll, so reruns reuse one instance rather than reinitializing (and re-running migrations) each time
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
- Eliminates a sign-in race where the bearer is returned before the session row is visible, which surfaced as a confusing 401 on the first authenticated request in tests.
- move cleanup into bun:test afterAll so it runs inside the test runner lifecycle, before Bun terminates worker threads - drop the globalThis-shared PGlite instance; one instance per process is sufficient now that cleanup is reliable
- Switch main API CORS to allowedHeaders: true so preflight echoes whatever the client requests - The /v1/proxy route forwards arbitrary upstream headers via X-Proxy-Passthrough-*; provider SDKs add x-api-key, x-stainless-*, openai-organization, anthropic-beta, etc., which a static allowlist cannot enumerate - Document the policy in AGENTS.md and note that the PostHog proxy retains its strict allowlist
| @@ -0,0 +1,24 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Going to pull this into separate PR
| @@ -1,3 +1,7 @@ | |||
| -- This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
Hmm not sure why this wasn't there before
| "vite-bundle-analyzer": "^1.2.1", | ||
| "vitest": "^4.1.4" | ||
| }, | ||
| "//overrides": "Force semver >= 7 at top-level so storybook can resolve semver/functions/sort.js. Without this, bun hoists semver@6 (pulled by babel/eslint-plugin-react) and storybook breaks at vite config load.", |
There was a problem hiding this comment.
I thought we removed this? It should get removed
| CORS_ORIGINS: `http://localhost:${samlVitePort}`, | ||
| TRUSTED_ORIGINS: `http://localhost:${samlVitePort},http://localhost:${mockSamlPort}`, | ||
| RATE_LIMIT_ENABLED: 'false', | ||
| DATABASE_DRIVER: 'pglite', |
There was a problem hiding this comment.
This change should be separate PR
| // X-Proxy-Passthrough-Authorization; Standalone mode (Tauri) hits | ||
| // Anthropic directly via the Rust HTTP plugin. Either way, the user's | ||
| // Anthropic key never goes through Thunderbolt's session auth path. | ||
| const db = getDb() |
There was a problem hiding this comment.
This is janky - should not be lazily getting db, getting settings, creating a fetch every single time - maybe we can store the custom fetch in a context provider and do something like useFetch
| * Pino redact paths covering the universal proxy's PII surface area. | ||
| * Caller-controlled URLs, bodies, and credentials must never reach a log line. | ||
| */ | ||
| const proxyRedactPaths = [ |
There was a problem hiding this comment.
This is a code smell - we should not do this. If we've designed the universal proxy well (I think we did) this is not necessary.
|
|
||
| const baseOptions = { | ||
| level, | ||
| redact: { paths: proxyRedactPaths, censor: '[REDACTED]' }, |
There was a problem hiding this comment.
(and here - shouldnt' need redaction logic)
| powersyncUrl: z.string().default(''), | ||
| powersyncJwtKid: z.string().default(''), | ||
| powersyncJwtSecret: z.string().default(''), | ||
| // PowerSync settings — defaults match the local dev stack in powersync-service/config/config.yaml |
There was a problem hiding this comment.
These can be part of the other dev env PR
| .string() | ||
| .default( | ||
| 'Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Challenge-Token,X-Mcp-Target-Url,Mcp-Authorization,Mcp-Session-Id,Mcp-Protocol-Version', | ||
| 'Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Challenge-Token,X-Proxy-Target-Url,X-Proxy-Follow-Redirects,X-Proxy-Passthrough-Content-Type,X-Proxy-Passthrough-Authorization,X-Proxy-Passthrough-Accept,X-Proxy-Passthrough-Cache-Control,X-Proxy-Passthrough-Mcp-Session-Id,X-Proxy-Passthrough-Mcp-Protocol-Version,X-Proxy-Passthrough-Anthropic-Version,X-Proxy-Passthrough-Anthropic-Beta,X-Proxy-Passthrough-Openai-Beta', |
There was a problem hiding this comment.
No longer need to hard code all of these in because of the cors plugin change.
| .string() | ||
| .default('mcp-session-id,set-auth-token,ratelimit-limit,ratelimit-remaining,ratelimit-reset,retry-after'), | ||
| .default( | ||
| 'set-auth-token,ratelimit-limit,ratelimit-remaining,ratelimit-reset,retry-after,X-Proxy-Final-Url,X-Proxy-Passthrough-Content-Type,X-Proxy-Passthrough-Mcp-Session-Id,X-Proxy-Passthrough-Mcp-Protocol-Version,X-Proxy-Passthrough-Location,X-Proxy-Passthrough-Anthropic-Version', |
| corsAllowHeaders: | ||
| process.env.CORS_ALLOW_HEADERS || | ||
| 'Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Challenge-Token,X-Mcp-Target-Url,Mcp-Authorization,Mcp-Session-Id,Mcp-Protocol-Version', | ||
| 'Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Challenge-Token,X-Proxy-Target-Url,X-Proxy-Follow-Redirects,X-Proxy-Passthrough-Content-Type,X-Proxy-Passthrough-Authorization,X-Proxy-Passthrough-Accept,X-Proxy-Passthrough-Cache-Control,X-Proxy-Passthrough-Mcp-Session-Id,X-Proxy-Passthrough-Mcp-Protocol-Version,X-Proxy-Passthrough-Anthropic-Version,X-Proxy-Passthrough-Anthropic-Beta,X-Proxy-Passthrough-Openai-Beta', |
| if (process.env.DATABASE_DRIVER === 'postgres' && !process.env.DATABASE_URL) { | ||
| throw new Error('DATABASE_URL is required when DATABASE_DRIVER=postgres') | ||
| } | ||
| // Default driver is postgres pointing at the local Docker stack (powersync-service/). |
There was a problem hiding this comment.
This should be part of dev env PR
| /** Memoized Exa client — exported so other modules (e.g. /search) reuse the same instance. */ | ||
| export const getExaClient = memoize((): Exa | null => { | ||
| const apiKey = getSettings().exaApiKey | ||
| if (!apiKey) return null |
There was a problem hiding this comment.
Let's use the old code format here - this is harder to read and adds no value. Let's also update the AGENTS.md to inform the /simplify skill NOT to do this kind of worthless "simplification" (should be part of the dev env PR)
| deps.logger.info(event) | ||
| } | ||
|
|
||
| const emitPostHog = (distinctId: string, properties: Record<string, unknown>, error?: string) => { |
There was a problem hiding this comment.
Why do we need this? What does it do? Don't understand the need.
| request_id: fields.request_id, | ||
| ...(fields.error ? { error: fields.error } : {}), | ||
| }) | ||
| emitPostHog( |
There was a problem hiding this comment.
Same here... what's the point of Posthog here?
| import { Elysia, type AnyElysia } from 'elysia' | ||
| import { noopObservability, type ObservabilityRecorder } from './observability' | ||
|
|
||
| const TARGET_PREFIX = 'tbproxy.target.' |
There was a problem hiding this comment.
These should be camel case
| } catch { | ||
| return { ok: false, reason: 'malformed' } | ||
| } | ||
| if (!target) return { ok: false, reason: 'malformed' } |
There was a problem hiding this comment.
If statements should always have { } - can we update the linter to require that?
| if (!target) return { ok: false, reason: 'malformed' } | ||
| // Strip *all* tbproxy.* entries — the namespace is reserved for proxy control. | ||
| const callerProtocols = protocols.filter((p) => !p.startsWith('tbproxy.')) | ||
| return { ok: true, target, callerProtocols } |
There was a problem hiding this comment.
This is hard to read - can we update prettier to format these better? (new line per property)
|
|
||
| /** Validate the decoded target URL. Hostname-only SSRF — DNS rebinding gap is documented. */ | ||
| export const validateWsTarget = (raw: string): ValidatedTarget => { | ||
| let target: URL |
There was a problem hiding this comment.
Is there a way to rewrite this to avoid let?
| } | ||
| extras.relay = state | ||
|
|
||
| let upstream: WebSocket |
| CORS_ALLOW_METHODS=GET,POST,PUT,DELETE,PATCH,OPTIONS | ||
| CORS_ALLOW_HEADERS=Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Mcp-Target-Url,Mcp-Authorization,Mcp-Session-Id,Mcp-Protocol-Version | ||
| CORS_EXPOSE_HEADERS=mcp-session-id,set-auth-token,ratelimit-limit,ratelimit-remaining,ratelimit-reset,retry-after | ||
| CORS_ALLOW_HEADERS=Content-Type,Authorization,Accept,Accept-Encoding,Accept-Language,Cache-Control,User-Agent,X-Requested-With,X-Client-Platform,X-Device-ID,X-Device-Name,X-Challenge-Token,X-Proxy-Target-Url,X-Proxy-Follow-Redirects,X-Proxy-Passthrough-Content-Type,X-Proxy-Passthrough-Authorization,X-Proxy-Passthrough-Accept,X-Proxy-Passthrough-Cache-Control,X-Proxy-Passthrough-Mcp-Session-Id,X-Proxy-Passthrough-Mcp-Protocol-Version,X-Proxy-Passthrough-Anthropic-Version,X-Proxy-Passthrough-Anthropic-Beta,X-Proxy-Passthrough-Openai-Beta |
There was a problem hiding this comment.
These should not be necessary anymore with cors() update
| @@ -0,0 +1,119 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
Why is this .spec instead of .test? All other tests are .test (or should be!) If other e2e tests are not, then let's leave it for consistency
|
|
||
| import { fetch as tauriFetch } from '@tauri-apps/plugin-http' | ||
| import { | ||
| PASSTHROUGH_PREFIX, |
There was a problem hiding this comment.
These should all be camel case
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Took a careful pass through this. The overall shape is solid.
I left a few inline comments where I think the new approach trades away things we agreed on in [thunderbird/thunderbolt-spec#2 (https://github.com/thunderbird/thunderbolt-spec/pull/2): the <img> proxy contract (we lose require-corp COEP), cache headers, the proxy_enabled user toggle, observability error classes, and a small perf note on content-encoding. Most of these already landed in #816 in their final shape, so the diffs over there might save us a round of trial and error.
None of this is meant to block your direction.
| configureServer: (server) => { | ||
| server.middlewares.use((req, res, next) => { | ||
| res.setHeader('Cross-Origin-Embedder-Policy', 'require-corp') | ||
| res.setHeader('Cross-Origin-Embedder-Policy', 'credentialless') |
There was a problem hiding this comment.
COEP got dropped from require-corp to credentialless here, which I'm guessing was the workaround for favicons going direct to upstream.
The downside is that it weakens cross origin isolation for the whole app.
If favicons route through /v1/proxy instead, you can keep require-corp on and still show every image.
I took that route in #816 using a small useProxyUrl() hook on the <img> callers.
|
|
||
| // Proxy-set headers (NOT prefixed): describe the proxy's own response framing | ||
| // and security posture. Forced — override anything the upstream might have sent. | ||
| out.set('Content-Security-Policy', 'sandbox') |
There was a problem hiding this comment.
On caching: upstream's Cache-Control flows through unchanged, so images either get cached too eagerly at CDN layers or not at all by the browser.
Setting Cache-Control: private, max-age=600 and CDN-Cache-Control: no-store here gives the browser a 10 minute cache while keeping CDNs out.
I added both in #816 at routes.ts:376-377 https://github.com/thunderbird/thunderbolt/blob/6a1e1c9f/backend/src/proxy/routes.ts#L376-L377
| * upstream describe the wrong thing. Set-Cookie family is dropped to preserve | ||
| * cookie isolation: the response's *origin* is Thunderbolt, not the upstream. */ | ||
| export const DROPPED_RESPONSE_HEADERS = new Set([ | ||
| 'content-length', |
There was a problem hiding this comment.
wondering about content-encoding being on the dropped list.
It works, but Bun decompresses every gzipped body by default, so the proxy ends up holding the uncompressed bytes in memory before sending them back.
If you pass decompress: false to the upstream fetch and forward content-encoding through, the browser does its own decode and you skip the rebuffer.
That's what #816 does at routes.ts:271-276. Perf note, not a correctness one.
| export const createProxyFetch = (options: ProxyFetchOptions): typeof fetch => { | ||
| const proxyUrl = `${options.cloudUrl.replace(/\/$/, '')}/proxy` | ||
| return (async (input, init) => { | ||
| const standalone = (options.isStandalone ?? isTauri)() |
There was a problem hiding this comment.
quick thought: this picks between direct fetch and the proxy, but there's no Settings toggle anywhere.
Tauri users on hosted mode end up routing every Anthropic / OpenAI call through the proxy with no way to opt out.
THU 471 and spec PR #2 introduced a proxy_enabled flag plus a Network section in Preferences for exactly this.
I kept it in #816 at preferences.tsx:104.
| } | ||
|
|
||
| return { | ||
| proxyRequest(fields) { |
There was a problem hiding this comment.
Curious about the recorder shape.
There's an error field, but nothing categorizing the failure (SSRF block, DNS timeout, idle, cap, upstream 5xx, auth reject), so PostHog dashboards can't filter by type. A small ProxyErrorType enum tagged on every failure fixes that. Pairing it with [OpenTelemetry span attributes (https://opentelemetry.io/docs/concepts/signals/traces/#attributes) (when trace.getActiveSpan() returns one) makes proxy events show up inside the trace already running on the request.
Check how I did it: #816
3 participants
Note
High Risk
High risk because it introduces and rewires a security-sensitive universal proxy (HTTP + WS), changes CORS behavior to echo requested headers, and alters logging/redaction paths—mistakes could enable SSRF, credential leakage, or break browser clients via CORS/preflight.
Overview
Adds a new authenticated universal proxy at
/v1/proxy(plus WebSocket relay) that forwards requests to arbitrary upstreams usingX-Proxy-Target-UrlandX-Proxy-Passthrough-*headers, with redirect handling, streaming caps/timeouts, SSRF protection (DNS pinning), and response header isolation via prefixed passthrough headers.Wires the proxy into the main app with new privacy-safe observability (
proxy_request/$proxy_request) and updates logging to aggressively redact proxy-related headers/bodies and drop passthrough headers from logs.Introduces new pro endpoints
GET /v1/search(Exa-backed, normalizes to HTTPS + favicon derivation) andPOST /v1/preview(OG metadata extraction with SSRF checks and capped HTML reads), and removes legacy/proproxy/link-preview/search implementations and the oldmcp-proxyroutes.Updates CORS strategy to
allowedHeaders: truefor the main API (to support arbitrary proxied headers), expands env/settings defaults (PowerSync defaults now point at local dev stack), adjusts DB defaults to prefer Postgres, and adds adevscript wrapper that optionally runs via 1Passwordop run.Reviewed by Cursor Bugbot for commit 3c3f78b. Bugbot is set up for automated code reviews on this repo. Configure here.