[Bug]: AppConfig::setTypedValue() decrypts the existing value when comparing, so a sensitive key is unwritable after a `secret` change

[marekjagielski]

Summary

OC\AppConfig::setTypedValue() guards against a no-op write by comparing the new value against the
current one. For a VALUE_SENSITIVE key that comparison decrypts the stored value, so once a
sensitive config value becomes undecryptable (an instance secret that no longer matches the one
the value was written under), the key can no longer be read or overwritten — every attempt
throws RuntimeException: HMAC does not match.

This makes the failure permanent and unrecoverable through the public API. It also silently defeats
apps that explicitly try to recover from it — notifications is one, see below.

https://github.com/nextcloud/server/blob/master/lib/private/AppConfig.php#L860-L867

if ($this->hasKey($app, $key, $lazy)) {
    /**
     * no update if key is already known with set lazy status and value is
     * not different, unless sensitivity is switched from false to true.
     */
    if ($origValue === $this->getTypedValue($app, $key, $value, $lazy ?? true, $type)   // ← decrypts, throws
        && (!$sensitive || $this->isSensitive($app, $key, $lazy))) {
        return false;
    }
}

getTypedValue() reaches Crypto::decrypt() for a sensitive key, which throws when the HMAC does
not verify. The exception escapes setTypedValue(), so the write never happens.

Steps to reproduce

Minimal:

1. Store a sensitive app config value:

   $appConfig->setValueString('myapp', 'mykey', 'value-1', sensitive: true);

2. Change secret in config.php (or restore a database into an instance whose secret differs —
   the realistic path).
3. Try to overwrite it with a fresh value:

   $appConfig->setValueString('myapp', 'mykey', 'value-2', sensitive: true);

Expected: the write succeeds — the caller supplied a complete new value and never asked to read
the old one. (Or at worst, a typed exception the caller can handle.)

Actual: RuntimeException: HMAC does not match. from lib/private/Security/Crypto.php#171.
The only way out is to delete the row directly so hasKey() returns false and the insert branch is
taken.

Real-world manifestation

notifications already anticipates a mismatched instance secret and tries to self-heal —
WebPushClient::getVapid():

try {
    $publicKey  = $this->appConfig->getAppValueString('webpush_vapid_pubkey');
    $privateKey = $this->appConfig->getAppValueString('webpush_vapid_privkey');
} catch (\Throwable) {
    // Decryption failed (e.g. mismatched instance secret), regenerate keys
    $publicKey = '';
    $privateKey = '';
}
if ($publicKey === '' || $privateKey === '') {
    $vapid = VAPID::createVapidKeys();
    $this->appConfig->setAppValueString('webpush_vapid_pubkey', $vapid['publicKey']);
    $this->appConfig->setAppValueString('webpush_vapid_privkey', $vapid['privateKey'], sensitive: true);
}

The catch handles the failed read exactly as intended — but the recovery write then hits
the compare above and throws again, this time outside the try. So the intended self-heal can never
complete, and the constructor throws on every instantiation.

Why this is worse than a log line: nothing catches it further up, so
OC\Log\ErrorHandler::onException logs it and the PHP process dies. On our instance this killed the
nightly OCA\UpdateNotification\BackgroundJob\UpdateAvailableNotifications job for six and a half
weeks — it did not complete a single run in that window, and because each crash left the job row
reserved, it eventually stopped being scheduled at all. occ user:delete also exited non-zero
after having already removed the account, leaving the home directory behind. Nothing was visible
in the UI, and the log line names only Crypto.php#171 — never the app, key, or caller — so the
cause is very hard to find. (occ background-job:history --status=crashed was what located it.)

Suggested fix

Make the no-op comparison tolerant of an unreadable current value — the caller of setTypedValue()
supplied a complete new value and did not ask for the old one. Roughly:

try {
    $sameValue = ($origValue === $this->getTypedValue($app, $key, $value, $lazy ?? true, $type));
} catch (\Exception) {
    $sameValue = false;   // cannot read the stored value -> it is not equal -> fall through and write
}
if ($sameValue && (!$sensitive || $this->isSensitive($app, $key, $lazy))) {
    return false;
}

That turns an unrecoverable state into a self-healing one for every app that overwrites a sensitive
key, and costs only the skip-identical-write optimisation in the rare undecryptable case.

Happy to open a PR if the approach looks right.

Server configuration

Nextcloud version: 34.0.3 (bug confirmed present on master at the time of writing)
Notifications app: 7.0.0-dev.1
Database: PostgreSQL 18 (CloudNativePG)
PHP version: as shipped in the official nextcloud:34-apache image

Trigger in our case: the instance secret was replaced during a disaster-recovery rebuild, so a
restored database carried values encrypted under the previous key.