-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix double utf8 encoding for payloads #81
Fix double utf8 encoding for payloads #81
Conversation
@@ -255,13 +261,6 @@ module.exports = function(parser) { | |||
}); | |||
}); | |||
|
|||
it('should err on invalid utf8', 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.
That case (string payload non utf8-encoded) should not happen.
Breaking change! |
@darrachequesne Also im wondering about this fragment of the source code. We are using on the server go's client instead of node's and as its more low-level programming language it tends to think more in terms of bytes than in characters. So basically when meeting a char like However im not sure how original node.js client works and what it expects as this header length - therefore where should this be fixed? here in the parser? or there in server's golang implementation? For now I get it work using heavy monkey-patching but would like to get rid of this asap const originalEncode = parser.encodePacket
parser.encodePacket = (packet, supportsBinary, utf8encode, callback) => originalEncode(
packet,
supportsBinary,
false,
msg => callback(Object.defineProperty(
{ toString: () => msg },
'length',
{ get: () => utf8.encode(msg).length }
))
) |
@darrachequesne also i aint sure how the protocol should be working exactly, especially in context of various transports combinations - but my monkey patch mentioned before was applied just to encoding function, decoding seems to work just fine, but this PR changes both Note: i think that the frame length should be counted in future releases of protocol in bytes than utf8 chars, it simply makes more sense for more backends. |
What updates are needed to enable socket.io 2.0 support for the cpp client? socketio/socket.io-client-cpp#173 |
This solves a minor, but nasty UTF-8 encoding-deconding problem, solved by socketio/engine.io-parser#81 and released since socket.io v2.0.0. Maybe the newer version would work too, but I'm too lazy to check right now.
I don't think that double utf8 encoding for payloads is a problem especially when browsers do not encode payloads as expected like socket servers do not respond with |
Following #80
Currently, a payload containing only strings (no binary) transmitted over
polling
transport gets utf8 encoded / decoded twice, one by the transport (the content type is set totext/plain; charset=UTF-8
) and one here (encodePacket
is called withutf8encode
to true).Fixes socketio/engine.io#315