-
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
Open
khannoussi-malek
wants to merge
2
commits into
netbirdio:main
Choose a base branch
from
khannoussi-malek:fix/667-multi-label-subdomain
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| 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: "a", expected: true, desc: "single character" }, | ||
| { input: "a-b.c-d", expected: true, desc: "hyphen inside every label" }, | ||
| { input: "", expected: true, desc: "empty handled by require_subdomain" }, | ||
| { input: "-app", expected: false, desc: "leading hyphen" }, | ||
| { input: "app-", expected: false, desc: "trailing hyphen" }, | ||
| { input: "-", expected: false, desc: "bare hyphen" }, | ||
| { input: "dev.-app", expected: false, desc: "leading hyphen, later label" }, | ||
| { | ||
| input: "dev-.app", | ||
| expected: false, | ||
| desc: "trailing hyphen, first label", | ||
| }, | ||
| { 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); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // Live input filter, applied on every keystroke. Dots are kept so multi-label | ||
| // subdomains ("dev.app") can be typed at all. It deliberately does not check | ||
| // dot placement: "dev." is a valid step on the way to "dev.app", so rejecting | ||
| // it here would make the field impossible to type in. Shape is checked | ||
| // separately by isValidSubdomain. | ||
| export function sanitizeSubdomain(value: string): string { | ||
| return value.toLowerCase().replace(/[^a-z0-9.-]/g, ""); | ||
| } | ||
|
|
||
| // Dot-separated DNS labels (RFC 1123): each label is alphanumeric at both | ||
| // ends, with hyphens allowed only inside. Rejects empty labels, so leading or | ||
| // trailing dots and consecutive dots ("dev..app") never reach the API. | ||
| const SUBDOMAIN_PATTERN = | ||
| /^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)*$/; | ||
|
|
||
| // Drives both the inline error in the input and the submit gate in the modal. | ||
| // 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); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Traced this and the bug is real, but I would rather not fix it in this PR.
Confirmed:
domainscomes fromuseFetchApiinReverseProxiesProviderand is passed straight into the modal, which mounts onmodalOpenwith no loading guard. On a cold SWR cache,parseDomainruns withdomains === undefined, falls through to the first-dot split, anddev.app.example.cominitializes as subdomaindev+ baseDomainapp.example.com. TheuseStateinitializer never re-runs when domains arrive, so it stays wrong.Two reasons to keep it separate:
main. Both the first-dot fallback and the once-only initializer predate this PR, and nothing here changes that path — this PR only touches the sanitize/validate step. You are right that multi-label domains make it easier to hit, but that is a change in the data, not in the code path.Happy to open a follow-up issue with the above repro, or send a separate PR if a maintainer would rather have it bundled.