fix(hsts): used Optional pattern in Enable #30
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request refactors the Sequence diagram for HSTS middleware configurationsequenceDiagram
participant C as Client
participant M as HSTS Middleware
participant H as HTTP Handler
C->>M: HTTP Request
activate M
alt TLS not enabled & (GET or HEAD)
M->>M: Configure HSTS headers
Note over M: Apply MaxAge
Note over M: Apply IncludeSubDomains
Note over M: Apply Preload
M->>C: Redirect to HTTPS
else
M->>H: Forward Request
H->>C: Response
end
deactivate M
Class diagram for HSTS configuration changesclassDiagram
class Config {
+int64 MaxAge
+bool IncludeSubDomains
+bool Preload
}
class Option {
<<interface>>
+func(c *Config)
}
Option ..> Config : modifies
note for Config "New configuration struct"
note for Option "New functional options pattern"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR changes default values for includeSubDomains and preload to true, which is a breaking change. Consider either keeping the old defaults or documenting this change prominently in the changelog.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 2 issues found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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
Comment on lines
21
to
25
| cfg := &Config{ | ||
| MaxAge: defaultMaxAge, | ||
| IncludeSubDomains: true, | ||
| Preload: true, | ||
| } |
There was a problem hiding this comment.
🚨 suggestion (security): Consider making IncludeSubDomains and Preload false by default
These are strong security settings that can have significant implications. Preloading in particular is difficult to undo and should be an explicit opt-in choice.
Suggested change
| cfg := &Config{ | |
| MaxAge: defaultMaxAge, | |
| IncludeSubDomains: true, | |
| Preload: true, | |
| } | |
| cfg := &Config{ | |
| MaxAge: defaultMaxAge, | |
| IncludeSubDomains: false, | |
| Preload: false, | |
| } |
| // | ||
| // The maximum age specifies the duration for which the HSTS policy is in effect. | ||
| // Note that the maximum age is specified in seconds, so "1h" would be equivalent to 3600. | ||
| func WithMaxAge(t time.Duration) Option { |
There was a problem hiding this comment.
🚨 suggestion (security): Add validation for MaxAge to ensure it's within browser-accepted limits
Most browsers have maximum limits for the HSTS max-age (e.g., Chrome limits it to 2 years). Consider adding validation to prevent compatibility issues.
Suggested implementation:
}
// maxHSTSAge is the maximum allowed duration for HSTS (2 years, matching browser limits)
const maxHSTSAge = 2 * 365 * 24 * time.Hour
// Option is a function that modifies a Config instance.// WithMaxAge sets the maximum age for the HSTS policy.
//
// The maximum age specifies the duration for which the HSTS policy is in effect.
// Note that the maximum age is specified in seconds, so "1h" would be equivalent to 3600.
// The maximum allowed duration is 2 years (63072000 seconds) to ensure browser compatibility.
func WithMaxAge(t time.Duration) Option {
return func(c *Config) {
if t <= 0 {
return
}
if t > maxHSTSAge {
t = maxHSTSAge
}
c.MaxAge = int64(t / time.Second)|
|
||
| import "time" | ||
|
|
||
| // Config represents the configuration options for HSTS (HTTP Strict Transport Security). |
There was a problem hiding this comment.
issue (complexity): Consider using a constructor function instead of the functional options pattern to simplify the configuration logic and reduce code size.
The functional options pattern adds unnecessary complexity for this simple configuration. Consider using a constructor function instead:
// NewConfig creates a new HSTS configuration with the specified parameters.
// Pass 0 for maxAge to use the default duration.
func NewConfig(maxAge time.Duration, includeSubDomains bool, preload bool) Config {
c := Config{
IncludeSubDomains: includeSubDomains,
Preload: preload,
}
if maxAge > 0 {
c.MaxAge = int64(maxAge / time.Second)
}
return c
}This approach:
- Reduces code size by ~80% while maintaining all functionality
- Keeps the validation logic for maxAge
- Makes the API just as clear at the call site
- Is easier to understand and maintain
Usage remains simple: cfg := NewConfig(time.Hour, true, false)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 90.39% 90.53% +0.14%
==========================================
Files 36 37 +1
Lines 1260 1279 +19
==========================================
+ Hits 1139 1158 +19
Misses 84 84
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. |
|
Here's the code health analysis summary for commits Analysis Summary
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed
hsts.EnableFixed
Added
Tests
Tasks to complete before merging PR:
make unit-testto check for any regressions 📋make lintto check for any issuesSummary by Sourcery
Enhancements:
Enablefunction signature to accept a variadic number of options.