docs(api): correct the configuration reference (NEH-191) - #252
Merged
Conversation
NEH-134 removed UVICORN_HOST, UVICORN_PORT and LOG_LEVEL from .env.example because nothing reads them, but API_REFERENCE.md kept serving all three from its own config block — including the security trap that issue was about, since an operator setting UVICORN_HOST to 127.0.0.1 changes nothing and the backend stays on 0.0.0.0. Dropped, with a note that host and port are fixed by the pixi tasks and belong at the firewall or proxy instead. CORS: the section said to edit allow_origins in app/main.py, which has read settings.CORS_ORIGINS for some time (main.py:33), so following it meant editing source to change a config value. Now documents the env var, with the note that production is same-origin behind Nginx anyway. DATABASE_URL is kept but labelled correctly: it is real, read only by alembic/env.py as an optional override, and never by the application. Its example also used a bare postgresql:// URL, which selects psycopg2 — not installed here — and is the exact mistake DEVELOPMENT.md warns about; corrected to postgresql+psycopg://. Points at .env.example as the authoritative variable list rather than maintaining a second copy, which is how these two drifted apart.
There was a problem hiding this comment.
Pull request overview
Aligns API configuration documentation with actual backend behavior.
Changes:
- Removes unsupported server environment variables.
- Clarifies Alembic-only
DATABASE_URLusage and psycopg3 syntax. - Documents environment-driven CORS configuration.
|
|
||
| The repository root `.env.example` is the authoritative list of supported variables; the block above shows only those relevant to the API. Keep the two in step — a variable the backend does not read belongs in neither. | ||
|
|
||
| The server host and port are not configurable by environment: the pixi `start` and `dev` tasks hardcode `--host 0.0.0.0 --port 8000`. Bind the backend to a specific interface at the firewall or the reverse proxy, not in `.env`. |
Review caught a real error in the note I added. It said to bind the backend to a specific interface "at the firewall or the reverse proxy", which conflates two different things: a firewall filters access and does not change a bind address, and Nginx is a reverse proxy that restricts nothing at all. As written an operator could believe deploying Nginx keeps port 8000 off the LAN. It does not. Replaced with the real mechanism: uvicorn does listen on 0.0.0.0:8000, and what keeps that off the venue LAN is scripts/setup-firewall.sh, where ufw defaults to deny-inbound and allows 8000/tcp only from the appliance compose network 172.30.0.0/24 — so Nginx reaches the backend and the LAN does not. Notes that container ports are handled instead by loopback binds in docker-compose.yml, since Docker's iptables chains sit ahead of ufw's, and that changing the actual bind means editing the uvicorn invocation in backend/pixi.toml.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/developers/API_REFERENCE.md:2089
- This says the two lists are kept in sync, but the newly documented
DATABASE_URLis absent from the authoritative.env.example. That makes the authority unclear for this supported Alembic override. Distinguish application variables from the extra migration-only option rather than claiming both lists match.
The repository root `.env.example` is the authoritative list of supported variables; the block above shows only those relevant to the API. Keep the two in step — a variable the backend does not read belongs in neither.
| CORS_ORIGINS=["http://digitool.local","http://localhost:5173","http://localhost:3000"] | ||
| ``` | ||
|
|
||
| Do **not** edit `allow_origins` in `app/main.py` — it reads `settings.CORS_ORIGINS`, so a source edit is overridden by configuration. |
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.
Closes NEH-191.
NEH-134 removed
UVICORN_HOST,UVICORN_PORTandLOG_LEVELfrom.env.examplebecause nothing reads them.API_REFERENCE.mdkept serving all three from its own configuration block, so the guidance that issue closed was still reaching anyone who read the reference instead of the template.That includes the security-relevant part: NEH-134's argument was that an operator hardening a unit by setting
UVICORN_HOST=127.0.0.1changes nothing, because the pixistart/devtasks hardcode--host 0.0.0.0 --port 8000. This file still told them to do it.Changes
Dead knobs dropped.
UVICORN_HOST,UVICORN_PORT,LOG_LEVEL— confirmed unread by grep acrossbackend/app,backend/pixi.tomland the compose files. Replaced with a line saying host and port are not environment-configurable and belong at the firewall or the reverse proxy.CORS section corrected. It said to set production origins by editing
allow_originsinapp/main.py. That has readsettings.CORS_ORIGINSfor some time (app/main.py:33), so following the reference meant editing source to change a configuration value — and the edit would be overridden anyway. Now documents the env var, with a note that the production stack is same-origin behind Nginx so CORS should not arise in normal use.DATABASE_URLlabelled accurately. It is not dead, contrary to first appearances:alembic/env.py:50reads it as an optional override, falling back to building the URL from theDATABASE_*settings. The application never reads it. It is now marked Alembic-only rather than sitting unqualified in a general app-config listing.One extra defect found in the same line. That
DATABASE_URLexample used a barepostgresql://URL. SQLAlchemy resolves that to psycopg2, which is not installed — the project uses psycopg3 — so anyone copying the documented value would get a driver error. It is also the exact mistakebackend/DEVELOPMENT.mdlists under "Common AI Mistakes to Avoid" (item 4). Corrected topostgresql+psycopg://.Root cause addressed. The block now points at
.env.exampleas the authoritative variable list instead of maintaining a second copy. Two independent lists is how these drifted apart, and it is why NEH-134's fix only covered half the problem.Docs-only; no code or behaviour changes.
Not covered
Whether the rest of
API_REFERENCE.mdneeds a full audit against the code is left open in NEH-191 as a team decision. It is a 2000+ line document and this is the second batch of defects found in it today by looking at one block, but a full audit should be scoped deliberately rather than absorbed here.