eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

False negative when target and rel attributes are dynamic

Open just-boris opened this issue 3 years ago • 5 comments

I have this code example

<a
    href={href}
    target={isExternal ? '_blank' : undefined}
    rel={isExternal ? 'noopener noreferrer' : undefined}
/>

Both target and rel attribute are dynamically defined, and always both present when needed. However, the rule reports it as an error:

Using target="_blank" without rel="noreferrer" is a security risk

Plugin version: 7.23.2

just-boris avatar Apr 09 '21 12:04 just-boris

It’s not possible to reliably handle dynamic cases like this statically. I agree this specific usage is safe.

Either use an override comment, or, conditionally render one of two anchor tags, both statically defined.

ljharb avatar Apr 09 '21 14:04 ljharb

Yeah, this is exactly what I did, added eslint-disable-next-line with a link to this issue.

I will keep this open and let you decide if you want to address this limitation anyhow. Perhaps, the rule could be more conservative and warn only when it is super confident there is an issue

just-boris avatar Apr 09 '21 15:04 just-boris

Conservative means whatever’s safest; warning here is safest. That said, false positives are harmful.

If this case can be handled, and it’s generalizable beyond this exact case, I’m happy to accept a PR.

ljharb avatar Apr 09 '21 15:04 ljharb

For now, target attribute can be assigned using ternary operator provided one of the consequent or alternate is assigned as _blank. A similar thing can be added for the rel attribute from which the value can be extracted from either of the consequent or alternate which contains Literal type of value and the value is equal to noreferrer or noopener noreferrer. What do you think @ljharb?

V2dha avatar Jul 11 '22 09:07 V2dha

I think that makes sense - any place we currently require a static string should also allow a ternary where both options are an acceptable static string.

ljharb avatar Jul 11 '22 16:07 ljharb