(GH-200) Make SecurityProtocol changes immediately #201
Conversation
| @@ -119,6 +119,7 @@ public static bool url_is_valid(Uri url) | |||
| request.Timeout = 15000; | |||
| //This would allow 301 and 302 to be valid as well | |||
| request.AllowAutoRedirect = true; | |||
| request.UserAgent = "Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.88 Safari/537.36"; | |||
There was a problem hiding this comment.
Perhaps we want to change this into something like "Chocolatey Package Validator v0.4.2"
There was a problem hiding this comment.
@mkevenaar I actually had the same thought, but I wasn't sure on what the "normal" thing to do here is. The Unit Tests pass when using this as a UserAgent string, so there is no immediate reason to "not" do this. Will wait for other opinions here before making this change.
There was a problem hiding this comment.
@gep13 choco.exe uses "Chocolatey Command Line"
https://github.com/chocolatey/choco/blob/564a65235ea9b019fe0abbf3f9929b98da49ce5c/src/chocolatey/infrastructure.app/ApplicationParameters.cs#L95
|
@gep13 To make things easier to validate in the future, extra tests containing the URLs from #200 might be a good idea. But they would be harder to maintain as URLs could change and therefore the tests could fail. @ferventcoder your thoughts? |
Changes to the SecurityProtocolType should be made before creating any request objects, otherwise those settings aren't applied to the request object that already exists.
I had the same thought. I have added some tests to include the URL's that came up in issue #200 |
Some websites don't respond unless a valid UserAgent header is send in the request.
It has been found that there are some URL's that are valid but that fail validation. These came up as part of this issue, so they have been added as specific test cases to prevent regressions. It could be that over time these URL's change to no longer test what they are meant for, but for now, they are a good indicator of the problems.
| { | ||
| base.Context(); | ||
|
|
||
| // This URL should require TLS 1.3 on the client, otherwise it won't be able to establish a connection |
There was a problem hiding this comment.
Just noticed this now. The url isn't just TLS 1.3, but also uses TLS 1.2: https://gf.dev/tests/2s0nf10qgbt
Minor thing, but just wanted to mention it.
Changes to the SecurityProtocolType should be made before creating any
request objects, otherwise those settings aren't applied to the request
object that already exists.
Relates to #200