bug: Strip padding from key content #452
Conversation
| @@ -395,6 +395,9 @@ class EndpointHandler(AutoendpointHandler): | |||
| "encryption-key", "content-type", | |||
| "authorization"] | |||
| cors_response_headers = ["location", "www-authenticate"] | |||
| # Remove trailing padding characters from complex header items like | |||
| # Crypto-Key and Encryption | |||
| strip_pad = re.compile("=+(?=[,;]|$)") | |||
There was a problem hiding this comment.
I think we'll want to account for the quote ([,;"]), too, in case they pass a quoted string. I don't think we really need to check that it's a valid quoted string, though. It might provide more helpful errors, and avoid routing/storing a message that we know is invalid. Your call.
| @@ -395,6 +395,9 @@ class EndpointHandler(AutoendpointHandler): | |||
| "encryption-key", "content-type", | |||
| "authorization"] | |||
| cors_response_headers = ["location", "www-authenticate"] | |||
| # Remove trailing padding characters from complex header items like | |||
| # Crypto-Key and Encryption | |||
| strip_pad = re.compile('=+(?=[,;]|$)') | |||
There was a problem hiding this comment.
strip_padding? Shortening it there makes it sound... odd.
|
r+ pending minor nits. strip_pad is meh, but the other method of conditionally including the space should at least have a comment indicating why its doing such a list splice with a boolean eval back to a int. |
|
Thinking about this some more, I'm now wondering if we shouldn't do this. Aurora 48 and Nightly 49 already reject padding, and we haven't seen breakage yet. The spec is also new enough to where we can discourage input like this, and Google will follow suit (they suggested prohibiting padding). Finally, we don't validate headers besides But I could also see this being a common enough mistake. I'll leave the final judgement to you and @bbangert. 😸 |
|
I view the padding issue as a needless annoyance to the developer. They already have several significant hurdles to overcome (encryption, user data management, scaling) and then us rejecting their submission because they included characters that literally signify nothing would certainly put me into a foul mood. It would be like having my thesis rejected because the staple was facing the wrong way. I can understand where being more pedantic would make sense if we had elements that required strict formatting (A somewhat legitimate example is the VAPID JWT header, where extra padding existing in the header or claims absolutely impacts the generated signature) but that case is not true for Crypto-Key or other fields. Doing a simple, low cost action like ignoring extra padding makes lives slightly better. I'm ok with doing that. |
The client will soon reject any key content that includes padding. The server will need to watch and strip it. We should also return a warning message, however the likelihood of that warning being noticed is minimal. Closes #451
|
r+ |
ghost
mentioned this pull request
The client will soon reject any key content that includes padding.
The server will need to watch and strip it. We should also return a
warning message, however the likelihood of that warning being noticed
is minimal.
Closes #451
@bbangert, @kitcambridge r?
(Let the great debate ensue!)