-
Notifications
You must be signed in to change notification settings - Fork 4
fix(ext): added support to X-Forwarded-Proto in https checking #49
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 adds support for the Sequence diagram for HTTPS check with X-Forwarded-ProtosequenceDiagram
participant Client
participant Proxy
participant Server
Client->>Proxy: GET /resource
Proxy->>Server: GET /resource X-Forwarded-Proto: https
Server->>Server: Check r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"
Server-->>Proxy: Response
Proxy-->>Client: Response
Updated class diagram for request loggingclassDiagram
class Context {
Request
Response
}
class Request {
+TLS
+Header
+Host
+URL
+RemoteAddr
}
class Header {
+Get(key string) string
}
Context -- Request : has
Request -- Header : has
note for Request "Added check for X-Forwarded-Proto"
Updated class diagram for HSTS middlewareclassDiagram
class Middleware {
+WriteHeader(opts ...Option) xun.Middleware
}
class Context {
Request
}
class Request {
+TLS
+Header
+Method
}
class Header {
+Get(key string) string
}
Middleware -- Context : uses
Context -- Request : has
Request -- Header : has
note for Request "Added check for X-Forwarded-Proto"
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
|
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 - here's some feedback:
Overall Comments:
- The added
Deploy
section in README.md looks good, but consider adding a brief explanation of the benefits of embedding assets. - The changes to
ext/reqlog/format.go
andext/hsts/hsts.go
look good, but consider adding a comment explaining why the port is hardcoded to 80/443.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 93.98% 94.00% +0.01%
==========================================
Files 54 54
Lines 2196 2203 +7
==========================================
+ Hits 2064 2071 +7
Misses 98 98
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changed
Fixed
X-Forwarded-Proto
in https checkingAdded
Deploy
section on README.mdTests
Tasks to complete before merging PR:
make unit-test
to check for any regressions 📋make lint
to check for any issuesSummary by Sourcery
Add support for
X-Forward-Proto
header and document the deployment process. Update the HTTPS checking logic to handle theX-Forward-Proto
header, enabling proper identification of HTTPS requests when the application is deployed behind a proxy. Include a new section in the README explaining how to deploy the application using embedded static assets and configuration files for a simplified and portable deployment process.Bug Fixes:
X-Forward-Proto
header in HTTPS checking logic to correctly identify HTTPS requests behind a proxy.Documentation:
//go:embed
directive for self-contained binaries.