[Bug]: Cross-origin iframe message listener messages can be duplicated

timabdulla commented 1 month ago

Preflight Checklist

[X] I have searched the issue tracker for a bug report that matches the one I want to file, without success.

What package is this bug report for?

rrweb

Version

2.0.0-alpha.17

Expected Behavior

The event listener for cross-origin iframe events should only ever be added once to prevent duplicate message-handling.

Actual Behavior

Every time attachIframe is called in the IframeManager, the event listener is added, without regard to whether or not it has already been added. This means that if attachIframe is called multiple times (e.g. if navigation occurs within the iframe, causing the load event to fire), multiple listeners will be installed.

This bug was introduced here: https://github.com/rrweb-io/rrweb/commit/5c27b763192bda9dd91806f95df7c1cd0ab083a6#diff-01af2d33dc0d57c42970a0ab06328c71ea48c696077c60d96023427f27410829R80

Steps to Reproduce

Create an HTML document that contains a same-origin iframe that itself contains a cross-origin iframe.
Cause navigation event to occur within the same-origin iframe while still having the iframe's document contain a cross-origin iframe.
Now, all messages from the cross-origin iframe will be handled twice, causing duplicate events.

Testcase Gist URL

No response

Additional Information

No response

chetan-187 commented 1 month ago

@timabdulla

I have tried to reproduce the issue with following steps.

Integrated https://app.example.com:3001/server-errors in https://app.example.com:3001/client-errors which will be same domain iframe
I have integrated https://docs.example.com:8080 in https://app.example.com:3001/server-errors, which will be cross origin iframe.

It looks like following screenshot. I am switching navigation of iframe same origin. I am not able to reproduce the issue. Can you explain, what I am doing wrong?

timabdulla commented 1 month ago

Hi @chetan-187,

This is my fault. I attempted to simplify the conditions under which the bug occurs, but I just made it harder to reproduce. Here are clearer steps:

Firstly, load the root page. I've given an example below [0].
Press record on the root page to initiate recording.
Then, press the button to create the nested iframe. The key here is that the iframe uses a srcdoc attribute that in turn contains a cross-origin iframe. The iframe itself has no src attribute. This will cause the if condition below to be true:

https://github.com/rrweb-io/rrweb/blob/53b83bb037f9cb30c93179548f436ed776f143ab/packages/rrweb-snapshot/src/snapshot.ts#L341-L353

The first call to listener (indirectly via the immediate setTimeout) will cause the cross-origin iframe message listener to be added in attachIframe; then when the srcdoc is processed, it will fire the load event, thus calling attachIframe again via listener.

What's important here is that the contentWindow of the iframe remains the same between the two calls to listener. This breaks the logic below:

https://github.com/rrweb-io/rrweb/blob/5c27b763192bda9dd91806f95df7c1cd0ab083a6/packages/rrweb/src/record/iframe-manager.ts#L78-L83

I have a fix I am using in testing that creates a map of known content windows to prevent the event handler from being installed more than once. That seems to work, but this is obviously a very complex project, so I thought I would seek the input of the maintainers and contributors to ensure there aren't unintended consequences to that fix.

[0] Minimal Reproduction Example. Note that you need to replace the cross-origin iframe's src with a real cross-origin iframe that also has rrweb.

  <head>
    <title>Reproduce bug</title>
  </head>
  <body>
    <button id="record-button">Record</button>
    <button onclick="createIframe()">Create Iframe</button>
    <div id="iframe-container"></div>
    <script type="module">
      import * as rrweb from 'https://cdn.jsdelivr.net/npm/rrweb@2.0.0-alpha.17/dist/rrweb.min.js';
      document.getElementById('record-button').onclick = () => {
        rrweb.record({
          emit(event) {
            console.log(event);
          },
          recordCrossOriginIframes: true,
        });
      }
    </script>
    <script>
      function createIframe() {
        const container = document.getElementById('iframe-container');
        const parentIframe = document.createElement('iframe');
        parentIframe.id = 'parent-iframe';
        parentIframe.srcdoc = "<iframe id='child-iframe' src='https://cross-origin.com/foo.html'></iframe>";
        container.appendChild(parentIframe);
      }
    </script>
  </body>
</html>

chetan-187 commented 1 month ago

hey @timabdulla, I can think of simple approach here, to remove the message listener before adding another.

Can be done by making iframe-manager a singleton class to main the instance of function or we can push the removeEventListener function to handlers here every other observer is pushed to stop the recording.

Will it work?

timabdulla commented 1 month ago

I believe that iframe-manager is already only instantiated once during a particular recording context. Removing it before adding it would work, though right now the handler is dynamically constructed, so you'd have to make it consistent.

El El sáb, 26 oct 2024 a las 10:55, Chetan Lohkare @.***> escribió:

hey @timabdulla https://github.com/timabdulla, I can think of simple approach here, to remove the message listener before adding another.

Can be done by making iframe-manager a singleton class to main the instance of function or we can push the removeEventListener function to handlers here every other observer is pushed to stop the recording.

Will it work?

— Reply to this email directly, view it on GitHub https://github.com/rrweb-io/rrweb/issues/1577#issuecomment-2439437763, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHWJR3BE55VTYRETNN5GKDZ5NKG3AVCNFSM6AAAAABPBXBN6CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZZGQZTONZWGM . You are receiving this because you were mentioned.Message ID: @.***>

chetan-187 commented 4 weeks ago

@timabdulla Is there any way, how can I make it consistent?

timabdulla commented 4 weeks ago

@chetan-187 I think it would be sufficient to simply turn handleMessage into a fat-arrow function. That way:

https://github.com/rrweb-io/rrweb/commit/5c27b763192bda9dd91806f95df7c1cd0ab083a6#diff-01af2d33dc0d57c42970a0ab06328c71ea48c696077c60d96023427f27410829R79-R83

could just become:

    if (this.recordCrossOriginIframes)
      iframeEl.contentWindow?.addEventListener(
        'message',
        this.handleMessage
      );

chetan-187 commented 4 weeks ago

But then, we are trying to access the context of the class in this handleMessage function. Won't it cause any problem? @timabdulla

rrweb-io / rrweb