Skip to content

fix(security): close the remaining triage advisories - #511

Merged
lindesvard merged 5 commits into
mainfrom
fix/security-triage-remaining
Sep 22, 2026
Merged

lindesvard merged 5 commits into
mainfrom
fix/security-triage-remaining

Conversation

@lindesvard

Copy link
Copy Markdown
Contributor

Summary

Closes the five remaining items from the security advisory triage. Stacked on #510 (needs the share-access helper from it); merge #510 first and this PR retargets to main.

Unauthenticated reads (GHSA-vrrm-p9p4-2gfg, GHSA-c98x-mph7-r6vp dup, GHSA-r4g5-vgpj-923m) — Medium

  • reference.getChartReferences had no access check. event.bots let anonymous callers through whenever a shareOverview row existed, ignoring public and the password.
  • Both now use a new hasAnonymousShareAccessToProject(projectId, cookies, kinds) in share.service.ts: the project must have a public share that is password-free or unlocked with a verified share-access cookie. Members still need project access. Share pages keep working because their viewers hold exactly that cookie.

ClickHouse escaping (GHSA-7fm7-rprq-g8rm, GHSA-gvwr-5684-wjqc, GHSA-gvr5-22f4-pj6g) — Medium

  • conversion.service: event names, dates and project id were interpolated raw inside quotes. Now sqlstring.escaped.
  • cohort.service profileColumnAccess: the column is an identifier and cannot be escaped, so it now enforces the same allowlist filter-where.service uses and throws on anything else.
  • sankey.service: quote-doubling replaced with sqlstring.escape in all eight places. grep -rn "replace(/'/g" packages/db/src is now empty.

Bull Board (GHSA-r627-6vrh-65p9) — Medium

  • Mounted only when BULLBOARD_USERNAME and BULLBOARD_PASSWORD are set, behind HTTP Basic auth with constant-time compare. Without them the worker logs a warning and serves only /healthcheck and /metrics (registered before the guard).
  • Coolify template gets generated credentials. Docs updated.
  • Behavior change for self-hosters: the dashboard is off until they set the two variables.

Secrets in logs (GHSA-xr2x-w49w-hp2c) — Low

  • Migration banner redacts credentials in DATABASE_URL / CLICKHOUSE_URL (userinfo and password/sslpassword/user query params; comma-separated ClickHouse URLs handled).
  • Email fallback logs one line with a redacted recipient instead of dumping template data.

Unsubscribe secret (GHSA-cv3v-4j56-hr88 side note) — Medium

  • No more literal default secret. Requires UNSUBSCRIBE_SECRET or COOKIE_SECRET, read at call time so a missing value fails that request with a clear error.
  • Coolify worker now gets COOKIE_SECRET. Before this, the worker on Coolify was signing unsubscribe links with the default and the API was verifying with the real secret, so those links were already broken there.

Test plan

  • New: reference.test.ts (anon refused / anon with unlocked share served / member without access refused / member served)
  • New: basic-auth.test.ts (accept, missing header, wrong user/password/malformed, colon in password)
  • Existing share and js-runtime suites still pass (75 + 10)
  • Typecheck: no errors in touched files
  • Manual: open a public overview share, confirm chart annotations and bot list still render
  • Manual: set the two Bull Board vars on a worker, confirm the browser prompts and the board loads; without them confirm /healthcheck still returns 200

Not in this PR

  • Live visitors websocket has no session check (only returns a count; leftover from the already-fixed GHSA-8wr9 draft)
  • Site-checker and MCP query-token points from GHSA-cv3v: design decisions, discussed in triage

🤖 Generated with Claude Code

lindesvard and others added 3 commits September 22, 2026 21:31
…rd bypass

Webhook JavaScript templates (GHSA-mc99-9jf5-22cq, GHSA-fmf9-23m7-xg84):
the validator only refused 'constructor' when it was the callee of a member
call or the target of an assignment. Reading it into a local, destructuring
it, a sequence-expression callee, a call-of-call and a tagged template all
walked past the checks and reached the Function constructor in the worker.
The validator now refuses reading constructor/__proto__/prototype/caller/
callee through any member access or destructuring pattern, refuses dynamic
computed keys (obj[expr]) so a forbidden name cannot be assembled at run
time, refuses tagged templates, and only accepts identifier, member and
inline-arrow callees.

Share password cookie (GHSA-p6c2-mq9r-cx3r): the overview, dashboard and
report share procedures and the db share-access validators unlocked a
password-protected share whenever a cookie named shared-<type>-<id>
existed, whatever its value. The cookie is now an HMAC over the share type,
id and current password hash keyed by COOKIE_SECRET, verified with a
constant-time compare, so it cannot be forged, cannot be replayed against
another share, and expires when the password changes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lated context

The validator was a denylist: it looked for known-bad shapes and let every
other node through, which is how each new escape got in. It is now an
allowlist of AST node types. Anything not on the list (tagged templates,
sequence expressions, switch, labels, var, ++, delete, computed keys,
object methods, TypeScript syntax, ...) is refused by default.

The language is deliberately small: object/array literals, spread, property
access with literal keys, template strings, ternaries, logical and
arithmetic operators, const/let, if, and the allowlisted built-in methods.
A template cannot call a function it defined itself: local identifiers are
never callable, so there is no recursion, no IIFE and no "store a reference
now, call it later" path. Inline arrows only appear as callbacks to the
allowlisted array methods. All templates currently saved in production are
in the test suite as fixtures.

execute() now runs the template in a fresh V8 context via node:vm with
eval and new Function disabled, a 250ms timeout and a 1MB output cap. The
payload crosses in as JSON and the result crosses out as JSON, so the
template never touches a host-realm object. This is defense in depth
behind the validator, not a boundary on its own.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e literal

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4bb630a7-6a3e-4108-abb2-925664dae74f

📥 Commits

Reviewing files that changed from the base of the PR and between 42fc862 and 466d543.

📒 Files selected for processing (23)
  • apps/public/content/docs/self-hosting/environment-variables.mdx
  • apps/worker/src/index.ts
  • apps/worker/src/utils/basic-auth.test.ts
  • apps/worker/src/utils/basic-auth.ts
  • packages/common/package.json
  • packages/common/server/share-access.ts
  • packages/db/code-migrations/migrate.ts
  • packages/db/src/services/cohort.service.ts
  • packages/db/src/services/conversion.service.ts
  • packages/db/src/services/sankey.service.ts
  • packages/db/src/services/share.service.ts
  • packages/email/src/index.tsx
  • packages/email/src/unsubscribe.ts
  • packages/js-runtime/src/execute.ts
  • packages/js-runtime/src/validate.test.ts
  • packages/js-runtime/src/validate.ts
  • packages/trpc/src/routers/auth.ts
  • packages/trpc/src/routers/event.ts
  • packages/trpc/src/routers/reference.test.ts
  • packages/trpc/src/routers/reference.ts
  • packages/trpc/src/routers/share.test.ts
  • packages/trpc/src/routers/share.ts
  • self-hosting/coolify.yml
 ________________________________________________
< PENDU: Prompt Engineered Nifty Debugging Unit. >
 ------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

lindesvard and others added 2 commits September 22, 2026 22:27
…put in bytes

Review follow-ups on the allowlist validator:

- An assignment could target a property of an allowed global
  (Math.round = (x) => Math.round(x)), replacing a built-in for the rest of
  the run and looping until the timeout. Assignments now have to be rooted
  at a local variable.
- A nested arrow is now only accepted as an argument passed directly to a
  call. Stored or assigned arrows are never callable, so they had no
  legitimate use.
- The output cap measured UTF-16 code units; it now measures UTF-8 bytes,
  which is what goes on the wire.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Unauthenticated reads (GHSA-vrrm-p9p4-2gfg, GHSA-c98x-mph7-r6vp,
GHSA-r4g5-vgpj-923m): reference.getChartReferences had no access check at
all, and event.bots let any anonymous caller through as long as a
shareOverview row existed, ignoring public and password. Both now go
through hasAnonymousShareAccessToProject, which requires a public share for
the project that is either password-free or unlocked with a verified
share-access cookie. Members still need project access.

ClickHouse escaping (GHSA-7fm7-rprq-g8rm, GHSA-gvwr-5684-wjqc,
GHSA-gvr5-22f4-pj6g): conversion.service interpolated event names, dates
and the project id raw inside quotes; they are now sqlstring-escaped. The
cohort profile filter column is an identifier and cannot be escaped, so
profileColumnAccess now enforces the same column allowlist that
filter-where.service uses and throws on anything else. sankey.service
replaced quote-doubling, which ClickHouse's backslash escapes defeat, with
sqlstring.escape in all eight places.

Bull Board (GHSA-r627-6vrh-65p9): the queue dashboard can add, retry and
clean jobs and was mounted with no auth. It now mounts only when
BULLBOARD_USERNAME and BULLBOARD_PASSWORD are set, behind HTTP Basic auth
with constant-time comparison, and /healthcheck and /metrics are registered
before it so they stay reachable. The Coolify template provides generated
credentials; docs updated.

Logs (GHSA-xr2x-w49w-hp2c): the migration banner printed full DATABASE_URL
and CLICKHOUSE_URL; credentials and credential-like query params are now
redacted. The email fallback dumped recipient, subject and template data
(including password-reset links); it now logs a one-line warning with a
redacted recipient.

Unsubscribe tokens (GHSA-cv3v-4j56-hr88 side note): the HMAC secret fell
back to a literal default, so links were forgeable. It now requires
UNSUBSCRIBE_SECRET or COOKIE_SECRET and fails the request otherwise. The
Coolify worker gets COOKIE_SECRET so it signs with the same key the API
verifies with.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@lindesvard
lindesvard force-pushed the fix/security-triage-remaining branch from b4a6cfb to 466d543 Compare September 22, 2026 20:27
Base automatically changed from fix/security-sandbox-and-share-cookie to main September 22, 2026 20:44
@lindesvard
lindesvard merged commit f8bb757 into main Sep 22, 2026
12 of 13 checks passed
@lindesvard
lindesvard deleted the fix/security-triage-remaining branch September 22, 2026 20:44
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