RESOLVED INVALID 94801
MediaStream API: Add toString to RTCIceCandidate and RTCSessionDescription
https://bugs.webkit.org/show_bug.cgi?id=94801
Summary MediaStream API: Add toString to RTCIceCandidate and RTCSessionDescription
Tommy Widenflycht
Reported 2012-08-23 05:36:20 PDT
Also fix a few review comments from an earlier patch.
Attachments
Patch (15.45 KB, patch)
2012-08-23 05:43 PDT, Tommy Widenflycht
no flags
Tommy Widenflycht
Comment 1 2012-08-23 05:43:55 PDT
Adam Barth
Comment 2 2012-08-23 10:19:12 PDT
Comment on attachment 160139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160139&action=review > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:93 > + String result = "({candidate:\"" + candidate() + "\",sdpMid:\"" + sdpMid() + "\",sdpMLineIndex:" + String::number(sdpMLineIndex()) + "})"; Is this meant to be JSON? (This isn't valid JSON.) Also sdpMid() can be an arbitrary string, right? That means it might include a " and break the format of the string. Perhaps we should use InspectorValue to create valid JSON?
Tommy Widenflycht
Comment 3 2012-08-24 01:38:58 PDT
Comment on attachment 160139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160139&action=review >> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:93 >> + String result = "({candidate:\"" + candidate() + "\",sdpMid:\"" + sdpMid() + "\",sdpMLineIndex:" + String::number(sdpMLineIndex()) + "})"; > > Is this meant to be JSON? (This isn't valid JSON.) Also sdpMid() can be an arbitrary string, right? That means it might include a " and break the format of the string. > > Perhaps we should use InspectorValue to create valid JSON? No Yes I know Yes For some reason this is exactly what the standardization committee wants.
Eric Rescorla
Comment 4 2012-08-24 06:25:54 PDT
Tommy, I suspect this is simply an error on the part of the specification writers, who for some reason seem to think it's a great idea to specify this stuff in terms of U+ code points rather than in terms of "JSON". Note that they also require "U+007D RIGTH CURLY BRACKET" It's probably worth raising this on the mailing list.
Tommy Widenflycht
Comment 5 2012-08-24 06:40:15 PDT
(In reply to comment #4) > Tommy, I suspect this is simply an error on the part of the specification writers, who for some reason seem to think it's a great idea to specify this stuff in terms of U+ code points rather than in terms of "JSON". Note that they also require "U+007D RIGTH CURLY BRACKET" > > It's probably worth raising this on the mailing list. As far as I know this is not supposed to be JSON but I will raise this again. And btw the code adds the right curly bracket.
Eric Rescorla
Comment 6 2012-08-24 06:45:53 PDT
Yes, my point was that you don't add a "rihgt curly bracket" which suggests that perhaps the spec may not be being as carefully edited as one might like. At a higher level, I don't recall the WG deciding explicitly that the inner string was not supposed to be JSON, and if it did, IMO that decision was wrong.
Tommy Widenflycht
Comment 7 2012-08-24 07:44:26 PDT
(In reply to comment #6) > Yes, my point was that you don't add a "rihgt curly bracket" which suggests that perhaps the spec may not be being as carefully edited as one might like. Ahh :) > At a higher level, I don't recall the WG deciding explicitly that the inner string was not supposed to be JSON, and if it did, IMO that decision was wrong. I am against producing JSON (since the objects should be transparent to how they are sent/received) but happily!? I am not part of the standardization committee. I have sent an email to the list about this.
Adam Barth
Comment 8 2012-08-24 08:03:44 PDT
It seems really silly to have something that is almost-but-not-quite JSON in a way that will XSS you if you try to parse it with eval.
Tommy Widenflycht
Comment 9 2012-08-27 01:37:40 PDT
Closing this bug since it is likely the stringifier will be removed from the spec, at least for now.
Note You need to log in before you can comment on or make changes to this bug.