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

Fix a looping regression for 303 responses without a Location header #342

Merged
merged 2 commits into from Jun 18, 2020

Conversation

ntyni
Copy link
Contributor

@ntyni ntyni commented Jun 15, 2020

The change in 6.45 had a side effect of making it look at max_redirect only when the Location header is set.

The libapache2-mod-perl2 test suite implements a test server that answers with HTTP code 303 without including such a header.

RFC 7231 is not very clear on whether Location is required, although
the response doesn't make much sense without it.

Some googling reveals this is not a new question.

https://stackoverflow.com/questions/16194988/for-which-3xx-http-codes-is-the-location-header-mandatory

For the sake of robustness I suppose libwww-perl needs to be fixed.

Bug-Debian: https://bugs.debian.org/962904

The change in 6.45 had a side effect of making it look at max_redirect
only when the Location header is set.

The libapache2-mod-perl2 test suite implements a test server that answers
with HTTP code 303 without including such a header.

RFC 7231 is not very clear on whether Location is required, although
the response doesn't make much sense without it.

Some googling reveals this is not a new question.

  https://stackoverflow.com/questions/16194988/for-which-3xx-http-codes-is-the-location-header-mandatory

For the sake of robustness I suppose libwww-perl needs to be fixed.

Bug-Debian: https://bugs.debian.org/962904
@ntyni
Copy link
Contributor Author

ntyni commented Jun 15, 2020

Not sure if this reintroduces the issue 6.45 was fixing. The t/redirect tests still pass, though.

oalders
oalders previously approved these changes Jun 16, 2020
Copy link
Member

@oalders oalders left a comment

Choose a reason for hiding this comment

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

Thanks @ntyni!

@oalders oalders requested a review from simbabque June 16, 2020 00:44
simbabque
simbabque previously approved these changes Jun 16, 2020
@oalders
Copy link
Member

oalders commented Jun 16, 2020

@ntyni are you able to have a look at xt/author/live/jigsaw/redirect-post.t? It's failing on Travis right now.

@ntyni ntyni dismissed stale reviews from simbabque and oalders via 7af78df June 17, 2020 19:00
Changes Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 17, 2020

Coverage Status

Coverage increased (+0.02%) to 60.231% when pulling a8ae2b7 on ntyni:redirects-without-location into dd7ea8a on libwww-perl:master.

@ntyni ntyni force-pushed the redirects-without-location branch from 7af78df to a8ae2b7 Compare June 17, 2020 20:02
@ntyni
Copy link
Contributor Author

ntyni commented Jun 17, 2020

@ntyni are you able to have a look at xt/author/live/jigsaw/redirect-post.t? It's failing on Travis right now.

I doubt it regressed with this. It's working for me now. Not sure if it's intentional that only some of the live/jigsaw/ tests are skipped for NO_JIGSAW=1 ?

@oalders
Copy link
Member

oalders commented Jun 17, 2020

One of the issues with these live tests is that they can start to fail if there's some throttling going on. I can see that it's now passing, so we can punt on it for now! :)

@oalders oalders merged commit 8c5b71d into libwww-perl:master Jun 18, 2020
afresh1 added a commit to GrantStreetGroup/p5-HealthCheck-Diagnostic-WebRequest that referenced this pull request Aug 25, 2020
This works around a bug in LWP::UserAgent 6.45.

libwww-perl/libwww-perl#342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants