-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix settings boefje settings via system env vars #3766
Conversation
Checklist for QA:
What works:Looks good. Everything seems to work. What doesn't work:n/a Bug or feature?:n/a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. One suggestion, looks good otherwise
I have pushed a better implementation that fixed a bug in my original change, but should also be easier to understand. It also caches the boefje system env variables, because there is no need to loop over the environment variables everytime a boefje gets executed. There are also unit tests now. |
Quality Gate passedIssues Measures |
Changes
Fixes a bug where the BOEFJE prefix was not removed before checking if the environment variable is allowed, resulting in it not allowing any of the environment variables.
QA notes
Fix was already tested yesterday by Z-CERT.
Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.