- Notifications
You must be signed in to change notification settings - Fork928
fix: ensure websocket close messages are truncated to 123 bytes#779
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMar 31, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #779 +/- ##==========================================- Coverage 64.18% 64.09% -0.10%========================================== Files 199 196 -3 Lines 11830 11657 -173 Branches 87 87 ==========================================- Hits 7593 7471 -122+ Misses 3419 3382 -37+ Partials 818 804 -14
Continue to review full report at Codecov.
|
ca133c8 to0835eccCompareThere 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.
Few minor nits. Good find Mr. Adler!
Uh oh!
There was an error while loading.Please reload this page.
coderd/ws.go Outdated
| builder := strings.Builder{} | ||
| builder.Grow(byteLen) | ||
| for _, char := range str { | ||
| if builder.Len()+len(string(char)) > byteLen { | ||
| break | ||
| } | ||
| _, _ = builder.WriteRune(char) | ||
| } | ||
| return builder.String() |
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.
Could we do[]byte(str)[:websocketCloseMaxLen] instead?
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.
You can't safely slice strings, because characters can be more than 1 byte. This could cause us to slice in the middle of a single character.
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.
Could we do[]rune(str)[:websocketCloseMaxLen] in that case?
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.
That would limit the rune count instead of the byte count
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 left a brainded comment
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.
You are correct sir
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 made it simpler tho and got rid of the string builder
Uh oh!
There was an error while loading.Please reload this page.
It's possible for websocket close messages to be too long, which causethem to silently fail without a proper close message. See error below:```2022-03-31 17:08:34.862 [INFO](stdlib)<close_notjs.go:72>"2022/03/31 17:08:34 websocket: failed to marshal close frame: reason string max is 123 but got \"insert provisioner daemon:Cannot encode []database.ProvisionerType into oid 19098 - []database.ProvisionerType must implement Encoder or be converted to a string\" with length 161"```
51f2225 toc3bf516CompareThere 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.
Approving for now, but I'd rather this doesn't become a top-level export in coderd. Not sure where to put it though :( I can noodle!
coderd/coderd.go Outdated
| // fmtWebsocketCloseMsg formats a websocket close message and ensures it is | ||
| // truncated to the maximum allowed length. | ||
| func FmtWebsocketCloseMsg(format string, vars ...any) string { |
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's unfortunate we'll have to export this to test it - hmph. Any ideas on where we could place this structurally? It's both a little unfortunate to export this fromcoderd, and to create a package just for this.
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.
Maybe we could do thecoderd_internal_test.go thing? I read about that after I wrote this!
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.
But yea I agree having this exported is weird
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 thinkhttpapi would be more appropriate.
httpapi.WebsocketCloseMessage seems clean 😎
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 like that! I'll fix it up tomorrow
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.
Woooonderful
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.
Minor nit: we could doWebsocketCloseSprintf instead, but chefs choice!
It's possible for websocket close messages to be too long, which cause
them to silently fail without a proper close message. See error below: