feat(ext): added hsts middleware #29
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new HSTS middleware that redirects HTTP requests to HTTPS and sets the Strict-Transport-Security header. The middleware is configurable with options for max-age, includeSubDomains, and preload. Sequence diagram for HSTS middleware HTTP to HTTPS redirectionsequenceDiagram
participant Client
participant HSTS
participant Server
Client->>HSTS: HTTP Request
alt is HTTP and GET/HEAD
HSTS->>HSTS: Check X-Forwarded-Proto
HSTS->>HSTS: Generate HTTPS URL
HSTS->>HSTS: Set Strict-Transport-Security header
HSTS-->>Client: 302 Redirect to HTTPS
else is HTTPS
HSTS->>Server: Forward Request
Server-->>Client: Response
end
Class diagram for HSTS middlewareclassDiagram
class HSTS {
+Enable(maxAge time.Duration, includeSubdomains bool, preload bool) xun.Middleware
-stripPort(hostPort string) string
}
note for HSTS "Middleware for enforcing HTTPS
and setting HSTS headers"
class xun.Middleware {
<<interface>>
+HandleFunc(next xun.HandleFunc) xun.HandleFunc
}
HSTS ..> xun.Middleware : implements
Flow diagram for HSTS middleware decision processflowchart TD
A[Request] --> B{Check Protocol}
B -->|HTTP| C{Check Method}
B -->|HTTPS| D[Forward to Next Handler]
C -->|GET/HEAD| E[Generate HTTPS URL]
C -->|Other| D
E --> F[Set HSTS Header]
F --> G[Redirect to HTTPS]
style A fill:#f9f,stroke:#333,stroke-width:2px
style D fill:#9f9,stroke:#333,stroke-width:2px
style G fill:#f99,stroke:#333,stroke-width:2px
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The HTTPS redirection logic appears incorrect - it's redirecting when already on HTTPS, which could cause an infinite redirect loop. (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue
- 🟡 Security: 1 issue found
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ext/hsts/hsts.go
Outdated
|
|
||
| isHTTPS := false | ||
| // Check X-Forwarded-Proto header first | ||
| forwardedProto := r.Header.Get("X-Forwarded-Proto") |
There was a problem hiding this comment.
🚨 issue (security): The X-Forwarded-Proto header should only be trusted from known proxy IPs to prevent spoofing.
Consider adding a configuration option to specify trusted proxy IPs and only accept the X-Forwarded-Proto header from these addresses.
| "github.com/yaitoo/xun" | ||
| ) | ||
|
|
||
| func TestHstsMiddleware(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Suggest adding tests for HTTPS requests.
It appears that the tests primarily focus on HTTP to HTTPS redirection. It's important to also verify the behavior when a request is already made over HTTPS. Specifically, ensure that the HSTS header is correctly set and that no redirection occurs in this scenario.
Suggested implementation:
func TestHstsMiddleware(t *testing.T) {
tests := []struct {
name string
scheme string
expectedStatus int
checkRedirect bool
checkHSTS bool
}{
{
name: "HTTP request should redirect to HTTPS",
scheme: "http",
expectedStatus: http.StatusMovedPermanently,
checkRedirect: true,
checkHSTS: false,
},
{
name: "HTTPS request should include HSTS header",
scheme: "https",
expectedStatus: http.StatusOK,
checkRedirect: false,
checkHSTS: true,
},
}
tr := http.DefaultTransport.(*http.Transport).Clone() for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := http.Client{
Transport: tr,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
},
}
server := httptest.NewTLSServer(xun.HSTS(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
})))
defer server.Close()
serverURL, _ := url.Parse(server.URL)
req, _ := http.NewRequest("GET", fmt.Sprintf("%s://%s", tt.scheme, serverURL.Host), nil)
resp, err := c.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, tt.expectedStatus, resp.StatusCode)
if tt.checkRedirect {
location := resp.Header.Get("Location")
require.True(t, strings.HasPrefix(location, "https://"))
}
if tt.checkHSTS {
hstsHeader := resp.Header.Get("Strict-Transport-Security")
require.NotEmpty(t, hstsHeader)
require.Contains(t, hstsHeader, "max-age=")
}
})
}You'll need to add these imports if they're not already present:
- "fmt"
- "net/http"
- "crypto/tls"
- "context"
- "net"
The test now uses a table-driven approach to test both HTTP and HTTPS scenarios. For the HTTPS case, it verifies:
- The response status is 200 OK (no redirect)
- The HSTS header is present
- The HSTS header contains the required max-age directive
You may need to adjust the expected status codes and header values based on your specific HSTS middleware implementation.
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The X-Forwarded-Proto header should only be trusted from known proxies (link)
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ext/hsts/hsts.go
Outdated
| forwardedProto := r.Header.Get("X-Forwarded-Proto") | ||
| if forwardedProto != "" { | ||
| isHTTPS = forwardedProto == "https" |
There was a problem hiding this comment.
🚨 issue (security): The X-Forwarded-Proto header should only be trusted from known proxies
Consider adding validation to ensure this header is only processed when coming from trusted proxy IPs. Otherwise, malicious clients could bypass HTTPS enforcement by spoofing this header.
| "github.com/yaitoo/xun" | ||
| ) | ||
|
|
||
| func TestHstsMiddleware(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Suggest adding tests for different HTTP methods.
The current tests only cover GET requests. It would be beneficial to add tests for other HTTP methods like POST, PUT, DELETE, etc., to ensure the middleware behaves correctly in all scenarios. For non-GET/HEAD requests, the middleware should not redirect and HSTS header should not be set.
Suggested implementation:
func TestHstsMiddleware(t *testing.T) {
testCases := []struct {
name string
method string
expectedStatus int
shouldRedirect bool
shouldHaveHSTS bool
}{
{
name: "GET request",
method: http.MethodGet,
expectedStatus: http.StatusOK,
shouldRedirect: true,
shouldHaveHSTS: true,
},
{
name: "HEAD request",
method: http.MethodHead,
expectedStatus: http.StatusOK,
shouldRedirect: true,
shouldHaveHSTS: true,
},
{
name: "POST request",
method: http.MethodPost,
expectedStatus: http.StatusOK,
shouldRedirect: false,
shouldHaveHSTS: false,
},
{
name: "PUT request",
method: http.MethodPut,
expectedStatus: http.StatusOK,
shouldRedirect: false,
shouldHaveHSTS: false,
},
{
name: "DELETE request",
method: http.MethodDelete,
expectedStatus: http.StatusOK,
shouldRedirect: false,
shouldHaveHSTS: false,
},
}
tr := http.DefaultTransport.(*http.Transport).Clone()You'll also need to:
- Modify the test implementation to iterate over the test cases
- Create requests using the specified method for each test case
- Add assertions to verify:
- Redirect behavior based on shouldRedirect
- HSTS header presence based on shouldHaveHSTS
- Expected status code
The exact implementation will depend on how the rest of the test is structured, but the test cases provide a framework for comprehensive HTTP method testing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 90.25% 90.39% +0.14%
==========================================
Files 35 36 +1
Lines 1231 1260 +29
==========================================
+ Hits 1111 1139 +28
- Misses 83 84 +1
Partials 37 37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changed
Fixed
Added
hstsmiddlewareTests
Tasks to complete before merging PR:
make unit-testto check for any regressions 📋make lintto check for any issuesSummary by Sourcery
New Features: