Skip to content

Conversation

@Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Apr 16, 2025

Description (*)

Big Inspired by #4721, but extended with:

  • Seperate base config for frontend and backend in the Magento settings.
  • Also allowing Layout Updates to add more directives.
  • (Should I also add Events for this?)

Related Pull Requests

Fixed Issues (if relevant)

  • fixes OpenMage/magento-lts#<issue_number>

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Template : admin Relates to admin template Template : base Relates to base template Component: Adminhtml Relates to Mage_Adminhtml XML Layout labels Apr 16, 2025
@Hanmac Hanmac added new feature Template : admin Relates to admin template Template : base Relates to base template Component: Adminhtml Relates to Mage_Adminhtml XML Layout and removed Template : admin Relates to admin template Template : base Relates to base template Component: Adminhtml Relates to Mage_Adminhtml XML Layout labels Apr 16, 2025
@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 17, 2025

@Tomasz-Silpion would you like to review this?

@empiricompany
Copy link
Contributor

empiricompany commented Apr 18, 2025

I'm interested in this feature, but looking at the implementation, is there a specific reason why a block that doesn't return anything was used, instead of setting the headers during the HTTP response dispatch phase (e.g., in an event like http_response_send_before)?

@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 18, 2025

I'm interested in this feature, but looking at the implementation, is there a specific reason why a block that doesn't return anything was used, instead of setting the headers during the HTTP response dispatch phase (e.g., in an event like http_response_send_before)?

I want to update it with different Layout updates.
Like our shop has some design where I need to add additional values

@empiricompany
Copy link
Contributor

My doubt is why add a layout update if the feature is not at the view level? Even the module you mentioned does not work on the layout, there is a particular reason for this choice?

@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 19, 2025

why add a layout update if the feature is not at the view level

Because layout updates are an easy way to switch and add values to this


$this->setXml($config->getNode());

if (Mage::app()->useCache('config_api')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why cache this in config_api?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the cache type and the cache id
I think it makes sense to mark this under cache_type=config ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the cache behavior of csp.xml but it doesn’t seem to be working as expected.
Here are the steps I followed:

  1. Enabled only the CONFIG cache
  2. Loaded the frontend and confirmed that *.cloudflare.com appears in the CSP script-src header
  3. Added a new host *.test.com to csp.xml under script-src
  4. Reloaded the frontend, and the changes were immediately reflected in the header — but it should still show only *.cloudflare.com until the CONFIG cache is refreshed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, i need to debug it, i took the Cache thing from there: #4721

Copy link
Contributor

Choose a reason for hiding this comment

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

instead to create a new csp.xml we can allow modules to add directly in config.xml
like ignore_user_agents with an unique key?

@sreichel
Copy link
Contributor

@Hanmac can you allow to create PRs to your repo?

Suggestion ... sreichel@be203c1

@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 22, 2025

@Hanmac can you allow to create PRs to your repo?

Suggestion ... sreichel@be203c1

$this->items[$type][] = $data;

CS Fixer complained about this line

@sreichel
Copy link
Contributor

Um, all checks are green ... https://github.com/sreichel/magento-lts/actions/runs/14600225861

(i have only removed a space)

@sreichel
Copy link
Contributor

Dont want to ask for adding tests, but to review #4758.

I'll add tests for Mage_Csp asap.

@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 23, 2025

@sreichel what do you think? Should I add events to the Block too?

@sreichel
Copy link
Contributor

What events are you thinking about?

@Hanmac
Copy link
Contributor Author

Hanmac commented Apr 23, 2025

What events are you thinking about?

Event in Block that takes the $directives as ArrayObject, and the block too. (to access the Layout?)

So the User can define more Header depending on other values.
For example, different Captcha Module?

Edit: might need to find a good name for the Event

@sreichel
Copy link
Contributor

@sreichel what do you think?

Das "Danke" manchen ein Fremdwort ist ...

@empiricompany

This comment was marked as resolved.

@empiricompany

This comment was marked as resolved.

@sreichel

This comment was marked as resolved.

@empiricompany

This comment was marked as resolved.

@empiricompany

This comment was marked as resolved.

@empiricompany

This comment was marked as resolved.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 23, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sreichel sreichel added this to the 20.15.0 milestone May 7, 2025
@sreichel
Copy link
Contributor

Closed with #4776

@sreichel sreichel closed this Jun 19, 2025
@Hanmac Hanmac deleted the cspModule branch June 19, 2025 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Adminhtml Relates to Mage_Adminhtml new feature phpstan Template : admin Relates to admin template Template : base Relates to base template XML Layout

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants