Skip to content

fix(config): do not echo secrets when setting config values [full-ci] - #41780

Merged
phil-davis merged 2 commits into
masterfrom
fix/41779-mask-secrets-in-config-set-output
Aug 19, 2026
Merged

fix(config): do not echo secrets when setting config values [full-ci]#41780
phil-davis merged 2 commits into
masterfrom
fix/41779-mask-secrets-in-config-set-output

Conversation

@DeepDiver1975

Copy link
Copy Markdown
Member

Description

config:system:set and config:app:set echoed the value they had just written
back to stdout, so every secret configured through occ ended up in the terminal
scrollback, the container log or the CI log of whoever ran the command.

Both commands now print ***REMOVED SENSITIVE VALUE*** instead of the value when
the config key holds a secret:

$ occ config:system:set wopi.token.key --value Pc7rTwpsnKfT3NfpvbChTPMxVfMr9X7t
System config value wopi.token.key set to string ***REMOVED SENSITIVE VALUE***

$ occ config:app:set onlyoffice jwt_secret --value VmNz3qhMWjsxnw4rMPpFxnM7C9hg9Xw
Config value jwt_secret for app onlyoffice set to ***REMOVED SENSITIVE VALUE***

Only the confirmation output changed — the stored value is written as before.

How a key is recognised as sensitive

The existing list of sensitive keys in OC\SystemConfig::$sensitiveValues stays
the single source of truth and is now reachable through a new
SystemConfig::isSensitiveKey(array $keys), which walks the very same nested
list that getFilteredValue() uses — including the repeating-0 convention, so
log.conditions 3 shared_secret is matched.

That list alone does not cover the two keys from the issue: wopi.token.key is
not on it, and app config is not covered by SystemConfig at all. A name-pattern
fallback in the command layer (credential, key, passwd, password, pwd,
salt, secret, token, matched case insensitively) therefore catches the keys
of apps, which core cannot know in advance.

IConfig::getFilteredSystemValue() was deliberately left untouched: it is public
API, and adding pattern matching there could hand apps a placeholder where they
expect a real value.

Two judgement calls worth a look

  • Booleans and null are never masked. They cannot hold a secret, and
    masking them broke useful feedback on real keys such as token_auth_enforced
    and grace_period.demo_key.show_popup. Integers and doubles are masked,
    because a numeric secret is possible.
  • Two cosmetic false positives remain: lost_password_link and
    grace_period.demo_key.link are string keys matching *password* / *key*,
    so their URLs now print masked. Left as is — a URL can embed a token, so this
    errs on the safe side.

Related Issue

Motivation and Context

This is how WOPI signing keys and ONLYOFFICE JWT secrets leak out of Docker
deployments that configure them from a startup hook — see
owncloud-docker/base#545 for a real-world log. The confirmation line does not
need the value to be useful.

The only workaround so far was Symfony's global -q, which is easy to miss and
also hides the genuine feedback.

How Has This Been Tested?

  • test environment: PHP 8.3.33, local checkout. The local instance is at 10.16.3
    while master is 11.0.0, so the PHPUnit bootstrap aborts with "Upgrade is
    required" — the unit and acceptance suites were not run locally and are left
    to CI.
    Verification was done by driving the real command classes through a
    real Symfony console with stubbed config backends:
  • test case 1: both scenarios from the issue produce the masked line, and the
    stored values are still verbatim (Pc7rTwpsnKfT3NfpvbChTPMxVfMr9X7t /
    VmNz3qhMWjsxnw4rMPpFxnM7C9hg9Xw).
  • test case 2: all 24 rows of the two new data providers assert both that the
    secret is absent from the output and that the placeholder is present.
  • test case 3: 41 cases covering isSensitiveKey() (list keys, nested keys,
    parents, numeric string path segments as the console passes them) and the name
    patterns.
  • test case 4: the 19 pre-existing castValue() expectations are byte-identical,
    so the added third parameter is backwards compatible.
  • test case 5: the pre-existing acceptance expectations (con / conkey,
    empty string) are unchanged, so no existing scenario needed rewriting.
  • php-cs-fixer reports 0 of 20 touched files needing changes. phpstan could
    not be installed locally (composer cannot authenticate against github.com in
    this environment), so it is left to CI.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

🤖 Generated with Claude Code

config:system:set and config:app:set printed the value that had just been
written back to stdout, so every secret configured through occ ended up in the
terminal scrollback, the container log or the CI log of whoever ran the command.

Both commands now print a placeholder instead of the value when the config key
holds a secret. Recognition reuses the existing list of sensitive keys in
OC\SystemConfig through the new isSensitiveKey(), and falls back to matching the
key name against the patterns credential, key, passwd, password, pwd, salt,
secret and token. That fallback covers the keys of apps, which core does not
know, such as wopi.token.key or jwt_secret.

Boolean values keep being shown, as they cannot hold a secret. Only the
confirmation output changed, the stored value is written as before.

Fixes #41779

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975
DeepDiver1975 requested a review from a team as a code owner August 18, 2026 22:13
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975 DeepDiver1975 changed the title fix(config): do not echo secrets when setting config values fix(config): do not echo secrets when setting config values [full-ci] Aug 18, 2026
@DeepDiver1975 DeepDiver1975 reopened this Aug 18, 2026

@phil-davis phil-davis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@phil-davis
phil-davis merged commit 400c5b2 into master Aug 19, 2026
66 of 69 checks passed
@phil-davis
phil-davis deleted the fix/41779-mask-secrets-in-config-set-output branch August 19, 2026 02:00
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.

config:system:set and config:app:set echo the secret they just stored

2 participants