Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
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:
WalkthroughAdds a new hostauthorization middleware: implementation, configuration, documentation, README index entry, and comprehensive tests/benchmarks that enforce Host header allowlisting (exact, leading-dot wildcard, CIDR, dynamic validator) and rejection via a configurable error handler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant App as Fiber App
participant HA as HostAuthorization
participant Store as AllowedHostsStore
participant Func as AllowedHostsFunc
participant Err as ErrorHandler
Client->>App: HTTP request (Host header)
App->>HA: invoke middleware
HA->>HA: normalize Host (strip port/brackets/trailing dot, lowercase)
HA->>Store: check exact / wildcard / CIDR matches
alt matched
Store-->>HA: allowed
HA->>App: c.Next() -> downstream handler
else not matched
HA->>Func: if configured, call AllowedHostsFunc(host)
alt func returns true
Func-->>HA: allowed
HA->>App: c.Next() -> downstream handler
else
HA->>Err: call ErrorHandler(c, ErrForbiddenHost)
Err-->>Client: 403 Forbidden (or custom response)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4199 +/- ##
==========================================
+ Coverage 91.22% 91.28% +0.05%
==========================================
Files 123 125 +2
Lines 11892 11971 +79
==========================================
+ Hits 10849 10928 +79
Misses 655 655
Partials 388 388
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.
Code Review
This pull request introduces a new hostauthorization middleware for Fiber v3, designed to protect against DNS rebinding attacks by validating the Host header against an allowlist of exact domains, subdomain wildcards, and CIDR ranges. The implementation includes comprehensive documentation and tests. Feedback focuses on making the configuration initialization more idiomatic for the Fiber framework and optimizing the performance of subdomain matching by pre-storing wildcard suffixes with their leading dots to avoid allocations during request processing.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/middleware/hostauthorization.md`:
- Around line 63-79: The CIDR Ranges doc misleadingly describes source-IP
filtering; update the wording to clarify that hostauthorization checks the
request Host header against AllowedHosts (not the client's IP). Edit the
examples and comments around hostauthorization.New and hostauthorization.Config
AllowedHosts entries to say things like "allowed Host header IP range" or "Host
header value" instead of "internal network" or "respond to requests from known
IP ranges" so readers understand this matches Host header strings/CIDR-like IPs
in the Host header only.
In `@middleware/hostauthorization/config.go`:
- Around line 13-41: Add the missing Exclude hook to the public Config by
declaring a field Exclude func(c fiber.Ctx) bool (optional, default nil) next to
Next and ErrorHandler in the Config struct; update the middleware code path that
currently checks Next to also call Exclude (both should be evaluated before any
host validation/AllowedHosts/AllowedHostsFunc logic) and treat a true return
from Exclude as a signal to skip host validation and not invoke ErrorHandler.
Ensure the field name is exactly Exclude and that callers can provide it the
same way as Next/ErrorHandler.
In `@middleware/hostauthorization/hostauthorization.go`:
- Around line 24-45: Normalize each configured host string using normalizeHost()
before any checks and storage: call normalizeHost(h) (after trimming) and skip
if it becomes empty; for CIDR parsing still use the normalized value for
net.ParseCIDR and append the result to parsed.cidrNets; for wildcard entries
(case strings.HasPrefix(h, ".")) store the suffix without the leading dot from
the normalized value into parsed.wildcardSuffixes; for exact matches insert the
normalized host into parsed.exact. Ensure all references use the normalized host
variable so comparisons match request hosts.
🪄 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: abe37dce-637a-44c8-80d2-c96bfa96bba2
📒 Files selected for processing (5)
.github/README.mddocs/middleware/hostauthorization.mdmiddleware/hostauthorization/config.gomiddleware/hostauthorization/hostauthorization.gomiddleware/hostauthorization/hostauthorization_test.go
|
Thanks for the contribution @mutantkeyboard , DNS rebinding protection is a valid concern and I appreciate the thorough work on docs and tests. Before we move forward, I want to raise a design question: should this be a standalone middleware or an extension of the existing
If we do keep it as a standalone middleware, a few things need addressing: Performance
API / Consistency
Documentation
Scope question
I'd like to hear your thoughts on the |
|
Hey @ReneWerner87, thank you for the review. Regarding I'm genuinely curious what did you have in mind there. My original inspiration for this was brannondorsey/host-validation, which is explicitly a standalone Performance
This one is actually a good catch - config entries with trailing dots won't match runtime-normalized hosts. Easy fix. CIDR docs This is actually a fair point. I can fix the wording to make clear it's matching the But regarding the overall architecture, I can integrate it into the existing WDYT? |
|
ok Go ahead with the leading-dot fix at init + normalizeHost on config entries, that alone should clean up the hot path nicely. And yes, please add an isolated benchmark for just the matching logic so we have real numbers for the middleware cost without pls also try the utils functions for the better performance |
|
@ReneWerner87, am I crazy or it seems like fasthttp 1.7.0 has broken multiple packages with stuff like |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
middleware/hostauthorization/hostauthorization_test.go (1)
288-309: Add explicitExcludeintegration coverage for parity.This file validates
Nextbypass behavior, but there’s no explicit integration test forExclude. Adding one would lock in the agreed middleware parity requirement and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/hostauthorization/hostauthorization_test.go` around lines 288 - 309, Add a parallel integration test mirroring Test_HostAuthorization_Next to exercise the Exclude hook: create Test_HostAuthorization_Exclude, spin up a fiber app, register the middleware with AllowedHosts set to ["example.com"] and Exclude set to a function that returns true for c.Path() == "/healthz", register a GET /healthz handler that returns "OK", then send a request with req.Host = "evil.com" to "/healthz" and assert the response status is 200; use the same patterns (t.Parallel(), app.Use(New(Config{Exclude: ...})), and app.Test(...)) so it verifies Exclude bypass parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/hostauthorization/hostauthorization_test.go`:
- Around line 262-266: The test currently uses require.Error(t, err) which is
too generic; tighten the assertion to verify the failure is specifically the
Host-header rejection from app.Test by replacing require.Error(t, err) with a
targeted check such as require.ErrorContains(t, err, "host") or
require.ErrorContains(t, err, "missing Host") (or use require.ErrorIs with the
fasthttp specific error if available) so the test asserts the expected
Host-header rejection signal from app.Test rather than any error.
---
Nitpick comments:
In `@middleware/hostauthorization/hostauthorization_test.go`:
- Around line 288-309: Add a parallel integration test mirroring
Test_HostAuthorization_Next to exercise the Exclude hook: create
Test_HostAuthorization_Exclude, spin up a fiber app, register the middleware
with AllowedHosts set to ["example.com"] and Exclude set to a function that
returns true for c.Path() == "/healthz", register a GET /healthz handler that
returns "OK", then send a request with req.Host = "evil.com" to "/healthz" and
assert the response status is 200; use the same patterns (t.Parallel(),
app.Use(New(Config{Exclude: ...})), and app.Test(...)) so it verifies Exclude
bypass parity.
🪄 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: 32515b96-3acc-427e-a678-e2573add4fa0
📒 Files selected for processing (1)
middleware/hostauthorization/hostauthorization_test.go
@ReneWerner87 @mutantkeyboard the PR for fasthttp was merged with failing tests. They added a bunch of security fixes that conflict with our security checks. |
|
@mutantkeyboard Test failures are now related to the new middleware. |
|
@gaby - I will fix that today |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
middleware/hostauthorization/hostauthorization_test.go (1)
275-279:⚠️ Potential issue | 🟡 MinorTighten the empty-host error branch assertion.
On Lines 276-277, returning on any error can make this test pass for unrelated failures. Assert the expected Host-related failure before returning.
Suggested fix
resp, err := app.Test(req) if err != nil { + require.ErrorContains(t, err, "Host") return } require.Equal(t, fiber.StatusForbidden, resp.StatusCode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/hostauthorization/hostauthorization_test.go` around lines 275 - 279, The test currently returns on any non-nil error from app.Test(req), which can mask unrelated failures; modify the app.Test(req) error branch to assert the error is the expected host-related failure (e.g., using require.ErrorContains(t, err, "host" or "empty host") or an equivalent check against the Host validation error) before returning, then keep the existing require.Equal(t, fiber.StatusForbidden, resp.StatusCode) assertion for the response path; reference the app.Test call and the err variable when adding the failure-specific assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/hostauthorization/hostauthorization_test.go`:
- Around line 455-457: The test currently calls require.ErrorIs inside the
ErrorHandler closure; move that assertion out of the callback by making
ErrorHandler side-effect-free (return the custom status/body only). Capture the
error returned by app.Test or assign the error to an outer variable inside the
closure (e.g., var handlerErr error; in ErrorHandler set handlerErr = err) and
then after calling app.Test perform require.ErrorIs(t, handlerErr,
ErrForbiddenHost) and any response assertions; reference ErrorHandler,
require.ErrorIs, ErrForbiddenHost, and app.Test when making the change.
---
Duplicate comments:
In `@middleware/hostauthorization/hostauthorization_test.go`:
- Around line 275-279: The test currently returns on any non-nil error from
app.Test(req), which can mask unrelated failures; modify the app.Test(req) error
branch to assert the error is the expected host-related failure (e.g., using
require.ErrorContains(t, err, "host" or "empty host") or an equivalent check
against the Host validation error) before returning, then keep the existing
require.Equal(t, fiber.StatusForbidden, resp.StatusCode) assertion for the
response path; reference the app.Test call and the err variable when adding the
failure-specific assertion.
🪄 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: 2b53a8e5-b6e4-4ba9-a4e6-fa3bc158bd36
📒 Files selected for processing (2)
middleware/hostauthorization/hostauthorization.gomiddleware/hostauthorization/hostauthorization_test.go
|
@gaby - this is now fixed. 💪💪 |
|
Given the current implementation and the distinct purposes, I recommend keeping Here's why:
Therefore, maintaining it as a dedicated middleware seems to be the most robust and flexible approach for this security feature. |
There was a problem hiding this comment.
Pull request overview
Adds a new hostauthorization middleware to validate the incoming Host header against an allowlist to mitigate DNS rebinding / Host-header attacks, with support for exact hosts, subdomain wildcards, CIDR ranges, and an optional dynamic validator.
Changes:
- Introduces
middleware/hostauthorizationwith config, parsing/normalization, and request-time host matching. - Adds a comprehensive test suite plus benchmarks for matching and end-to-end middleware behavior.
- Adds end-user documentation and lists the middleware in
.github/README.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| middleware/hostauthorization/hostauthorization.go | Implements allowlist parsing + host matching and the middleware handler. |
| middleware/hostauthorization/config.go | Defines middleware configuration, defaults, and exported error. |
| middleware/hostauthorization/hostauthorization_test.go | Adds unit/integration tests and benchmarks for matching behavior and proxy semantics. |
| docs/middleware/hostauthorization.md | Documents usage, matching/normalization rules, and proxy/RFC notes. |
| .github/README.md | Adds the new middleware to the project’s middleware list. |
Description
Add a new
hostauthorizationmiddleware that validates the incoming Host header against a configurable allowlist, rejecting unauthorized hosts with 403 Forbidden. This protects against DNS rebinding attackswhere an attacker-controlled domain resolves to the application's internal IP, causing browsers to send requests with a malicious Host header.
The middleware supports exact host matching, subdomain wildcards (.myapp.com), and CIDR ranges (10.0.0.0/8), with an optional dynamic validator function for runtime host resolution. It complements the
Domain()router (#4100) —
Domain()handleshost-basedrouting whilehostauthorizationacts as a security gate.Fixes #4148
Changes introduced
middleware. Next, ErrorHandler follow established Fiber conventions.
dependencies, no state, no caching.
Domain()router.Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md