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
base: main
Are you sure you want to change the base?
feature-2924: Add an option to suppress server identification headers #3770
Conversation
…ion` and `server` headers
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!
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?
Testing has been done 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.
Will look at integration tests now.
Is this related to any issues?
Closes #2924
Does this change need documentation?
Probably - not done yet.
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?