Validate content type header parameter#176
Conversation
Codecov Report
@@ 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
📣 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). |
There was a problem hiding this comment.
Could you please clarify the comment on line 433
There was a problem hiding this comment.
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.
| // Basic check that the content type is of form type/subtype. | ||
| // We don't check the precise definition though (RFC 6838 Section 4.2). |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Thanks, @thomas-fossati for the dialog and non-blocking comment.
|
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>
RFC 9052 Section 3.1 impose this restrictions on the content type textual format:
We are currently not performing any of those validations. This PR fix that.