fix: Hardcoded CSP Nonce Tags in ResponseTrait #9937
Conversation
michalsn
added
the
bug
paulbalandan
left a comment
paulbalandan
left a comment
There was a problem hiding this comment.
The tests don't make sense. You should be testing instead the behavior when the response is sent when CSP is not enabled.
Co-authored-by: John Paul E. Balandan, CPA <paulbalandan@gmail.com>
patel-vansh
force-pushed
the
fix/hardcoded-csp-tags
branch
from
97b1297 to
65b1a04
Compare
|
@patel-vansh Please rebase to resolve conflicts. |
|
@michalsn Done. |
| } else { | ||
| $this->body = str_replace(['{csp-style-nonce}', '{csp-script-nonce}'], '', $this->body ?? ''); | ||
| } | ||
| $this->CSP->finalize($this); |
There was a problem hiding this comment.
With this change, if CSP is disabled and auto-nonce is also disabled, then replacement with '' would not happen. Can you add a test where $CSPEnabled is false and $autoNonce is also false where the response body will still be correctly replaced with empty strings on the tags?
There was a problem hiding this comment.
If $autoNonce is disabled, then wouldn't be expected behaviour that no replacing is done? I mean, if user sets $autoNonce to false, then they won't be adding {csp-style-nonce} in the first place isn't it?
There was a problem hiding this comment.
Or, I guess, you mean, if we won't do the replacement, then it will be a breaking change. Then, yeah. You're right. I am adding a else block in finalize function.
There was a problem hiding this comment.
But, now, two tests are failing.
There was a problem hiding this comment.
I can't figure it out why these two tests were not failing in older code.
There was a problem hiding this comment.
I think this can be handled all inside generateNonces method inside CSP class.
Typing on phone, but this is what I have in mind.
- finalize method - remove the if block, generateNonces method is called always
- generateNonces method - if autoNonce is false, just return early. then in the preg_replace callback function, do a check if csp is enabled to determine what replacement will be made on the tags
There was a problem hiding this comment.
If I do this, then the 2nd point you told earlier will have problem. We can do like, return early only if CSP is enabled and autoNonce is false. As, generateNonces will be called no matter any combination of CSP and autoNonce is there, so we have to add this AND case there.
There was a problem hiding this comment.
Any other approach coming to your mind?
There was a problem hiding this comment.
Thanks. I will just check this later when I have access to my laptop 👍
3 participants
Description
This PR fixes #9935.
Created one method in
system/HTTP/ContentSecurityPolicy.phpto clear all nonce placeholders.Checklist: