🐛 bug: reject malformed host authorities in hostauthorization#4293
🐛 bug: reject malformed host authorities in hostauthorization#4293gaby wants to merge 5 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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 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.
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.
| func isValidPort(raw string) bool { | ||
| if raw == "" { | ||
| return false | ||
| } | ||
|
|
||
| port, err := strconv.Atoi(raw) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return port >= 0 && port <= 65535 | ||
| } |
There was a problem hiding this comment.
The isValidPort function uses strconv.Atoi, which allows leading signs (e.g., +80) and doesn't strictly enforce that the input consists only of digits. According to RFC 7230, the port should be a sequence of digits. Additionally, we can optimize this by avoiding strconv.Atoi and checking the length first.
| func isValidPort(raw string) bool { | |
| if raw == "" { | |
| return false | |
| } | |
| port, err := strconv.Atoi(raw) | |
| if err != nil { | |
| return false | |
| } | |
| return port >= 0 && port <= 65535 | |
| } | |
| func isValidPort(raw string) bool { | |
| if raw == "" || len(raw) > 5 { | |
| return false | |
| } | |
| var port int | |
| for i := 0; i < len(raw); i++ { | |
| if raw[i] < '0' || raw[i] > '9' { | |
| return false | |
| } | |
| port = port*10 + int(raw[i]-'0') | |
| } | |
| return port <= 65535 | |
| } |
| func parseNormalizedAuthority(authority string) (string, bool) { | ||
| authority = utils.TrimSpace(authority) | ||
| if authority == "" || strings.Contains(authority, "@") { | ||
| return "", false | ||
| } | ||
|
|
||
| host := authority | ||
| if strings.HasPrefix(authority, "[") { | ||
| idx := strings.IndexByte(authority, ']') | ||
| if idx <= 1 { | ||
| return "", false | ||
| } | ||
|
|
||
| host = authority[1:idx] | ||
| rest := authority[idx+1:] | ||
| if rest != "" { | ||
| if !strings.HasPrefix(rest, ":") { | ||
| return "", false | ||
| } | ||
| if !isValidPort(rest[1:]) { | ||
| return "", false | ||
| } | ||
| } | ||
| } else { | ||
| if strings.Contains(authority, "[") || strings.Contains(authority, "]") { | ||
| return "", false | ||
| } | ||
|
|
||
| if i := strings.LastIndexByte(authority, ':'); i != -1 { | ||
| host = authority[:i] | ||
| port := authority[i+1:] | ||
|
|
||
| if strings.IndexByte(host, ':') >= 0 || !isValidPort(port) { | ||
| return "", false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| host = normalizeHost(host) | ||
| if host == "" { | ||
| return "", false | ||
| } | ||
|
|
||
| return host, true | ||
| } |
There was a problem hiding this comment.
The parseNormalizedAuthority function performs multiple scans of the authority string (e.g., strings.Contains, strings.HasPrefix, strings.IndexByte, strings.LastIndexByte). While correct for security, this could be optimized to reduce the number of passes over the string, especially since this middleware runs on every request. Consider a single-pass parser if performance becomes a bottleneck.
| }{ | ||
| {name: "plain host", input: "example.com", expected: "example.com", expectOK: true}, | ||
| {name: "host with valid port", input: "example.com:8080", expected: "example.com", expectOK: true}, | ||
| {name: "ipv6 with port", input: "[::1]:443", expected: "::1", expectOK: true}, | ||
| {name: "userinfo style authority", input: "allowed.com:443@attacker.example", expectOK: false}, | ||
| {name: "invalid port syntax", input: "allowed.com:http", expectOK: false}, | ||
| {name: "malformed bracket", input: "[::1", expectOK: false}, | ||
| } |
There was a problem hiding this comment.
Consider adding more edge cases to Test_ParseNormalizedAuthority, such as bare IPv6 (e.g., ::1), empty ports (e.g., example.com:), ports out of range (e.g., example.com:65536), and authorities with extra trailing data (e.g., [::1]extra).
{name: "plain host", input: "example.com", expected: "example.com", expectOK: true},
{name: "host with valid port", input: "example.com:8080", expected: "example.com", expectOK: true},
{name: "ipv6 with port", input: "[::1]:443", expected: "::1", expectOK: true},
{name: "ipv6 without port", input: "[::1]", expected: "::1", expectOK: true},
{name: "bare ipv6", input: "::1", expectOK: false},
{name: "empty port", input: "example.com:", expectOK: false},
{name: "port out of range", input: "example.com:65536", expectOK: false},
{name: "userinfo style authority", input: "allowed.com:443@attacker.example", expectOK: false},
{name: "invalid port syntax", input: "allowed.com:http", expectOK: false},
{name: "malformed bracket", input: "[::1", expectOK: false},
{name: "extra data after bracket", input: "[::1]extra", expectOK: false},|
@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.26% 91.23% -0.03%
==========================================
Files 132 132
Lines 12933 12968 +35
==========================================
+ Hits 11803 11832 +29
- Misses 715 717 +2
- Partials 415 419 +4
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.