From 436a9abd67703f605739cde20f990ca84cc5b290 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 17:54:11 +0000 Subject: [PATCH] One list of secret names, and a tripwire for the drift Adversarial review of #197/#198 found a bug I shipped: compose.yaml declares OAUTH_AUTH0_CLIENT_SECRET and OAUTH_GOOGLE_CLIENT_SECRET as Docker secrets, and SECRET_NAMES did not list them. That gap is silent. A secret in it is mounted into the container and never read, so the value falls back to the environment -- and a deployment that moved it out of .env and into a Docker secret, which is exactly what the secrets work invites, would lose it. The first symptom is a login that stops working, a long way from the cause. There were three hand-maintained lists: SECRET_NAMES, compose.yaml's secrets block, and a copy inside chat-fastapi.py that had drifted from SECRET_NAMES in both directions -- missing four names and carrying one the shared list lacked. chat-fastapi.py now uses SECRET_NAMES. Two tests make the drift loud: every secret compose declares must be one the app loads, and no second literal list may exist. Removing the OAuth names again fails the first, which is the check that matters. env_template documented none of the OAuth settings, which is why they were easy to miss. All seven are there now, commented out, with a note that the secrets can be mounted instead. LITERAL_API_KEY stays the one documented exception -- loadable, unused by any deployment here. Co-Authored-By: Claude Opus 5 --- bin/chat-fastapi.py | 14 ++++-------- env_template | 13 +++++++++++ src/util/secrets.py | 20 ++++++++++++---- tests/util/test_secrets.py | 47 +++++++++++++++++++++++++++++++++----- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/bin/chat-fastapi.py b/bin/chat-fastapi.py index c7169ce..c49a9f1 100644 --- a/bin/chat-fastapi.py +++ b/bin/chat-fastapi.py @@ -11,18 +11,12 @@ from fastapi import FastAPI, Request, Response from fastapi.responses import HTMLResponse, RedirectResponse -from util.secrets import get_secret, load_secrets_to_environ +from util.secrets import SECRET_NAMES, get_secret, load_secrets_to_environ load_dotenv() -load_secrets_to_environ( - [ - "CHAINLIT_AUTH_SECRET", - "OAUTH_AUTH0_CLIENT_SECRET", - "OAUTH_GOOGLE_CLIENT_SECRET", - "OPENAI_API_KEY", - "TAVILY_API_KEY", - ] -) +# The same list chat-chainlit uses. This was a second, hand-maintained copy +# that had already drifted from it in both directions. +load_secrets_to_environ(SECRET_NAMES) app = FastAPI() diff --git a/env_template b/env_template index 9ec2303..f98d680 100644 --- a/env_template +++ b/env_template @@ -40,3 +40,16 @@ LOCALE=en_US TIMEZONE=America/Toronto # The guest instance's public URI, served alongside the authenticated one. CHAINLIT_URI_NO_LOGIN= + +# OAuth sign-in. Only the providers whose CLIENT_ID is set are offered; leave a +# pair blank to disable that provider. The *_SECRET values can be supplied as +# Docker secrets instead (see the note at the top). +#OAUTH_AUTH0_CLIENT_ID= +#OAUTH_AUTH0_CLIENT_SECRET= +#OAUTH_AUTH0_DOMAIN= +#OAUTH_GOOGLE_CLIENT_ID= +#OAUTH_GOOGLE_CLIENT_SECRET= +#OAUTH_ORCID_CLIENT_ID= +#OAUTH_ORCID_CLIENT_SECRET= +# Signs the session cookie. Required for any OAuth provider to work. +#CHAINLIT_AUTH_SECRET= diff --git a/src/util/secrets.py b/src/util/secrets.py index 16a04ef..11ab66c 100644 --- a/src/util/secrets.py +++ b/src/util/secrets.py @@ -118,14 +118,26 @@ def load_secrets_to_environ(names: Iterable[str]) -> None: # The values worth mounting rather than passing as environment variables. Names # match env_template, so a deployment can move one across without renaming it. +# This is the ONE list. compose.yaml's `secrets:` block must be a subset of it -- +# tests/util/test_secrets.py asserts that, because the failure is silent: a +# secret declared in compose and missing here is mounted into the container and +# never read, so the value falls back to the environment. A deployment that +# moved a secret OUT of its .env and into a Docker secret would then lose it, +# and the first symptom is a login that stops working. +# +# That had already happened: compose.yaml declared the two OAuth secrets and +# this tuple did not list them. SECRET_NAMES = ( + "CHAINLIT_AUTH_SECRET", + "CLOUDFLARE_SECRET_KEY", + "LITERAL_API_KEY", + "OAUTH_AUTH0_CLIENT_SECRET", + "OAUTH_GOOGLE_CLIENT_SECRET", + "OAUTH_ORCID_CLIENT_SECRET", "OPENAI_API_KEY", - "POSTGRES_PASSWORD", "PGADMIN_DEFAULT_PASSWORD", - "CLOUDFLARE_SECRET_KEY", + "POSTGRES_PASSWORD", "TAVILY_API_KEY", - "CHAINLIT_AUTH_SECRET", - "LITERAL_API_KEY", ) diff --git a/tests/util/test_secrets.py b/tests/util/test_secrets.py index 622821c..30ee9a1 100644 --- a/tests/util/test_secrets.py +++ b/tests/util/test_secrets.py @@ -124,16 +124,51 @@ def test_missing_secrets_directory_is_not_an_error( assert get_secret("OPENAI_API_KEY") == "sk-from-env" +def test_every_secret_compose_declares_is_one_the_app_loads() -> None: + """The drift that had already happened, now a tripwire. + + compose.yaml declared OAUTH_AUTH0_CLIENT_SECRET and + OAUTH_GOOGLE_CLIENT_SECRET; SECRET_NAMES did not list them. A secret in that + gap is mounted into the container and never read, so its value silently + falls back to the environment -- and a deployment that moved it out of .env + and into a Docker secret would lose it. The first symptom is a login that + stops working, a long way from the cause. + """ + import yaml + + compose = yaml.safe_load( + (Path(__file__).parent.parent.parent / "compose.yaml").read_text() + ) + declared = set(compose.get("secrets") or {}) + missing = sorted(declared - set(SECRET_NAMES)) + assert not missing, ( + f"compose.yaml mounts these but nothing loads them: {missing}. " + "Add them to SECRET_NAMES in src/util/secrets.py." + ) + + +def test_only_one_list_of_secret_names_exists() -> None: + """chat-fastapi.py kept its own hand-maintained copy, which had drifted from + SECRET_NAMES in both directions -- it was missing four and had one extra.""" + source = ( + Path(__file__).parent.parent.parent / "bin" / "chat-fastapi.py" + ).read_text() + assert "load_secrets_to_environ(SECRET_NAMES)" in source + assert '"OPENAI_API_KEY",' not in source, "no second literal list" + + def test_the_secret_names_are_ones_the_deployment_actually_uses() -> None: """A name here that no template mentions would never be mounted.""" template = Path(__file__).parent.parent.parent / "env_template" declared = template.read_text() - unknown = [ - n - for n in SECRET_NAMES - if n not in declared and n not in {"CHAINLIT_AUTH_SECRET", "LITERAL_API_KEY"} - ] - assert not unknown, f"not in env_template: {unknown}" + # LITERAL_API_KEY is the one exception: it configures the Literal AI + # integration, which no deployment here uses yet, so it is loadable but + # undocumented on purpose. + unknown = [n for n in SECRET_NAMES if n not in declared and n != "LITERAL_API_KEY"] + assert not unknown, ( + f"in SECRET_NAMES but absent from env_template: {unknown}. " + "An operator cannot supply a setting nobody told them about." + ) def test_db_uri_uses_the_socket_and_never_tcp(