JS-2366 Implement rule 9834: no identical links - #7931
guillemsarda wants to merge 1 commit into
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| function normalizeDestination(href: string): string { | ||
| const hasScheme = /^[a-zA-Z][a-zA-Z\d+.-]*:/.test(href); | ||
| let url: URL; | ||
| try { | ||
| url = new URL(href, DUMMY_BASE); | ||
| } catch { | ||
| return href; | ||
| } | ||
| const scheme = hasScheme ? (url.protocol === 'https:' ? 'http:' : url.protocol) : ''; | ||
| const keepFragment = ROUTING_FRAGMENT_PATTERN.test(url.hash); | ||
| return `${scheme}${url.pathname}${url.search}${keepFragment ? url.hash : ''}`; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: normalizeDestination discards the host, conflating distinct URLs
normalizeDestination builds the comparison key from scheme + pathname + search + hash and never includes url.host, so https://example.com/about and https://evil.com/about both normalize to http:/about. Two anchors with the same text pointing at different domains are therefore silently treated as the same destination (false negative), and //cdn.example.com/about collapses to /about, colliding with a root-relative link. Conversely /about ("/about") and https://example.com/about ("http:/about") compare as different, so same-origin links written in mixed relative/absolute form are reported as a false positive. The fixture's http/https case on lines 12-13 passes only because the host is dropped, so it does not actually exercise scheme equivalence.
Keep the authority in the normalized key when the href supplies one:
function normalizeDestination(href: string): string {
const hasScheme = /^[a-zA-Z][a-zA-Z\d+.-]*:/.test(href);
const hasAuthority = hasScheme || href.startsWith('//');
let url: URL;
try {
url = new URL(href, DUMMY_BASE);
} catch {
return href;
}
const scheme = hasScheme ? (url.protocol === 'https:' ? 'http:' : url.protocol) : '';
const authority = hasAuthority && url.host ? `//${url.host}` : '';
const keepFragment = ROUTING_FRAGMENT_PATTERN.test(url.hash);
return `${scheme}${authority}${url.pathname}${url.search}${keepFragment ? url.hash : ''}`;
}
Was this helpful? React with 👍 / 👎
| function getStaticTextFromExpression(expression: estree.Expression): string | undefined { | ||
| if (expression.type === 'Literal') { | ||
| return typeof expression.value === 'string' ? expression.value : ''; | ||
| } | ||
| if (expression.type === 'TemplateLiteral' && expression.expressions.length === 0) { | ||
| return expression.quasis.map(quasi => quasi.value.cooked ?? '').join(''); | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Numeric JSX children are dropped from the accessible name
getStaticTextFromExpression returns '' for every non-string Literal, and computeChildContribution uses it for JSXExpressionContainer children. A numeric literal renders as visible text in JSX, so <a href="/step/1">Step {1}</a> and <a href="/step/2">Step {2}</a> both compute the accessible name "step", the hrefs differ, and the rule raises a false positive on two links whose real accessible names are "Step 1" and "Step 2". Only null, true and false render as nothing; numbers and bigints should be stringified (which is also the correct behaviour on the attribute-value path in getStaticText).
Stringify numeric literals; only null/boolean render as empty:
function getStaticTextFromExpression(expression: estree.Expression): string | undefined {
if (expression.type === 'Literal') {
// null and booleans render nothing in JSX; numbers render their string form.
return expression.value === null || typeof expression.value === 'boolean'
? ''
: String(expression.value);
}
if (expression.type === 'TemplateLiteral' && expression.expressions.length === 0) {
return expression.quasis.map(quasi => quasi.value.cooked ?? '').join('');
}
return undefined;
}
Was this helpful? React with 👍 / 👎
| /** | ||
| * Computes the anchor's accessible name following aria-label > text content | ||
| * (skipping aria-hidden subtrees, using alt on nested images) > title, as established by | ||
| * S6827's decorator. Returns null when the name cannot be resolved statically, or is empty. | ||
| */ | ||
| function computeAccessibleName( | ||
| element: TSESTree.JSXElement, | ||
| attributes: JsxAttributes, | ||
| context: Rule.RuleContext, | ||
| elementType: (node: TSESTree.JSXOpeningElement) => string, | ||
| ): string | null { | ||
| const ariaLabelAttribute = getProp(attributes, 'aria-label') as JSXAttribute | undefined; | ||
| if (ariaLabelAttribute) { | ||
| const staticValue = getStaticText(ariaLabelAttribute.value); | ||
| if (staticValue === undefined) { |
There was a problem hiding this comment.
💡 Edge Case: aria-labelledby is ignored when computing the accessible name
computeAccessibleName only consults aria-label, text content and title. Per the accessible-name computation aria-labelledby takes precedence over all of them, so <a href="/a" aria-labelledby="lbl1">Details</a> and <a href="/b" aria-labelledby="lbl2">Details</a> are reported as duplicate-text links even though their real accessible names come from different referenced elements. Since the referenced text is not statically resolvable, the conservative behaviour matching the rest of the rule is to bail out (return null) when aria-labelledby is present.
Exclude anchors whose name is delegated to aria-labelledby:
): string | null {
// aria-labelledby wins over every other naming mechanism, but the referenced
// text cannot be resolved statically, so the anchor's identity is unknown.
if (getProp(attributes, 'aria-labelledby')) {
return null;
}
const ariaLabelAttribute = getProp(attributes, 'aria-label') as JSXAttribute | undefined;
Was this helpful? React with 👍 / 👎
CI failed: Build failure during rspec refresh due to a missing S9384 rule metadata JSON file for the newly implemented rule.Overview1 build failure encountered across 1 analyzed log, caused by a missing rule definition file during the rule data synchronization step. FailuresMissing Rule Metadata File (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
1 participant
Part of
Summary by Gitar
S9384to detect anchor elements with identical accessible names pointing to different destinationsS9384covering various link scenarios and normalization rulesThis will update automatically on new commits.