fix: properly replace integrity tags in script resources when experimentalModifyObstructiveThirdPartyCode is true #23820
Conversation
|
Thanks for taking the time to open a PR!
|
AtofStryker
force-pushed
the
fix/stripped-integrity-dynamic
branch
from
7ab2cbd
to
8764bde
Compare
AtofStryker
added
the
experimental: modify third party code
AtofStryker
changed the title
AtofStryker
requested review from
chrisbreiding,
emilyrohrbough,
mjhenkes and
mschile
|
does not look to have an impact on recaptchas in regards to #23728 |
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -49,7 +62,7 @@ export function stripStream ({ modifyObstructiveThirdPartyCode }: Partial<Securi | |||
| jiraTopWindowGetterRe, | |||
| jiraTopWindowGetterUnMinifiedRe, | |||
| ...(modifyObstructiveThirdPartyCode ? [ | |||
| integrityTagReplacementRe, | |||
| buildIntegrityReplacementRe(isHtml), | |||
There was a problem hiding this comment.
can buildIntegrityReplacementRe & returnReplacedIntegrityExpression return the array ?
...(modifyObstructiveThirdPartyCode ? buildIntegrityReplacementRe(isHtml) : []There was a problem hiding this comment.
They cannot since they need to be used in the regular strip function above, which doesn't leverage arrays to replace. The good part is once this does become GA with modifyObstructiveCode, we can just merge this into the already flat array
| const integrityTagReplacementRe = new RegExp(`(${STRIPPED_INTEGRITY_TAG}|integrity)(=(?:\"|\')sha(?:256|384|512)-.*?(?:\"|\'))`, 'g') | ||
| const buildIntegrityReplacementRe = (isHtml = true) => { | ||
| if (!isHtml) { | ||
| // only replace integrity if a trailing period (.) or string exists inside JS/Other resources |
There was a problem hiding this comment.
Could you add more code comments here to help explain why there is a difference? My head already hurts just trying to parse these regexes.
There was a problem hiding this comment.
I can give it a shot. I am having issues thinking of something useful though so I'm going to try typing my thoughts out 😆 . The main goal here is only replace integrity in JavaScript if the word integrity or cypress-stripped-integrity is preceded by a . or quotes. So something like
foo.integrity = 'sha256-some-hash'
var integrity = 'sha256-some-hash'
becomes
foo['cypress-stripped-integrity'] = 'sha256-some-hash'
var integrity = 'sha256-some-hash'
It isn't so much what we match on as much as it is what we replace it with (the brackets vs the tagname). At some point in the future I am starting to think we might want to apply both of these because JS like above could be in HTML, but I am a bit hesitant to try it because it could have side effects we aren't testing for, and we haven't run into that problem yet. Now that I think about it I might want to go. Did any of that make sense or is it more of a jumbled ramble?
There was a problem hiding this comment.
I gave it ago and it's complicated with some added lookbacks 😄. I think we might want to hold off on that for now until it actually surfaces as a problem (if it even is one). Maybe we can meet up on Monday to see if we can improve the documentation?
There was a problem hiding this comment.
I actually got it to work! I tried adding some docs in 41e82c8 but I don't think they are really great. It also means the regex is more complicated, but handles more cases correctly without sacrificing performance. Regardless of resource type, both regex functions need to execute because HTML can have nested javascript inside of it, and both regexes need to run to rewrite correctly
AtofStryker
force-pushed
the
fix/stripped-integrity-dynamic
branch
2 times, most recently
from
18e7e3a
to
41e82c8
Compare
…en modifyObstructiveThirdPartyCode is enabled
…erimentalModifyObstructiveThirdPartyCode is true
integrity. Now accurately applies to other broad strokes
…vascript integrity tags in certain cases
AtofStryker
force-pushed
the
fix/stripped-integrity-dynamic
branch
from
7ca6061
to
700c75e
Compare
…o/cypress into fix/stripped-integrity-dynamic
…pped-integrity-dynamic
User facing changelog
Fixes an error with incorrect JS source rewriting when the
experimentalModifyObstructiveThirdPartyCodeflag is enabledAdditional details
When running their test suite with
experimentalModifyObstructiveThirdPartyCodeenabled, user was getting a left hand assignment syntax error, which was the result of a third party recaptcha script being rewritten to nullify script integrity, resulting in the below error:With added tests and changed to the regex-rewritter, this is now yeilding the following correctly:
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?