Skip to content

fix(lint/noBlankTarget): still report when the href attribute is dynamic#9434

Merged
siketyan merged 2 commits intobiomejs:mainfrom
siketyan:fix/GH-9433
Mar 11, 2026
Merged

fix(lint/noBlankTarget): still report when the href attribute is dynamic#9434
siketyan merged 2 commits intobiomejs:mainfrom
siketyan:fix/GH-9433

Conversation

@siketyan
Copy link
Member

@siketyan siketyan commented Mar 10, 2026

Note

AI Assistance Disclosure: I used the Claude Agent to create a changeset. I didn't use any AI/LLMs for code changes.

Summary

Closes #9433

The rule has the allowedDomains option to ignore some URLs starts with the specified domain names. While this option requires the href attribute to be static, the rule still should report a diagnostic for dynamic values.

Test Plan

Added snapshot tests.

Docs

N/A

@siketyan siketyan requested review from a team March 10, 2026 14:30
@siketyan siketyan self-assigned this Mar 10, 2026
@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2026

🦋 Changeset detected

Latest commit: 035fb05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Mar 10, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

Walkthrough

This patch release updates the noBlankTarget linter rule in @biomejs/biome to properly handle dynamic href attributes. The implementation now guards the static value extraction with a conditional check, ensuring domain allowance checks only proceed when the href yields a static value and non-empty allow_domains exist. Test cases have been expanded to cover dynamic href patterns using optional chaining.

Suggested labels

A-Linter, L-JavaScript

Suggested reviewers

  • ematipico
  • dyc3
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing the noBlankTarget lint rule to report dynamic href attributes even with allowedDomains configured.
Description check ✅ Passed The description clearly relates to the changeset, explaining the fix for the noBlankTarget lint rule to handle dynamic href attributes properly.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/biome_js_analyze/tests/specs/security/noBlankTarget/invalid.jsx`:
- Line 13: Add a new invalid fixture that replicates this
anchor-with-target="_blank" case but runs with a config enabling allowDomains so
the regression is pinned; locate the existing invalid.jsx test (the <a
href={company?.website} target="_blank"></a> case) and duplicate it into a new
spec that loads a config object or meta enabling allowDomains (matching how
other tests set options) so the analyzer runs with allowDomains=true rather than
default options.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59c7554b-45c8-4c2c-8d77-0b89c28dbf27

📥 Commits

Reviewing files that changed from the base of the PR and between 795bad4 and 035fb05.

⛔ Files ignored due to path filters (2)
  • crates/biome_js_analyze/tests/specs/security/noBlankTarget/invalid.jsx.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/security/noBlankTarget/valid.jsx.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (4)
  • .changeset/lovely-clouds-change.md
  • crates/biome_js_analyze/src/lint/security/no_blank_target.rs
  • crates/biome_js_analyze/tests/specs/security/noBlankTarget/invalid.jsx
  • crates/biome_js_analyze/tests/specs/security/noBlankTarget/valid.jsx

<a target="_blank" href="//example.com/17" rel></a>
<a target="_blank" href={dynamicLink}></a>
<a target={'_blank'} href="//example.com/18"></a>
<a href={company?.website} target="_blank"></a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Please add an allowDomains-configured invalid fixture as well.

The bug being fixed only shows up once allowDomains is enabled, but this new case still runs under default options. A config-backed case here would pin the actual regression, not just its nearby cousin.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/biome_js_analyze/tests/specs/security/noBlankTarget/invalid.jsx` at
line 13, Add a new invalid fixture that replicates this
anchor-with-target="_blank" case but runs with a config enabling allowDomains so
the regression is pinned; locate the existing invalid.jsx test (the <a
href={company?.website} target="_blank"></a> case) and duplicate it into a new
spec that loads a config object or meta enabling allowDomains (matching how
other tests set options) so the analyzer runs with allowDomains=true rather than
default options.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2026

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 156 skipped benchmarks1


Comparing siketyan:fix/GH-9433 (035fb05) with main (795bad4)

Open in CodSpeed

Footnotes

  1. 156 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this rule been ported to HTML yet? If so we should verify that vue/svelte/etc have the same fix

@siketyan
Copy link
Member Author

@siketyan siketyan merged commit bf12092 into biomejs:main Mar 11, 2026
19 checks passed
@github-actions github-actions bot mentioned this pull request Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Linter Area: linter L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

💅 noBlankTarget not working as described in documentation

3 participants