Update references and clarify section 3.5 in the current version of the spec #112

baek9 · 2022-01-25T03:21:35Z

I am submitting a new pull request according to the comments(#84 (comment), #84 (comment)) from @annevk. It updates the CSP reference and modifies the 3.5 The integrity attribute to be clear in the current version of the spec where options are not defined.

Issue Number: #84

Preview | Diff

…he spec

annevk · 2022-01-25T07:54:07Z

Do we need to define hash-with-options as conforming at all? We need to deal with ? in the processing model, but I don't see why it needs to be listed here, if this is only about conformance requirements for web developers.

baek9 · 2022-01-25T15:59:32Z

Do we need to define hash-with-options as conforming at all? We need to deal with ? in the processing model, but I don't see why it needs to be listed here, if this is only about conformance requirements for web developers.

The purpose is to rule out hash-with-options, but at the same time suggest that future versions of the spec may extend it. So, as you said, it might seem like a conformance requirement for web developers.

So, going further than suggested, you're saying that dealing with options in the new 3.3.2 Parsed metadata is enough, so items like hash-with-options could be removed, right?

Then, even if hash-with-options is deleted, I think it is necessary to add an additional explanation that some of the contents of 3.5 Integrity metadata may change in the future.

annevk · 2022-01-25T16:05:51Z

Yeah, exactly. @mozfreddyb does that seem reasonable to you?

baek9 · 2022-01-26T01:49:55Z

index.bs

@@ -451,24 +451,21 @@ spec: SHA2; urlPrefix: http://csrc.nist.gov/publications/fips/fips180-4/fips-180
  valid metadata as described by the following ABNF grammar:

  <pre dfn-type="grammar" link-type="grammar">
-      <dfn>integrity-metadata</dfn> = *<a>WSP</a> <a>hash-with-options</a> *(1*<a>WSP</a> <a>hash-with-options</a> ) *<a>WSP</a> / *<a>WSP</a>


2.2 Grammatical Concepts defines WSP with reference to 2.4.1 Common parser idioms. The link is broken and redirects to https://html.spec.whatwg.org. On the other hand, Terms defined by reference defines WSP and VCHAR with reference to RFC5234:ABNF.

Therefore, in 2.2 Grammatical Concepts, it seems that WSP should be modified to refer to RFC5234:ABNF as well as VCHAR.

mozfreddyb · 2022-01-26T15:59:17Z

Yeah, exactly. @mozfreddyb does that seem reasonable to you?

Yeah, it's probably fine if the Parse metadata discards them..

baek9 · 2022-01-26T23:34:14Z

index.bs

-
-  <a>WSP</a> (white space) characters are defined in <a href="http://www.w3.org/TR/html5/infrastructure.html#space-character">Section 2.4.1 Common parser idioms</a> of the HTML 5 specification as
-  <code>White_Space characters</code>. [[!HTML5]]
+  [[!ABNF]] defines <a>VCHAR</a> (printing characters) and <a>WSP</a> (white space).


Please check.

So the previous text makes no sense, but I wonder if it was meant to match https://infra.spec.whatwg.org/#ascii-whitespace? That would be LWSP / %x0C.

This also seems like a problem with the parser. It references https://infra.spec.whatwg.org/#strictly-split but doesn't define spaces. I suspect it meant to reference https://infra.spec.whatwg.org/#split-on-ascii-whitespace?

Are there tests for this?

As you said, ASCII whitespace seems to be equivalent to LWSP. And the implementation, Chromium, use ASCIISpace defined as follows.

inline bool IsASCIISpace(CharType c) { return c <= ' ' && (c == ' ' || (c <= 0xD && c >= 0x9)); }

In this situation, as you said, I think it would be better to refer to split-on-ascii-whitespace rather than strictly-split for the parser. And it seems fitting to use LWSP, which refers to ASCII whitespace, instead of WSP. The ABNF in Section 3.5 will also need to be modified accordingly. Does it make sense?

So Chrome's definition also counts U+000B as whitespace if I'm reading that correctly. That seems wrong.

And note that LWSP excludes U+000C (which is why I added that above).

I think we should use ASCII whitespace here and file a bug on Chrome for them including U+000B. And use LWSP / %x0C in ABNF.

baek9 · 2022-01-26T23:35:24Z

index.bs

+  Note: Since no `options` are not defined (see the
+  [[#integrity-metadata-description]]), the above ABNF syntax does not consider
+  them. If `options` are defined in a future version, the ABNF syntax should be
+  modified accordingly.


Would it be ok this way? @annevk

Modulo nits, I think so.

annevk · 2022-02-10T14:07:29Z

index.bs

-
-  <a>WSP</a> (white space) characters are defined in <a href="http://www.w3.org/TR/html5/infrastructure.html#space-character">Section 2.4.1 Common parser idioms</a> of the HTML 5 specification as
-  <code>White_Space characters</code>. [[!HTML5]]
+  [[!ABNF]] defines <a>VCHAR</a> (printing characters) and <a>WSP</a> (white space).


So the previous text makes no sense, but I wonder if it was meant to match https://infra.spec.whatwg.org/#ascii-whitespace? That would be LWSP / %x0C.

This also seems like a problem with the parser. It references https://infra.spec.whatwg.org/#strictly-split but doesn't define spaces. I suspect it meant to reference https://infra.spec.whatwg.org/#split-on-ascii-whitespace?

Are there tests for this?

annevk · 2022-02-10T14:09:42Z

index.bs

-  will define a more specific syntax for options, so it is defined here as broadly
-  as possible.
+
+  Note: Since no `options` are not defined (see the


Suggested change

Note: Since no `options` are not defined (see the

Note: Since no `options` are defined (see the

annevk · 2022-02-10T14:09:50Z

index.bs

+
+  Note: Since no `options` are not defined (see the
+  [[#integrity-metadata-description]]), the above ABNF syntax does not consider
+  them. If `options` are defined in a future version, the ABNF syntax should be


Suggested change

them. If `options` are defined in a future version, the ABNF syntax should be

them. If `options` are defined in a future version, the ABNF syntax will be

annevk · 2022-02-10T14:09:57Z

index.bs

+  Note: Since no `options` are not defined (see the
+  [[#integrity-metadata-description]]), the above ABNF syntax does not consider
+  them. If `options` are defined in a future version, the ABNF syntax should be
+  modified accordingly.


Modulo nits, I think so.

domfarolino · 2024-11-28T00:03:23Z

@baek9 Just curious, do you still plan on working on this PR / #84 further?

Update references and clarify section 3.5 in the current version of t…

28ad1fb

…he spec

baek9 mentioned this pull request Jan 26, 2022

Edit 3.3.2 Parse metadata in more detail #110

Merged

baek9 commented Jan 26, 2022

View reviewed changes

baek9 added 2 commits January 27, 2022 08:16

Remove items related to options from the ABNF syntax

01a3f02

fix WSP reference

f9db377

baek9 commented Jan 26, 2022

View reviewed changes

annevk reviewed Feb 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update references and clarify section 3.5 in the current version of the spec #112

Update references and clarify section 3.5 in the current version of the spec #112

baek9 commented Jan 25, 2022 •

edited by pr-preview bot

Loading

annevk commented Jan 25, 2022

baek9 commented Jan 25, 2022 •

edited

Loading

annevk commented Jan 25, 2022

baek9 Jan 26, 2022 •

edited

Loading

mozfreddyb commented Jan 26, 2022

baek9 Jan 26, 2022

annevk Feb 10, 2022

baek9 Feb 11, 2022 •

edited

Loading

annevk Feb 11, 2022

baek9 Jan 26, 2022

annevk Feb 10, 2022

annevk Feb 10, 2022

annevk Feb 10, 2022

annevk Feb 10, 2022

annevk Feb 10, 2022

domfarolino commented Nov 28, 2024

	Note: Since no `options` are not defined (see the
	Note: Since no `options` are defined (see the

	them. If `options` are defined in a future version, the ABNF syntax should be
	them. If `options` are defined in a future version, the ABNF syntax will be

Update references and clarify section 3.5 in the current version of the spec #112

Are you sure you want to change the base?

Update references and clarify section 3.5 in the current version of the spec #112

Conversation

baek9 commented Jan 25, 2022 • edited by pr-preview bot Loading

annevk commented Jan 25, 2022

baek9 commented Jan 25, 2022 • edited Loading

annevk commented Jan 25, 2022

baek9 Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

mozfreddyb commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baek9 Feb 11, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domfarolino commented Nov 28, 2024

baek9 commented Jan 25, 2022 •

edited by pr-preview bot

Loading

baek9 commented Jan 25, 2022 •

edited

Loading

baek9 Jan 26, 2022 •

edited

Loading

baek9 Feb 11, 2022 •

edited

Loading