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.

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

Fix redirect limited(0) should not follow redirects #1645

Merged
merged 4 commits into from Oct 10, 2022
Merged

Fix redirect limited(0) should not follow redirects #1645

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d240fe5
Fix redirect limited(0) should not follows redirects
Oct 7, 2022
b42bc56
Remove test because it doesn't cover the bug
Oct 8, 2022
e0a5baf
Add test for redirect limit being set to 0
Oct 9, 2022
7cc1bb9
Merge branch 'master' into master
phankydn Oct 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/redirect.rs
View file
Open in desktop
Expand Up @@ -128,7 +128,7 @@ impl Policy {
match self.inner {
PolicyKind::Custom(ref custom) => custom(attempt),
PolicyKind::Limit(max) => {
if attempt.previous.len() == max {
if attempt.previous.len() >= max {
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this still be without the change? The previous length should be 0, and max is 0... Is there a time that isn't true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reqwest/src/async_impl/client.rs

Line 1975 in a0a7db0

self.as_mut().urls().push(url);

Yes, it isn't true because the url is pushed to the previousbefore calling check, so the length always has at least 1 item.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, of course! Want to readd the test but with a URL in the previous Vec as well, so we catch this if it ever changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test.

attempt.error(TooManyRedirects)
} else {
attempt.follow()
Expand Down Expand Up @@ -277,6 +277,18 @@ fn test_redirect_policy_limit() {
}
}

#[test]
fn test_redirect_policy_limit_to_0() {
let policy = Policy::limited(0);
let next = Url::parse("http://x.y/z").unwrap();
let previous = vec![];

match policy.check(StatusCode::FOUND, &next, &previous) {
ActionKind::Error(err) if err.is::<TooManyRedirects>() => (),
other => panic!("unexpected {:?}", other),
}
}

#[test]
fn test_redirect_policy_custom() {
let policy = Policy::custom(|attempt| {
Expand Down