-
Notifications
You must be signed in to change notification settings - Fork 197
fix(reverse-proxy): allow multi-label subdomains in service input #728
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?
Changes from 1 commit
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,59 @@ | ||
| import { isValidSubdomain, sanitizeSubdomain } from "./subdomain.js"; | ||
|
|
||
| type Case<T> = { input: string; expected: T; desc?: string }; | ||
|
|
||
| function run<T>(name: string, cases: Case<T>[], fn: (s: string) => T): number { | ||
| console.log(`\n=== ${name} ===`); | ||
| let failures = 0; | ||
| for (const { input, expected, desc } of cases) { | ||
| const actual = fn(input); | ||
| const ok = actual === expected; | ||
| if (!ok) failures++; | ||
| const label = desc | ||
| ? `${JSON.stringify(input)} (${desc})` | ||
| : JSON.stringify(input); | ||
| console.log( | ||
| `${ok ? "✓" : "✗"} ${label.padEnd(40)} → ${JSON.stringify(actual)}` + | ||
| (ok ? "" : ` (expected: ${JSON.stringify(expected)})`), | ||
| ); | ||
| } | ||
| return failures; | ||
| } | ||
|
|
||
| let failures = 0; | ||
|
|
||
| failures += run<string>( | ||
| "sanitizeSubdomain", | ||
| [ | ||
| { input: "dev.app", expected: "dev.app", desc: "keeps dots (#667)" }, | ||
| { input: "a.b.c.d", expected: "a.b.c.d", desc: "deeply nested" }, | ||
| { input: "DEV.App", expected: "dev.app", desc: "lowercased" }, | ||
| { input: "my-app", expected: "my-app", desc: "keeps hyphens" }, | ||
| { input: "dev.app!", expected: "dev.app", desc: "strips punctuation" }, | ||
| { input: "dev app", expected: "devapp", desc: "strips spaces" }, | ||
| { input: "dev_app", expected: "devapp", desc: "strips underscores" }, | ||
| { input: "dev.", expected: "dev.", desc: "in-progress typing survives" }, | ||
| { input: "", expected: "" }, | ||
| ], | ||
| sanitizeSubdomain, | ||
| ); | ||
|
|
||
| failures += run<boolean>( | ||
| "isValidSubdomain", | ||
| [ | ||
| { input: "myapp", expected: true, desc: "single label" }, | ||
| { input: "dev.app", expected: true, desc: "multi-label (#667)" }, | ||
| { input: "a.b.c.d", expected: true, desc: "deeply nested" }, | ||
| { input: "my-app.dev", expected: true, desc: "hyphenated label" }, | ||
| { input: "", expected: true, desc: "empty handled by require_subdomain" }, | ||
| { input: ".app", expected: false, desc: "leading dot" }, | ||
| { input: "dev.", expected: false, desc: "trailing dot" }, | ||
| { input: "dev..app", expected: false, desc: "consecutive dots" }, | ||
| { input: ".", expected: false, desc: "bare dot" }, | ||
| { input: "..", expected: false, desc: "bare dots" }, | ||
| ], | ||
| isValidSubdomain, | ||
| ); | ||
|
|
||
| console.log(`\n${failures} test(s) failed`); | ||
| process.exit(failures > 0 ? 1 : 0); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Live input filter. Dots are kept so multi-label subdomains ("dev.app") can | ||
| // be typed at all — dot placement is only checked on submit, because an | ||
| // anchored check would reject valid in-progress input like "dev." mid-typing. | ||
| export function sanitizeSubdomain(value: string): string { | ||
| return value.toLowerCase().replace(/[^a-z0-9.-]/g, ""); | ||
| } | ||
|
|
||
| // Dot-separated DNS labels. Empty labels are rejected, so leading/trailing | ||
| // dots and consecutive dots ("dev..app") never reach the API. | ||
| const SUBDOMAIN_PATTERN = /^[a-z0-9-]+(\.[a-z0-9-]+)*$/; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| // An empty subdomain is valid here; whether one is required at all is a | ||
| // separate concern (ReverseProxyDomain.require_subdomain). | ||
| export function isValidSubdomain(value: string): boolean { | ||
| return value === "" || SUBDOMAIN_PATTERN.test(value); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| ReverseProxyDomainType, | ||
| } from "@/interfaces/ReverseProxy"; | ||
| import { useReverseProxies } from "@/contexts/ReverseProxiesProvider"; | ||
| import { sanitizeSubdomain } from "./subdomain"; | ||
|
|
||
| // Helper to parse domain into subdomain and base domain. | ||
| // When availableDomains is provided, matches against them first (longest match wins) | ||
|
|
@@ -90,11 +91,7 @@ export function useReverseProxyDomain({ | |
| const [subdomain, setSubdomain] = useState(() => { | ||
| return ( | ||
| parsed?.subdomain || | ||
| initialSubdomain | ||
| ?.toLowerCase() | ||
| .replace(/\s+/g, "-") | ||
| .replace(/[^a-z0-9-]/g, "") || | ||
| "" | ||
| sanitizeSubdomain(initialSubdomain?.replace(/\s+/g, "-") ?? "") | ||
| ); | ||
|
Comment on lines
91
to
95
Author
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. Traced this and the bug is real, but I would rather not fix it in this PR. Confirmed: Two reasons to keep it separate:
Happy to open a follow-up issue with the above repro, or send a separate PR if a maintainer would rather have it bundled. |
||
| }); | ||
|
|
||
|
|
||
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.
Fixed in ead8c19 — the comment was stale against my own final code. It now says the sanitizer deliberately skips dot-placement checks (so
dev.survives mid-typing), andisValidSubdomaincarries a note that it drives both the inline error and the modal gate.