Conversation
`EXPRESS_SESSION_SECRET` fell back to the literal `hyperdx is cool 👋`, so anyone able to read the source could sign a `connect.sid` for a session id of their choosing. Generate a random 32-byte secret per process instead, and warn at startup, because a generated secret means every user is signed out when the process restarts and replicas no longer share sessions. A configured secret is still honoured, except for the public default older releases shipped — that is replaced like an unset one, so existing deployments stop signing with it without having to notice. The local all-in-one entry script no longer exports the placeholder for the same reason. Refs hyperdxio#3089
🦋 Changeset detectedLatest commit: 954a8bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@28Hus is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
external
needs-vouch
|
Hi @28Hus, thanks for the pull request! Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches. To get vouched, open an issue saying hello and what you're working on: https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide. |
Greptile SummaryThis PR removes the repository-public session signing secret.
Confidence Score: 5/5The PR appears safe to merge; the prior replica-documentation concern is resolved and no new actionable issues were identified. The current implementation removes the public signing key, uses cryptographic randomness when configuration is absent, warns about the operational consequences, and now documents that replicas cannot share sessions without a configured secret.
|
| Filename | Overview |
|---|---|
| packages/api/src/config.ts | Replaces the public signing-secret fallback with a cryptographically random per-process value and exposes whether it was generated. |
| packages/api/src/api-app.ts | Warns operators when the API uses a generated session secret. |
| packages/api/src/tests/config.test.ts | Covers configured, unset, empty, and independently generated session secrets. |
| docker/hyperdx/entry.local.base.sh | Stops all-in-one images from exporting the public default secret. |
| DEPLOY.md | Documents restart invalidation and the requirement for a shared secret across replicas. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[API startup] --> B{EXPRESS_SESSION_SECRET configured?}
B -->|Yes| C[Use configured shared secret]
C --> D[Sessions remain valid across restarts and replicas]
B -->|No| E[Generate random per-process secret]
E --> F[Log deployment warning]
E --> G[Sessions become invalid after restart]
E --> H[Replicas cannot verify each other's cookies]
Reviews (2): Last reviewed commit: "Update DEPLOY.md" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Deep Review✅ No critical issues found. This is a well-scoped, correctly implemented security fix. The public default session secret is removed with no fallback, an unset/empty value produces a per-process 256-bit random secret ( The per-process secret's runtime consequences — sign-out on restart and non-shared sessions across replicas — are the intended, documented trade-off of removing the committed secret, surfaced via the startup warning and the changeset. They are operational costs, not regressions this diff should block on. 🔵 P3 nitpicks (1)
Reviewers (4): correctness, security, testing, maintainability. Testing gaps:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # "all-in-one-auth" so that the INGESTION_API_KEY bootstrap path is enabled. | ||
| export HYPERDX_IMAGE=$([[ "${IS_LOCAL_APP_MODE}" == "DANGEROUSLY_is_local_app_mode💀" ]] && echo "all-in-one-noauth" || echo "all-in-one-auth") | ||
| export EXPRESS_SESSION_SECRET="hyperdx is cool 👋" | ||
| # EXPRESS_SESSION_SECRET is intentionally unset so images don't ship a public |
There was a problem hiding this comment.
🟠 major — All-in-one auth image now signs every user out on each container restart
entry.local.base.sh is the entrypoint for both all-in-one variants, and the auth variant (HYPERDX_IMAGE=all-in-one-auth, line 24) runs real authentication against a MongoDB persisted at /data/db (line 61). Dropping the export means the API falls into the per-process branch in packages/api/src/config.ts:21, so docker restart / image upgrade invalidates every connect.sid and logs all users out — and this image isn't covered by the EXPRESS_SESSION_SECRET guidance added to DEPLOY.md. Generate the secret once per install in the entry script and persist it beside the Mongo data instead of shipping a public one, e.g. SECRET_FILE=/data/db/.express_session_secret; [ -s "$SECRET_FILE" ] || openssl rand -hex 32 > "$SECRET_FILE"; export EXPRESS_SESSION_SECRET="$(cat "$SECRET_FILE")" (guarded by [ -z "${EXPRESS_SESSION_SECRET}" ] so an operator-supplied value still wins).
There was a problem hiding this comment.
Agreed with this too - all in one is a playground-esque env- I think it's fine to keep the env variable here too with a comment
|
|
||
| Additionally, you'll want to set the `EXPRESS_SESSION_SECRET` environment | ||
| variable to a random string. | ||
| Additionally, set the `EXPRESS_SESSION_SECRET` environment variable to a random |
There was a problem hiding this comment.
🟠 major — Compose-stack instruction to set EXPRESS_SESSION_SECRET has no effect — the var isn't forwarded
The app service in docker-compose.yml:55-92 lists its environment explicitly and has no env_file, so EXPRESS_SESSION_SECRET set in the root .env or the host shell never reaches the container; the API keeps generating one per process and users are still signed out on every docker compose up. Forward it the way TOKEN_ENCRYPTION_KEY is (docker-compose.yml:76, documented in .env:33-35): add EXPRESS_SESSION_SECRET: ${EXPRESS_SESSION_SECRET:-} to the app service and a commented placeholder to .env.
There was a problem hiding this comment.
We should make this change as well so we can allow passing custom variable from .env.local at root
| # Ports come from scripts/dev-env.sh (worktree isolation) or root .env (defaults) | ||
| HYPERDX_LOG_LEVEL=debug | ||
| EXPRESS_SESSION_SECRET="hyperdx is cool 👋" | ||
| # EXPRESS_SESSION_SECRET is intentionally unset; see packages/api/src/config.ts |
There was a problem hiding this comment.
🟠 major — Local dev now logs the developer out on every nodemon restart
packages/api/package.json:99 runs the API under nodemon, so every edit under packages/api/src restarts the process and regenerates the secret, invalidating the dev session cookie and forcing a re-login on each save (dev runs with auth on — IS_LOCAL_APP_MODE is unset here). Now that config.ts has no committed fallback, a literal in this dev-only file can't leak into an image, and the file already carries dev placeholders like INGESTION_API_KEY (line 25); restore a value such as EXPRESS_SESSION_SECRET="local-development-only".
There was a problem hiding this comment.
Agreed - for local dev we should keep this value
| }; | ||
|
|
||
| if (config.IS_EXPRESS_SESSION_SECRET_GENERATED) { | ||
| logger.warn( |
There was a problem hiding this comment.
🔵 minor — The new warning is suppressed in exactly the deployments that lack a secret
logger.warn is gated by MAX_LEVEL (packages/api/src/utils/logger.ts:11,116), and docker/hyperdx/entry.local.base.sh:3 unconditionally exports HYPERDX_LOG_LEVEL="error" while lines 76-82 redirect all app output to /var/log/app.log — so all-in-one operators, the ones this PR leaves without a secret, see nothing in docker logs. Raise it to logger.error and move it next to the other startup checks in Server.start (packages/api/src/server.ts:92, where verifyTokenEncryption reports the analogous TOKEN_ENCRYPTION_KEY-unset state), ideally with the same alertable counter pattern; module-scope in api-app.ts also fires it in local (no-auth) mode where sessions aren't used for authentication at all (api-app.ts:67).
There was a problem hiding this comment.
Please check this - I think it makes sense
PR Review4 finding(s): 🔴 0 critical · 🟠 3 major · 🔵 1 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
| @@ -0,0 +1,9 @@ | |||
| --- | |||
| '@hyperdx/api': patch | |||
There was a problem hiding this comment.
This should be a minor as it changes how the secret lookup works
| CLICKHOUSE_USER=api | ||
| RUN_SCHEDULED_TASKS_EXTERNALLY=true | ||
| EXPRESS_SESSION_SECRET="hyperdx is cool 👋" | ||
| # EXPRESS_SESSION_SECRET is intentionally unset; see packages/api/src/config.ts |
There was a problem hiding this comment.
Let's just remove this line
brandon-pereira
left a comment
brandon-pereira
left a comment
There was a problem hiding this comment.
@28Hus thanks for the PR - left some feedback. mostly its small.
2 participants
Why
EXPRESS_SESSION_SECRETfell back to the literalhyperdx is cool 👋, which iscommitted to this repository (#3089). Anyone who can read the source can sign a
connect.sidfor a session id of their choosing, so the value gives the sessioncookie no integrity.
What changed
When
EXPRESS_SESSION_SECRETis unset, the API now generates a random secret atstartup instead of falling back to a public string, and logs a warning. A
configured value is used as-is.
As @brandon-pereira asked, this makes the variable effectively required: without
it, restarting the API signs every user out, and replicas do not share sessions.
DEPLOY.mdnow says so, and the changeset documents the symptom.The public placeholder is also removed from
docker/hyperdx/entry.local.base.sh,which was exporting it to every all-in-one image.
Testing
packages/apiunit suite passes (1024 tests)Fixes #3089