TSX tags seem to be matched incorrectly and fixed incorrectly

[yoav-lavi]

Describe the bug
When matching TSX tags for a fix, Semgrep seems to match the element name and other non matching elements rather than the entire tag. This causes the fix to be implemented on the tag name -

<$TAG></$TAG>

matches

<div></div>

and

<div>{3}</div>

plus the suggested fix is

<<div />></div>

To Reproduce
https://semgrep.dev/s/N4Lz/

Expected behavior
TSX tags should be matched in their entirety and not replace just the element name

Screenshots

What is the priority of the bug to you?
P1

[yoav-lavi]

This may be related to the deep expression syntax as it uses < and > as well

[ievans]

thanks for the great bug report @yoav-lavi, autofix is still experimental (https://semgrep.dev/docs/experiments/overview/#autofix) but this is probably an easy fix and one we should definitely do!

[yoav-lavi]

@ievans the matching itself also seems to be wrong in this instance (see <div>{3}</div> which is being matched even though it does not fit the pattern)

[yoav-lavi]

It looks like it's matching div and div>{3 respectively which is an issue with the match itself rather than the fix (the autofix may actually be fine here based on what it matched)

[yoav-lavi]

Just to give the motivation for using this rule - the idea was to detect empty opening and closing tags to suggest / fix with self closing tags

<div></div>

to

<div />

[yoav-lavi]

@ievans Also notice in the image that the fix suggestion is constx and consty with no spacing between the keyword and the variable name, which is a separate issue

[aryx]

We have some builtin equivalences for JSX which might get in the way ...

[mschwager]

We have some builtin equivalences for JSX which might get in the way ...

Do you mean get in the way of the autofix or FP here?

Since autofix is experimental functionality I think we should focus on the FP. @aryx shouldn't the pattern require a ... to match the first example here? https://semgrep.dev/s/Av7g/

[aryx]

I can take a look

[aryx]

Right now in semgrep, the pattern <$EL /> will match lots of JSX elements: <foo />, but also <foo ></foo>, also <foo>xxx</foo>, etc.
and internally a pattern like <$EL></$EL> is actually transformed in <$EL />. This is maybe too much because it prevents users like @yoav-lavi to explicitely match code like <div></div> but not <div />.

@minusworld @mschwager @ievans maybe we should disable most of those equivalences. That means we will have to rewrite some of our semgrep rules like https://github.com/returntocorp/semgrep-rules/blob/develop/typescript/react/security/audit/react-css-injection.yaml to use instead of patterns like <$EL style={$STYLE} />
the more verbose <$EL style={$STYLE} >...</$EL>.
What do you think? We should even maybe require to use ... for the attributes to says more attributes are ok (to be consistent with what we do elsewhere @mschwager ?)
Then the only builtin equivalence we can still do is that <$EL ...>...<$EL> would still match code like <foo></foo> or <foo />.
Feedback?

[aryx]

@yoav-lavi the matching and autofix seems to work better on the latest: https://semgrep.dev/s/N4Lz/?version=develop
Now remains to fix how we want semgrep JSX pattern to match code and which equivalence to put by default (as I mentioned above).
Progress still!

[yoav-lavi]

Yes, it seems to work better in that version, thanks! By the way, the editor seems to treat JSX as an error for some reason @aryx

[aryx]

ok, closing this issue. For the equivalance thing I've created another issue: #2812