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

http_client: Handle relative redirects #6049

Merged
merged 1 commit into from
Nov 19, 2019
Merged

http_client: Handle relative redirects #6049

merged 1 commit into from
Nov 19, 2019

Conversation

theopolis
Copy link
Member

@theopolis theopolis commented Nov 17, 2019 •
edited

This should fix #6043, where I think the root cause is the URI parsing. If there is no protocol/host/port then the URI is parsed incorrectly. We missed this due to the rarity of relative-redirects and due to the HTTP client being difficult to test.

This also adds a max of 10 redirects before the client gives up.

@theopolis
Copy link
Member Author 

osquery/remote/http_client.cpp:425:38: error: invalid operands to binary expression ('boost::optional<std::string>' (aka 'optional<basic_string<char, char_traits<char>, allocator<char> > >') and 'const char *')
          redir_url = req.protocol() + "://" + req.remoteHost() + redir_url;
                      ~~~~~~~~~~~~~~ ^ ~~~~~
/usr/include/c++/v1/iterator:761:1: note: candidate template ignored: could not match 'reverse_iterator<type-parameter-0-0>' against 'char const[4]'
operator+(typename reverse_iterator<_Iter>::difference_type __n, const reverse_iterator<_Iter>& __x)
^

zwass
zwass approved these changes Nov 19, 2019
View reviewed changes
osquery/remote/http_client.cpp
}
} else {
// Absolute URI.
init_request = true;
Copy link
Member

Choose a reason for hiding this comment

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

A possible optimization would be to check whether the protocol, host, and port match before setting this and initializing a new connection. Not necessary IMO for merging this PR.

@theopolis theopolis merged commit fa8ac48 into osquery:master Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwass zwass zwass approved these changes

Assignees
No one assigned
Labels
bug remote
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http_client fails to handle 301 with just a path properly
2 participants
@theopolis @zwass