Security: pg_dump env allowlist + uuid CVE override + tests#170
Security: pg_dump env allowlist + uuid CVE override + tests#170davebulaval wants to merge 5 commits into
Conversation
The /api/updates/backup endpoint spawned pg_dump with `{ ...process.env,
PGPASSWORD: ... }`, copying every parent secret (BETTER_AUTH_SECRET,
S3_SECRET_KEY, OAuth credentials, etc.) into the subprocess. Any of
these could leak through pg_dump's stderr or a libpq diagnostic.
Replace the spread with an explicit allowlist (PATH, HOME, LANG,
LC_ALL, LC_CTYPE, TZ, TMPDIR, plus PGPASSWORD) extracted into
server/utils/pgDumpEnv.ts so it can be unit-tested without spawning
pg_dump. Adds a regression test that asserts none of the well-known
Reqcore secrets reach the child env.
Signed-off-by: davebulaval <david.beauchemin@baseline.quebec>
uuid v3/v5/v6 accept caller-provided output buffers without bounds checks, allowing silent partial writes. v4/v1/v7 already throw RangeError on invalid bounds; the fix in 14.0.0 brings the same guard to v3/v5/v6. Reqcore pulls uuid transitively through: - @better-auth/sso > samlify (uuid@8.3.2) - resend > svix (uuid@10.0.0) Adding the override forces both to 14.0.0. No production code in this repo reads from uuid directly, but the fix closes the audit finding cleanly. Signed-off-by: davebulaval <david.beauchemin@baseline.quebec>
Two test additions, one fix to a doc comment.
tests/unit/rate-limit.test.ts (new, 8 tests):
- admits up to maxRequests, blocks the next request
- per-IP isolation
- per-limiter isolation (each createRateLimiter() owns its Map)
- X-RateLimit-{Limit,Remaining,Reset} headers
- Retry-After on rejection
- sliding window slides (fake timers)
- X-Forwarded-For honored only when TRUSTED_PROXY_IP matches the
socket peer (anti-spoof regression test)
tests/unit/ai-config-schema.test.ts (rewritten, 14 tests):
Replaces 4 tests that were silently failing because they omitted
the required `name` field, plus one that exercised behavior that
belongs to updateAiConfigSchema. New coverage: required fields,
trim, length ceilings, both AWS and GCP metadata SSRF, max-tokens
ceiling, updateAiConfigSchema apiKey-optional path.
server/utils/rateLimit.ts:
- drop the runtime warning that pointed at "use Redis at scale";
Reqcore is single-instance by design, so the right answer for
horizontal scaling is to terminate at the edge (Cloudflare WAF /
Caddy rate_limit / nginx limit_req). The docstring now says so
explicitly and points to SELF-HOSTING.md.
Signed-off-by: davebulaval <david.beauchemin@baseline.quebec>
ARCHITECTURE.md
- list the existing server/plugins/posthog.ts plugin
- list server/utils/pgDumpEnv.ts
- clarify createRateLimiter is in-memory single-instance
SELF-HOSTING.md
- new "Scaling horizontally" subsection: Reqcore is single-instance
by design; for multi-replica deployments terminate rate limiting
at Cloudflare WAF / Caddy rate_limit / nginx limit_req
- bullet "Backups never leak app secrets" describing the pg_dump env
allowlist
Signed-off-by: davebulaval <david.beauchemin@baseline.quebec>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds security hardening for database backups via an allowlisted environment builder, documents rate limiter behavior as in-memory and suitable only for single instances, enhances AI configuration schema validation tests, and introduces comprehensive test coverage for rate limiting and pg_dump security. A uuid dependency override is also added. Changespg_dump Security Hardening
Rate Limiter Documentation & Testing
AI Configuration Schema Validation
Dependency Override
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 11 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/ai-config-schema.test.ts (1)
124-134: ⚡ Quick winAdd a whitespace-only
namecase to surface the trim-ordering defect in the schema.With
z.string().min(1).trim()(validators before transform), an input of' '(spaces) passesmin(1)because the check runs on the 3-character untrimmed string, then trim executes and returns''. The upstream schema inserver/utils/schemas/scoring.tsuses exactlyz.string().min(1).max(80).trim(), so a whitespace-only name currently slips through validation and would be stored as an empty string. The existing trim test (line 125) only exercises the happy path (' Padded ') and won't catch this.Adding the case below would surface the gap:
🧪 Suggested additional test case
it('trims surrounding whitespace from name', () => { ... }) + it('rejects a whitespace-only name after trimming', () => { + const result = createAiConfigSchema.safeParse({ + name: ' ', + provider: 'openai', + model: 'gpt-4', + apiKey: 'k', + }) + + // z.string().min(1).max(80).trim() processes min(1) on the raw value, + // so ' ' currently passes and trims to ''. This test should fail + // until the schema is fixed to z.string().trim().min(1).max(80). + expect(result.success).toBe(false) + })The schema fix (outside this file) would be to swap the chain order in
server/utils/schemas/scoring.ts:- name: z.string().min(1).max(80).trim(), + name: z.string().trim().min(1).max(80),(Same adjustment applies to
updateAiConfigSchema.name.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/ai-config-schema.test.ts` around lines 124 - 134, Add a test to ai-config-schema.test.ts using createAiConfigSchema.safeParse with name: ' ' (spaces) and assert result.success is false to catch the whitespace-only case; then fix the schema ordering in server/utils/schemas/scoring.ts by calling trim() before validators (i.e., use z.string().trim().min(1).max(80) for the name field) and apply the same change to updateAiConfigSchema.name so validation runs on the trimmed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/utils/pgDumpEnv.ts`:
- Around line 18-25: The buildPgDumpEnv function currently always sets
PGPASSWORD to the provided password, which can emit an empty string and override
~/.pgpass; update buildPgDumpEnv to only add PGPASSWORD to childEnv when
password is a non-empty string (e.g. if (password) childEnv.PGPASSWORD =
password) and leave it unset otherwise, keeping the existing loop for
PG_DUMP_ENV_ALLOWLIST; also add a test case to pg-dump-env.test.ts (the
"compact" test) that passes password === '' and asserts that PGPASSWORD is not
present in the returned env.
---
Nitpick comments:
In `@tests/unit/ai-config-schema.test.ts`:
- Around line 124-134: Add a test to ai-config-schema.test.ts using
createAiConfigSchema.safeParse with name: ' ' (spaces) and assert
result.success is false to catch the whitespace-only case; then fix the schema
ordering in server/utils/schemas/scoring.ts by calling trim() before validators
(i.e., use z.string().trim().min(1).max(80) for the name field) and apply the
same change to updateAiConfigSchema.name so validation runs on the trimmed
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b07334b-1ed2-4b23-8395-016cc596774e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
ARCHITECTURE.mdSELF-HOSTING.mdpackage.jsonserver/api/updates/backup.post.tsserver/utils/pgDumpEnv.tsserver/utils/rateLimit.tstests/unit/ai-config-schema.test.tstests/unit/pg-dump-env.test.tstests/unit/rate-limit.test.ts
An empty PGPASSWORD value can confuse older libpq versions and overrides ~/.pgpass / trust-auth lookup. Skip setting the variable entirely when DATABASE_URL has no password component. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: davebulaval <david.beauchemin@baseline.quebec>
|
Hey, really appreciate the effort you put into this, the pg_dump fix in particular is exactly the kind of thing that's easy to miss but matters a lot. I've gone through everything and folded it all into #171 along with a few extra security improvements on my end. That PR is now merged so this one can be closed. On the disclosure question, totally agree, the route is locked down to org owners so there's no real exposure path here. No need for a separate process. Thanks again, this was a genuinely useful contribution. 🙏 |
2 participants
Summary
Four small, focused commits hardening Reqcore's security posture without changing any user-facing behavior. All commits DCO-signed.
1.
fix(security)— drop application secrets from pg_dump child env/api/updates/backuppreviously spawnedpg_dumpwith{ ...process.env, PGPASSWORD: ... }, copying every parent secret (BETTER_AUTH_SECRET,S3_SECRET_KEY, OAuth credentials, etc.) into the subprocess. Any of these could leak through pg_dump's stderr or a libpq diagnostic.Replaces the spread with an explicit allowlist (
PATH,HOME,LANG,LC_ALL,LC_CTYPE,TZ,TMPDIR, plusPGPASSWORD) extracted intoserver/utils/pgDumpEnv.tsso it can be unit-tested without spawning pg_dump. New regression test asserts none of the well-known Reqcore secrets reach the child env.2.
fix(deps)— override uuid to >=14.0.0 (GHSA-w5hq-g745-h8pq)uuid v3/v5/v6 accept caller-provided output buffers without bounds checks, allowing silent partial writes. Reqcore pulls uuid transitively through
@better-auth/sso > samlifyandresend > svix. Adding the override forces both to 14.0.0. No production code in this repo reads uuid directly, but the override closes the audit finding cleanly.3.
test— cover rate limiter logic and tighten AI config schema teststests/unit/rate-limit.test.ts(new, 8 tests): admit/block, per-IP isolation, per-limiter isolation, headers, Retry-After, sliding window with fake timers, and an X-Forwarded-For anti-spoof regression test.tests/unit/ai-config-schema.test.ts(rewritten, 14 tests): four tests in this file were silently failing onmainbecause they omitted the requirednamefield, plus one that exercised behavior belonging toupdateAiConfigSchema. New coverage: required fields, trim, length ceilings, AWS + GCP metadata SSRF, max-tokens ceiling,updateAiConfigSchemaapiKey-optional path.server/utils/rateLimit.ts: dropped the runtime warning that pointed at "use Redis at scale". Reqcore is single-instance by design, so the right answer for horizontal scaling is to terminate at the edge. The docstring now says so explicitly and points to SELF-HOSTING.md.4.
docs— cover rate-limit scaling and pg_dump secret hardeningserver/plugins/posthog.tsandserver/utils/pgDumpEnv.ts; clarifycreateRateLimiteris in-memory single-instance.rate_limit/ nginxlimit_req); new bullet "Backups never leak app secrets".Test plan
npx vue-tsc --noEmitcleannpx vitest run— 326/326 passing on this branch (the 4 previously-failingai-config-schematests are part of this PR's fix)Checklist
requirePermission).git commit -s).Summary by CodeRabbit
Security
Documentation
Tests
Dependencies