-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Fix: Gutenberg strips the rel='nofollow' in links when copying content #73872 #74288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @diffray-bot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an issue where Gutenberg was stripping custom rel attributes (like rel="nofollow") from links when copying and pasting content with target="_blank".
Key Changes:
- Modified the link processing logic to preserve existing
relattributes whentarget="_blank"is present - Only sets default
rel="noreferrer noopener"when therelattribute is missing or empty
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/blocks/src/api/raw-handling/phrasing-content-reducer.js
Outdated
Show resolved
Hide resolved
…s include noreferrer noopener when we have target as _blank
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/blocks/src/api/raw-handling/phrasing-content-reducer.js
Outdated
Show resolved
Hide resolved
…s per copilot feedbacks
Changes SummaryThis PR fixes issue #73872 by modifying the link attribute handling in raw HTML content. When a link has target="_blank", the code now preserves existing rel attributes (like 'nofollow') while ensuring 'noreferrer' and 'noopener' are always included for security. Previously, any existing rel value would be completely replaced with only 'noreferrer noopener'. Type: bugfix Components Affected: Raw HTML handling - Phrasing content reducer, Link (anchor) element processing Files Changed
Risk Areas: Security attribute handling: The logic ensures noreferrer/noopener are always present, maintaining security posture, Attribute deduplication: Uses Set to remove duplicates but filters noreferrer/noopener separately - ensure this handles all edge cases, HTML sanitization: This is part of raw HTML content handling which is security-critical in a content management system Suggestions
Full review in progress... | Powered by diffray |
| // In jsdom-jscore, 'node.target' can be null. | ||
| // TODO: Explore fixing this by patching jsdom-jscore. | ||
| if ( node.target && node.target.toLowerCase() === '_blank' ) { | ||
| node.rel = 'noreferrer noopener'; | ||
| const existingRels = ( node.rel || '' ) | ||
| .split( /\s+/ ) | ||
| .filter( Boolean ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 CRITICAL - Unsafe textDecoration.includes() - undefined dereference
Agent: bugs
Category: bug
Description:
The code calls .includes() on textDecoration without checking if it's undefined. When textDecoration is not set in the style, this throws TypeError.
Suggestion:
Add a null/undefined check before calling .includes(): if (textDecorationLine === 'line-through' || (textDecoration && textDecoration.includes('line-through')))
Why this matters: Null references cause runtime crashes and unexpected behavior.
Confidence: 95%
Rule: bug_null_pointer
Review ID: ced57fa9-b348-43a4-a5aa-efb8247c8512
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| it( 'should preserve existing rel attribute when target is _blank', () => { | ||
| const input = | ||
| '<a href="https://wordpress.org" target="_blank" rel="nofollow noopener">WordPress</a>'; | ||
| const output = | ||
| '<a href="https://wordpress.org" target="_blank" rel="noreferrer noopener nofollow">WordPress</a>'; | ||
| expect( | ||
| deepFilterHTML( input, [ phrasingContentReducer ], {} ) | ||
| ).toEqual( output ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM - Missing test: duplicate rel values deduplication
Agent: testing
Category: quality
Description:
The code uses Array.from(new Set()) to deduplicate rel values but no test verifies this behavior.
Suggestion:
Add test case with duplicate rel values like 'nofollow nofollow' to verify deduplication works correctly.
Why this matters: New parameters without tests are a regression risk and indicate incomplete PR.
Confidence: 70%
Rule: test_new_parameter_coverage
Review ID: ced57fa9-b348-43a4-a5aa-efb8247c8512
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 5 issues: 3 kept (1 critical bug, 2 test coverage), 2 filtered (low-value performance/style) Issues Found: 3💬 See 2 individual line comment(s) for details. 📊 2 unique issue type(s) across 3 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Unsafe textDecoration.includes() - undefined dereferenceAgent: bugs Category: bug Why this matters: Null references cause runtime crashes and unexpected behavior. File: Description: The code calls .includes() on textDecoration without checking if it's undefined. When textDecoration is not set in the style, this throws TypeError. Suggestion: Add a null/undefined check before calling .includes(): if (textDecorationLine === 'line-through' || (textDecoration && textDecoration.includes('line-through'))) Confidence: 95% Rule: 🟡 MEDIUM - Missing test: duplicate rel values deduplication (2 occurrences)Agent: testing Category: quality Why this matters: New parameters without tests are a regression risk and indicate incomplete PR. 📍 View all locations
Rule: ℹ️ 1 issue(s) outside PR diff (click to expand)
🟡 MEDIUM - Missing test: multiple non-standard rel valuesAgent: testing Category: quality Why this matters: New parameters without tests are a regression risk and indicate incomplete PR. File: Description: Test only covers single additional rel value ('nofollow'), doesn't test preserving multiple custom rel values. Suggestion: Add test with multiple rel values like 'nofollow external sponsored' to verify all are preserved correctly. Confidence: 65% Rule: Review ID: |
What?
Closes #73872
Why?
target="_blank", it should not strip the rel attributes.How?
relattribute, use that, else fallback to default rel value.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2025-12-31.at.3.52.56.PM.mov