Skip to content

fix(overrides): refuse a save that would drop tokens, and keep the file path out of errors - #701

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/694-overrides-reject-and-hide-path
Sep 28, 2026
Merged

rubenvdlinde merged 1 commit into
developmentfrom
fix/694-overrides-reject-and-hide-path

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Saving token overrides answered 200 while dropping tokens, and a write failure returned the absolute file path.

The defect

POST /settings/overrides passed the input to CustomOverridesService::write(), which silently dropped unknown token names and values containing {, }, ; or comment markers. The answer was still 200 with written set to the size of the input, and the audit entry recorded the raw input as new. A write failure returned the exception text, which names the file path.

The test that was red

tests/Unit/Controller/OverridesControllerValidationTest.php drives the controller with the real CustomOverridesService writing into a temporary app directory. On development, testUnknownTokenIs400 and testWrongValueIs400 got 200, testNonStringValueIs400 hit a type error, and testWriteFailureHidesPath failed. testValidSaveReportsAndAuditsWhatWasWritten guards the happy path.

The fix

  • CustomOverridesService::findRejected() lists every token write() would not persist, with the reason: not in the registry, not a string, or a value that would break out of the :root block. The writer and the check share one isUnsafeValue().
  • setOverrides() refuses the whole save with 400 when anything is rejected, naming the tokens in error and rejected, and writes nothing. So written and the audit entry now describe what reached the file.
  • A write failure in save and in import answers a generic message instead of the exception text.

The editor already shows data.error when status is not ok, so the admin now sees which tokens were refused. One side effect to know about: a stale token left in custom-overrides.css from an older registry now makes a save through the token set switcher (which merges the stored overrides) answer 400 naming that token, where it used to be dropped without a word.

The typed value checks from authoring-token-value-types task 2.1 (a wrong colour or length) are not part of this change; this closes the silent drop and the path leak.

Verified

  • composer check:strict: exit 0 (its test:all skips in a bare clone, so PHPUnit ran separately).
  • PHPUnit with the server's lib/private on the autoloader: 855 tests, 11 errors and 7 failures, the same 18 names as on development 551954e (missing Symfony and server classes outside a Nextcloud tree).
  • npm run lint, format, test:l10n, check:l10n-js, check:manifest, test:unit: all exit 0.
  • Hydra gates --scope-to-diff: exit 0.

Fixes #694

…le path out of errors

A save with an unknown token name or a value the writer strips answered
200 with written set to the input size, and the audit entry listed tokens
that never reached the file. It now answers 400 naming those tokens and
writes nothing. A write failure answers a generic message instead of the
exception text, which carried the absolute file path.

Fixes #694
@rubenvdlinde
rubenvdlinde merged commit e7df182 into development Sep 28, 2026
32 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/thematiq @ ae4c05e

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ✅
composer ✅ ✅ 107/107
npm ✅ ✅ 2/2
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅
Newman ✅
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates ✅

Quality workflow — 2026-09-28 15:12 UTC

Download the full PDF report from the workflow artifacts.

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.

1 participant