Skip to content
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

Another improvement on the HTTPRequest documentation #52381

Merged

Conversation

arthurpaulino
Copy link
Contributor

@arthurpaulino arthurpaulino commented Sep 3, 2021

Raising flag about lack of encryption when sending data as a query string in the URL.

Sorry, I had just opened a PR on this same part of the documentation, but I think this is a rather important piece of information to be delivered to our gamedev folks.

@arthurpaulino arthurpaulino requested a review from a team as a code owner September 3, 2021 15:15
@arthurpaulino
Copy link
Contributor Author

And I have no idea why the static checks are breaking. Does anyone know why?

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 3, 2021

And I have no idea why the static checks are breaking. Does anyone know why?

Seems to be this: pypa/setuptools-scm#608

We'll look into it.

@Calinou
Copy link
Member

Calinou commented Sep 3, 2021

Is this actually the case? I was always convinced that when using HTTPS, the only bit of information that could leak to third parties was the domain name browsed due to DNS queries being made over a unencrypted protocol.

Of course, query strings may be logged in HTTP server logs regardless, which makes them a bad idea for sending credentials and the like.

@arthurpaulino
Copy link
Contributor Author

arthurpaulino commented Sep 3, 2021

Is this actually the case? I was always convinced that when using HTTPS, the only bit of information that could leak to third parties was the domain name browsed due to DNS queries being made over a unencrypted protocol.

Of course, query strings may be logged in HTTP server logs regardless, which makes them a bad idea for sending credentials and the like.

Good points! On a quick search I found these topics:

What you said seems to be accurate. But should we warn the user about the logs or let it be? I feel more inclined to raising awareness when possible. I can adjust the PR if you think it's a good idea. Otherwise we can just drop it.

@Faless
Copy link
Collaborator

Faless commented Sep 3, 2021

due to DNS queries being made over a unencrypted protocol.

And also when using SNI.

But as noted, HTTP request headers are encrypted in SSL.
So while using it for sensitive information might not be a great idea stating they are not encrypted is simply wrong.
Additionally, if you have control over both the client (i.e. it's not a web browser visiting a page), and the server (e.g. your VPS where you did setup logs correctly), you can safely use query parameters for that scope.

I'm okay with mentioning potential pitfalls, but we should not bring false informations.

@arthurpaulino
Copy link
Contributor Author

Alright, let me fix it and then we can decide after that.

@arthurpaulino arthurpaulino force-pushed the httprequest-note-improvement-2 branch from 150525f to af83b9e Compare September 3, 2021 16:31
@arthurpaulino
Copy link
Contributor Author

Done! Thanks for the information. I learned something 🎉

I hope the way I wrote it is correct and objective enough.

@Faless
Copy link
Collaborator

Faless commented Sep 3, 2021

Even if ssl_validate_domain is false, the information is encrypted if the URL is HTTPS (though susceptible to MITM attacks).
Even if ssl_validate_domain is true, the information is not encrypted if the url is HTTP.

So, this is not exactly correct, and might generate even more confusion :/

@arthurpaulino
Copy link
Contributor Author

arthurpaulino commented Sep 3, 2021

More learning! Let me clean up that part some more.

@arthurpaulino arthurpaulino force-pushed the httprequest-note-improvement-2 branch 2 times, most recently from 510f773 to a71a8fc Compare September 3, 2021 17:21
@arthurpaulino
Copy link
Contributor Author

Okay, is this version better?

@YuriSizov YuriSizov requested a review from a team September 5, 2021 15:21
@mhilbrunner
Copy link
Member

I'd just replace it with a comment stating that requests transmitting sensitive information should use encryption and to avoid using GET parameters for such information if possible.

@arthurpaulino arthurpaulino force-pushed the httprequest-note-improvement-2 branch from a71a8fc to ec18270 Compare September 6, 2021 18:12
@arthurpaulino
Copy link
Contributor Author

Thanks everyone for the input!
PR updated. LMK if anything needs to be changed.

@mhilbrunner
Copy link
Member

mhilbrunner commented Sep 7, 2021

Thanks for sparking this discussion :) For me, the new text currently says:

However, when sending sensitive data (such as login credentials), it's recommended to make a proper POST request using encryption and the [code]request_data[/code] parameter.

Recommending POST requests this heavily just because of sensitive data seems ill advised.

In common use, the main difference being GET and POST is retrieving data vs. submitting/modifying data (i.e, posting). (See REST.)
The difference isn't sensitive vs. non-sensitive data, which this now somewhat implies.
Both may need login credentials. In these cases, putting credentials in HTTP headers is common.

So we should

  1. suggest using encryption (SSL/TLS), especially when transmitting sensitive information (but its a good practice anyway)
  2. recommend against transmitting sensitive information in HTTP GET URL parameters (and maybe suggest using a HTTP POST request or HTTP headers)

Something like this?

It's recommended to use transport encryption (SSL/TLS) and to avoid sending sensitive information (such as login credentials) in HTTP GET URL parameters. Consider using HTTP POST requests or HTTP headers for such information instead.

@arthurpaulino arthurpaulino force-pushed the httprequest-note-improvement-2 branch from ec18270 to 75530c5 Compare September 7, 2021 14:48
@arthurpaulino
Copy link
Contributor Author

@mhilbrunner I've changed it to your wording but I've done it in a new note as the previous paragraph was already quite dense. Please let me know what you think 👍

@mhilbrunner mhilbrunner merged commit 72ac470 into godotengine:master Sep 7, 2021
@mhilbrunner
Copy link
Member

mhilbrunner commented Sep 7, 2021

Thanks for helping to improve the docs :)

@arthurpaulino arthurpaulino deleted the httprequest-note-improvement-2 branch September 7, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants