Skip to content
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

feat: hide internal information in case of error #3

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

bench
Copy link
Contributor

@bench bench commented Mar 14, 2022

Currently, your plugin displays the error message returned by the go library when it fails to reach the modsecurity container.

This approach is interesting for debugging and it facilitates the setup of the plugin.

However, it also reveals information about the internal architecture that should remain hidden.

As an example, the message below reveals that I'm using kubernetes, that my modsecurity pod is deployed in my kube-system namespace, and that my CIDR looks like 172.20.X.X

image

By reading the source code of traefik and some other plugins, I see that it is more common to log the information and return only an HTTP error code without details.

So I propose a very simple modification here and I open the discussion.

Thanks in advance and thx for your time.

Signed-off-by: Benjamin Chenebault benjamin.chenebault@gmail.com

Signed-off-by: Benjamin Chenebault <benjamin.chenebault@keeneye.tech>
@acouvreur acouvreur self-requested a review March 15, 2022 10:49
@acouvreur
Copy link
Owner

Hi! Thanks for the suggestion!
I'll look into it, seems simple enough and reasonable enough to merge it :)

@acouvreur acouvreur merged commit 19cdb47 into acouvreur:main Mar 16, 2022
@github-actions
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@bench bench deleted the hide_internal_logic branch March 17, 2022 15:31
@bench
Copy link
Contributor Author

bench commented Mar 17, 2022

Hi Alexis,
Thank you very much for accepting this change.

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

Successfully merging this pull request may close these issues.

2 participants