Repository navigation
fix(auth): read a numeric iss with OpenRegister's string cast so the weak-secret guard covers it - #2622
Conversation
…weak-secret guard covers it Robert's re-review of #2154: refuseWeakHmacSecret() returned early when `iss` was not a string, while OpenRegister reads the claim with a string cast, so a token with `"iss": 12345` skipped the guard and was verified against the consumer named "12345", however short its secret. The guard now casts a scalar `iss` the same way and only skips a non-scalar, which OpenRegister refuses as unknown. Test: an integer iss against an HS256 consumer named "12345" with a short secret is refused. Minor from the same review: the boundary cases now pin the whole HMAC_MIN_SECRET_BYTES table — HS384 at 47 refused and 48 accepted, HS512 at 63 refused and 64 accepted, next to HS256 at 31 and 32. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Quality Report — ConductionNL/integriq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 143/143 | |||
| npm | ✅ | ✅ 647/647 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ 5258/5258 · 1 skipped (no test failed, the job failed for another reason — see its log) | ||||
| Newman | ✅ 132/132 | ||||
| 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 | ✅ | ||||
| Hydra gates (axe) | ✅ |
Quality workflow — 2026-10-08 14:59 UTC
Download the full PDF report from the workflow artifacts.
…f skipping it The guard returned early for every non-scalar iss, on the claim that OpenRegister refuses it as unknown. It does not: OpenRegister only checks empty($payload['iss']) and then casts with (string), so ["x"] becomes the consumer named "Array" with a warning that Nextcloud's error handler only logs (OR v2.1.36-beta AuthorizationService.php:261). Such a consumer with a short secret got past the guard. The guard now skips an empty iss only, which OpenRegister refuses itself, and throws for any other non-scalar. The unreachable '' check after the cast is gone. The comment and the throws tag say what the code does. testANonScalarIssuerIsRefused covers a list and an object iss against a short-secret consumer named "Array". Reverting the branch to return turns it red; making a numeric iss skip again turns testANumericIssuerIsGuardedToo red (both checked with phpunit on this commit). The test helpers' docblocks name the new $name and $issuer parameters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Persistence audit — pre-existing items (not blocking this PR)
Not applicable here: OAuth grants and refresh tokens, CI/CD, SAML/OIDC callbacks, cron and startup. 🤖 Generated with Claude Code |
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
(Submitted as a comment: GitHub does not accept an approval on your own pull request.)
The numeric-iss fix is correct: the guard now reads iss with the same (string) cast as OpenRegister's AuthorizationService.php:261, and the boundary table covers all three HS algorithms. The review found one blocker on the branch next to it, a non-scalar iss skipping the guard, and it is fixed on this PR in 541b86d.
✅ Fixed before posting (1)
- 🔴 A non-scalar
issskipped the guard while OpenRegister still authenticated it: fixed in 541b86d, which refuses any non-empty non-scalarissand addstestANonScalarIssuerIsRefused. Reverting the branch toreturnturns that test red (phpunit on 541b86d).
Mode: Standard, size S (2 files, 60 lines before the fix).
Gates: quality / Hydra Gates had not started on 541b86d when this was posted (the Code Quality run was queued), so this round carries no gate findings. Locally on 541b86d: OpenRegisterCredentialBridgeHmacSecretTest 5 tests OK, tests/Unit/Service/Consumer 23 tests OK, phpcs and phpstan clean on the bridge.
Persistence audit: 21 checks. The in-diff item is the finding above. Seven pre-existing items outside this PR's scope are listed in this comment, not blocking.
PR description: it still says a non-scalar iss "is skipped, which OpenRegister refuses as unknown"; that sentence predates 541b86d.
Jira context: WOO-589 (the beta releases for the app store). This guard is what protects OpenRegister 2.1.36-beta, which has no short-secret check of its own.
🤖 Generated with Claude Code
Why
Robert's third review of the beta promotion #2154 (verdict): one concern under Strict and one minor on the guard that #2618 added.
What
A numeric
issskips the weak-secret guard.refuseWeakHmacSecret()returned early whenisswas not a string, while OpenRegister reads the claim with a string cast (findIssuer(issuer: (string)$payload['iss'], …)), so"iss": 12345skipped the guard and was verified against the consumer named"12345", however short its secret. The guard now casts a scalarissthe same way. It skips only an emptyiss, which OpenRegister refuses itself ("No issuer mentioned"), and refuses any other non-scalar: OpenRegister only checksempty()and would cast["x"]to the consumer named"Array"(found in review, fixed in 541b86d). This matters because openregister#4488 is ondevelopmentonly; the bridge is the protection on the 2.1.36-beta in the store.Boundary cases only for HS256. The tests now pin the whole
HMAC_MIN_SECRET_BYTEStable: HS256 31 refused / 32 accepted, HS384 47 / 48, HS512 63 / 64.Verification
tests/Unit/Service/Consumer/OpenRegisterCredentialBridgeHmacSecretTest.php: newtestANumericIssuerIsGuardedToo(integerissagainst an HS256 consumer named"12345"with a short secret → refused, nobody acts as its user); the accepted-size test runs for all three algorithms. NewtestANonScalarIssuerIsRefused(a list and an objectissagainst a short-secret consumer named"Array"→ refused).Part of WOO-589.
🤖 Generated with Claude Code