WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1
2012-08-23 05:43:55 PDT
Created
attachment 160139
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug