Use 'get a structured header' term from Fetch #2

zcorpan · 2019-12-12T12:36:47Z

Change duplicate requirements in the Framework section to non-normative note.

Fail parsing if SH parameters are present, until SH is more stable.

zcorpan · 2019-12-12T12:40:20Z

Why isn't https://tools.ietf.org/html/draft-ietf-httpbis-header-structure-14#section-4.2 used as the entry point with the header's byte sequence value? It seems the spec now doesn't skip OWS and doesn't support parameters. Is that intentional?

mikewest

Thanks!

index.bs

zcorpan · 2019-12-12T23:19:39Z

OK I've changed this to call the main entry point 4.2 with the byte sequence, and handle the returned tuple (bare_item, parameters).

It wasn't clear to me whether Structured Headers would fail parsing if the type isn't a token, so I explicitly said that should happen.

I also banned parameters, but maybe we want to ignore parameters instead?

I also removed an inline issue since that issue appears to be resolved, and Structured Headers how takes a byte sequence as input and converts it to an ASCII string.

mikewest · 2019-12-13T09:38:52Z

@mnot and @annevk might have opinions about how they'd like this integration to work from a Structured Headers/Fetch standpoint. It's probably reasonable, for example, to add something like a "get a structured header name from list algorithm to header lists that would take care of the encoding questions?

In the status quo, this patch matches my understanding of what we need to do. SH gives you a raw parse of the header, and then it falls upon the spec to determine whether the result is acceptable.

annevk

Yeah, I would prefer we put an abstraction in Fetch.

In whatwg/fetch#930 I sketched something where for a header list there's an algorithm that takes a name and a type and returns failure or a representation of type. I think that still makes sense.

One other consideration I suppose is whether we should map structured header values to Infra values somehow for ease of use in specification algorithms and compatibility with IDL.

index.bs

mikewest · 2019-12-13T09:41:02Z

index.bs

-    2.  Let |parsed policy| be the result of executing the [$Structured Header parsing algorithm$]
-        with <var ignore>input_string</var> set to |header|, and <var ignore>header_type</var> set
-        to "`item`".
+        If |parsed policy| is not a token, fail parsing.


Suggested change

If |parsed policy| is not a token, fail parsing.

If |parsed policy| is not a [=structured header/token=], fail parsing.

mikewest · 2019-12-13T09:52:15Z

index.bs

-        passing it into the parsing algorithm?
-
-    3.  If |parsed policy| is "`require-corp`", set |policy| to "`require-corp`".
+    2.  If parsing did not fail and |parsed policy| is "`require-corp`", then set |policy| to


This is fine for the moment, but I don't really like the implicitness of "parsing failed". I'd prefer for the parsing algorithm to return something that we interpret as failure. Like null? (Obviously, this isn't the first place we're using this pattern, URL parsing comes to mind as well, but I don't like it there either. :) )

@annevk @mnot

I don't really mind how Structured Headers words it, but I agree that whatever we add in Fetch will have to do something like that. Bridging with the IETF world is always going to look a little clunky I suppose, though this is miles ahead of the pack.

(I suspect we want null for header not present, failure for failures, and instance of typed thingie.)

I see now that URL parser does "return failure", which is nicer than I remember. That explicit model seems reasonable to me. It might be nice to define that pattern via Infra?

@mikewest could you open an issue on SH regarding this?

As discussed in #930 and WICG/cross-origin-embedder-policy#2, this patch adds a "get" method on header lists that allows consistent extraction of structured headers.

mnot · 2019-12-13T23:57:22Z

I probably do have opinions, but I'm out until the first week in Jan...

zcorpan · 2019-12-19T12:17:16Z

In web-platform-tests/wpt#20852 (comment) @domenic said that parameters should be ignored, not fail parsing. This PR says to fail parsing. Which should we do?

From the Structured Headers spec, I think the intent is that unknown parameters should generally be ignored.

mikewest · 2019-12-19T12:50:49Z

From structured headers' perspective, we should be able to parse headers with all the junk that can be attached to bits and pieces of the header's value according to the SH grammar. From this specific header's perspective, it may be reasonable to impose additional restrictions on the header's syntax.

My main concern here is forward compatibility with new things we introduce. In particular for COEP, I expect that we'll want to introduce something like require-cors in the future (because one-character differences that cause massive behavioral shifts are awesome!). I have no plans to introduce any parameters to require-corp, but who knows. The future is dark and murky. It might be reasonable to treat require-corp;param1=value1 as require-corp, and it might be reasonable to ignore the header entirely.

It occurs to me now that we've dealt with unknown in the past for things like Referrer-Policy by ignoring the whole header, which allowed developers to do things like Referrer-Policy: new-funky-value, old-boring-fallback;. This model is a little more complicated in SH, because if we parse the header as a single item, it'll fail. We'd need to parse it as a list, and then ignore individual items in the list if they don't make sense, and then ensure that there aren't any additional items after the item we picked.

That's a bit weird.

I wonder if @mnot has opinions about this from the HTTP side (though I think he's going to be out until January), or @annevk from the Fetch side.

domenic · 2019-12-19T15:57:53Z

The SH spec seems pretty clear on the intent:

However, both Items and Inner Lists allow parameters as an extensibility mechanism; this means that values can later be extended to accommodate more information, if need be. As a result, header specifications are discouraged from defining the presence of an unrecognised parameter as an error condition.

I tend to agree with the SH spec, and don't see any reason to disallow parameters. They could be useful in the future and disallowing them just creates future compatibility problems, for no gain that I can see.

As discussed in #930 and WICG/cross-origin-embedder-policy#2, this adds a "get" algorithm on header lists that allows consistent extraction of structured header values.

mikewest · 2020-01-09T12:49:58Z

whatwg/fetch@8ca6148 landed in Fetch; @zcorpan, would you be interested in taking another pass at this, using that algorithm?

zcorpan · 2020-01-09T14:50:43Z

Done.

This now has conflicts with master, but I'd like to squash before rebasing, since index.html is checked in I anticipate that will make a rebase without first squashing painful.

See whatwg/fetch#983 Change duplicate requirements in the Framework section to non-normative note. Fail parsing if SH parameters are present, until SH is more stable.

zcorpan · 2020-01-16T18:56:34Z

I've squashed and rebased this now.

annevk · 2020-01-17T08:44:36Z

index.bs

-        ISSUE(httpwg/http-extensions#662): The ASCII requirements of Structured Headers are
-        somewhat unclear to me. Do we need to check whether |header| is an [=ASCII string=] before
-        passing it into the parsing algorithm?
+    4. If |parameters| is not empty, then return |policy|.


I think we should ignore parameters as we do for Content-Type.

Have you changed your opinion since whatwg/html#5172 (comment) ?

I believe this PR is equivalent to "byte-case-sensitive comparison of the value with the allowed strings".

Well, I think that's what we want long term. Perhaps instead we should add an open issue of sorts about changing that once SH is stable?

I've filed whatwg/html#5220

Do you think there should be an inline issue in the spec also?

Yeah, I meant an inline marker.

mikewest · 2020-01-31T12:59:53Z

index.bs

-    3.  If |parsed policy| is "`require-corp`", set |policy| to "`require-corp`".
+        ISSUE: This step is expected to change when the Structured Headers for HTTP specification is
+        more stable. See [whatwg/html issue #5172](https://github.com/whatwg/html/issues/5172).
+        [[I-D.ietf-httpbis-header-structure]]


I'm fine with landing this just to correct the current text, but I think we're at the point where the IETF WG considers SH pretty close to done. I'd be surprised if something like parameterized items changed substantially at this point.

(So, LGTM from my side. If @annevk's also happy, land it. :))

annevk · 2020-04-07T13:21:03Z

Did 31591be (#8) supersede this?

zcorpan · 2020-06-12T10:50:54Z

Looks like it, @annevk. I'll close this. cc @yutakahirano

mikewest suggested changes Dec 12, 2019

View reviewed changes

index.bs Outdated Show resolved Hide resolved

annevk reviewed Dec 13, 2019

View reviewed changes

index.bs Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

mikewest approved these changes Dec 13, 2019

View reviewed changes

mikewest mentioned this pull request Dec 13, 2019

"Get" a structured header from a header list. whatwg/fetch#983

Merged

jugglinmike mentioned this pull request Dec 19, 2019

Disambiguate header parsing #5

Closed

zcorpan mentioned this pull request Dec 19, 2019

[html] Add tests for parsing COEP values web-platform-tests/wpt#20852

Merged

jugglinmike mentioned this pull request Dec 19, 2019

[COOP] Header syntax whatwg/html#5172

Closed

zcorpan changed the title ~~Fix the URL for parsing structured headers~~ Fix spec integration: get a structured header Jan 9, 2020

Use 'get a structured header' term from Fetch

d14cdf6

See whatwg/fetch#983 Change duplicate requirements in the Framework section to non-normative note. Fail parsing if SH parameters are present, until SH is more stable.

zcorpan force-pushed the parsing-structured-header-xref branch from 7ecea11 to d14cdf6 Compare January 16, 2020 18:53

zcorpan changed the title ~~Fix spec integration: get a structured header~~ Use 'get a structured header' term from Fetch Jan 16, 2020

annevk reviewed Jan 17, 2020

View reviewed changes

zcorpan mentioned this pull request Jan 20, 2020

Parse and ignore Structured Header parameters in COEP and COOP whatwg/html#5220

Closed

Inline issue about handling of SH parameters

2677961

zcorpan mentioned this pull request Jan 23, 2020

Introduce "reporting endpoint" to COEP #8

Merged

mikewest approved these changes Jan 31, 2020

View reviewed changes

zcorpan closed this Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 'get a structured header' term from Fetch #2

Use 'get a structured header' term from Fetch #2

zcorpan commented Dec 12, 2019 •

edited

Loading

zcorpan commented Dec 12, 2019

mikewest left a comment

zcorpan commented Dec 12, 2019 •

edited

Loading

mikewest commented Dec 13, 2019

annevk left a comment

mikewest Dec 13, 2019

mikewest Dec 13, 2019

mikewest Dec 13, 2019

annevk Dec 13, 2019 •

edited

Loading

mikewest Dec 13, 2019

mnot Jan 6, 2020

mnot commented Dec 13, 2019

zcorpan commented Dec 19, 2019

mikewest commented Dec 19, 2019

domenic commented Dec 19, 2019

mikewest commented Jan 9, 2020

zcorpan commented Jan 9, 2020

zcorpan commented Jan 16, 2020

annevk Jan 17, 2020

zcorpan Jan 18, 2020

annevk Jan 19, 2020 •

edited

Loading

zcorpan Jan 20, 2020

annevk Jan 20, 2020

zcorpan Jan 20, 2020

mikewest Jan 31, 2020

mikewest Jan 31, 2020

annevk commented Apr 7, 2020

zcorpan commented Jun 12, 2020

	If \|parsed policy\| is not a token, fail parsing.
	If \|parsed policy\| is not a [=structured header/token=], fail parsing.

Use 'get a structured header' term from Fetch #2

Use 'get a structured header' term from Fetch #2

Conversation

zcorpan commented Dec 12, 2019 • edited Loading

zcorpan commented Dec 12, 2019

mikewest left a comment

Choose a reason for hiding this comment

zcorpan commented Dec 12, 2019 • edited Loading

mikewest commented Dec 13, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnot commented Dec 13, 2019

zcorpan commented Dec 19, 2019

mikewest commented Dec 19, 2019

domenic commented Dec 19, 2019

mikewest commented Jan 9, 2020

zcorpan commented Jan 9, 2020

zcorpan commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 7, 2020

zcorpan commented Jun 12, 2020

zcorpan commented Dec 12, 2019 •

edited

Loading

zcorpan commented Dec 12, 2019 •

edited

Loading

annevk Dec 13, 2019 •

edited

Loading

annevk Jan 19, 2020 •

edited

Loading