Closed Bug 1413709 Opened 2 years ago Closed 2 years ago

add tests to detect improper ice restart from answer changing ufrag/pwd

Categories

(Core :: WebRTC: Signaling, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

Details

Attachments

(1 file)

In Bug 1405940 I fixed a crash that was ultimately the result of an answer (during renegotiation) containing modified ICE credentials causing an ICE restart when the offerer did not indicate an ICE restart.
Comment on attachment 8924333 [details]
Bug 1413709 - add tests to detect improper ice restart by answer.

https://reviewboard.mozilla.org/r/195572/#review201042

::: dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html:31
(Diff revision 1)
> +    test.chain.insertAfter("PC_LOCAL_GET_ANSWER",
> +      [
> +        function PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS(test) {
> +          test._remote_answer.sdp =
> +            test._remote_answer.sdp.replace(/a=ice-pwd:.*\r\n/g,
> +                                            "a=ice-pwd:bad-pwd\r\n")
> +                                   .replace(/a=ice-ufrag:.*\r\n/g,
> +                                            "a=ice-ufrag:bad-ufrag\r\n");
> +        }
> +      ], false, 1 // insert after the second PC_LOCAL_GET_ANSWER
> +    );
> +
> +    test.chain.replaceAfter("PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS",
> +      [
> +        function PC_LOCAL_EXPECT_SET_REMOTE_DESCRIPTION_FAIL(test) {
> +          return test.setRemoteDescription(test.pcLocal,
> +                                           test._remote_answer,
> +                                           STABLE)
> +           .then(() => ok(false, "setRemoteDescription must fail"),
> +                 e => is(e.name, "InvalidSessionDescriptionError",
> +                         "setRemoteDescription must fail and did"))
> +         }
> +      ], 0 // replace after first (only) PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS
> +    );

Couldn't we do this with a single replaceAfter?

::: dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html:51
(Diff revision 1)
> +          return test.setRemoteDescription(test.pcLocal,
> +                                           test._remote_answer,
> +                                           STABLE)
> +           .then(() => ok(false, "setRemoteDescription must fail"),
> +                 e => is(e.name, "InvalidSessionDescriptionError",
> +                         "setRemoteDescription must fail and did"))

Do we need a ; here?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:123
(Diff revision 1)
>    }
>  
>    void
>    AddTransportData(JsepSessionImpl& session, TransportData& tdata)
>    {
>      // Values here semi-borrowed from JSEP draft.

I think this comment needs to move to GenerateNewIceCredentials.
Attachment #8924333 - Flags: review?(docfaraday) → review+
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/6f3b7e0969c9
add tests to detect improper ice restart by answer. r=bwc
Assignee: nobody → mfroman
https://hg.mozilla.org/mozilla-central/rev/6f3b7e0969c9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.