-
Notifications
You must be signed in to change notification settings - Fork 798
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
feature-2924: Add an option to suppress server identification headers #3770
feature-2924: Add an option to suppress server identification headers #3770
Conversation
Thank you for contributing this PR @byarr! To my knowledge, these headers are used by the SDKs in order to check compatibility with the backend. Have you done any testing regarding compatibility with the SDKs? Our original concern in #2924 was that naively implementing this option would lead to users enabling it while being unaware of the impact on their clients. I personally am happy to support an option to disable these headers, but I would like be aware of the impact of doing so before merging. |
Not yet. Probably should have raised this a draft PR. I'll take a look a the SDK behaviour next. |
Hi @byarr! Thanks a lot for contributing this change. We have discussed about it internally and we have agreed that it is worth offering this option to users regardless of potential compatibility impacts, especially when implemented as an environment variable like you did in your PR. Ideally, we would like our SDKs to rely on active mechanisms of identifying the server and, in the mean time, we will document that disabling the server identification headers may affect compatibility when using SDKs or third-party software integrations. For the time being, this option will remain useful to users that are okay with the potential impact of removing headers to benefit privacy. Regarding the specific changes that you made, they look good! I would probably change the name of the environment variable and option to be more generic so that it can include future headers that are used to identify the service. I would propose If you are able, it would be nice to have an integration test in |
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.
I have requested some small changes, but otherwise the change is great!
f9e7618
to
da982e8
Compare
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.
I suggested some nitpicky changes, one of which is required to pass the cargo fmt --check
action. After that, I am happy to merge and update the documentation myself. Thanks again for this contribution!
Hi @byarr! We would really like to merge your PR. Are you able to approve the suggestions? Those are all the changes that would be required to merge. I can write the documentation for the change myself. |
Co-authored-by: Gerard Guillemas Martos <gguillemas@users.noreply.github.com>
Co-authored-by: Gerard Guillemas Martos <gguillemas@users.noreply.github.com>
Co-authored-by: Gerard Guillemas Martos <gguillemas@users.noreply.github.com>
Approved! |
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.
Approved! Thank you for the contribution 😄
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.
LGTM!
remove-version-and-server-headers had recent pushes 29 minutes ago Add a hide_server_id_headers option that suppresses the
surreal-version
andserver
headersThank you for submitting this pull request! We really appreciate you spending the time to work on these changes.
What is the motivation?
Offering up information such as server running and the particular version can make it easier for bad actors to exploit known vulnerabilities.
What does this change do?
This adds a CLI option to not emit the
server
andsurreal-version
headers. By setting this flag, the server no longer emits this information.What is your testing strategy?
Integration test added.
Also tested manually. Starting the server without the flag set, curl and check the headers are present. Start the server again with the flag set - use curl to confirm the headers are not present.
Is this related to any issues?
Closes #2924
Does this change need documentation?
Server options need updating e.g. https://github.com/surrealdb/docs.surrealdb.com/blob/main/doc-surrealdb_versioned_docs/version-1.x/cli/start.mdx?plain=1#L196
If this pull request requires changes, updates, or improvements to the documentation, then add a corresponding issue on the docs.surrealdb.com repository, and link to it here.
Have you read the Contributing Guidelines?