fix: maxBodySize can not short circuit on frameLengh #181
Conversation
In the case of a framed message that encrypts less than the frameLenght, maxBodySize was doing the wrong thing. Remove the check. Add tests vectors that cover the case.
There was a problem hiding this comment.
To check my understanding:
The check against maxBodySize cannot be done in parseHeaderStream because it is not until we get to the body that we can decide whether we have exceeded that limit or not. Because we have a check in VerifyStream we are still good, and removing this check in ParseHeaderStream now allows the case where the actual data encrypted is less than the maxBodySize, but the frameLength is greater than the maxBodySize.
| export interface ParseHeaderOptions { | ||
| maxBodySize?: number | ||
| } | ||
|
|
||
| export class ParseHeaderStream extends PortableTransformWithType { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not sure I'm following...
There was a problem hiding this comment.
Oh weird, for some reason I missed that change. Ignore me.
| @@ -83,4 +83,21 @@ describe('decrypt', () => { | |||
| { encoding: 'base64' } | |||
| )).to.rejectedWith(Error, 'Invalid Signature') | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
No unit tests for ParseHeaderStream?
There was a problem hiding this comment.
Specifically, no. But implicitly, the stream test in this file will exercise just about everything.
There was a problem hiding this comment.
Unless there is some reason why unit tests for ParseHeaderStream specifically would not be useful, we should add them.
Either in this PR, or (because the logic added in this PR seems to be sufficiently tested below) tracked in a new issue.
In the case of a framed message that encrypts less than the frameLenght,
maxBodySize was doing the wrong thing.
Remove the check.
Add tests vectors that cover the case.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: