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.

Sign up for GitHub

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

Jump to bottom

Validate content type header parameter #176

Merged
merged 2 commits into from
Oct 6, 2023
Merged

Validate content type header parameter #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1368b54
validate the header content type
qmuntal Sep 23, 2023
5508600
add test case
qmuntal Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion headers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"math/big"
"strings"

"github.com/fxamacker/cbor/v2"
)
Expand Down Expand Up @@ -471,9 +472,24 @@ func validateHeaderParameters(h map[any]any, protected bool) error {
return fmt.Errorf("header parameter: crit: %w", err)
}
case HeaderLabelContentType:
if !canTstr(value) && !canUint(value) {
is_tstr := canTstr(value)
if !is_tstr && !canUint(value) {
return errors.New("header parameter: content type: require tstr / uint type")
}
if is_tstr {
v := value.(string)
if len(v) == 0 {
return errors.New("header parameter: content type: require non-empty string")
}
if v[0] == ' ' || v[len(v)-1] == ' ' {
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
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
Contributor 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 +487 to +488
Copy link
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
Contributor 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
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
Contributor 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
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
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.

if strings.Count(v, "/") != 1 {
return errors.New("header parameter: content type: require text of form type/subtype")
}
}
case HeaderLabelKeyID:
if !canBstr(value) {
return errors.New("header parameter: kid: require bstr type")
Expand Down
35 changes: 35 additions & 0 deletions headers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,41 @@ func TestProtectedHeader_MarshalCBOR(t *testing.T) {
},
wantErr: "protected header: header parameter: Countersignature version 2: not allowed",
},
{
name: "content type empty",
h: ProtectedHeader{
HeaderLabelContentType: "",
},
wantErr: "protected header: header parameter: content type: require non-empty string",
},
{
name: "content type leading space",
h: ProtectedHeader{
HeaderLabelContentType: " a/b",
},
wantErr: "protected header: header parameter: content type: require no leading/trailing whitespace",
},
{
name: "content type trailing space",
h: ProtectedHeader{
HeaderLabelContentType: "a/b ",
},
wantErr: "protected header: header parameter: content type: require no leading/trailing whitespace",
},
{
name: "content type no slash",
h: ProtectedHeader{
HeaderLabelContentType: "ab",
},
wantErr: "protected header: header parameter: content type: require text of form type/subtype",
},
qmuntal marked this conversation as resolved.
Show resolved Hide resolved
{
name: "content type too many slashes",
h: ProtectedHeader{
HeaderLabelContentType: "a/b/c",
},
wantErr: "protected header: header parameter: content type: require text of form type/subtype",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down