-
Notifications
You must be signed in to change notification settings - Fork 4
fix(hsts): split hsts into WriteHeader and Redirect #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's Guide by SourceryThis pull request splits the HSTS middleware into two separate middlewares: Sequence diagram for HSTS middleware before and after splitsequenceDiagram
participant Client
participant HSTS_Before as HSTS (Before)
participant HSTS_After as HSTS (After)
participant App
Note over HSTS_Before: Single middleware handling both HTTP and HTTPS
Client->>HSTS_Before: HTTP Request
HSTS_Before-->>Client: Redirect to HTTPS
Client->>HSTS_Before: HTTPS Request
HSTS_Before->>HSTS_Before: Set STS Header
HSTS_Before->>App: Forward Request
Note over HSTS_After: Split into two middlewares
Client->>HSTS_After: HTTP Request
rect rgb(200, 200, 200)
Note right of HSTS_After: Redirect Middleware
HSTS_After-->>Client: Redirect to HTTPS
end
Client->>HSTS_After: HTTPS Request
rect rgb(200, 200, 200)
Note right of HSTS_After: WriteHeader Middleware
HSTS_After->>HSTS_After: Set STS Header
HSTS_After->>App: Forward Request
end
Class diagram for HSTS middleware changesclassDiagram
class Config {
+int64 MaxAge
+bool IncludeSubDomains
+bool Preload
}
class HSTS_Old {
+Enable(opts ...Option) Middleware
}
note for HSTS_Old "Before: Single middleware"
class HSTS_New {
+WriteHeader(opts ...Option) Middleware
+Redirect() Middleware
}
note for HSTS_New "After: Split into two middlewares"
HSTS_Old ..> Config
HSTS_New ..> Config
Flow diagram for HSTS request handlingflowchart TD
A[Request] --> B{Is HTTPS?}
B -->|No| C[Redirect Middleware]
B -->|Yes| D[WriteHeader Middleware]
C --> E[Return 302 Found]
C --> F[Redirect to HTTPS URL]
D --> G[Set STS Header]
D --> H[Continue Request]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 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.
Here's the code health analysis summary for commits Analysis Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 90.53% 90.66% +0.12%
==========================================
Files 37 37
Lines 1279 1286 +7
==========================================
+ Hits 1158 1166 +8
+ Misses 84 83 -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
hsts.Enable
intohsts.Redirect
for http request, andhsts.WriteHeader
for https requestFixed
Added
Tests
Tasks to complete before merging PR:
make unit-test
to check for any regressions 📋make lint
to check for any issuesSummary by Sourcery
Split the HSTS middleware into two separate middlewares:
Redirect
andWriteHeader
.Redirect
redirects HTTP requests to HTTPS, andWriteHeader
sets theStrict-Transport-Security
header for HTTPS requests.Enhancements:
Tests:
Redirect
andWriteHeader
middlewares.