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

Accept-Encoding tests fail with Hyper #7169

Closed
kevinburkemeter opened this issue Jun 2, 2021 · 1 comment
Closed

Accept-Encoding tests fail with Hyper #7169

kevinburkemeter opened this issue Jun 2, 2021 · 1 comment
Assignees

Comments

@kevinburkemeter
Copy link

@kevinburkemeter kevinburkemeter commented Jun 2, 2021

A lot of tests verify the client sends something like this (borrowed from test 221):

<verify>
<strippart>
s/^Accept-Encoding: .*/Accept-Encoding: xxx/
</strippart>
<protocol>
GET /%TESTNUMBER HTTP/1.1
Host: %HOSTIP:%HTTPPORT
User-Agent: curl/%VERSION
Accept: */*
Accept-Encoding: xxx

</protocol>
<errorcode>
61
</errorcode>
</verify>

The client actually sends Accept-Encoding: deflate, gzip[CR][LF]. This gets replaced by the <strippart> bit to Accept-Encoding: xxx[LF] - the .* matches, and removes, the carriage return. However, the <protocol> section compensates for this by omitting the carriage return from the expected client message, only on that Accept-Encoding line. I've taken a photo to make this clear - the ^M is a carriage return.

fg 2021-06-01 21-22-42

This is mostly ok, except that runtests.pl then calls subNewlines on Accept-Encoding: xxx[LF], which restores the carriage return and fails the test, for Hyper specifically.

I think the fix is to make the strippart matching better - match on a space character, a comma, or [a-zA-Z], but don't match on carriage returns, and then modify the <protocol> bits to include the carriage return, which is what actually gets sent by the client.

@bagder bagder self-assigned this Jun 2, 2021
@bagder
Copy link
Member

@bagder bagder commented Jun 2, 2021

Thanks! I agree with the analysis and fix. I could easily make test 221 work with this:

diff --git a/tests/data/test221 b/tests/data/test221
index ce80637b1..2d769bcc7 100644
--- a/tests/data/test221
+++ b/tests/data/test221
@@ -53,18 +53,18 @@ http://%HOSTIP:%HTTPPORT/%TESTNUMBER --compressed
 
 #
 # Verify data after the test has been "shot"
 <verify>
 <strippart>
-s/^Accept-Encoding: .*/Accept-Encoding: xxx/
+s/^Accept-Encoding: [a-zA-Z, ]*/Accept-Encoding: xxx/
 </strippart>
 <protocol>
 GET /%TESTNUMBER HTTP/1.1
 Host: %HOSTIP:%HTTPPORT

I'll work on applying this on more Accept-Encoding tests and then do a PR...

bagder added a commit that referenced this issue Jun 2, 2021
The previous strip also removed the CR which turned problematic.

Reported-and-analyzed-by: Kevin Burke
Fixes #7169
@bagder bagder closed this in 9dc0baf Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants