Repository navigation
feat: support public ports in redirects and host links #5933
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: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| function parsePort(env, name, fallback) { | ||
| const value = env[name]; | ||
| if (value === undefined) return fallback; | ||
| const port = Number(value); | ||
| if (!/^\d+$/.test(value) || !Number.isInteger(port) || port < 1 || port > 65535) { | ||
| throw new Error(`${name} must be an integer between 1 and 65535`); | ||
| } | ||
| return port; | ||
| } | ||
|
|
||
| export function getPublicPorts(env = process.env) { | ||
| return { | ||
| http: parsePort(env, "PUBLIC_HTTP_PORT", 80), | ||
| https: parsePort(env, "PUBLIC_HTTPS_PORT", 443), | ||
| }; | ||
| } | ||
|
|
||
| export function renderPublicPortsConfig(ports) { | ||
| const suffix = ports.https === 443 ? "" : `:${ports.https}`; | ||
| return [ | ||
| "# Generated from PUBLIC_HTTP_PORT / PUBLIC_HTTPS_PORT at container startup.", | ||
| "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;"]), | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import assert from "node:assert/strict"; | ||
| import { test } from "node:test"; | ||
| import { getPublicPorts, renderPublicPortsConfig } from "./public-ports.js"; | ||
|
|
||
| test("uses standard ports when deployment variables are absent", () => { | ||
| assert.deepEqual(getPublicPorts({}), { http: 80, https: 443 }); | ||
| assert.match(renderPublicPortsConfig(getPublicPorts({})), /default "";/); | ||
| assert.doesNotMatch(renderPublicPortsConfig(getPublicPorts({})), /error_page/); | ||
| }); | ||
|
|
||
| test("shares custom ports with the API and produces the HTTPS redirect suffix", () => { | ||
| const ports = getPublicPorts({ PUBLIC_HTTP_PORT: "232", PUBLIC_HTTPS_PORT: "233" }); | ||
| assert.deepEqual(ports, { http: 232, https: 233 }); | ||
| assert.match(renderPublicPortsConfig(ports), /default ":233";/); | ||
| assert.match( | ||
| renderPublicPortsConfig(ports), | ||
| /error_page 497 =307 https:\/\/\$host\$npm_public_https_port_suffix\$request_uri;/, | ||
| ); | ||
| }); | ||
|
|
||
| test("accepts the full valid port range independently for each protocol", () => { | ||
| assert.deepEqual(getPublicPorts({ PUBLIC_HTTP_PORT: "1", PUBLIC_HTTPS_PORT: "65535" }), { | ||
| http: 1, | ||
| https: 65535, | ||
| }); | ||
| }); | ||
|
|
||
| test("rejects invalid input before emitting Nginx configuration", () => { | ||
| for (const key of ["PUBLIC_HTTP_PORT", "PUBLIC_HTTPS_PORT"]) { | ||
| for (const value of ["", "0", "65536", "-1", "233.5", "2e2", " 233", "233; return 200;"]) { | ||
| assert.throws(() => getPublicPorts({ [key]: value }), { | ||
| message: `${key} must be an integer between 1 and 65535`, | ||
| }); | ||
| } | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import fs from "node:fs"; | ||
| import { getPublicPorts, renderPublicPortsConfig } from "../lib/public-ports.js"; | ||
|
|
||
| const filename = process.argv[2] || "/etc/nginx/conf.d/public-ports.conf"; | ||
| fs.writeFileSync(filename, renderPublicPortsConfig(getPublicPorts()), { mode: 0o644 }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| #!/command/with-contenv bash | ||
| # shellcheck shell=bash | ||
|
|
||
| set -e | ||
|
|
||
| log_info "Configuring public ports ..." | ||
| /command/with-contenv node /app/scripts/configure-public-ports.mjs |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { cleanup, fireEvent, render, screen } from "@testing-library/react"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { DomainsFormatter } from "./DomainsFormatter"; | ||
|
|
||
| const { health } = vi.hoisted(() => ({ health: { publicPorts: { http: 232, https: 233 } } })); | ||
| vi.mock("src/context", () => ({ useLocaleState: () => ({ locale: "en" }) })); | ||
| vi.mock("src/hooks/useHealth", () => ({ useHealth: () => ({ data: health }) })); | ||
| vi.mock("src/locale", () => ({ formatDateTime: () => "", T: () => null })); | ||
|
|
||
| afterEach(cleanup); | ||
|
|
||
| describe("public host links", () => { | ||
| it.each([ | ||
| ["https", 232, 233, "example.com:233", "https://example.com:233"], | ||
| ["http", 232, 233, "example.com:232", "http://example.com:232"], | ||
| ["https", 80, 443, "example.com", "https://example.com"], | ||
| ["http", 80, 443, "example.com", "http://example.com"], | ||
| ] as const)("formats %s with HTTP %s / HTTPS %s", (scheme, http, https, text, href) => { | ||
| health.publicPorts = { http, https }; | ||
| render(<DomainsFormatter domains={["example.com"]} linkScheme={scheme} />); | ||
| const link = screen.getByRole("link", { name: text }); | ||
| expect(link.getAttribute("href")).toBe(href); | ||
| }); | ||
|
|
||
| it("preserves certificate-list links that have no host scheme", () => { | ||
| health.publicPorts = { http: 232, https: 233 }; | ||
| render(<DomainsFormatter domains={["example.com"]} />); | ||
| expect(screen.getByRole("link", { name: "example.com" }).getAttribute("href")).toBe("http://example.com"); | ||
| }); | ||
|
|
||
| it("keeps wildcard links non-navigable", () => { | ||
| health.publicPorts = { http: 232, https: 233 }; | ||
| render(<DomainsFormatter domains={["*.example.com"]} linkScheme="https" />); | ||
| expect(fireEvent.click(screen.getByRole("link", { name: "*.example.com:233" }))).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ docker run --rm \ | |
| -v "$(pwd)/backend:/app" \ | ||
| -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
| echo -e "${BLUE}❯ ${GREEN}Testing Complete${RESET}" | ||
|
|
||
| # Build | ||
|
|
||
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.
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.