Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

Stop overriding Host and User-Agent headers #365

Merged
merged 3 commits into from Jun 19, 2019

Conversation

OnlyForF1
Copy link

Vapor's HTTPClient implementation would previously always override the
Host and User-Agent headers when sending a request to the server. At
best this was undesired behaviour, at worst it would result in
HTTP 400 Bad Request responses.

Now, default values will only be provided if Host or User-Agent headers
were not already set.

Vapor's HTTPClient implementation would previously always override the
Host and User-Agent headers when sending a request to the server. At
best this was undesired behaviour, at worst it would result in
HTTP 400 Bad Request responses.

Now, default values will only be provided if Host or User-Agent headers
were not already set.
@0xTim
Copy link
Member

0xTim commented Jun 11, 2019

Nice! Could you add some tests?

@tanner0101
Copy link
Member

tanner0101 commented Jun 11, 2019

We use https://github.com/swift-server/swift-nio-http-client now on master. I'm not sure if that package has the same problem, but it's worth checking.

This repo will likely be deprecated soon.

@OnlyForF1
Copy link
Author

OnlyForF1 commented Jun 13, 2019

@tanner0101 I am aware of this and I'll fix it there too. For now I am using Vapor 3 so I prioritised fixing that. On first glance it looks like that repo is not affected, I might write some unit tests to ensure there are no regressions in the future. (The behaviour in this repo was a regression)

@0xTim I've added unit tests, I had to change the visibility of some sections from private to internal. Let me know what you think.

@tanner0101
Copy link
Member

@OnlyForF1 ah, sorry, I didn't see that this was merging into 3.

@tanner0101 tanner0101 added the bug Something isn't working label Jun 13, 2019
@tanner0101 tanner0101 added this to In Progress in Vapor 3 via automation Jun 13, 2019
@OnlyForF1
Copy link
Author

I've refactored the tests to use httpbin, and reverted the visibility changes.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@tanner0101 tanner0101 merged commit 3808ed0 into vapor:3 Jun 19, 2019
@penny-coin
Copy link

Hey @OnlyForF1, you just merged a pull request, have a coin!

You now have 1 coins.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
No open projects
Vapor 3
  
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants