-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Security Fixes for RESTful APIs #41136
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
Welcome @divyaruhil! It looks like this is your first PR to milvus-io/milvus 🎉 |
@divyaruhil Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
Invalid PR Title Format Detected Your PR submission does not adhere to our required standards. To ensure clarity and consistency, please meet the following criteria:
Required Title Structure:
Where Example:
Please review and update your PR to comply with these guidelines. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41136 +/- ##
===========================================
+ Coverage 73.93% 80.57% +6.64%
===========================================
Files 330 1509 +1179
Lines 30068 213372 +183304
===========================================
+ Hits 22230 171929 +149699
- Misses 7838 35267 +27429
- Partials 0 6176 +6176
🚀 New features to boost your workflow:
|
@divyaruhil thanks. There appears to be a minor issue with the code formatting. Please refer to the information provided below:
Some files have not been gofmt'd. To fix these errors, |
@@ -1748,6 +1748,8 @@ func RequestHandlerFunc(c *gin.Context) { | |||
c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") | |||
c.Writer.Header().Set("Access-Control-Allow-Headers", "Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization, accept, origin, Cache-Control, X-Requested-With") | |||
c.Writer.Header().Set("Access-Control-Allow-Methods", "GET, HEAD, POST, PUT, DELETE, OPTIONS, PATCH") | |||
c.Writer.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") // Ensures HTTPS usage |
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.
It is recommended that Strict-Transport-Security
, max-age
, and includeSubDomains
be made configurable. This way, open source users can modify them as needed.
Thankyou @czs007 , for the review comments, i'll make the required changes. |
@divyaruhil Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
Hi @czs007, I've made the updates based on your review comments. Please let me know if there's anything else you'd like me to address. |
@divyaruhil E2e jenkins job failed, comment |
@divyaruhil go-sdk check failed, comment |
@divyaruhil cpp-unit-test check failed, comment |
@divyaruhil E2e jenkins job failed, comment |
@divyaruhil go-sdk check failed, comment |
@divyaruhil cpp-unit-test check failed, comment |
@divyaruhil cpp-unit-test check failed, comment |
Hey @czs007, When you have a moment, could you please take a look and review this? I'd really appreciate your feedback. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: czs007, divyaruhil The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Hi @czs007, thank you for approving the PR and for your help! It looks like the integration test is failing, but it doesn’t seem related to our changes. Would it be possible to re-trigger the test? |
hi @cydrain , can u please have a look at this PR , whenever your time allows and approve it for merging , also Integration test seems to be failing but that's not related to any of the changes we made , is there a way we can re-trigger it ? |
hi @czs007 , Please let me know if anything else is required from my end . |
Signed-off-by: Divya <divyaruhil999@gmail.com>
Signed-off-by: Divya <divyaruhil999@gmail.com>
Signed-off-by: Divya <divyaruhil999@gmail.com>
Signed-off-by: Divya <divyaruhil999@gmail.com>
Signed-off-by: Divya <divyaruhil999@gmail.com>
/lgtm |
Fix for Issue :- #41128