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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
#3510: mark content script nonce on DOM #3718
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3718 +/- ##
==========================================
- Coverage 45.99% 45.99% -0.01%
==========================================
Files 829 829
Lines 24741 24746 +5
Branches 5188 5189 +1
==========================================
+ Hits 11380 11381 +1
- Misses 12435 12439 +4
Partials 926 926
Continue to review full report at Codecov.
|
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.
Did you confirm that Symbol was the problem? It would still be surprising to me.
In that case, LGTM except a couple of notes
const PIXIEBRIX_SYMBOL = Symbol.for("pixiebrix-content-script"); | ||
const uuid = uuidv4(); | ||
// Should set attribute as early as possible | ||
document.documentElement.setAttribute(PIXIEBRIX_CONTENT_SCRIPT_NONCE, uuid); |
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.
Keep in mind that imports are executed before this code, so placing it up here doesn't make any difference.
In a future PR, I'd suggest to:
- Move all the code except the duplication checker out to contentScript2.js (example)
- Keep only the logic
if not yet loaded, import(contentScript2.js)
here
This guarantees that no code is ever executed twice. It also makes the check faster since the new contentScript.js would be very light.
@@ -39,9 +45,6 @@ import { initToaster } from "@/utils/notify"; | |||
import { isConnectionError } from "@/errors/errorHelpers"; | |||
import { showConnectionLost } from "@/contentScript/connection"; | |||
|
|||
const PIXIEBRIX_SYMBOL = Symbol.for("pixiebrix-content-script"); | |||
const uuid = uuidv4(); | |||
|
|||
registerMessenger(); | |||
registerExternalMessenger(); | |||
registerBuiltinBlocks(); |
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.
Should all of these also be moved inside init?
What does this PR do?
Checklist