Fix parsing of legacy url_rewrite_program responses#2420
Conversation
squid-anubis
added
the
M-failed-description
This comment was marked as resolved.
This comment was marked as resolved.
rousskov
changed the title
squid-anubis
removed
the
M-failed-description
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for fixing this code!
I added a couple of polishing requests. Please also add yourself to CONTRIBUTORS, preferably using details matching your GitHub account; you can use the diff in the upcoming CI test failure as a guidance for CONTRIBUTORS modification.
I adjusted PR title to satisfy formatting requirements. I rewrote PR description to remove debugging, remove excessive/dangerous details, and to satisfy formatting requirements. Please adjust further if needed!
P.S. We should probably followup with the complete removal of legacy helper support in master/v8, but that requires non-trivial work, and the proposed surgical fix is the best next step.
The test actually succeeded despite GitHub marking your pull request with "First-time contributor". Apparently, this happens when there are non-ASCII characters in the otherwise missing entry. See the test log for details. Please add your entry using ASCII characters. P.S. I apologize that we are not accepting non-ASCII CONTRIBUTORS entries at this time. This is a known problem, and we will eventually address it. |
|
BTW, there is no need to squash PR branch commits. Our merge bot will do that automatically... |
Legacy helper responses start with a URL instead of OK rewrite_url=... and such. 2016 commit ddc77a2 introduced two bugs when handling legacy responses: - Response parsing code triggered MemBuf assertions when 0-terminating the parsing buffer for certain URLs. The bug affected legacy helper responses with and without space characters. - Squid code attempted to accept/use helper-returned URLs with embedded space character(s), despite a WARNING implying that the post-space characters are not going to become a part of the new URL. Signed-off-by: Renaud Metrich <rmetrich@redhat.com> Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for polishing this up! No further action is needed at this time. If there are no new change requests from others, then this PR will be automatically merged in zero to ten days (depending on whether others approve it as well).
rousskov
added
M-cleared-for-merge
kinkie
added
the
backport-to-v7
yadij
changed the title
yadij
removed
the
S-could-use-an-approval
Legacy helper responses start with a URL instead of `OK rewrite_url=...` and such. 2016 commit ddc77a2 introduced two bugs when handling legacy responses: * Response parsing code triggered MemBuf assertions when 0-terminating the parsing buffer for certain URLs. The bug affected legacy helper responses with and without space characters. * Squid code attempted to accept/use helper-returned URLs with embedded space character(s), despite a WARNING implying that the post-space characters are not going to become a part of the new URL.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
squidadm
removed
the
backport-to-v7
|
queued for backport to v7 |
6 participants
Legacy helper responses start with a URL instead of
OK rewrite_url=...and such. 2016 commit ddc77a2 introduced two bugs when handling legacy
responses:
Response parsing code triggered MemBuf assertions when 0-terminating
the parsing buffer for certain URLs. The bug affected legacy helper
responses with and without space characters.
Squid code attempted to accept/use helper-returned URLs with embedded
space character(s), despite a WARNING implying that the post-space
characters are not going to become a part of the new URL.