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
utils/curl: fix headers check for protected urls #13198
Conversation
Review period skipped due to |
Library/Homebrew/utils/curl.rb
Outdated
details[:status].to_i == 403 && | ||
details[:headers].match?(/^Set-Cookie: visid_incap_/i) && | ||
details[:headers].match?(/^Set-Cookie: incap_ses_/i) | ||
details[:headers].fetch("set-cookie", "").match?(/visid_incap_|incap_ses_/i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the logic here because the same header won't satisfy both /^Set-Cookie: visid_incap_/i
and /^Set-Cookie: incap_ses_/i
(I assume that details[:headers]
is a regular hash that can't contain several values with the same key, and we got string values here).
/cc @samford
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
At the moment, headers from #parse_curl_output
only include the last instance of a given header (i.e., an earlier header will be overwritten by a later header). :headers
was just a string before (i.e., the entire header text from a response in curl
output), so this previously checked all instances of a given header in a response.
There can be multiple Set-Cookie
headers in a response, so we'll need to modify #parse_curl_output
to properly handle multiple instances of a header. The simplest route would be to collect values in an array when a header appears more than once (I've seen this done in some libraries). We will have to update logic that accesses the value (e.g., the methods in this PR) to handle a potential string or array value but there's no good way around that (i.e., concatenating values from multiple headers into one string can lead to problems).
I'll tinker with this a bit (later today) and try to get a PR up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bayandin! Change looks good, optional style tweak which you can decline if desired.
2747edd
to
c726385
Compare
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This PR fixes headers check in
url_protected_by_cloudflare? and
url_protected_by_incapsula?` methods.Spotted in Homebrew/homebrew-core#100147