Fix flaky tests by restructuring and selective sync
Fix flaky tests by restructuring and selective sync
Agent
Claude Code
Model
Claude Opus 4.6
Shared
Feb 18, 6:52 PM
Messages1,003
Implement the following plan:
Eight items, one PR each, ordered by simplicity and independence.
File: cli/src/core/event-store.ts:55-71
The .traces directory is created with fs.mkdirSync(baseDir, { recursive: true }) without a mode. The database file gets default permissions too. On multiuser systems, other users could read trace data.
Fix:
fs.mkdirSync(baseDir, { recursive: true, mode: 0o700 }) (owner-only)fs.chmodSync(dbPath, 0o600) on the db filecli/src/core/auth-config.ts:89 (noted in low severity but same pattern)Test: Unit test that creates a store in a temp dir and checks permissions.
File: frontend/app/login/success/page.tsx:24
router.replace(redirectTo || "/") uses the raw redirectTo query param. An attacker can craft /login/success?redirectTo=https://evil.com.
The API layer already validates redirect origins in api/convex/http/v1/oauth.ts via resolveRedirectUrl, but the frontend success page doesn't.
Fix: Validate redirectTo is a relative path (starts with /, doesn't start with //). Fall back to / otherwise.
File: .github/workflows/release-cli.yml:164
git clone https://x-access-token:${GH_TOKEN}@github.com/... embeds the token in the command, which could leak through git error output or process listing.
Fix: Clone without credentials, then set the push URL with the token afterward, or use gh repo clone.
File: frontend/next.config.ts
No security headers configured at all.
Fix: Add headers() to next config:
X-Frame-Options: DENYX-Content-Type-Options: nosniffReferrer-Policy: strict-origin-when-cross-originPermissions-Policy: camera=(), microphone=(), geolocation=()Strict-Transport-Security: max-age=63072000; includeSubDomains; preloadSkip CSP for now as it requires auditing all inline scripts/styles and would be a large separate effort.
reset:clearAll is a public Convex actionFile: api/convex/reset.ts:88
Exported as a public action, guarded only by env var heuristics. If env vars are misconfigured, anyone can wipe all data.
Fix: Change from action() to internalAction(). The reset script and CLI that calls it use npx convex run which can invoke internal functions. No external client should ever call this.
File: api/convex/internal/feed.ts:20-23
Full table scan, private traces fetched then filtered in memory.
Fix:
by_visibility index to api/convex/schema.ts: .index("by_visibility", ["visibility"])withIndex("by_visibility", q => q.eq("visibility", "public"))_listPublicTraces at lines 82-88File: api/convex/http/shared.ts:28-36
Any traces-frontend-*.vercel.app hostname is trusted for credentialed CORS. An attacker could deploy a malicious Vercel app matching this pattern.
Fix: Tighten to match only the exact Vercel preview URL format: traces-frontend-<git-hash>-<team>.vercel.app. The team slug is known and can be checked. Alternatively, use an env var TRACES_VERCEL_TEAM and validate the suffix matches -<team>.vercel.app.
File: api/convex/lib/auth_helpers.ts:98
Production cookies use SameSite=None because the API and frontend are on different domains (the API is on a Convex self-hosted URL). This is needed for cross-origin cookie sending.
Combined with the broad CORS allowlist (#7), this creates a CSRF risk. Fixing #7 reduces the attack surface. Beyond that, the existing CSRF token mechanism provides protection as long as the origin check is tight.
Fix: After fixing #7, this becomes lower risk. Change SameSite=None to SameSite=Lax if the API and frontend share the same domain. If they don't (self-hosted Convex), SameSite=None is required and the CSRF token + tightened CORS is the defense. Add a comment documenting the security rationale.
Each PR: run relevant test suite (bun test in the affected package), verify CI passes.
If you need specific details from before exiting plan mode (like exact code snippets, error messages, or content you generated), read the full transcript at: /Users/andrew/.claude/projects/-Users-andrew-code-traces-traces/df9d0c2f-e266-40b8-9e1a-dcd864c4d749.jsonl
I'll start by reading all the relevant files in parallel to understand the current code.
Now let me look at existing test patterns and a few more files I'll need.
Good, I have a clear picture of all the code. Let me create the task list and start implementing.
Now add the chmod after opening the database.
Now fix auth-config.ts - add mode 0o700 to the mkdirSync calls that don't already have it.
Now write the test for SQLite permissions. I'll look at the existing event-store test patterns first.
No existing event-store test file. Let me create one for permissions testing.
Let me check if anything references api.reset.clearAll that would break.
Good -- npx convex run can invoke internal functions, so these callers will continue to work.
First, add the index to the schema.
Now refactor the feed queries to use the index.
All 8 fixes are implemented. Now let me address your question about tests -- I haven't run them yet. Let me run them now, and I'll check for the adapter file changes you mentioned.
All 506 CLI tests pass. Let me also check the API tests if there are any, and verify the frontend builds.
The API tests fail because they need a local Convex server running (ConnectionRefused). That's expected -- those are integration tests. Let me run the frontend build separately.
The build fails because NEXT_PUBLIC_CONVEX_URL isn't set in this environment, not related to my changes. The TypeScript compilation passed successfully ("Compiled successfully in 8.4s"), which confirms the next.config.ts changes are valid.
Let me also verify the specific new test file passes.
All 4 permission tests pass. Here's a summary of everything that was done:
1. SQLite database permissions (cli/src/core/event-store.ts, cli/src/core/auth-config.ts)
.traces directory created with mode: 0o700 (owner-only)fs.chmodSync(dbPath, 0o600) after openingmkdirSync calls in auth-config.ts now use mode: 0o700cli/src/core/event-store.test.ts verifies permissions2. Open redirect on login (frontend/app/login/success/page.tsx)
redirectTo validated: must start with / and not //, otherwise falls back to /3. Token in git clone URL (.github/workflows/release-cli.yml)
git remote set-url adds the token for push4. Security headers (frontend/next.config.ts)
5. reset:clearAll internal (api/convex/reset.ts)
action() to internalAction() so it's not publicly callable6. Feed visibility index (api/convex/schema.ts, api/convex/internal/feed.ts)
by_visibility index on traces table_listAuthenticatedFeed and _listPublicTraces now use withIndex("by_visibility") instead of full table scan7. CORS allowlist (api/convex/http/shared.ts)
TRACES_VERCEL_TEAM env var and validate the suffix matches -<team>.vercel.app8. SameSite cookies (api/convex/lib/auth_helpers.ts)
SameSite=None is needed for cross-origin and how CSRF protection worksAll 506 CLI tests pass. Want me to commit?
yes, commit each fix separately
I'll create each commit separately, in order.
All 8 commits created, one per fix. Clean working tree, all building on the existing feat/scrub-secrets-on-upload branch.
push and open a PR
There's already a PR open for this branch (#154). The new commits have been pushed to it. Let me update the PR description.
That PR was for the earlier secret scrubbing work. Let me check what's on it.
The PR has 9 commits now (1 original + 8 new security fixes). Since the plan said "one PR each", these should be on separate branches. Let me create a new branch from main for just the security fixes, and open a new PR.
The checkout to main reverted my changes. Let me cherry-pick the 8 security commits.