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.

Sign up for GitHub

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

Jump to bottom

fix: make trusted_domains and overwrite.cli.url sensitive config values #45085

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: make trusted_domains and overwrite.cli.url sensitive config values #45085

wants to merge 1 commit into from

Conversation

jmechnich
Copy link

Summary

Add trusted_domains and overwrite.cli.url to the list of sensitive configuration parameters.

Checklist

@susnux susnux requested review from nickvergessen and a team April 28, 2024 23:08
nickvergessen
nickvergessen requested changes Apr 29, 2024
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I'm still against this. Especially if it will result in people asking for the uncensored report to find out about broken URLs.
The only way to convince me would be to add some sensitive check that shows whether the URLs are valid, overwrite.cli.url is within the array, contains the protocol and whether it contains a path

@jmechnich
Copy link
Author

jmechnich commented Apr 29, 2024 •
edited

@nickvergessen
I can see why, from a developer’s perspective, it would be ideal to have as much information available as possible e.g. when a user opens a new issue.
However, from a user’s perspective, given that there is already a large amount of automation implemented in this process, it appears incomplete to me that I’d still have to go over the configuration dump and change those fields that at least I do not necessarily want to make public (the “principle of least astonishment” comes to mind too but that might be subjective).
If you feel that misconfigurations of one of those fields are a common source of error, you are probably right that there should be a check implemented for it, either on configuration load (so without explicit user interaction) or triggered explicitly by one of the occ *check commands, for example.
Thanks for discussing this!

@nickvergessen
Copy link
Member

I'll add it on my todo for the next hackweek

@nickvergessen nickvergessen mentioned this pull request May 16, 2024
Open
7 tasks
@nickvergessen
Copy link
Member

Draft at #45382

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: occ config:list system does not redact trusted_domains and overwrite.cli.url
2 participants
@jmechnich @nickvergessen