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

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

Conversation

jmechnich
Copy link

Summary

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

Checklist

…values

Signed-off-by: Jörg Mechnich <joerg.mechnich@gmail.com>
@susnux susnux requested review from nickvergessen and a team April 28, 2024 23:08
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

@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
Copy link
Member

Draft at #45382

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: occ config:list system does not redact trusted_domains and overwrite.cli.url
4 participants