Also fix a few review comments from an earlier patch.
Created attachment 160139 [details] Patch
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?
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.
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.
(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.
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.
(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.
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.
Closing this bug since it is likely the stringifier will be removed from the spec, at least for now.