-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
add size enforcement and tests #37
add size enforcement and tests #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sold on the additional tests, but sure.
png.go
Outdated
return fmt.Errorf("data too big: %d is bigger than %d", dataSize, v0MaxSize) | ||
} | ||
|
||
func ToPngNoMaxSize(w io.Writer, data []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the original ToPng()
check the size before generating the PNG and I needed a way to make a PNG that was too large in order to test that receiving party had the same size limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? The size check is before any png detection, right? Just send it too much data and check the error type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels really wrong to bake in functionality we don't want to have a test for if len(data) > V0MaxSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it to just send too much data. Though the test is not "perfect" due to the b64 encoding in various places that changes the size of the data. I don't think I could untangle that with out a bit of a refactor. Which leads me to wonder, do we still need the "boxer" logic at all since v2 relies only on the challenge logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I wonder how many v1 clients are left. Maybe we should rip it out soon
No description provided.