Repository navigation
feat(nginx): refuse directive-unsafe names and duplicate upstream ports - #144
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f667f8db10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # compose's own service-name charset — also exactly what nginx can | ||
| # interpolate safely into upstream/server_name tokens. | ||
| _NGINX_SAFE_NAME = re.compile(r"[a-zA-Z0-9._-]+\Z") |
There was a problem hiding this comment.
This pattern has no length bound, so a Compose-valid service name containing, for example, 100 ASCII letters passes normalization and is inserted into server_name, while the generated nginx configuration fixes server_names_hash_bucket_size at 64 (apps/utils/nginx/template/nginx.conf.jinja:35). Nginx cannot place a sufficiently long name in that bucket and exits with could not build the server_names_hash, leaving the opaque failed deployment this validation is intended to prevent; either bound the generated server name or size the nginx hash bucket accordingly.
Useful? React with 👍 / 👎.
compose-containment Feature 5. The service name and each container port
reach the nginx conf VERBATIM (upstream {name}_{port}, server_name
{name}.{port}, listen {port}) — docker-compose validates the charset at
'up' time, so a bad name today surfaces as an opaque failed deploy whose
injected conf was never read, with only a container-name hint.
Two named refusals, in the same normalization pass the pipeline already
routes through (so preflight will enforce them too):
- service names outside compose's own charset [a-zA-Z0-9._-] (which is
exactly the nginx-interpolation-safe set) refuse with the name quoted;
- the same container port published twice by one service ('8080:80' +
'8081:80') yields the SAME port_name and therefore a duplicate
'upstream' block — nginx [emerg] at sidecar start. This is the REAL
collision: several server blocks sharing a listen with distinct
server_names is legal nginx (the epic's original 'duplicate listen'
framing was corrected in the HLD). Cross-service port reuse stays
legal (distinct upstreams). Both short and long syntax covered —
targets normalize to strings before the check.
Calibrated honestly (per the HLD): this is a diagnosability fix, not an
RCE fix — the threat was an opaque failed deploy.
The whole live catalog passes the new checks (31/31 entries scanned);
long-syntax 'target' had to be stringified before comparison so 80 and
'80' collide as they should. 7 new tests incl. end-to-end via
create_greffon_info; suite 1031 passed with only the two known
pre-existing-on-main failures. (Verified via a stub dockerd answering
the version handshake on a unix socket — the suite mocks docker
throughout and only the import handshake needed the wedged daemon.)
Adversarial review round (SHIP + 3 P2s) — folded: the \Z anchoring
pinned by test fixtures ('app\n' and 'café' — a 569JNRXghklanchored regex would
accept the newline into server_name); server_names_hash_bucket_size
bumped 64 -> 128 (charset-legal ~70-char names [emerg]'d the hash
build); the L4 over-approximation documented at the check (manager tier
metadata is invisible at the shape layer; refusing is the safe side).
f667f8d to
a1921a3
Compare
compose-containment Feature 5 — nginx directive safety, per the HLD: service names and container ports reach the conf verbatim (
upstream {name}_{port},server_name {name}.{port},listen {port}). Docker-compose validates the charset atuptime, so a bad name today is an opaque failed deploy whose injected conf was never read.Two named refusals in the normalization pass the pipeline already routes through (so the Feature-6 preflight will enforce them too):
[a-zA-Z0-9._-]— compose's own constraint, which is exactly the nginx-interpolation-safe set. Bad names refuse with the name quoted.8080:80+8081:80→port_name app_80twice → duplicateupstreamblock → nginx[emerg]). This is the real collision — severalserverblocks sharing alistenwith distinctserver_names is legal nginx (the epic's original "duplicate listen" framing was corrected in the HLD). Cross-service port reuse stays legal (distinct upstreams). Short and long syntax both covered — targets normalize to strings before comparison, so80and"80"collide as they should.Calibrated honestly per the HLD: a diagnosability fix, not an RCE fix. Whole live catalog passes the new checks (31/31 scanned). 7 new tests incl. end-to-end via
create_greffon_info; suite 1031 passed with only the two known pre-existing-on-main failures.Local-verification note: the Docker Desktop engine was wedged (socket accepts, never responds), and the suite only needs the daemon for the import handshake — everything else is mocked. Verified against a stub dockerd answering the version handshake on a unix socket; full suite green that way.