-
Notifications
You must be signed in to change notification settings - Fork 843
Scan: Replace inline onclick with target="_blank" for CSP compliance
#46340
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
target="_blank" for CSP compliance
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
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 improves Content Security Policy (CSP) compliance in the Jetpack Scan module by replacing an inline onclick event handler with the native HTML target="_blank" attribute. The change removes the CSP violation while maintaining the same user experience of opening the Calypso scanner URL in a new tab when threats are detected.
Key Changes:
- Removed inline
onclick="window.open( this.href ); return false;"handler from the admin bar notice - Replaced with declarative
target="_blank"attribute for CSP-compliant behavior - Maintained consistent code formatting with proper alignment
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/plugins/jetpack/modules/scan/class-admin-bar-notice.php | Replaced inline onclick handler with target="_blank" in the admin bar node configuration |
| projects/plugins/jetpack/changelog/JETPACK-1146 | Added changelog entry documenting the CSP compliance bugfix |
Comments suppressed due to low confidence (1)
projects/plugins/jetpack/modules/scan/class-admin-bar-notice.php:179
- When using
target="_blank", it's a security best practice to also includerel="noopener noreferrer"to prevent the new page from accessingwindow.openerand to prevent referrer leakage. This pattern is consistently used throughout the Jetpack codebase for external links. Consider adding$node['meta']['rel'] = 'noopener noreferrer';alongside the target attribute.
$node['meta']['target'] = '_blank';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage SummaryCoverage changed in 1 file.
|
Fixes JETPACK-1146
Fixes #46304
Proposed changes:
onclick="window.open( this.href ); return false;"withtarget="_blank"in the Scan admin bar notice.Why this works:
The original inline
onclickhandler was used to open the external Calypso scanner URL in a new tab. However, inline event handlers violate Content Security Policy (CSP) because they requireunsafe-inlinein the script-src directive.The HTML
target="_blank"attribute achieves the same behavior natively, both result in: user clicks → link opens in new tab. The HTML approach is CSP-compliant and works without JavaScript.Other information:
Jetpack product discussion
N/A - Security/CSP compliance fix
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Deactivate the Protect plugin if active (it has its own admin bar notice and will prevent Scan's notice from showing)
Add this snippet to
tools/docker/mu-plugins/0-snippets.phpto simulate threat detection:Visit any wp-admin page
Look for the red shield icon with "Threat found" in the admin bar (top right)
Inspect the HTML element and verify:
target="_blank"is present on the linkonclickattribute existsClick the link and verify it opens in a new tab
Remove the test snippet after testing