refactor: move domain check into small helper util#1000
Conversation
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a reusable domain validator for URL parsing, hostname normalization, scheme and port checks, then integrates it into OAuth redirect safety and static ACL lookup with updated tests. ChangesDomain validation and consumers
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/service/access_controls_service.go (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an explicit local copy instead of
&<range-variable>.
configis the loop variable fromfor app, config := range service.config.Apps; returning/storing&configdirectly works on modern Go but the project has previously preferred an explicit copy (result := config; ... &result) for clarity.Based on learnings, "prefer the explicit local copy pattern (e.g.,
result := config; return &result) rather than returning&<range-variable>directly... the project prefers the explicit copy for clarity."Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/access_controls_service.go` at line 54, In the config lookup loop within the access-control service, avoid returning pointers to the range variable config directly. Create an explicit local copy before returning or storing the pointer at both referenced locations, preserving the existing behavior while following the project’s copy-then-address pattern.Source: Learnings
internal/controller/oauth_controller.go (1)
326-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Validate(expected, actual)args are reversed vs. the access-control usage.Here
expectedis the untrustedredirectURIandactualis the trustedcontroller.runtime.AppURL;access_controls_service.go'sv.Validate(config.Config.Domain, domain)puts the trusted value first. It doesn't change the boolean outcome, but produces confusing error text (e.g. "expected port 8080, got 443" instead of the intuitive direction) when debugging redirect-safety failures.♻️ Proposed fix
- err = v.Validate(redirectURI, controller.runtime.AppURL) + err = v.Validate(controller.runtime.AppURL, redirectURI)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/oauth_controller.go` at line 326, Swap the arguments to Validate in the OAuth redirect-safety check so the trusted controller.runtime.AppURL is passed as expected and the untrusted redirectURI as actual, matching the access-control usage and correcting validation error direction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/access_controls_service.go`:
- Line 56: Update the app-name subdomain fallback in the access-control
validation flow to compare normalized lowercase host and app values, matching
the behavior of Validate via formatHostname. Preserve the existing prefix
boundary using app+"." so mixed-case hosts such as FOO.example.com match the
configured app correctly.
- Around line 39-63: Update lookupStaticACLs so validation errors for individual
configured domains are treated as non-matches rather than returned, allowing
iteration to continue and valid apps to match regardless of map order; also make
the app-name fallback comparison case-insensitive when checking the domain
prefix against app+".". Preserve direct domain matches and return nameMatch only
after all apps are evaluated.
In `@pkg/validators/domain_validator_test.go`:
- Around line 249-253: Update the “Non matching hostnames should fail” test case
in the Validate test table to provide an errorFunc assertion for the expected
validation error, so it does not fall through to require.NoError when expected
and actual hostnames differ.
In `@pkg/validators/domain_validator.go`:
- Around line 99-147: Introduce exported sentinel or typed errors in
DomainValidator.Validate/getURL for invalid input, scheme mismatch, port
mismatch, and hostname mismatch, wrapping them while preserving useful details.
In internal/controller/oauth_controller.go lines 334-338, replace error-message
prefix checks with errors.Is/errors.As against the new errors; in
internal/service/access_controls_service.go line 49, recognize only the
hostname-mismatch error and treat other validation errors as failures.
---
Nitpick comments:
In `@internal/controller/oauth_controller.go`:
- Line 326: Swap the arguments to Validate in the OAuth redirect-safety check so
the trusted controller.runtime.AppURL is passed as expected and the untrusted
redirectURI as actual, matching the access-control usage and correcting
validation error direction.
In `@internal/service/access_controls_service.go`:
- Line 54: In the config lookup loop within the access-control service, avoid
returning pointers to the range variable config directly. Create an explicit
local copy before returning or storing the pointer at both referenced locations,
preserving the existing behavior while following the project’s copy-then-address
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 224f3f3f-683d-45f3-a89d-f7cf8da78283
📒 Files selected for processing (6)
internal/controller/oauth_controller.gointernal/controller/oauth_controller_test.gointernal/service/access_controls_service.gointernal/service/access_controls_service_test.gopkg/validators/domain_validator.gopkg/validators/domain_validator_test.go
| func (service *AccessControlsService) lookupStaticACLs(domain string) (*model.App, error) { | ||
| var nameMatch *model.App | ||
|
|
||
| v := validators.NewDomainValidator(validators.DomainValidatorOptions{}) | ||
|
|
||
| // First try to find a matching app by domain, then fallback to matching by app name (subdomain) | ||
| for app, config := range service.config.Apps { | ||
| if config.Config.Domain == domain { | ||
| err := v.Validate(config.Config.Domain, domain) | ||
| // Maybe not the best way to check if a match succeeded, but expected is only | ||
| // used on match errors and not parsing errors | ||
| if err != nil && !strings.HasPrefix(err.Error(), "expected") { | ||
| return nil, err | ||
| } | ||
| if err == nil { | ||
| service.log.App.Debug().Str("name", app).Msg("Found matching container by domain") | ||
| return &config | ||
| return &config, nil | ||
| } | ||
| if strings.SplitN(domain, ".", 2)[0] == app { | ||
| if strings.HasPrefix(domain, app+".") { | ||
| service.log.App.Debug().Str("name", app).Msg("Found matching container by app name") | ||
| nameMatch = &config | ||
| } | ||
| } | ||
|
|
||
| return nameMatch | ||
| return nameMatch, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One bad app config can abort unrelated ACL lookups.
v.Validate(config.Config.Domain, domain)now returns early on non-match parse/format errors, so a single malformed app domain can make lookups for other valid apps fail nondeterministically as map iteration order changes.- The app-name fallback is also case-sensitive (
strings.HasPrefix(domain, app+".")), so mixed-case hostnames miss the fallback match.
Treat invalid configured domains as non-matches here, or validate them at config load so request-time lookup can’t fail on one bad entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/access_controls_service.go` around lines 39 - 63, Update
lookupStaticACLs so validation errors for individual configured domains are
treated as non-matches rather than returned, allowing iteration to continue and
valid apps to match regardless of map order; also make the app-name fallback
comparison case-insensitive when checking the domain prefix against app+".".
Preserve direct domain matches and return nameMatch only after all apps are
evaluated.
| return &config, nil | ||
| } | ||
| if strings.SplitN(domain, ".", 2)[0] == app { | ||
| if strings.HasPrefix(domain, app+".") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
App-name subdomain fallback is case-sensitive, unlike the exact-domain match.
strings.HasPrefix(domain, app+".") compares the raw domain value, while the exact-match path (Validate → formatHostname) lowercases before comparing. A mixed-case Host header (e.g. FOO.example.com) would fail this fallback even though it should match app "foo".
🐛 Proposed fix
- if strings.HasPrefix(domain, app+".") {
+ if strings.HasPrefix(strings.ToLower(domain), strings.ToLower(app)+".") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.HasPrefix(domain, app+".") { | |
| if strings.HasPrefix(strings.ToLower(domain), strings.ToLower(app)+".") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/access_controls_service.go` at line 56, Update the app-name
subdomain fallback in the access-control validation flow to compare normalized
lowercase host and app values, matching the behavior of Validate via
formatHostname. Preserve the existing prefix boundary using app+"." so
mixed-case hosts such as FOO.example.com match the configured app correctly.
| // 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 | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two security-relevant call sites branch on raw Validate error text instead of typed errors. Validate only returns fmt.Errorf-built strings, so both consumers must guess the failure category via prefix-matching the message; any future wording tweak in the validator silently breaks that control flow with no compile-time signal.
pkg/validators/domain_validator.go#L99-L147: expose sentinel/typed errors (e.g.ErrSchemeMismatch,ErrPortMismatch,ErrHostnameMismatch,ErrInvalidInput) fromValidate/getURLso callers can useerrors.Is/errors.Asinstead of string matching.internal/controller/oauth_controller.go#L334-L338: replacestrings.HasPrefix(err.Error(), "expected port") || strings.HasPrefix(err.Error(), "expected scheme") || err.Error() == "input url is invalid"witherrors.Ischecks against the new sentinel errors.internal/service/access_controls_service.go#L49: replace!strings.HasPrefix(err.Error(), "expected")with anerrors.Is/errors.Ascheck against the "hostname mismatch" sentinel to distinguish a genuine non-match from a real parsing/config failure.
📍 Affects 3 files
pkg/validators/domain_validator.go#L99-L147(this comment)internal/controller/oauth_controller.go#L334-L338internal/service/access_controls_service.go#L49-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/validators/domain_validator.go` around lines 99 - 147, Introduce exported
sentinel or typed errors in DomainValidator.Validate/getURL for invalid input,
scheme mismatch, port mismatch, and hostname mismatch, wrapping them while
preserving useful details. In internal/controller/oauth_controller.go lines
334-338, replace error-message prefix checks with errors.Is/errors.As against
the new errors; in internal/service/access_controls_service.go line 49,
recognize only the hostname-mismatch error and treat other validation errors as
failures.
| if u.Scheme == "https" { | ||
| return "443" | ||
| } | ||
| return "80" |
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 http to give port 80, and throw an error for unknown scheme otherwise?
edit: Maybe still return 80, but also log it - not sure on the best choice...
2 participants
Summary by CodeRabbit