🐛 bug: reject malformed host authorities in hostauthorization#4293
🐛 bug: reject malformed host authorities in hostauthorization#4293gaby wants to merge 6 commits into
Conversation
|
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:
WalkthroughThe hostauthorization middleware now parses and strictly validates request authorities via parseNormalizedAuthority, normalizes the host (including Punycode), and denies malformed authorities; tests cover parsing edge cases and middleware rejection of malformed forwarded hosts. ChangesHost Authority Validation
sequenceDiagram
participant Client
participant HostAuthorizationMiddleware
participant parseNormalizedAuthority
participant HostMatcher
participant ErrorHandler
Client->>HostAuthorizationMiddleware: HTTP request (Host / X-Forwarded-Host)
HostAuthorizationMiddleware->>parseNormalizedAuthority: parse & normalize authority
alt parse ok
parseNormalizedAuthority-->>HostAuthorizationMiddleware: normalized host
HostAuthorizationMiddleware->>HostMatcher: matchHost(normalized host)
HostMatcher-->>HostAuthorizationMiddleware: allow/deny
else parse failed
parseNormalizedAuthority-->>HostAuthorizationMiddleware: error
HostAuthorizationMiddleware->>ErrorHandler: ErrForbiddenHost
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds stricter authority validation to the hostauthorization middleware so that malformed Host / X-Forwarded-Host values (e.g. authorities containing userinfo @, mismatched IPv6 brackets, non-numeric ports) are rejected before allowlist matching. This closes a host-header confusion bypass where input like allowed.com:443@attacker.example could match an allowed entry once parsed via c.Hostname().
Changes:
- New
parseNormalizedAuthority+isValidPorthelpers that validate raw authority syntax and return a normalized host. - Middleware now validates
c.Host()(raw authority including X-Forwarded-Host when proxy-trusted) instead of relying on the parsedc.Hostname(). - Added a unit test for the new parser and a regression test for the malformed
X-Forwarded-Hostrejection path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/hostauthorization/hostauthorization.go | Adds authority parser/validator and switches handler to validate the raw host. |
| middleware/hostauthorization/hostauthorization_test.go | Adds tests for the new parser and the malformed X-Forwarded-Host regression. |
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request enhances the host authorization middleware by introducing a dedicated authority parser that handles IPv6 addresses and port validation, effectively preventing bypasses using malformed authority strings. Review feedback suggests optimizing the port validation to strictly adhere to RFC 7230, reducing string scans in the parsing logic for better performance, and expanding the test suite to cover additional edge cases such as bare IPv6 and out-of-range ports.
|
@copilot address all the pull request comments. Use fiberutils when possible. |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/c7f5c5e6-d645-4a41-9f20-4d0e2cd95a8d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4293 +/- ##
=======================================
Coverage ? 91.34%
=======================================
Files ? 132
Lines ? 13029
Branches ? 0
=======================================
Hits ? 11901
Misses ? 712
Partials ? 416
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:
|
|
@copilot Address all the code review comments |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/fc7674f4-4439-4715-b478-6cfe5ba94c8a Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@middleware/hostauthorization/hostauthorization.go`:
- Around line 125-149: The bracketed-authority branch currently accepts any
token inside '[' ']' and should instead only accept valid IP-literals; update
the bracketed handling (the code that sets host = authority[1:idx] and rest :=
authority[idx+1:]) to validate that host is a real IP-literal (IPv6 or
RFC-6874/IPvFuture as appropriate) and reject (return "", false) for any non-IP
reg-name like "allowed.com"; you can call net.ParseIP (or an existing
IPv6/IPvFuture validator) and keep the existing port check via isValidPort for
rest, failing closed on any validation failure.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98f2cb87-165c-4493-a260-24d64877de1c
📒 Files selected for processing (2)
middleware/hostauthorization/hostauthorization.gomiddleware/hostauthorization/hostauthorization_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/hostauthorization/hostauthorization_test.go
| if authority[0] == '[' { | ||
| idx := -1 | ||
| for i := 1; i < len(authority); i++ { | ||
| switch authority[i] { | ||
| case '@', '[': | ||
| return "", false | ||
| case ']': | ||
| idx = i | ||
| i = len(authority) | ||
| } | ||
| } | ||
| if idx <= 1 { | ||
| return "", false | ||
| } | ||
|
|
||
| host = authority[1:idx] | ||
| rest := authority[idx+1:] | ||
| if rest != "" { | ||
| if rest[0] != ':' { | ||
| return "", false | ||
| } | ||
| if !isValidPort(rest[1:]) { | ||
| return "", false | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject bracketed non-IP literals before normalization.
Line 140 currently accepts any non-empty token inside [ ], so malformed authorities like [allowed.com] or [allowed.com]:443 normalize to allowed.com and can still satisfy the allowlist. That leaves a host-authorization bypass in the exact path this patch is trying to harden. The bracketed branch should only accept real IP-literals and fail closed for bracketed reg-names.
🤖 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 `@middleware/hostauthorization/hostauthorization.go` around lines 125 - 149,
The bracketed-authority branch currently accepts any token inside '[' ']' and
should instead only accept valid IP-literals; update the bracketed handling (the
code that sets host = authority[1:idx] and rest := authority[idx+1:]) to
validate that host is a real IP-literal (IPv6 or RFC-6874/IPvFuture as
appropriate) and reject (return "", false) for any non-IP reg-name like
"allowed.com"; you can call net.ParseIP (or an existing IPv6/IPvFuture
validator) and keep the existing port check via isValidPort for rest, failing
closed on any validation failure.
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/fc7674f4-4439-4715-b478-6cfe5ba94c8a Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Motivation
c.Hostname()(a parsed hostname) without validating the original raw authority, allowing malformed authorities likeallowed.com:443@attacker.exampleto bypass an allowlist whenTrustProxyis enabled.AllowedHoststo prevent Host header poisoning/authority confusion.Description
parseNormalizedAuthority(authority string) (string, bool)to validate raw authority syntax (rejecting userinfo@, malformed brackets, invalid ports) and return a normalized host for matching.isValidPortto ensure ports are numeric and within0-65535.c.Host()(raw authority) using the new parser and only perform allowlist matching on the validated/normalized host.parseNormalizedAuthorityand a regression test which verifies a malformedX-Forwarded-Hostis rejected whenTrustProxyis enabled; modified files:middleware/hostauthorization/hostauthorization.go,middleware/hostauthorization/hostauthorization_test.go.