Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: genTextAreaValueMutation doesn't seem to mask attribute.value #1596

Closed
1 task done
sdemjanenko opened this issue Nov 8, 2024 · 2 comments
Closed
1 task done
Assignees
Labels
bug Something isn't working

Comments

@sdemjanenko
Copy link

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-snapshot

Version

2.0.0-alpha.17

Expected Behavior

I have a React app that controls a textarea by setting the value prop. RRWeb records the attribute change as well as the change event. The data looks like

Attribute change event:

{
    "adds": [],
    "attributes": [
        {
            "attributes": {
                "value": "(903) 993-7305"
            },
            "id": 121
        }
    ],
    "removes": [],
    "source": 0,
    "texts": []
} 

Input change event:

{
    "id": 121,
    "isChecked": false,
    "source": 5,
    "text": "**************"
}

I am expecting the attributes.value to be masked.

Actual Behavior

The attributes.value is unmasked. The issue appears to be in genTextAreaValueMutation, specifically the code

item.attributes.value = Array.from(
  dom.childNodes(textarea),
  (cn) => dom.textContent(cn) || '',
).join('');

There doesn't appear to be any masking logic that is applied to this value (or upstream of this function).

Steps to Reproduce

Create a controlled textarea in React.

Testcase Gist URL

No response

Additional Information

No response

@eoghanmurray
Copy link
Contributor

Could you verify that #1596 fixed things?
My tests only recreate the bug when setting the textarea value using innerText
So this will only fix things if that's what React does when you say 'setting the value prop' .

@sdemjanenko
Copy link
Author

This was coming up when I wrapped a textarea with https://www.npmjs.com/package/typed.js. In that package it has logic that looks like

replaceText(str) {
    if (this.attr) {
      this.el.setAttribute(this.attr, str);
    } else {
      if (this.isInput) {
        this.el.value = str;
      } else if (this.contentType === 'html') {
        this.el.innerHTML = str;
      } else {
        this.el.textContent = str;
      }
    }
  }

which matches your textContent comment.

gnpaone added a commit to Midpath-Software/rrweb that referenced this issue Feb 5, 2025
* Fix up the 'should replace the existing DOM nodes on iframe navigation with `isAttachIframe`' test (rrweb-io#1636)

- it was working for me when the test was run in isolation (`-t` option), but when the entire cross-origin-iframes test was run, the change of iframe contents didn't seem to happen in time

* [chore]: Update actions/upload-artifact to v4 (rrweb-io#1643)

* update actions/upload-artifact to v4

---------

Co-authored-by: Eoghan Murray <[email protected]>

* Fix a code path where masking could be skipped on textareas (rrweb-io#1599)

* Fixes rrweb-io#1596

* [chore] Cache yarn packages for CI (rrweb-io#1646)

* [chore] Cache yarn packages for CI

* Cache yarn in release.yml

* [chore] Update deprecated download artifact on CI (rrweb-io#1647)

* I'm merging even though ESLint is stlll failing in Github Actions as I believe it's running actions _without_ this PR applied yet

---------

Co-authored-by: Eoghan Murray <[email protected]>
Co-authored-by: Kevin Townsend <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>
gnpaone added a commit to Midpath-Software/rrweb that referenced this issue Feb 7, 2025
* Fix up the 'should replace the existing DOM nodes on iframe navigation with `isAttachIframe`' test (rrweb-io#1636)

- it was working for me when the test was run in isolation (`-t` option), but when the entire cross-origin-iframes test was run, the change of iframe contents didn't seem to happen in time

* [chore]: Update actions/upload-artifact to v4 (rrweb-io#1643)

* update actions/upload-artifact to v4

---------

Co-authored-by: Eoghan Murray <[email protected]>

* Fix a code path where masking could be skipped on textareas (rrweb-io#1599)

* Fixes rrweb-io#1596

* [chore] Cache yarn packages for CI (rrweb-io#1646)

* [chore] Cache yarn packages for CI

* Cache yarn in release.yml

* [chore] Update deprecated download artifact on CI (rrweb-io#1647)

* I'm merging even though ESLint is stlll failing in Github Actions as I believe it's running actions _without_ this PR applied yet

* Fix env puppeteer error in cross-origin-iframes.test.ts (rrweb-io#1629)

* chore(ci): track bundle size (rrweb-io#1630)

* chore(ci): track bundle size

---------

Co-authored-by: pauldambra <[email protected]>

* Fix adapt css with split (rrweb-io#1600)

Fix for rrweb-io#1575 where postcss was raising an exception

* adapt the entire CSS as a whole in one pass with postcss, rather than adapting each split part separately
* break up the postcss output again and assign to individual text nodes (kind of inverse of splitCssText at record side)
* impose an upper bound of 30 iterations on the substring searches to preempt possible pathological behavior
* add tests to demonstrate the scenario and prevent regression

More technical details:
* Fix algorithm; checks against `ix_end` within loop were incorrect when `ix_start` was bigger than zero.  
* Fix that length check against wrong array was causing 'should record style mutations with multiple child nodes and replay them correctly' test to fail. 
Note on last point: I haven't looked into things more deeply than that the test was complaining about missing .length after `replayer.pause(1000);`

* Warn instead of fail on exceptions thrown from postcss (rrweb-io#1580)

* postcss was introduced in rrweb-io#1458 for use within adaptCssForReplay
* rrweb-io#1600 fixes the main case where invalid css could be introduced when if valid css from the output of `sheet.cssRules` was split according to how it was split across text nodes of the <style>
* the guard introduced here is still useful as we likely in future will switch to capturing the raw stylesheet contents (both <style> and <link>), at which point we will be much less confident of getting valid css

* Fix splitCssText again (rrweb-io#1640)

Fixes a browser 'lock up' at record time due to a presence of large amounts of css in <style> elements, which are split over multiple text nodes, which triggers the new code added in rrweb-io#1437 (see that PR for full explanation of why this all exists).  rrweb-io#1437 was not written with performance in mind as it was believed to be an edge case, but things like Grammarly browser extension (rrweb-io#1603) among other scenarios were triggering pathological behavior, some of which was solved in rrweb-io#1615.
See also rrweb-io#1640 (comment) for further discussion.

* Fix the case when there are multiple matches and we end up not finding a unique one - just go with the best guess when there are many splits by looking at the previous chunk's size
* Also add '0px' -> '0' stylesheet normalization, which also fixes the sample problem in a different way
* Add new test and modify it so that it can trigger a failure in the absence of the '0px' normalization; there may be other unknown ways of triggering a similar bug, so ensure that the primary 'best guess' method doesn't suffer a regression
* Leverage the 'best guess' method so that we can quit after 100 iterations trying to find a unique substring; hopefully this bit along with the `iterLimit` already added will prevent any future pathological cases.

Failing example extracted from large files identified by Paul D'Ambra (Posthog) ... see comment from MartinWorkfully: PostHog/posthog-js#1668

* fix: move patch function into utils to improve bundling (rrweb-io#1631)

* fix: move patch function into utils to improve bundling

---------

Co-authored-by: pauldambra <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>

---------

Co-authored-by: Eoghan Murray <[email protected]>
Co-authored-by: Kevin Townsend <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>
Co-authored-by: Paul D'Ambra <[email protected]>
Co-authored-by: pauldambra <[email protected]>
Co-authored-by: John Henry Gunther <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants