-
Notifications
You must be signed in to change notification settings - Fork 3
nginx: Support query params & fix OPCS-4 port conflict #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Phoevos Kalemkeris <[email protected]>
Signed-off-by: Phoevos Kalemkeris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes two bugs in the nginx configuration:
- Adds explicit query parameter forwarding to upstream services in the reverse proxy
- Resolves a port conflict between the ICD-10 and OPCS-4 services by changing OPCS-4's port from 28181 to 28182
Key changes:
- Query string handling logic added to preserve URL parameters when proxying requests
- OPCS-4 service port updated to avoid conflict with ICD-10 service
- Path handling improved to correctly handle requests with and without trailing slashes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docker/nginx/etc/nginx/sites-enabled/medcat-opcs4 | Changed listening ports from 28181 to 28182 to avoid conflict with medcat-icd10 service |
| docker/nginx/etc/nginx/nginx.conf | Added query string preservation logic and improved path handling for proxied requests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| proxy_pass http://$upstream$subpath; | ||
| proxy_pass http://$upstream$path$query_string_part; |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proxy_pass target host is built from the unvalidated $service path segment (captured in the location regex and then used to construct $upstream), which allows clients to control the upstream host for /cms/<service>/... requests. An attacker can send requests like /cms/attacker-controlled-host/some/path?x=y to force nginx to proxy arbitrary HTTP traffic to internal or external hosts on the configured ports, effectively creating an SSRF/open-proxy primitive. To mitigate this, restrict $service to a strict allowlist of known backends (e.g. via dedicated location blocks or a map) instead of passing arbitrary user-controlled values into proxy_pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is interesting but not directly relevant to this PR, but what's described has in fact been the case since I introduced dynamic routing through the proxy, the whole point of which was to move away from strictly defined targets so that we can proxy to models deployed dynamically through the CMG.
Rationale behind dynamic routing through proxy
When running in airgapped environments, a proxy server is usually configured to handle all traffic deemed to be targeting resources outside of the local network. This also affects containerised applications using Docker service names for communication. For these to work, we explicitly add the names of the expected targets to the containers' no_proxy list. However, this can only work when we know the names of all potential targets in advance (e.g. a backend server that only talks to a database and an object store). In the case of the CogStack Model Gateway, which allows the dynamic deployment of user-defined model servers, the list of targets can change dynamically. For that reason, instead of keeping track of all servers and attempting to maintain an up-to-date no_proxy list, we choose to proxy all requests through a single service, the name of which we can safely add to the CMG containers' no_proxy list.
Risk
In airgapped setups like our production environments, the risk posed by the unrestricted proxy pass is minimal. Any traffic exiting the cluster (outbound) is restricted by the configured Squid proxy, meaning that only local services can be targeted through the proxy. With that in mind, if an attacker is able to create arbitrary containers inside the Docker network, they hardly need the proxy to compromise our servers.
On the other hand, in environments with unrestricted outbound traffic, this can indeed be an actual security concern. For this reason, we can look into moving the dynamic routing logic to a separate internal proxy managed by the CMG project and revert to solely using the strictly configured upstreams here. In any case, that should be part of a different PR.
This PR introduces 2 fixes addressing bugs previously introduced to the nginx config: