Repository navigation
feat: support public ports in redirects and host links - #5933
lennondotw wants to merge 1 commit into
Conversation
Use client-facing deployment ports in Force SSL redirects and manager host links when default ports are inaccessible. Validate runtime configuration, honor file-based environment variables, and retain standard-port behavior.
|
| "map $host $npm_public_https_port_suffix {", | ||
| `\tdefault "${suffix}";`, | ||
| "}", | ||
| ...(ports.https === 443 ? [] : ["error_page 497 =307 https://$host$npm_public_https_port_suffix$request_uri;"]), |
There was a problem hiding this comment.
SSL listener redirect can disappear
If a host’s advanced configuration defines a server-level error_page, such as one for 404 responses, Nginx does not inherit this HTTP-level 497 handler. With a nonstandard public HTTPS port, plain HTTP sent to that host’s SSL listener returns the default error instead of the intended 307 redirect.
| -w /app \ | ||
| "${TESTING_IMAGE}" \ | ||
| sh -c 'yarn install && yarn lint . && rm -rf node_modules' | ||
| sh -c 'yarn install && yarn lint . && node --test lib/public-ports.test.js && rm -rf node_modules' |
There was a problem hiding this comment.
This CI command runs the new backend unit test, but it never runs the disposable-container smoke test against the image it builds. CI can therefore pass without checking that the image starts with the generated Nginx configuration or serves the new redirects. Adding the smoke test to this workflow would cover those integration failures.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Why
Some deployments run in network environments that do not allow incoming connections to TCP ports 80 and 443. They can expose NPM on permitted public ports through Docker mappings or router forwarding, but Force SSL redirects and the manager's host links still assume the standard ports. The generated URLs then point clients at an inaccessible port.
For example, with
232:80and233:443, this change makes an HTTP request tohttp://example.com:232/path?x=1redirect tohttps://example.com:233/path?x=1, and makes the manager display and openexample.com:233for a certificate-enabled host.Changes
PUBLIC_HTTP_PORTandPUBLIC_HTTPS_PORT, defaulting to 80/443, with validation and support for the existing__FILEmechanism.public_portsobject in the existing health response. Proxy, Redirection and 404 Host links select HTTPS when a certificate is configured, otherwise HTTP; nonstandard ports appear in both the label and URL. Certificate-list links retain their existing behavior.This addresses public/client-facing ports, unlike #5212 and #4127, which change Nginx's internal listening ports for host networking. It does not add fork-specific image publishing or deployment automation.
Validation
nginx -t, GET/POST redirect targets with paths and queries, the ACME exception, trusted forwarded HTTPS, and HTTP sent to the nonstandard SSL port. Run withpython3 test/public-ports-smoke.py <built-image>.Related Host-header fix
Related: #5934 preserves the client-facing port in forwarded Host headers, including custom locations and the missing-Host fallback. This PR handles redirects and manager links; #5934 handles upstream request authority. The two changes are based independently on
developand can be reviewed/merged separately.Type of Change
AI Usage