refactor: move domain check into small helper util #1000
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
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,160 @@ | ||
| // Package validators provides validators for various types of data. | ||
| // | ||
| // Domain validator is a simple utility that ensures two domains are exact | ||
| // matches while ensuring that techniques used to bypass such checks do | ||
| // not impact the validation. | ||
|
|
||
| package validators | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net" | ||
| "net/url" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "golang.org/x/net/idna" | ||
| ) | ||
|
|
||
| // DomainValidatorOptions is a set of options for DomainValidator. | ||
| type DomainValidatorOptions struct { | ||
| // Ensure domains have the same scheme. | ||
| WithScheme bool | ||
| // Ensure domains have the same port. | ||
| WithPort bool | ||
| // Specify a list of allowed schemes IF WithScheme is set to true. | ||
| // Leave empty to allow any scheme. | ||
| AllowedSchemes []string | ||
| } | ||
|
|
||
| // DomainValidator is a simple utility that ensures two domains are exact | ||
| // matches while ensuring that techniques used to bypass such checks do | ||
| // not impact the validation. | ||
| type DomainValidator struct { | ||
| opts DomainValidatorOptions | ||
| } | ||
|
|
||
| // NewDomainValidator creates a new DomainValidator. | ||
| func NewDomainValidator(opts DomainValidatorOptions) *DomainValidator { | ||
| return &DomainValidator{ | ||
| opts: opts, | ||
| } | ||
| } | ||
|
|
||
| func (v *DomainValidator) getURL(i string) (*url.URL, error) { | ||
| u, err := url.Parse(i) | ||
|
|
||
| if !v.opts.WithScheme && (err != nil || u.Host == "") { | ||
| u, err = url.Parse("tinyauth://" + i) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse input url: %w", err) | ||
| } | ||
|
|
||
| if u.Host == "" { | ||
| return nil, fmt.Errorf("input url is invalid") | ||
| } | ||
|
|
||
| if v.opts.WithPort && !v.opts.WithScheme && u.Port() == "" { | ||
| return nil, fmt.Errorf("port validation is enabled but port is missing in input url and schemes are not enabled") | ||
| } | ||
|
|
||
| if v.opts.WithScheme { | ||
| // Empty scheme means that we parsed the url with the tinyauth:// placeholder | ||
| if u.Scheme == "tinyauth" { | ||
| return nil, fmt.Errorf("input url is missing scheme") | ||
| } | ||
| if len(v.opts.AllowedSchemes) > 0 && !slices.Contains(v.opts.AllowedSchemes, u.Scheme) { | ||
| return nil, fmt.Errorf("scheme %s not allowed", u.Scheme) | ||
| } | ||
| } | ||
|
|
||
| return u, nil | ||
| } | ||
|
|
||
| func (v *DomainValidator) getEffectivePort(u *url.URL) string { | ||
| if u.Port() != "" { | ||
| return u.Port() | ||
| } | ||
| if u.Scheme == "https" { | ||
| return "443" | ||
| } | ||
| return "80" | ||
|
Collaborator
There was a problem hiding this comment. This is assuming that only http/s is coming in - the code is written pretty agnostically, so wouldn't it be safer to actively check for edit: Maybe still return |
||
| } | ||
|
|
||
| func (v *DomainValidator) formatHostname(hostname string) (string, error) { | ||
| hostname = strings.ToLower(hostname) | ||
| hostname = strings.TrimSuffix(hostname, ".") | ||
| if net.ParseIP(hostname) != nil { | ||
| return "", fmt.Errorf("ip addresses are not supported") | ||
| } | ||
| hostname, err := idna.Lookup.ToASCII(hostname) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to convert hostname to ascii: %w", err) | ||
| } | ||
| return hostname, nil | ||
| } | ||
|
|
||
| // Validate ensures that two domains are exact matches with the | ||
| // options defined in the DomainValidatorOptions. It ensures that the | ||
| // inputs are proper URLs and contain a host. It lowercases the hostnames | ||
| // and removes the trailing dot. Finally, it checks that the hostnames are | ||
| // equal unless WithScheme or WithPort is set to true where it also | ||
| // validates the scheme and port respectively. | ||
| func (v *DomainValidator) Validate(expected, actual string) error { | ||
| eu, err := v.getURL(expected) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| au, err := v.getURL(actual) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if v.opts.WithScheme { | ||
| if eu.Scheme != au.Scheme { | ||
| return fmt.Errorf("expected scheme %s, got %s", eu.Scheme, au.Scheme) | ||
| } | ||
| } | ||
|
|
||
| if v.opts.WithPort { | ||
| if v.getEffectivePort(eu) != v.getEffectivePort(au) { | ||
| return fmt.Errorf("expected port %s, got %s", v.getEffectivePort(eu), v.getEffectivePort(au)) | ||
| } | ||
| } | ||
|
|
||
| euf, err := v.formatHostname(eu.Hostname()) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| auf, err := v.formatHostname(au.Hostname()) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if euf != auf { | ||
| return fmt.Errorf("expected hostname %s, got %s", euf, auf) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
Comment on lines
+99
to
+147
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Two security-relevant call sites branch on raw
📍 Affects 3 files
🤖 Prompt for AI Agents |
||
|
|
||
| // SafeHostname uses the internal validation for domains that Validator uses | ||
| // to parse a hostname. It ensures the input URL is a valid URL, that a host | ||
| // is present and that the hostname is lowercased and without a trailing dot. | ||
| func (v *DomainValidator) SafeHostname(input string) (string, error) { | ||
| u, err := v.getURL(input) | ||
|
|
||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return v.formatHostname(u.Hostname()) | ||
| } | ||
Oops, something went wrong.
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
App-name subdomain fallback is case-sensitive, unlike the exact-domain match.
strings.HasPrefix(domain, app+".")compares the rawdomainvalue, while the exact-match path (Validate→formatHostname) lowercases before comparing. A mixed-caseHostheader (e.g.FOO.example.com) would fail this fallback even though it should match app"foo".🐛 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents