🐛 bug: trim only one trailing dot in host normalization#4307
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR refines host normalization in the authorization middleware by introducing a ChangesTrailing dot trimming refinement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the host normalization logic to ensure only a single trailing dot is removed from hostnames, replacing the previous behavior that stripped all trailing dots. New test cases were added to validate this specific behavior for both plain hostnames and those with ports. The reviewer suggested a more idiomatic implementation for the new trimOneTrailingDot function using strings.TrimSuffix to improve code clarity and safety.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4307 +/- ##
==========================================
+ Coverage 91.28% 91.33% +0.04%
==========================================
Files 132 132
Lines 13036 13040 +4
==========================================
+ Hits 11900 11910 +10
+ Misses 718 713 -5
+ Partials 418 417 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request adjusts host canonicalization in the hostauthorization middleware to remove only one trailing root-label dot (.), ensuring malformed multi-dot hosts (e.g. example.com..) don’t get normalized into a value that could match allowlisted entries unintentionally.
Changes:
- Replaced full right-trim of dots with a
trimOneTrailingDothelper to remove at most one trailing.during host normalization. - Updated
normalizeHostto apply the one-dot trim consistently on both the fast path (no port) and general path (SplitHostPort / bracket handling). - Added regression tests covering multiple trailing dots for plain hosts and authority strings with ports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/hostauthorization/hostauthorization.go | Changes host normalization to trim at most one trailing dot via a dedicated helper. |
| middleware/hostauthorization/hostauthorization_test.go | Adds regression coverage ensuring multi-dot inputs only lose one trailing dot (including with ports). |
Motivation
example.com..) from being silently accepted and matched against allowlist entries.Description
utils.TrimRight(host, '.')with a newtrimOneTrailingDot(host string) stringhelper that removes at most one trailing.from the hostname inmiddleware/hostauthorization/hostauthorization.go.normalizeHostto usetrimOneTrailingDoton both the fast path and the general path before lowercasing and punycode conversion.middleware/hostauthorization/hostauthorization_test.goto assert that multiple trailing dots are only trimmed once for both plain hosts and authority strings with ports.