Skip to content

fix(security): isolate exports and harden history and packaging - #19

Merged
the-luap merged 1 commit into
mainfrom
fix/collector-audit-hardening
Sep 8, 2026
Merged

the-luap merged 1 commit into
mainfrom
fix/collector-audit-hardening

Conversation

@the-luap

@the-luap the-luap commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Slow downloads can retain shared database connections, old report timestamps can disable all-time history, and ignored nested runtime files can enter the public source archive. This change addresses all three issues.

  • Serialize each export into a private anonymous file and release its database snapshot before streaming to the client. Bound each process to two active exports, one per participant, 512 MiB per export, and a 30-second deadline. Disconnects, failures, and timeouts close the file and release admission. EXPORT_TEMP_PATH defaults to the existing writable storage directory.
  • Reject newly submitted reports before 2020-01-01 and exclude retained outliers from history bounds. Preserve supported delayed reports from 2020, idempotent retries, original private exports, and all published wire schemas.
  • Exclude nested storage, build, and coverage directories from source packaging; align Docker exclusions for nested runtime directories. The downloadable source still rebuilds without Git metadata.

Validation: npm run test:required passes with 117 backend/integration tests and 38 Chromium tests, using SQLite, PostgreSQL 17, and the CI-pinned PicPeak client. New regressions exercise paused HTTP downloads, unrelated reads and opt-out, export limits and cleanup, timestamp poisoning, and archive rebuilds. The hardened Docker runtime smoke test passes. A Docker build containing synthetic private runtime files excludes them from both the shipped application and public archive. npm audit reports zero known dependency vulnerabilities.

@the-luap

the-luap commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed all 11 changed files directly (git show pr-19:<path> against origin/main), not just the diff — including tracing the export lifecycle, the report-date floor on both write and read paths, and the packaging test's actual archive contents.

✅ Export isolation (server/exports.js)

The refactor is sound: prepareExport now writes into a private anonymous file (fs.open(..., "wx+", 0o600) immediately followed by fs.unlink, so no pathname survives for backups/crash leftovers) while holding the readSnapshot transaction, then the transaction closes, and only afterward does pipeline(file.createReadStream(...), res, { signal }) stream to the client. That's the actual fix for "slow downloads retain shared DB connections" — the DB work and the network transfer are no longer the same critical section.

Checked the parts that are easy to get wrong here and they're all handled:

  • Admission race: owner resolution (await collector.reader(credential)) happens before the active.has/size check, but since nothing else awaits between that resolution and active.add(owner), JS's run-to-completion semantics make this correctly serialized — two concurrent requests for the same credential can't both pass the check. Confirmed by the "session token resolves to the same owner" test (a session token for participant a correctly hits EXPORT_IN_PROGRESS while a's installation-id-authenticated export is still open).
  • Cleanup ordering: finally closes the file handle before deleting the (already-unlinked, so no-op) filename, and always drops active admission — verified by the paused/timeout/oversized tests all asserting fs.readdir(directory) is empty afterward.
  • Error-before-headers: because writes go to the temp file first, EXPORT_TOO_LARGE/query errors surface as clean JSON via the error middleware — res.set/.attachment() only happen after prepareExport returns, so res.headersSent is still false for essentially every failure except a mid-stream abort, which the global handler already special-cases (res.headersSent → res.destroy()).

The test/audit-hardening.test.js additions are unusually rigorous for this kind of fix: real HTTP server with res.pause() to force genuine backpressure, polling db.client.pool.numUsed() to prove release, an unrelated delete (opt-out) succeeding while a download is paused (proves the DB isn't held), and a size-limited config to hit EXPORT_TOO_LARGE atomically. That's testing the actual claim, not a mock.

✅ Report-date floor (reportDates.js, collector.js, history.js)

Ingest-side (collector.js): packet.payload.report_date < FIRST_REPORT_DATE throws INVALID_REPORT_DATE before any state mutation — test confirms sequence stays 0 and only one nonces/operations row exists after 3 rejected attempts (no partial side effects).

Read-side (history.js): base() now filters report_date >= FIRST_REPORT_DATE for aggregate history, while validDate's new value >= FIRST_REPORT_DATE check is a plain lexicographic string comparison — safe here specifically because it runs before the regex/format check but is ANDed with it, so a malformed string that happens to sort >= the floor (e.g. "9999-99-99") still gets caught by the subsequent Date.parse validation. Not a bug, just worth knowing the ordering is for readability, not correctness.

Good design choice confirmed by the test: pre-2020 outlier rows already in reports stay fully intact and byte-for-byte returnable via /api/participant/lookup (a user's own private export), they're just excluded from the shared aggregate /history bounds. Matches "preserve original private exports" exactly.

✅ Packaging (.dockerignore, archive-source.js)

dist/storage → **/dist/**/storage in .dockerignore is the correct fix — Docker's ignore patterns without a **/ prefix only match at the build-context root, so a nested leftover (e.g. web/some-package/dist, or a stray storage from local testing) previously rode straight through COPY . . into the image. archive-source.js's localNames addition closes the same gap for the downloadable source tarball.

test/packaging.test.js plants synthetic private files at 8 different nested depths and a symlink pointing outside the source tree, asserts none of it lands in the tar, and round-trips the archive (extract → re-run the script → byte-identical tarball) to catch any Git-metadata dependency. That symlink case specifically is the kind of thing this fix could plausibly have missed if it were implemented naively.

Verified but not independently re-derived

  • readSnapshot/server/database.js — unchanged by this PR (confirmed via diff), so transaction rollback-on-error semantics inside prepareExport are trusted as pre-existing, correct behavior.
  • /api/participant/lookup's unauthenticated installation_id-as-capability model — unchanged by this PR, pre-existing design (the 64-hex installation_id functions as the credential), not a new gap.

Verdict

CI green, mergeStateStatus: CLEAN. No blockers or concerns found — merge cleared from my side.

@the-luap
the-luap merged commit 48f1ae5 into main Sep 8, 2026
2 checks passed
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.

1 participant