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

Tests for HTTP/1.1 Parsing (NUL, CR, LF, Colon and more) #50629

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

JannisBush
Copy link
Contributor

Parsing (invalid) HTTP/1.1 responses is not fully defined in HTTP and Fetch and browser behavior diverges from the specs and between different browsers and there are not many tests in WPT yet. The aim of this pull request is to document the current browser behavior for various edge cases and start standardizing edge case behavior.

This pull requests adds a bunch of tests for (invalid) HTTP/1.1 responses for various edge cases. The tests are marked as tentative.
The tests are about the general parsing of (invalid) HTTP/1.1 messages and are not about the parsing of individual (invalid) headers such as how browsers should handle invalid bytes in CSP headers (Pull Request for that here).

All tests use the following base response and inject various bytes in different places to test both status-line and header-line parsing.
There are two types of tests:

  • fetch tests: they fetch the resource and either fail (Network Error) or fetch the resource (Valid)
  • framing tests: they frame the resource in an IFrame. They can result in Network Error (browser renders an error frame), active-XFO (browser renders an error frame due to XFO), or render the iframe (no-XFO, either the value is invalid or the full header line containing XFO is ignored). Note that the tests currently do not distinguish between Network Error and active-XFO.
HTTP/1.1 200 OK
X-Frame-Options: DENY
Content-Length: 5

Test.

The following shows all the injection points and their names:

<first>HT<in-http>TP<before-slash>/<after-slash>1<before-dot>.<after-dot>1<after-ver> <before-num>200<after-num> <before-reason>OK<after-reason>
<leading>X-Frame<in-name>-Options<before-colon>:<after-colon>DE<in-value>NY<after-value>
Content-Length: 5

Test.

The following bytes are injected: LF, CR, HTAB, SP, NUL, COLON.

The expected behavior is described in a mapping:

let expected = {
    "SP": {
        "first": ["Network Error", "Network Error"],
        ...
        "in-value": ["Valid", "no-XFO"],
        "after-value": ["Valid", "active-XFO"]
    },
    ...
}

Currently, there are 102*2=204 tests. In the future, other characters could be injected or other tests can be added.
The initial expected mapping is strict and expects "Network Error" for almost everything that is not explicitly allowed.
The current results are (both on HTTP and HTTPS):

  • Chrome: 114 Pass, 90 Fail
  • Firefox: 61 Pass, 143 Fail
  • Safari: 92 Pass, 112 Fail

See also:

Copy link
Contributor

@jdm jdm left a comment

Choose a reason for hiding this comment

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

This is very thorough! I appreciate all of the clear explanations for the expected results.

@annevk
Copy link
Member

annevk commented Mar 3, 2025

Thank you for writing these, could you please address the lint issue?

One way of addressing it might be to instead of having lots of .asis files generate those dynamically on the server as some other HTTP parsing tests are doing. But you might also be able to exclude a whole directory for a specific rule, maybe?

@wpt-pr-bot wpt-pr-bot requested a review from jgraham March 4, 2025 18:30
@JannisBush
Copy link
Contributor Author

Thank you for writing these, could you please address the lint issue?

One way of addressing it might be to instead of having lots of .asis files generate those dynamically on the server as some other HTTP parsing tests are doing. But you might also be able to exclude a whole directory for a specific rule, maybe?

I fixed the linting errors (mostly by adding them to the correct ignore locations).
I intentionally generated the asis files such that one can easily look at the exact bytes send with a hex editor.

@jdm
Copy link
Contributor

jdm commented Mar 4, 2025

The end of the log at https://community-tc.services.mozilla.com/tasks/WDE8qX2hSEi6Srt9TIiQuQ/runs/0/logs/public/logs/live.log points out various test results that appear to be unstable.

@JannisBush
Copy link
Contributor Author

The end of the log at https://community-tc.services.mozilla.com/tasks/WDE8qX2hSEi6Srt9TIiQuQ/runs/0/logs/public/logs/live.log points out various test results that appear to be unstable.

The issue is that there is no reliably way to check whether a frame finished loading/resulted in an error and the test relies on a timeout.
I copied the approach from here: https://github.com/web-platform-tests/wpt/blob/29d1210ae907e5bace1ecf047efde527c6377069/fetch/h1-parsing/resources-with-0x00-in-header.window.js#L16C1-L22C12

I increased the timeout from 1s to 2s and hope that it now results in stable results

@jdm
Copy link
Contributor

jdm commented Mar 10, 2025

@JannisBush
Copy link
Contributor Author

Mmm, still quite a bit of instability in https://community-tc.services.mozilla.com/tasks/DoF4XaEySBSPqEBcjKMm8g/runs/0/logs/public/logs/live.log :(

Right. Probably because we are running a lot of framing tests on the same top-level page.
Should I simply increase the timeout to a higher value (e.g., 5s) or should we try to split the tests and run them in batches?

@annevk
Copy link
Member

annevk commented Mar 10, 2025

I intentionally generated the asis files such that one can easily look at the exact bytes send with a hex editor.

I think it would be easier to analyze failures if one didn't have to do that and you could just see them written out in JSON as escapes. Converted to actual bytes at the last moment.

It probably makes sense to split this file using variants, yeah.

@JannisBush
Copy link
Contributor Author

JannisBush commented Mar 11, 2025

I intentionally generated the asis files such that one can easily look at the exact bytes send with a hex editor.

I think it would be easier to analyze failures if one didn't have to do that and you could just see them written out in JSON as escapes. Converted to actual bytes at the last moment.

I had some issues with the other handlers and only the asis handler enabled me to send exactly the responses I wanted to generate. The name of the file also includes the description of what is included where and one only needs to look at the file if one wants to verify that the files are correct.

It probably makes sense to split this file using variants, yeah.

Done

Seems like even with variants (max 6 frames per test page) and higher timeouts (4s), the results are still unstable in Chrome (stable in Firefox). I tried to reproduce it locally but failed. There might be another issue here or Chrome itself is unstable here.

@annevk
Copy link
Member

annevk commented Mar 11, 2025

@foolip @past are there known stability issues with Chromium?

@past
Copy link
Member

past commented Mar 11, 2025

@foolip @past are there known stability issues with Chromium?

All the existing tests seem pretty solid on Chromium, if you click on the test name and then click on Show History Timeline. These failures are all specific to the new test, but I don't know why that is the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants