refactor: move domain check into small helper util#1000
Conversation
dosubot
Bot
added
the
size:L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
📝 WalkthroughWalkthroughAdds a reusable domain validator for URL, hostname, scheme, port, and IDNA handling. OAuth redirect checks and static ACL lookup now use it, with updated subdomain fallback behavior and comprehensive validator tests. ChangesDomain validation and consumers
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthController
participant DomainValidator
participant RuntimeConfig
OAuthController->>RuntimeConfig: read AppURL and CookieDomain
OAuthController->>DomainValidator: validate AppURL and redirectURI
DomainValidator-->>OAuthController: validation result
OAuthController->>DomainValidator: derive safe redirect hostname
DomainValidator-->>OAuthController: normalized hostname
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/access_controls_service.go (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer explicit local copy when taking the address of a loop variable.
Based on learnings, when returning or storing a pointer to a
for rangeloop value, prefer the explicit local copy pattern (e.g.,cfg := config; return &cfg) rather than taking the address of the range variable directly. This avoids confusion regarding loop-variable capture semantics.
internal/service/access_controls_service.go#L50-L50: create a local copy ofconfigbefore returning its address.internal/service/access_controls_service.go#L57-L57: create a local copy ofconfigbefore assigning its address tonameMatch.🤖 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 50, In internal/service/access_controls_service.go at lines 50-50 and 57-57, update the loop handling to copy each range variable into an explicit local before taking its address: return the local copy at the first site and assign the local copy’s address to nameMatch at the second. Use the existing config value without changing matching behavior.Source: Learnings
🤖 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.
Nitpick comments:
In `@internal/service/access_controls_service.go`:
- Line 50: In internal/service/access_controls_service.go at lines 50-50 and
57-57, update the loop handling to copy each range variable into an explicit
local before taking its address: return the local copy at the first site and
assign the local copy’s address to nameMatch at the second. Use the existing
config value without changing matching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05bf647f-d5cb-49a7-96ed-43e864fb0103
📒 Files selected for processing (4)
internal/controller/oauth_controller.gointernal/service/access_controls_service.gopkg/validators/domain_validator.gopkg/validators/domain_validator_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/validators/domain_validator_test.go
- pkg/validators/domain_validator.go
- internal/controller/oauth_controller.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary by CodeRabbit
pkgcode in both dev and distribution images.