Skip to content

Conversation

@nielsnuebel
Copy link
Contributor

This PR solves the problem of an undefined variable $cspValue copied from the method setCspHeader to compileAutomaticCspHeaderRules.

@nielsnuebel nielsnuebel requested a review from zero-24 as a code owner February 7, 2020 06:07
@nielsnuebel nielsnuebel changed the title fix change undefined $cspValue to $row in method compileAutomaticCspH… [4.0]fix change undefined $cspValue to $row in method compileAutomaticCspH… Feb 7, 2020
@jwaisner
Copy link
Member

jwaisner commented Feb 7, 2020

@nielsnuebel Please provide test instructions for your PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27835.

Copy link
Contributor

@zero-24 zero-24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm row is only valid here: https://github.com/joomla/joomla-cms/pull/27835/files#diff-0ea3df9101b205d7c283ccf8ef68d621R359-R381 everything outside this is not correct. I agree that the current is wrong to.


// Append the script hashes placeholder
if ($scriptHashesEnabled && strpos($cspValue->directive, 'script-src') === 0)
if ($scriptHashesEnabled && strpos($row->directive, 'script-src') === 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is supposed to be $cspHeaderValue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to adjust the following line for a better name. https://github.com/joomla/joomla-cms/pull/27835/files#diff-0ea3df9101b205d7c283ccf8ef68d621L359

@wilsonge no it is not. It runs to the following errors when I change it to $cspHeaderValue.
image

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 10, 2020
@richard67
Copy link
Member

richard67 commented Feb 11, 2020

@nielsnuebel Could you check @wilsonge 's comment above and implement the suggested change, i.e. change $row->directive to $cspHeaderValue->directive in lines 412 and 418? This should then also solve @zero-24 ' review comment.
P.S.: And provide some testing instructions please.

@nielsnuebel
Copy link
Contributor Author

testing instructions:

  1. go to backend -> system -> Content Security Policy -> Options -> enabled CSP, Mode = Detect
  2. go to site and CSP will be created
  3. go back to backend -> system -> Content Security Policy -> Options -> enabled CSP, Mode = Automatic
    image
  4. go to -> system -> Content Security Policy -> change an item status to published for Example
    image
  5. Set in Global Configuration error_reporting to development and now you will get some errors on frotend and one in the Backend depends [4.0][plugin][httpheaders] - array to string conversion #26505

@Quy
Copy link
Contributor

Quy commented Feb 13, 2020

Here is var_dump $cspHeaderCollection:

array(3) {
  ["default-src"]=>
  string(64) " 'unsafe-inline' 'unsafe-inline' 'unsafe-inline' 'unsafe-inline'"
  ["script-src"]=>
  string(0) ""
  ["style-src"]=>
  string(0) ""
}

and var_dump $cspHeaderkey:

string(11) "default-src"

Thus, one can assume $cspValue->directive should be $cspHeaderkey

@Quy Quy added the PR-4.0-dev label Feb 13, 2020

// Append the script hashes placeholder
if ($scriptHashesEnabled && strpos($cspValue->directive, 'script-src') === 0)
if ($scriptHashesEnabled && strpos($row->directive, 'script-src') === 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($scriptHashesEnabled && strpos($row->directive, 'script-src') === 0)
if ($scriptHashesEnabled && strpos($cspHeaderkey, 'script-src') === 0)


// Append the style hashes placeholder
if ($styleHashesEnabled && strpos($cspValue->directive, 'style-src') === 0)
if ($styleHashesEnabled && strpos($row->directive, 'style-src') === 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($styleHashesEnabled && strpos($row->directive, 'style-src') === 0)
if ($styleHashesEnabled && strpos($cspHeaderkey, 'style-src') === 0)

@Quy
Copy link
Contributor

Quy commented Mar 11, 2020

Closing in favor of #28318

@Quy Quy closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Test instructions missing Updates Requested Indicates that this pull request needs an update from the author and should not be tested.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants