Skip to content

Validate content type header parameter#176

Merged
SteveLasker merged 2 commits intomainfrom
ctyvalid
Oct 6, 2023
Merged

Validate content type header parameter#176
SteveLasker merged 2 commits intomainfrom
ctyvalid

Conversation

@qmuntal
Copy link
Copy Markdown
Member

@qmuntal qmuntal commented Sep 25, 2023

RFC 9052 Section 3.1 impose this restrictions on the content type textual format:

Text values follow the syntax of "/", where and are defined in Section 4.2 of [RFC6838]. Leading and trailing whitespace is not permitted.

We are currently not performing any of those validations. This PR fix that.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 25, 2023

Codecov Report

Merging #176 (5508600) into main (e7ac36d) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   91.99%   92.19%   +0.20%     
==========================================
  Files          12       12              
  Lines        1961     1973      +12     
==========================================
+ Hits         1804     1819      +15     
+ Misses        108      105       -3     
  Partials       49       49              
Files Coverage Δ
headers.go 93.64% <100.00%> (+0.94%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

return errors.New("header parameter: content type: require no leading/trailing whitespace")
}
// Basic check that the content type is of form type/subtype.
// We don't check the precise definition though (RFC 6838 Section 4.2).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please clarify the comment on line 433

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Testing that the content type conforms to RFC 6838 Section 4.2 is not trivial, and other COSE libraries are not enforcing that neither, i.e.: google/coset. That's why I'm just doing some basic validations.

Comment on lines +432 to +488
// Basic check that the content type is of form type/subtype.
// We don't check the precise definition though (RFC 6838 Section 4.2).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am OK with a quick and dirty sanity check, but it seems it wouldn't be prohibitively difficult to do the full sanitisation. 6838 restricted-name's ABNF is [a-zA-Z0-9][a-zA-Z0-9!#$&\-^_.+]{0,126} in regex terms, so if we check that v matches

^[a-zA-Z0-9][a-zA-Z0-9!#$&\-^_.+]{0,126}/[a-zA-Z0-9][a-zA-Z0-9!#$&\-^_.+]{0,126}$

we'd be done?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we'd be done?

Nop. It can also contain parameters and subparameters. As per RFC 6838 Section 4.3, parameter names must conform to restricted-name ABNF, but the parameter value format is not specified, leaving it to each registrant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, this bit of the spec is confusing:

"Text values follow the syntax of "<type-name>/<subtype-name>", where <type-name> and <subtype-name> are defined in Section 4.2 of [RFC6838].

as it seems to suggest cty would use a subset of the media type string...

But in any case, that's possibly even better from our perspective because we can use mime.ParseMediaType?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, this bit of the spec is confusing:

It is confusing, indeed.

But in any case, that's possibly even better from our perspective because we can use mime.ParseMediaType?

We could be being too restrictive by using that method, as MIME is not the only allowed content type format, and it might impose additional restrictions, as noted in RFC 6838 Section 4.2:

Additionally, various protocols, including but not limited to HTTP and MIME, MAY impose additional restrictions on the media types they can transport. (See [RFC2046] for additional information on the restrictions MIME imposes.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, this bit of the spec is confusing:

It is confusing, indeed.

😄

But in any case, that's possibly even better from our perspective because we can use mime.ParseMediaType?

We could be being too restrictive by using that method, as MIME is not the only allowed content type format,

note that mime.ParseMediaType also does HTTP (or so it says)

and it might impose additional restrictions, as noted in RFC 6838 Section 4.2:

Additionally, various protocols, including but not limited to HTTP and MIME, MAY impose additional restrictions on the media types they can transport. (See [RFC2046] for additional information on the restrictions MIME imposes.)

While theoretically true, MIME and HTTP really cover a lot of ground -- i.e., I'd be surprised if we hit a case that'd need special treatment.

Anyway, I should have made clearer that this is non-blocking 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @thomas-fossati for the dialog and non-blocking comment.

Copy link
Copy Markdown
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

@SteveLasker
Copy link
Copy Markdown
Contributor

Awaiting @qmuntal to resolve the merge conflict, then we can merge.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@SteveLasker SteveLasker merged commit 2c01a2e into main Oct 6, 2023
@qmuntal qmuntal deleted the ctyvalid branch October 6, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants