[management] Streamline domain validation#5211
Conversation
📝 WalkthroughWalkthroughCentralizes and replaces scattered domain validation code with a shared validation package, updates call sites across management and zones to use the new validators, and expands domain validation tests (including punycode, single-letter TLDs, wildcard rules, and length boundaries). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/management/domain/validate.go (1)
63-78: Add a total-length guard to list validation.
ValidateDomainsenforces max length viaFromString, butValidateDomainsListcan accept >253‑char domains. If callsites use this for user input, overlong domains can slip through. Consider adding a length check here (and mirroring it inIsValidDomain/IsValidDomainNoWildcard).🩹 Suggested guard in ValidateDomainsList
for _, d := range domains { d := strings.ToLower(d) + if len(d) > 253 { + return fmt.Errorf("domain exceeds maximum length: %s", d) + } if !domainRegex.MatchString(d) { return fmt.Errorf("invalid domain format: %s", d) } }
🤖 Fix all issues with AI agents
In `@management/cmd/management.go`:
- Around line 81-83: The validation rejects FQDNs with trailing dots; before
calling IsValidDomainNoWildcard on dnsDomain in management.go, normalize
dnsDomain by removing a single trailing '.' (and ensure resulting string is
non-empty) so domains like "example.com." become "example.com" prior to
validation; update any error messages to reference the normalized value or,
alternatively, document that trailing-dot FQDNs are not accepted if you prefer
to keep current behavior.
🧹 Nitpick comments (2)
shared/management/domain/validate_test.go (1)
193-203: Remove the duplicate test case.Lines 193-203 repeat the previous “Wildcard with dot domain” case. Dropping one keeps the suite leaner.
🧹 Remove the duplicate block
- { - name: "Wildcard with dot domain", - domains: []string{".*.example.com"}, - expected: nil, - wantErr: true, - },management/server/nameserver.go (1)
310-312: Consider rejecting wildcards anywhere in the domain, not just as a prefix.The current check only rejects domains starting with
*., but a domain likea.*.example.com(wildcard in the middle) would pass this check and be sent toValidateDomains. WhileValidateDomainsmight reject it due to the regex, it would be cleaner to have explicit wildcard handling here.💡 Optional: Reject wildcards anywhere in the domain
func validateDomain(d string) error { - if strings.HasPrefix(d, "*.") { + if strings.Contains(d, "*") { return errors.New("wildcards not allowed") }
lixmal
force-pushed
the
fix-domain-validation
branch
from
58b0e63 to
fd63dbd
Compare
|
2 participants
Describe your changes
Per-Component Validation Changes
util.IsValidDomainIsValidDomainNoWildcardutil.IsValidDomainIsValidDomainutil.IsValidDomainIsValidDomainNoWildcarddns.IsDomainNameValidateDomains+ wildcard checkValidateDomainsdns.IsDomainNameIsValidDomainNoWildcardIsValidDomainNoWildcardIsValidDomainNoWildcardSummary
shared/management/domainpackageStack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Tests
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.