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

CORB confirmation sniffing for HTML, XML and JSON security prefix. #88

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

anforowicz
Copy link

@anforowicz anforowicz commented Oct 22, 2018

Hello @annevk, @csreis, @GPHemsley,

This is my attempt to start documenting CORB confirmation sniffing algorithm (as tracked by #87 and in whatwg/fetch#681 (comment)).

I hope that this pull request is close to being acceptable, but I do note that:

  • This is the first time I am submitting a pull request for https://github.com/whatwg/mimesniff
    and so it is likely that I made some newbie miskates. I am open to any feedback.
  • This pull request covers HTML, XML and JSON-security-prefix sniffing, but does not
    cover JSON sniffing (as implemented in Chromium in CrossOriginReadBlocking::SniffForJSON).
    I hope that this omission is okay. I am worried that sniffing for JSON can end up more complicated
    than the other kinds of sniffing, because 1) the current implementation requires a small state machine
    and 2) the current implementation is imperfect (e.g. doesn't recognize JSON lists).
  • I will need to eventually figure out how to integrate this pull request (CORB confirmation sniffing algorithm) with the core CORB algorithm in the Fetch spec. I hope it is okay to wait with that until this pull request is reviewed and approved first.

Please take a look?

-Lukasz

PS. I forgot to add my name to the contributors list. Let me fix this in a moment.


Preview | Diff

@anforowicz
Copy link
Author

Some notes on testing...

This pull requests has some limited test coverage via the following Web Platform Tests,:

  • wpt/fetch/corb/script-html-js-polyglot.sub.html which covers "CORB confirmation sniffing for HTML" section from the pull request, focusing on handling of HTML/Javascript comments and returning "possibly not HTML" for possible polyglots.
  • wpt/fetch/corb/img-png-mislabeled-as-html.sub.html, wpt/fetch/corb/script-js-mislabeled-as-html.sub.html, style-css-mislabeled-as-html.sub.html which covers "CORB confirmation sniffing for HTML" section from the pull request, focusing on returning "possibly not HTML" for images, Javascript scripts and CSS stylesheets

I note that adding more CORB tests is problematic (especially for the cases where confirmation sniffing succeeds), because CORB should in general be invisible to the web content - see also wpt/fetch/corb/README.md

Also... actually I see that I need to add text/css exclusion to pass style-css-with-json-parser-breaker.sub.html... Let me fix that in a moment...

@anforowicz
Copy link
Author

I note that adding more CORB tests is problematic

Hmmm... actually, it should be possible to add some test coverage for JSON parser breakers - I am trying to do that here: https://crrev.com/c/1295185.

Base automatically changed from master to main January 15, 2021 07:47
@@ -139,7 +139,7 @@ production. By definition it is a superset of the <a>HTTP token code points</a>.

<p>
A <dfn>whitespace byte</dfn> (abbreviated
<abbr lt="whitespace byte">0xWS</abbr>) is any one of the following
<abbr lt="whitespace byte">1xWS</abbr>) is any one of the following
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Author

Choose a reason for hiding this comment

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

This was accidental I think.

At any rate, CORB will most likely be replaced with ORB (https://github.com/annevk/orb) and this will mean that this CL can be abandoned.

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

Successfully merging this pull request may close these issues.

2 participants