Name the NEPTUNE_NOTEBOOK/HTTPS conflict instead of blaming missing certs - #2252
Merged
Merged
Conversation
kmcginnes
added this pull request to stack #2253
September 23, 2026 21:40
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 23, 2026 22:14
43fa106 to
9f10a70
Compare
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 23, 2026 23:36
9f10a70 to
ae02189
Compare
kmcginnes
removed this pull request from stack #2253
September 23, 2026 23:36
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 24, 2026 00:04
ae02189 to
9aacf1d
Compare
kmcginnes
added this pull request to stack #2256
September 24, 2026 00:08
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 24, 2026 17:07
9aacf1d to
e939afe
Compare
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 24, 2026 18:05
e939afe to
dcfe979
Compare
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 24, 2026 18:21
dcfe979 to
c7c518e
Compare
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 24, 2026 18:43
c7c518e to
980cd1b
Compare
7 tasks
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 25, 2026 16:38
c2c7977 to
560ea2c
Compare
kmcginnes
marked this pull request as ready for review
September 25, 2026 20:56
…erts process-environment.sh writes NEPTUNE_NOTEBOOK to .env, but EnvironmentValuesSchema never declared it, so the server could not see it. An operator who set PROXY_SERVER_HTTPS_CONNECTION=true alongside NEPTUNE_NOTEBOOK=true got a startup failure about missing certificate files, when the real cause is that the notebook preset chose not to generate any. Declare NEPTUNE_NOTEBOOK in the schema and reject the pair in resolveServerConfig with a message naming both variables and how to resolve it. The server refuses to start rather than quietly discarding an explicit TLS request. The notebook preset also overwrote the value it read out of config.json before writing .env, so a conflict expressed that way was invisible. The preset now applies only when nothing was requested, which makes the conflict detectable on both configuration routes.
The standard Docker image declares ENV NEPTUNE_NOTEBOOK=$NEPTUNE_NOTEBOOK without a build argument, so the variable arrives set but empty. The schema rejected the empty value and the server exited at startup.
process-environment.sh applies the notebook preset only when the value is exactly "true". The server now agrees, so TRUE or 1 no longer makes it refuse a server the shell set up for HTTPS, and no value can fail the parse.
The preset never serves TLS, so the certificates were never used. When HTTPS was also requested and HOST was unset, setup-ssl.sh exited on the missing HOST before the server could name the real conflict.
Moves the entrypoint work directory and runner into testing.ts so other tests can drive the real entrypoint. The setup-ssl.sh stub now exits without HOST, as the real script does when no certificates exist.
Runs the real entrypoint and process-environment.sh for each image, docker -e, and config.json combination, then asserts the .env contents, whether certificates were generated, and whether the server serves HTTP, serves TLS, or refuses with the notebook conflict.
Nothing on main ever passes an empty value for this variable. Treating it as unset made the server disagree with the shell's default of true, so the server silently served HTTP while the shell believed it had generated certificates for HTTPS.
…tarts the same way
…t.sh The last-match read fixed NEPTUNE_NOTEBOOK's post-restart conflict detection, but applying it to the HTTPS read too changed behavior: a restarted container used to reuse its TLS certificate because the first-match read on a repeated "true" doesn't satisfy the exact-match check, so setup-ssl.sh never reran. Put that read back exactly as it is on main and explain why the two reads differ.
process-environment.sh previously kept whatever value PROXY_SERVER_HTTPS_CONNECTION already had under the NEPTUNE_NOTEBOOK preset, so a non-boolean config.json value (null, a number, an unrecognized string) reached .env and failed the server's boolean schema instead of resolving to HTTP as the preset intends. Only a case-insensitive "true" is now kept; every other value is forced to false, matching main's behavior except when both values are genuinely true.
… message The conflict message told operators to drop PROXY_SERVER_HTTPS_CONNECTION without saying it could be set through -e flags or config.json, which is where the notebook preset actually sees it from.
The comment described the parse error stopping "one that runs on main," which reads as a diff against a prior state rather than a rationale for the current behavior.
Both tests were named "grep ignores similarly-named variables"; the one under the notebook preset describe block now names the variable it covers.
kmcginnes
force-pushed
the
name-neptune-notebook-https-conflict
branch
from
September 25, 2026 21:22
1332522 to
8a1da20
Compare
5 tasks
kmcginnes
added a commit
that referenced
this pull request
Sep 25, 2026
## Description When the configuration folder (`./packages/graph-explorer`) is mounted read-only, every write `process-environment.sh` makes to `.env` fails, but the script still exits 0. `docker-entrypoint.sh` then finds no `PROXY_SERVER_HTTPS_CONNECTION` in `.env`, prints "SSL disabled", and the server comes up on plain HTTP. The documented default is TLS, and nothing in the logs says why it changed. **Before.** The operator saw `Permission denied` lines from the shell, then `SSL disabled. Skipping self-signed certificate generation.`, then a server on HTTP only. **Now.** The container exits with code 1 before it writes anything and prints: ``` Graph Explorer can't start because it can't write ./packages/graph-explorer/.env. The container writes its settings to the configuration folder at startup, so ./packages/graph-explorer must be writable. Check that it isn't mounted read-only. ``` A small `require_writable` helper does the check. It always checks `.env`. It also checks `defaultConnection.json` when `PUBLIC_OR_PROXY_ENDPOINT` is set, and it runs that check first so a failure leaves `.env` untouched. It also drops a trailing slash from `CONFIGURATION_FOLDER_PATH` so the message doesn't print `//.env`. I didn't turn on `set -e` in `process-environment.sh`. The rest of the script wasn't written for it, and it could stop a working container on an unrelated line. `config.json` mounts at `/graph-explorer/config.json`, which is outside the configuration folder, so a read-only `config.json` mount still works. None of the deploy guides, samples, or the ECS task definition mount the configuration folder. Setups that run the image as a non-root user will now get this message instead of falling back to HTTP with no explanation. ## How to read 1. [process-environment.sh](https://github.com/aws/graph-explorer/pull/2267/files#diff-e62ded2836ad4c01e5bdd53e5007666fdfaa0c41a375e274e54779d8bc60ef32): the helper and the two checks 2. [config-pipeline.test.ts](https://github.com/aws/graph-explorer/pull/2267/files#diff-9aec24fb724d133e3283ba2528fe843e863cfef9aae861720184b06747c5c8ba): new rows in #2252's deployment scenario table run the real `docker-entrypoint.sh` and `process-environment.sh` with a read-only configuration folder (standard and notebook images), a read-only `.env`, and a read-only `defaultConnection.json`. Each expects exit 1, the message above, no server start, and an untouched folder. A `readOnly` field on the row sets up the permissions. 3. `docs/agents/testing.md`: documents the `readOnly` row field and the `cannotWrite(file)` expectation 4. `docs/references/configuration.md`: the `CONFIGURATION_FOLDER_PATH` entry now says what gets written there and that the folder must be writable, and notes that the `config.json` read-only mount is unaffected 5. `docs/guides/troubleshooting.md`: a new section named after the startup message, with the causes and the fix 6. `docs/guides/deploy-to-ecs-fargate.md`: a note against `readonlyRootFilesystem` or a non-root `user` on the task definition ## Validation - The new scenario rows fail on `main` and pass here. They skip when run as root, since root ignores file modes. - `pnpm checks` and `pnpm test` pass (2861 tests). - I built the image from this branch and ran it under Colima: - With `--read-only`, and separately with the configuration folder mounted `:ro`, the container exited 1 with the message above. - Started normally with `HOST=localhost`, it came up on `https://localhost`. - Started with `-e NEPTUNE_NOTEBOOK=true`, it came up on `http://localhost`. - I also confirmed the `CONFIGURATION_FOLDER_PATH` workaround the docs describe: with the default configuration folder still mounted `:ro`, pointing `CONFIGURATION_FOLDER_PATH` at an empty writable directory let the container start normally. An empty directory is enough; nothing needs to be pre-seeded there. ## Related Issues - Related to #551 - Related to #2187 - Related to #1673 ### Check List - [x] I confirm that my contribution is made under the terms of the Apache 2.0 license. - [x] I have verified `pnpm checks` passes with no errors. - [x] I have verified `pnpm test` passes with no failures. - [x] I have covered new added functionality with unit tests if necessary. - [x] I have updated documentation if necessary.
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.
Description
When
NEPTUNE_NOTEBOOKandPROXY_SERVER_HTTPS_CONNECTIONare both true, the server now refuses to start and says why. Before, this pair failed with "certificate files are missing", which sent people looking for certificates the notebook preset never generates.How the pieces fit:
NEPTUNE_NOTEBOOKis inEnvironmentValuesSchemanow. It parses with an exact match on"true", the same testprocess-environment.shand the entrypoint use, and it never fails the parse.TRUEor1mean "not the preset" in all three places.A
superRefineon the schema rejects the pair atPROXY_SERVER_HTTPS_CONNECTION, with a message that names both variables and both ways out. The parse runs beforeresolveServerConfiglooks for certificates, so the operator sees the conflict first.resolveServerConfiggoes back to checking only files on disk.process-environment.shkeeps an HTTPS value under the preset only when it reads astruein any case, and otherwise applies the preset'sfalseas before. That way aconfig.jsonnull,0or"yes"still serves HTTP. Onmainthe preset overwrote the value it had just read fromconfig.json, so the preset could discard an explicit HTTPS request without an error. Aconfig.jsonwith both true used to serve HTTP. Now it refuses.docker-entrypoint.shskipssetup-ssl.shunder the preset, because an HTTP-only server never uses the certificates. Without that, a notebook container with noHOSTwould exit insetup-ssl.shbefore the server could name the conflict.The entrypoint starts node with the
NEPTUNE_NOTEBOOKvalue from.env. The container's own value can differ from.env, sinceconfig.jsonreplaces it, and dotenv never overrides a variable that's already set. Passing the.envvalue means the server checks for the conflict exactly when the shell applied the preset.Restarts.
process-environment.shappends to.envon every start, and a container keeps that file acrossdocker restartand--restart always. The entrypoint read every matching line forNEPTUNE_NOTEBOOK, so on the second start it came through astrue\ntrue, the preset check failed, and the certificate error came back. That read now takes the last line, which is what dotenv does.PROXY_SERVER_HTTPS_CONNECTIONkeeps its original first-match read on purpose. A restarted TLS container's.envalso gets a repeatedtrue, and the first-match read doesn't satisfy the exact"true"check, sosetup-ssl.shis skipped. The container keeps serving TLS on the certificate from its first start instead of generating a new one and breaking trust for anyone who accepted the original root CA.Breaking changes
NEPTUNE_NOTEBOOK=trueandPROXY_SERVER_HTTPS_CONNECTION=truetogether now refuses to start. That includes aconfig.jsonwith both true, which used to serve HTTP.-e PROXY_SERVER_HTTPS_CONNECTION=trueand certificates mounted into the container. OnmainI expect that setup served TLS, since the shell skipped generation and the server found the mounted files. I doubt anyone runs it, but it's a change. The error names both ways out.console.error("Failed to parse environment values")instead of a pinoFATALline, and under the preset the log style iscloudwatch. An alarm that matchesFATALwon't fire on it. The exit code is still 1. The certificate skip message also changed from "SSL disabled. Skipping..." to "Neptune Notebook preset enabled. Skipping...".Validation
config-pipeline.test.tshas a scenario table. Each row runs the realdocker-entrypoint.shandprocess-environment.sh, with onlysetup-ssl.shand the node start stubbed, then applies dotenv precedence, the Zod schema andresolveServerConfig. Rows cover both images (NEPTUNE_NOTEBOOKset to""ortrue),-evalues,config.json, a missingHOST, and a second start in the same container. Each asserts what lands in.env, whether certificates get generated, and what the server does at startup. Parse-failure rows check which key failed.I ran the table against
main's scripts and server code. The only rows that differ are the ones where both values end up true, which now refuse instead of failing later on missing certificates. The TLS restart row matchesmain: certificates aren't regenerated on the second start, and the container keeps serving TLS on the certificate from the first. Rows for known quirks pin what happens today. For example,config.jsonwith HTTPS true plus-e PROXY_SERVER_HTTPS_CONNECTION=falsegenerates certificates and then serves HTTP. That precedence split is tracked in #1673.I also ran both images in Docker:
-e PROXY_SERVER_HTTPS_CONNECTION=trueexits 1 with the conflict message.docker starton the same container gives the same message.HOST=localhostserves TLS, and still serves TLS on the same root CA fingerprint afterdocker restart.docs/agents/testing.mdnow describes the entrypoint test harness, anddocs/adr/20260925-refuse-neptune-notebook-https-conflict.mdrecords why the pair is refused instead of letting one side win.pnpm checksandpnpm testpass: 224 files, 2819 tests.Related Issues
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.