Trim NODE_ENV before the key gate reads it, so a stray space cannot ship the public key - #354
Merged
davidmckayv merged 3 commits intoSep 4, 2026
Conversation
…hip the public key A deployment that never changed KEY_ENCRYPTION_KEY encrypts its credential vault with the key printed in .env.example. It is a valid key - right length, right encoding - so nothing else about it fails a check, and the refusal under NODE_ENV=production is the only thing standing between "copied the example file" and that outcome. That refusal compared environment.NODE_ENV raw. The other production refusal in the same file, on private-host browsing, reads it through `optional` and therefore trims, and its comment says exactly why: Through `optional`, so the comparison trims. Read raw, `NODE_ENV="production "` out of an env file would slip past a gate that the switch beside it, which does trim, would still trip. Both sides of the comparison come out of one env file, and a trailing space there is invisible: Docker's env_file preserves it verbatim, and so does every hosting dashboard with a text box. So `NODE_ENV=production ` tripped one gate, slipped past the other, and started the deployment on a key published in this repository with a console warning nobody reads at boot. The question moves into `isProduction`, used by both, so the next gate that needs it cannot pick the wrong way to ask. `environment.NODE_ENV` was the only raw environment read left in the file; everything else already goes through required, optional or commaSeparated. The example key had no test of its own at all, which is part of why this drifted, so the production refusal and the local warning are now pinned alongside the whitespace case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kevin9327
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
September 3, 2026 22:10
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
# Conflicts: # CHANGELOG.md
davidmckayv
approved these changes
Sep 4, 2026
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Security fix: routes both production gates through one trimmed isProduction(). Reviewed diff, resolved CHANGELOG against main, validated locally (config tests + format). Approving CI.
davidmckayv
approved these changes
Sep 4, 2026
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Security fix: both production refusals now route through one trimmed isProduction(). Reviewed diff, resolved CHANGELOG, validated locally. CI green.
davidmckayv
approved these changes
Sep 4, 2026
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Reviewed diff against current main; resolved CHANGELOG keep-both; validated locally (format + composed test run). CI green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
server/src/config.tshas two hard refusals that turn a local-only setting into a boot failure on adeployed server. Both compare
NODE_ENVagainst"production". They did not compare it the same way.The comment is exactly right, and it was describing the gate above it. Both sides of the comparison
come out of one env file, and a trailing space there is invisible: Docker's
env_filepreserves thevalue verbatim, and so does every hosting dashboard with a text box.
NODE_ENV=productiontrips theprivate-hosts refusal and slips past the key refusal.
What slips past matters more than what trips.
PLACEHOLDER_KEYis the key in.env.example, and thefile says why it is dangerous: it is a valid key — right length, right encoding — so nothing else
about it fails a check, and a deployment that never changed it encrypts its whole credential vault
with a value printed in this public repository while looking exactly like one that did. With the
space, that deployment starts. The only sign is a
console.warnat boot, in the same stream aseverything else a container prints on startup.
The fix. The question moves into one
isProduction(environment)used by both gates, so a thirdgate that needs it cannot pick the wrong way to ask.
environment.NODE_ENVwas the only rawenvironment read left in the file — every other read already goes through
required,optionalorcommaSeparated— so this closes the class, not just the instance.Dockerfile:208andcharts/openbot/templates/_helpers.tpl:128both setNODE_ENVcleanly, so thewhitespace has to come from a hand-written source: a
.env, adocker composeenvironment:entry,a manifest somebody typed, or a PaaS text box. Those are the deployments least likely to have
rotated the key.
Where it runs
at boot inside
loadConfig.answers the same way. Before this, every replica agreed too — they all agreed on the wrong
answer. The concrete outcome of the change is that such a deployment refuses to start at all,
on every replica, instead of starting on the public key.
Boundary and audit
the audit store exists, which is how the sibling refusal beside it already behaves.
Changelog
A stray space in NODE_ENV no longer lets the public example key through, underUnreleased.Proof
Three tests added to
server/tests/config.test.ts, next to the ones that already pin theprivate-hosts gate. Fail-before confirmed by restoring
environment.NODE_ENV === "production"andre-running:
With the fix:
Honest note on the other two:
refuses the example encryption key on a production deploymentandwarns about the example key and still starts under NODE_ENV=%ppass before and after. They arehere because that gate had no test of its own at all, which is part of why it drifted from the one
beside it.
Gates: