Skip to content

[Security] add comments in firewall configuration of security.rst to make the fi… #19612

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

bizmate
Copy link
Contributor

@bizmate bizmate commented Feb 27, 2024

add comments in firewall configuration of security.rst to make the firewall precedence more explicit

@carsonbot carsonbot added this to the 5.4 milestone Feb 27, 2024
@carsonbot carsonbot changed the title add comments in firewall configuration of security.rst to make the fi… [Security] add comments in firewall configuration of security.rst to make the fi… Feb 29, 2024
@javiereguiluz javiereguiluz merged commit 03a7e5d into symfony:5.4 Feb 29, 2024
@javiereguiluz
Copy link
Member

Diego, thanks for this contribution. I agree that it's important to mention how the firewall order is crucial. However, the proposed text had some issues in my opinion. First, it said "add a new firewall here ..." that could be understood like some instruction to newcomers. But they will wonder, why should I add a new firewall here? Is it mandatory? Which firewall should I add? etc.

Second, the comment looked like it belonged to the dev firewall instead of the entire config example. So, when merging, I reworded your contribution as follows: 8c14b05 and 8e2d950

@bizmate
Copy link
Contributor Author

bizmate commented Feb 29, 2024

Diego, thanks for this contribution. I agree that it's important to mention how the firewall order is crucial. However, the proposed text had some issues in my opinion. First, it said "add a new firewall here ..." that could be understood like some instruction to newcomers. But they will wonder, why should I add a new firewall here? Is it mandatory? Which firewall should I add? etc.

Second, the comment looked like it belonged to the dev firewall instead of the entire config example. So, when merging, I reworded your contribution as follows: 8c14b05 and 8e2d950

LGTM

Thank you for merging it. Hopefully it will help and save others who dont know the ins and outs of this section some time when configuring firewalls :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants