This isn't quite right, unfortunately. We are using the privacy level of the target node here for all the descendant nodes, but that isn't how it should work; we should instead be using the privacy level of each individual node.

Ideally we'd handle it in exactly the same way that normal masking is handled: as part of rejectInvisibleOrMaskedElementsFilter. Indeed, you are doing that, because you updated shouldMaskNode() to return true for NodePrivacyLevel.MASK_UNLESS_ALLOWLISTED, and rejectInvisibleOrMaskedElementsFilter uses shouldMaskNode() to do its work.

That means that you should, in an ideal world, be able to revert all changes to getTextualContentWithTreeWalker(). You shouldn't need allowlistMaskEnabled since rejectInvisibleOrMaskedElementsFilter will skip MASK_UNLESS_ALLOWLISTED elements. (You'd also need to update the } else if (nodePrivacyLevel === NodePrivacyLevel.MASK) { check in getActionNameFromElement() to check for MASK_UNLESS_ALLOWLISTED as well.)

BUT: maintaining consistency here is kinda a bummer, because the current approach we use for NodePrivacyLevel.MASK is not great. By skipping all descendants of nodes with privacy level MASK, we make it impossible to apply MASK at a higher level and selectively override it. We'd be transferring that limitation to MASK_UNLESS_ALLOWLISTED if we maintain consistency. Not ideal considering we want people to adopt this feature, in most cases, by setting defaultPrivacyLevel, so we expect almost every node in the document to have privacy level MASK_UNLESS_ALLOWLISTED.

We're caught between a rock and a hard place here. Adopting the existing behavior of MASK is not good for the usability of this feature (and frankly not good for the usability of MASK), but it will be really confusing if MASK and MASK_UNLESS_ALLOWLISTED have different inheritance rules.

The best solution may be to allow overriding for both MASK and MASK_UNLESS_ALLOWLISTED when the tree walker is enabled, and gate the tree walker with a new value for enablePrivacyForActionName. We can then try to migrate everybody over to the new model in a future major. But we should discuss this; it's a bigger issue than what can be resolved in a PR review.

So, after some additional discussion, we concluded that we'll ship with a version that maintains consistency with the existing treatment of NodePrivacyLevel.MASK, and then solve the other issues as a separate task.

After some thought, I think that means that the handling of NodePrivacyLevel.MASK_UNLESS_ALLOWLISTED must be different than NodePrivacyLevel.MASK. That's because MASK_UNLESS_ALLOWLISTED just will not work right unless it's applied at the text node level.

Say you have this setup:

<div id="target" dd-privacy-level="mask-unless-allowlisted">
  <span id="a">a</span>
  <span id="b">b</span>
</div>

Say that both a and b are allowlisted strings.

What should shouldMaskNode() return for #target?

If we say "shouldMaskNode() should return true, since #target is MASK_UNLESS_ALLOWLISTED", then in the enablePrivacyForActionName: true model we will stop recursion there, and neither a or b will appear in the action name, even though they're both allowlisted.

If we say "shouldMaskNode() should return true if its text content is allowlisted", then the situation doesn't seem any better. #target not a text node, so it doesn't have its own text content. If we use its innerText, then we get ab, and ab is not allowlisted, so again we will stop recursion there and neither a nor b will appear in the action name.

The only reasonable answer is that shouldMaskNode() should return false for #target. This implies that we can't use the same strategy we use for today's version of MASK; there's no easy way for us to stop traversal when we see MASK_UNLESS_ALLOWLISTED, because we don't know whether it actually means "mask" or "don't mask" until we go deeper in the tree. (And the answer may be different for different descendant elements.) shouldMaskNode() can only reasonably return true based on MASK_UNLESS_ALLOWLISTED for text nodes.

Handling this would be easier if you switched to a recursive implementation, as @BenoitZugmeyer suggested, but we should be able to make it work with the tree walker as well. (Though with less ideal performance.) I'll add some comments.

In rejectInvisibleOrMaskedElementsfilter(), you should switch from getNodeSelfPrivacyLevel() to getNodePrivacyLevel(). This is part that's not ideal from a performance perspective. You can reduce the impact by providing a NodePrivacyLevelCache argument, so that at least we won't compute the answer over and over for the same nodes.

You will also need to update rejectInvisibleOrMaskedElementsFilter() to call getNodePrivacyLevel() even for text nodes. (Right now text nodes are always accepted, but now we need to reject them sometimes.)

So, after some additional discussion, we concluded that we'll ship with a version that maintains consistency with the existing treatment of NodePrivacyLevel.MASK, and then solve the other issues as a separate task.

After some thought, I think that means that the handling of NodePrivacyLevel.MASK_UNLESS_ALLOWLISTED must be different than NodePrivacyLevel.MASK. That's because MASK_UNLESS_ALLOWLISTED just will not work right unless it's applied at the text node level.

Say you have this setup:

<div id="target" dd-privacy-level="mask-unless-allowlisted">
  <span id="a">a</span>
  <span id="b">b</span>
</div>

Say that both a and b are allowlisted strings.

What should shouldMaskNode() return for #target?

If we say "shouldMaskNode() should return true, since #target is MASK_UNLESS_ALLOWLISTED", then in the enablePrivacyForActionName: true model we will stop recursion there, and neither a or b will appear in the action name, even though they're both allowlisted.

If we say "shouldMaskNode() should return true if its text content is allowlisted", then the situation doesn't seem any better. #target not a text node, so it doesn't have its own text content. If we use its innerText, then we get ab, and ab is not allowlisted, so again we will stop recursion there and neither a nor b will appear in the action name.

The only reasonable answer is that shouldMaskNode() should return false for #target. This implies that we can't use the same strategy we use for today's version of MASK; there's no easy way for us to stop traversal when we see MASK_UNLESS_ALLOWLISTED, because we don't know whether it actually means "mask" or "don't mask" until we go deeper in the tree. (And the answer may be different for different descendant elements.) shouldMaskNode() can only reasonably return true based on MASK_UNLESS_ALLOWLISTED for text nodes.

Handling this would be easier if you switched to a recursive implementation, as @BenoitZugmeyer suggested, but we should be able to make it work with the tree walker as well. (Though with less ideal performance.) I'll add some comments.

You can revert this change, since we'll rely on rejectInvisibleOrMaskedElementsFilter() to exclude MASK_UNLESS_ALLOWLISTED elements that need to be masked.

In rejectInvisibleOrMaskedElementsfilter(), you should switch from getNodeSelfPrivacyLevel() to getNodePrivacyLevel(). This is part that's not ideal from a performance perspective. You can reduce the impact by providing a NodePrivacyLevelCache argument, so that at least we won't compute the answer over and over for the same nodes.

You will also need to update rejectInvisibleOrMaskedElementsFilter() to call getNodePrivacyLevel() even for text nodes. (Right now text nodes are always accepted, but now we need to reject them sometimes.)

Instead of unconditionally returning true here, you should use an implementation like this:

case NodePrivacyLevel.MASK_UNLESS_ALLOWLISTED:
  return isTextNode() ? !isAllowlisted(node.textContent) : false

I don't think this works with Session Replay serializeAttributes which censors attribtutes depending on the element privacy level. So I moved back this logic inside rejectInvisibleOrMaskedElementsfilter() again.

Per discussion on slack, this is fixed by handling form elements like mask-user-input does (cr. @sethfowler-datadog 's solution)

This test no longer checks MASK_UNLESS_ALLOWLISTED (which is correct, since shouldMaskNode() doesn't necessarily return true for MASK_UNLESS_ALLOWLISTED now!) but the test name still includes MASK_UNLESS_ALLOWLISTED. So, you'll want to revert this change to the test name.

In addition, you should add some new tests in this describe() block that cover the different cases for MASK_UNLESS_ALLOWLISTED. Off the top of my head, you should check:

1. Text nodes which have allowlisted textContent.
2. Text nodes with whitespace-only textContent.
3. Text nodes which have non-allowlisted, non-whitespace textContent.
4. Non-text nodes (e.g. normal HTML elements).

The test name is return true if ... is not ..., so I assumed we should add MASK_UNLESS_ALLOWLISTED there?

Ah yes, you're absolutely right; the is not threw me off! Current test name LGTM in that case.

Does this fail if you don't include the textContent && part here? If so, don't we have a bug? isAllowlisted() should be treating all whitespace-only strings as allowlisted. So, I think this is all you need:

If the test fails without the textContent && part, it'd be good to understand why.

You marked this one as resolved but it seems like you didn't change the test... 😃

I'm realizing now that this test doesn't test the thing it says it's testing anyway. The test name suggests this test is a test about both attributes and text content, but the test expectations involve only text content. It seems to me that you have text content pretty well covered by the other tests, so maybe remove text content from the test name; let's make this test focus on attributes and ensure that we have test coverage for them.

So, first, we need the body of the test to be like this:

Then you just need to implement getAllAttributeValues(), following the pattern established by getAllTextContents().

Nice, having these thorough tests will give us a lot of peace of mind down the road. Thank you!

Nit: It's a bit tricky to tell from the diff, but it seems like none of the strategies use the nodePrivacyLevel parameter defined in this type anymore, so you can probably just delete this line. (But only if that's true!)

You'll also need to remove the nodePrivacyLevel parameter from getActionNameFromElementForStrategies().

So now this test checks attributes, but the test name says obfuscates text content not in allowlist...

...while this one checks text contents, but the test name says obfuscates attributes and non-allowlisted text content.

So the names are backwards. But the actual tests look good! Please swap the bodies of these two tests, and then I think we're good to go.