Skip to content

Transfer scheme and host to new request when redirected#20

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.

Sign up for GitHub

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

Jump to bottom
Merged
artemredkin merged 12 commits into from
Apr 27, 2019
Merged

Transfer scheme and host to new request when redirected #20

artemredkin merged 12 commits into from
Apr 27, 2019

Conversation

@PopFlamingo
Copy link
Contributor

@PopFlamingoPopFlamingo commented Apr 25, 2019•
edited
Loading

This solves a bug that could cause infinite redirections (eg: requesting example.com, being redirected to www.example.com, but requesting again example.com as host hadn't been correctly modified).

@PopFlamingo
Transfer scheme and host to new request when redirected
fdb9dc1
This solves a bug that could cause infinite redirections (eg: requesting example.com, being redirected to www.example.com, but requesting again example.com as host hadn't been correctly specified)
@swift-server-bot
Copy link

swift-server-bot commented Apr 25, 2019

Can one of the admins verify this patch?

PopFlamingo
PopFlamingo commented Apr 25, 2019
View reviewed changes
@tomerd
Copy link
Contributor

tomerd commented Apr 25, 2019

@swift-server-bot test this please

tanner0101
tanner0101 reviewed Apr 26, 2019
View reviewed changes
Copy link
Member

@tanner0101tanner0101 left a comment

Choose a reason for hiding this comment

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

We should probably have a test case for this. Maybe we can use https://httpbin.org/#/Redirects ? Or we could model this with the HttpBin type already in the test module.

@PopFlamingo
Copy link
ContributorAuthor

PopFlamingo commented Apr 26, 2019

@tomerd@tanner0101 I added a test using httpbin.org, checking Host seems harder with a local server.

@tanner0101tanner0101 added the kind/bug Feature doesn't work as expected. label Apr 26, 2019
tanner0101
tanner0101 reviewed Apr 26, 2019
View reviewed changes
Copy link
Member

@tanner0101tanner0101 left a comment

Choose a reason for hiding this comment

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

Thanks for adding a test!

@PopFlamingo
Copy link
ContributorAuthor

PopFlamingo commented Apr 26, 2019

@tanner0101 Updated! 🙂

tanner0101
tanner0101 approved these changes Apr 26, 2019
View reviewed changes
Copy link
Member

@tanner0101tanner0101 left a comment

Choose a reason for hiding this comment

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

This LGTM. I'd like @artemredkin's approval before merging though since he initially wrote the RedirectHandler.

artemredkin
artemredkin reviewed Apr 26, 2019
View reviewed changes
artemredkin
artemredkin reviewed Apr 26, 2019
View reviewed changes
@artemredkinartemredkin merged commit 44ddc8b into swift-server:masterApr 27, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomerdtomerdtomerd left review comments

@artemredkinartemredkinartemredkin approved these changes

+1 more reviewer

@tanner0101tanner0101tanner0101 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/bugFeature doesn't work as expected.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PopFlamingo@swift-server-bot@tomerd@artemredkin@tanner0101