fix: ensure websocket close messages are truncated to 123 bytes #779
Conversation
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 to
0835ecc
Compare
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.
Could we do []byte(str)[:websocketCloseMaxLen] instead?
There was a problem hiding this comment.
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.
Could we do []rune(str)[:websocketCloseMaxLen] in that case?
There was a problem hiding this comment.
That would limit the rune count instead of the byte count
There was a problem hiding this comment.
i made it simpler tho and got rid of the string builder
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: ``` 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 to
c3bf516
Compare
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.
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 from coderd, and to create a package just for this.
There was a problem hiding this comment.
Maybe we could do the coderd_internal_test.go thing? I read about that after I wrote this!
There was a problem hiding this comment.
But yea I agree having this exported is weird
There was a problem hiding this comment.
I think httpapi would be more appropriate.
httpapi.WebsocketCloseMessage seems clean 😎
There was a problem hiding this comment.
I like that! I'll fix it up tomorrow
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: