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

[timabdulla]

Preflight Checklist

• 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: 5c27b76#diff-01af2d33dc0d57c42970a0ab06328c71ea48c696077c60d96023427f27410829R80

Steps to Reproduce

1. Create an HTML document that contains a same-origin iframe that itself contains a cross-origin iframe.
2. Cause navigation event to occur within the same-origin iframe while still having the iframe's document contain a cross-origin iframe.
3. 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]

@timabdulla

I have tried to reproduce the issue with following steps.

1. Integrated https://app.example.com:3001/server-errors in https://app.example.com:3001/client-errors which will be same domain iframe
2. 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]

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:

1. Firstly, load the root page. I've given an example below [0].
2. Press record on the root page to initiate recording.
3. 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:

rrweb/packages/rrweb-snapshot/src/snapshot.ts

Lines 341 to 353 in 53b83bb

	// check blank frame for Chrome
	const blankUrl = 'about:blank';
	if (
	win.location.href !== blankUrl ||
	iframeEl.src === blankUrl ||
	iframeEl.src === ''
	) {
	// iframe was already loaded, make sure we wait to trigger the listener
	// till _after_ the mutation that found this iframe has had time to process
	setTimeout(listener, 0);
	return iframeEl.addEventListener('load', listener); // keep listing for future loads
	}

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:

rrweb/packages/rrweb/src/record/iframe-manager.ts

Lines 78 to 83 in 5c27b76

	// Receive messages (events) coming from cross-origin iframes that are nested in this same-origin iframe.
	if (this.recordCrossOriginIframes)
	iframeEl.contentWindow?.addEventListener(
	'message',
	this.handleMessage.bind(this),
	);

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/[email protected]/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]

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]

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 <#1577 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHWJR3BE55VTYRETNN5GKDZ5NKG3AVCNFSM6AAAAABPBXBN6CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZZGQZTONZWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[chetan-187]

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

[timabdulla]

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

5c27b76#diff-01af2d33dc0d57c42970a0ab06328c71ea48c696077c60d96023427f27410829R79-R83

could just become:

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

[chetan-187]

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