Skip to content

fix(portal): read portaliq's signing secret through IAppConfig - #1897

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/read-portal-secret-through-iappconfig
Oct 14, 2026
Merged

rubenvdlinde merged 1 commit into
developmentfrom
fix/read-portal-secret-through-iappconfig

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What broke live

learniq is not part of the Zuiddrecht demo, but it carries the same bug, so it gets the same fix.

In the Zuiddrecht demo test of 13 and 14 October 2026, every portal action that portaliq forwards to learniq answered 401.

Portaliq stores its jwt_signing_secret as a sensitive app value (setValueString(..., lazy: false, sensitive: true) in lib/Repair/InitializeSettings.php), so Nextcloud keeps it encrypted at rest. PortalAssertionVerifier::secret() (lib/Portal/PortalAssertionVerifier.php:220) read it with the legacy IConfig::getAppValue('portaliq', 'jwt_signing_secret'). In Nextcloud that path (AppConfig::getValue()) returns the stored value without decrypting it: a $AppConfigEncryption$... string of about 281 characters instead of the 32-character secret. Every HMAC check on X-Portal-Subject then failed.

The fix

Read the secret the way the minter does (PortalSessionService::__construct() in portaliq): IAppConfig::getValueString('portaliq', 'jwt_signing_secret', '', lazy: false). getValueString decrypts sensitive values. lazy: false matches how portaliq stores the key. opencatalogi already reads it this way and works.

In this app the verifier needed IConfig only for this read, so its constructor now takes ?IAppConfig $config instead (same parameter name, so existing named call sites keep working; DI autowires the new type). There was no instance-secret fallback here to keep, and none is added.

Why not a verifier service in portaliq: ADR-046 says no leaf app depends on portaliq (duck typing only), so a shared class would add the coupling the contract forbids. Reading the shared app value through the right API keeps the apps independent.

Verified

  • New regression test PortalAssertionVerifierTest::testTheSensitiveSecretIsReadDecrypted (a mock implementing both IConfig and IAppConfig, so the old code runs its legacy read and fails on the signature, not on a type error): the mocked IConfig::getAppValue() returns a ciphertext-looking value and the mocked IAppConfig::getValueString() returns the secret. It fails on the old code (Failed asserting that null is of type array) and passes with the fix.
  • COMPOSER_PROCESS_TIMEOUT=0 composer check:strict: exit 0, ALL CHECKS PASSED (lint, phpcs, phpmd, psalm, phpstan, 3,381 tests). No NEW findings, no INHERITED findings.
  • Not verified live: the coordinator verifies on the :8080 demo instance.

Follow-up, not in this PR

None for this app.

🤖 Generated with Claude Code

Portaliq stores jwt_signing_secret as a sensitive app value, so the
legacy IConfig::getAppValue() returns the encrypted value. The verifier
HMAC-checked every X-Portal-Subject assertion against that ciphertext,
and every portal action answered 401.

The verifier now depends on IAppConfig instead of IConfig and reads
getValueString('portaliq', 'jwt_signing_secret', '', lazy: false), which
decrypts it. Adds a regression test that fails on the old read path.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ ff6629d

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 110/110
npm ✅ ✅ 747/747
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ✅ 3376/3376 · 5 skipped
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 locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ❌

Quality workflow — 2026-10-14 10:17 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 966d41b into development Oct 14, 2026
46 of 48 checks passed
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