[bbrands02]

Summary

• PHP's SAPI folds a request header into a $_SERVER key by uppercasing it and turning every - into _ (X-Corsa-Secret -> HTTP_X_CORSA_SECRET) -- a step that cannot be undone later. EndpointService::getHeaders() (and FlowToken's equivalent) strip the prefix and lowercase, so the resulting headers bag keys a hyphenated header under its underscored form.
• The configured authentication.header lookup only lowercased the configured name, never folding the hyphen to an underscore — so any hyphenated custom header (the normal HTTP convention: X-Api-Key, X-Corsa-Secret, ...) could never match its own value, and the endpoint 403'd unconditionally regardless of the caller supplying the correct secret.
• Found and fixed while wiring up a real inbound endpoint using a custom X-Corsa-Secret header for apikey auth; confirmed live end-to-end once fixed.

Test plan

• New EndpointServiceCustomHeaderAuthTest (3 tests: hyphenated header matches, missing header still 403s, default Authorization header unaffected)
• Full tests/Unit/Service/ suite green (3083 tests)

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/integriq @ 8db5ce5

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			✅	✅ 646/646
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 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-22 10:21 UTC

Download the full PDF report from the workflow artifacts.

[bbrands02]

All four points investigated. One (point 1) was a well-founded concern that turned out to already be handled correctly; point 3 applied as suggested. Pushed d922611 → 90ba6d1.

1. [Medium] Regression tests may be skipped — investigated directly, not skipped in the supported environment. Ran the class: all 3 tests execute and pass (`3 / 3 (100%)`, 0 skips). The skip guard in `setUp()` is real, but it never fires in the standard unit-test/CI environment: `tests/bootstrap.php:347-352` loads a stub for `OC\AppFramework\Http` unconditionally whenever there's no real Nextcloud install present (`$ncPresent === false`) — exactly the condition this test suite runs under. I confirmed this two ways: locally (3/3 pass) and directly in the CI PHPUnit job log for this PR, where the job never even reaches test execution (see point 4) — but the same stub-loading logic is shared by the whole suite, including `EndpointServiceNcSessionRuleTest` (same skip-guard pattern, same file), which does run its non-skipped tests in every CI run to date. No production or test changes made here — the dependency already comes from the existing bootstrap, not a gap that needs filling.

2. [Low] Header normalization consistency — verified across every path that reaches `processAuthenticationRule()`. Traced all 4 call sites of `processRules()`: the main request path and `resumeFromApproval()` build `headers` from `FlowToken`'s own `getHeaders()`; `checkConditions()`/`handleSourceRequest()` use `EndpointService::getHeaders()` directly — both independently confirmed to produce the identical lowercase+underscore convention (neither does anything repo-specific; it's just how $_SERVER keys are formed). The `replay()` path reads from a stored `requestSnapshot` rather than rebuilding headers live, but that snapshot (`ExecutionTraceService::extractStepInput()`) is just a faithful echo of what one of the two methods above already produced at the original request time — it's not a third, independently-sourced representation. No gaps found; no fallback lookups added.

3. [Low] Production comments — shortened as suggested, keeping the one fact that matters at the call site (the fold's own existence) and dropping the restated background. One correction to my own commit message, for accuracy: I said the full rationale "already lives in the method-level comment above `processAuthenticationRule()`" — it doesn't; there's no such docblock in the code itself, that context lives in this PR's description/commit history instead. Didn't want to let an inaccurate claim stand uncorrected.

4. CI verification:

• Formatting: pre-existing on `development`, same 3 unrelated frontend files as flagged on a sibling PR (`src/integration/leafReport.js`, `SyncedFromTab.vue`, `tests/vitest/leafReport.spec.js`) — this PR is PHP-only, touches none of them.
• REUSE: currently passing on the latest check run, contradicting the review's listing.
• PHPUnit / Newman: confirmed not related to the new authentication tests — the job log shows it never reaches test execution at all. Both fail at the exact same root cause already reported on a sibling PR today: a PHP fatal `Maximum call stack size ... Infinite recursion?` inside openregister's own dependency-injection container, aborting CI's "enable the openregister sibling app" step before any of this repo's tests run. This is a live bug on openregister's `development`, unrelated to this PR, already flagged separately.

Full suite: `EndpointServiceCustomHeaderAuthTest` + `EndpointServiceNcSessionRuleTest` 8/8 passing, 0 skipped. phpcs clean.

[github-actions]

Quality Report — ConductionNL/integriq @ f4f8261

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					❌
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					❌
Hydra gates (axe)					✅

---

Quality workflow — 2026-10-23 12:05 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

🟡 Concern — the test never checks the bag shape that the fix depends on

The fix is correct only because getHeaders() keys X-Corsa-Secret as x_corsa_secret. This test does not exercise that: it writes ['x_corsa_secret' => 's3cr3t'] by hand. I confirmed that test 1 goes red with the old strtolower(...) line restored, so it guards the lookup. It does not guard the coupling. If either getHeaders() (EndpointService.php:2320, FlowToken.php:132) were changed to emit hyphens, as Nextcloud's IRequest::getHeader() convention does, all three tests would stay green while every hyphenated custom header 403s in production again.

Suggested fix: add a test that goes through the real producer. Set $_SERVER['HTTP_X_CORSA_SECRET'] = 's3cr3t', build the bag with (new FlowToken(...))->setRequestOriginal(<IRequest mock>)['headers'] (or invoke EndpointService::getHeaders($_SERVER, true) through reflection), pass ['headers' => <that bag>] to processAuthenticationRule(), and expect authorizeApiKey('s3cr3t', ...). Unset the $_SERVER key in tearDown().

Done when:

• the new test goes red when getHeaders()'s key map is mutated to str_replace('_', '-', strtolower(substr($key, 5)))
• it also goes red with the pre-fix $lookupKey = strtolower(...) line

[WilcoLouwerse]

🔧 Proposed fix in #2730, commits 3a10438e2 and f9868cf50 — a new test builds the bag with getHeaders() from an HTTP_X_CORSA_SECRET server entry and goes red with the pre-fix lookup line or without the HTTP_ prefix strip. The thread stays open until it is merged.

[WilcoLouwerse]

🟢 Follow-up (pre-existing, outside this diff) — the same hyphen/underscore mismatch lives in two more lookups against this bag

(Anchored here because both sites are outside the diff.) This line fixes one lookup of a hyphenated name against the getHeaders() / FlowToken::getHeaders() bag, whose keys are always underscored (HTTP_X_API_KEY -> x_api_key). Two other lookups against the same bag still use the hyphenated form, so they never match:

1. lib/Service/ApprovalService.php:97 — STRIPPED_HEADERS = ['authorization', 'proxy-authorization', 'cookie', 'x-api-key'], compared at :978 with in_array(strtolower($headerName), ...) against requestOriginal / requestAmended headers, which come from FlowToken::setRequestOriginal() -> getHeaders($_SERVER). x_api_key and proxy_authorization therefore never match, and an X-Api-Key value is persisted unredacted in every approval snapshot. Because this PR makes custom-header apikey endpoints work, the configured custom header (for example x_corsa_secret) is now also persisted in clear on any endpoint that has an approval rule. (This is the approval-snapshot side. The execution-trace side is covered separately.)
2. lib/Service/EndpointService.php:2121-2124 — getRequestAmended()['headers']['Accept-Crs'] never matches accept_crs, so Content-Crs is never echoed on schema endpoints.

Impact: item 1 is a credential at rest in approval-request objects; item 2 is a silent functional no-op.

Suggested fix: in ApprovalService::stripSensitiveHeaders(), compare the folded name, in_array(str_replace('-', '_', strtolower((string)$headerName)), ['authorization', 'proxy_authorization', 'cookie', 'x_api_key'], true), and also strip the endpoint's configured authentication.header, folded the same way. In handleSchemaRequest(), read ['headers']['accept_crs']. Not a blocker for this PR: both sites predate it. I'm proposing a follow-up issue for them; it will be linked here.

Done when:

• a unit test on ApprovalService::stripSensitiveHeaders() with requestOriginal.headers = ['x_api_key' => 'k', 'proxy_authorization' => 'p'] asserts both are ***redacted***, and goes red with the current STRIPPED_HEADERS restored
• git grep -n "\['Accept-Crs'\]" lib/ returns nothing
• new branches covered by: the stripSensitiveHeaders() unit seam (private, reached through reflection as EndpointServiceCustomHeaderAuthTest does)

[WilcoLouwerse]

📋 Tracked in #2728 (pre-existing; not a blocker for this PR).

[WilcoLouwerse]

🟢 Minor — fold the bag keys as well, so the lookup no longer depends on who built the bag

$normalisedHeaders (line 3237, outside the hunk) only lowercases. The lookup key now always has underscores, so any producer that keys a header with hyphens now fails closed (403) where it used to match: the after-phase $data['headers'] (Nextcloud Response::getHeaders(), line 756), or a mapping rule that rewrites headers / _headers before the authentication rule (including on resumeFromApproval() via updateRequestWithRuleData(), line 4319). None of these is a realistic authentication source today, so this is hardening, not a regression. The SAPI producers already collapse - and _ into one key, so folding the bag adds no new collisions.

Suggested: line 3237 becomes $normalisedHeaders[str_replace('-', '_', strtolower((string)$key))] = $value;, with a test that passes ['headers' => ['X-Corsa-Secret' => 's3cr3t']].

[WilcoLouwerse]

🔧 Proposed fix in #2730, commit 4f436304f — the bag keys are folded the same way as the configured name, with a test sending X-Corsa-Secret as a bag key that goes red without the fold. The thread stays open until it is merged.

[WilcoLouwerse]

🟢 Minor — the skip guard never fires, and its message is wrong

tests/bootstrap.php:350-352 loads tests/stubs/OC/AppFramework/Http.php whenever Nextcloud is absent, so OC\AppFramework\Http always exists. The three tests ran (not skipped) under the unit bootstrap. "which the unit bootstrap does not stub" is false, and the guard is dead. Live copies of the sentence: tests/Unit/Service/EndpointServiceCustomHeaderAuthTest.php:54 (this PR) and tests/Unit/Service/EndpointServiceNcSessionRuleTest.php:99 (pre-existing, where it was copied from). Suggest dropping the guard here, or rewording it to "Requires OC\AppFramework\Http (stubbed by tests/bootstrap.php when Nextcloud is absent)."

[WilcoLouwerse]

🔧 Proposed fix in #2730, commit 82a000ac2 — the skip message (and the docblock that repeated it) now says the stub is loaded by tests/bootstrap.php, in this test and in EndpointServiceNcSessionRuleTest.php. The thread stays open until it is merged.

[WilcoLouwerse]

🟢 Follow-up (persistence audit, pre-existing) — a custom auth header can land unredacted in execution_trace

(Anchored here because the code is outside the diff.) Trace input is redacted by SensitiveFieldRegistry::redactArray() (EndpointService.php:2941), which matches key names only (SensitiveFieldRegistry.php:47-83). x_corsa_secret is caught because it contains secret, but a custom header such as X-Partner-Code is not, and the configured authentication.header is never passed as extraExactNames (SensitiveFieldRegistry.php:131). Before this fix such endpoints always 403'd; now they work, so the API key would be stored in every rule step's trace input.

Suggested fix: pass the endpoint's configured auth header names (folded form) as extraExactNames when redacting the trace input. Not a blocker for this PR; proposed as a follow-up issue.

[WilcoLouwerse]

📋 Tracked in #2728 (pre-existing; not a blocker for this PR).

[WilcoLouwerse]

🟢 Follow-up (persistence audit, pre-existing) — authentication denials are traced as success and not logged

(Anchored here because the code is outside the diff.) A 401/403 JSONResponse from processAuthenticationRule() is recorded in the execution trace with status: 'success' (EndpointService.php:2935-2946; the step is added before the JSONResponse check at 2948) and the method returns at 2949-2950, before the only logger->info at 2956. A denial on the now-working custom-header path leaves no failure signal.

Suggested fix: record a failed status for a rule that returns a 4xx JSONResponse, and log the denial (logger->warning, without the header value). Not a blocker for this PR; proposed as a follow-up issue.

[WilcoLouwerse]

📋 Tracked in #2728 (pre-existing; not a blocker for this PR).

[WilcoLouwerse]

🟢 Follow-up (persistence audit, pre-existing) — rule-level API keys have no expiry or last-used record

(Anchored here because the code is outside the diff.) configuration.authentication.keys is a static map on the rule (EndpointService.php:3294), with no expiry and no last-used field; revoking a key means editing the rule. This fix makes custom-header apikey rules usable in production, so the gap now applies to live traffic.

Not a blocker for this PR; proposed as a follow-up issue.

[WilcoLouwerse]

📋 Tracked in #2728 (pre-existing; not a blocker for this PR).

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Strict · full)

The one-line fold is correct: on every inbound path that occurs in practice (before-phase getHeaders(), resume from approval, replay), a hyphenated configured header now matches its value. A missing header still returns 403, and the fold adds no new collision. One thing blocks under Strict: the new test hands the lookup an underscored bag directly, so it never pins the getHeaders() coupling the fix depends on.

🔧 Fix PR: #2730 — 3 of 7 findings, one commit each

Merge it into fix/endpoint-hyphenated-custom-auth-header, then re-request the review. The other findings stay on their threads.

🟡 Concerns (1)

• the test never checks the bag shape that the fix depends on — tests/Unit/Service/EndpointServiceCustomHeaderAuthTest.php:103. Test 1 goes red with the old line restored (mutation-checked), but all three tests stay green if getHeaders() ever emits hyphenated keys again, because none of them builds the bag from $_SERVER. (fix proposed in #2730)

🟢 Minor (6)

• fold the bag keys as well, so the lookup no longer depends on who built the bag — lib/Service/EndpointService.php:3280. A hyphen-keyed bag (after-phase response headers, a mapping rule that rewrote headers) now fails closed; folding the keys at :3237 makes the lookup symmetric. (fix proposed in #2730)
• the skip guard never fires, and its message is wrong — tests/Unit/Service/EndpointServiceCustomHeaderAuthTest.php:54. tests/stubs/OC/AppFramework/Http.php exists; the same sentence is also in EndpointServiceNcSessionRuleTest.php:99. (fix proposed in #2730)
• Follow-up (pre-existing, outside this diff) — the same hyphen/underscore mismatch lives in two more lookups against this bag — lib/Service/ApprovalService.php:97. STRIPPED_HEADERS never matches x_api_key / proxy_authorization, so those headers are stored unredacted in approval snapshots; Accept-Crs at EndpointService.php:2121 never matches either.
• Follow-up (persistence audit, pre-existing) — a custom auth header can land unredacted in execution_trace — lib/Service/EndpointService.php:2941. Redaction matches by name pattern only, and the configured auth header name is never passed as extraExactNames.
• Follow-up (persistence audit, pre-existing) — authentication denials are traced as success and not logged — lib/Service/EndpointService.php:2935. The step is recorded before the JSONResponse check, and the method returns before the only log line.
• Follow-up (persistence audit, pre-existing) — rule-level API keys have no expiry or last-used record — lib/Service/EndpointService.php:3294. Revoking a key means editing the rule.

✅ Verified clean

• The fold loosens nothing: the value still has to match a configured key in authorizeApiKey(), and getHeaders() / FlowToken::getHeaders() admit only HTTP_* keys, so a folded name cannot reach REMOTE_ADDR, PHP_AUTH_PW or similar.
• Fail-closed holds: a missing header still returns 403 (testAMissingCustomHeaderStill403s), and nc-session is dispatched before the lookup.
• EndpointsController::handlePath() carries #[PublicPage] with #[AnonRateLimit(limit: 300, period: 60)], plus the per-consumer InboundRateLimitService.
• Class sweep: processWebhookSignatureRule and the CTI providers read through IRequest::getHeader with hyphenated names, which is consistent; no other configured-name lookup against the getHeaders() bag misses the fold.
• The new code comment matches the code.

Scope decision: full PR, 2 files, 163 lines (band S), Strict because it changes endpoint authentication. The three pre-existing items and the approval-redaction mismatch are 🟢 follow-ups, not blockers, because this PR does not introduce them; it makes custom-header auth work at all.
Follow-up issue: #2728 tracks the four pre-existing 🟢 follow-ups.
Persistence audit: run on the head; 0 MISSING, 3 PARTIAL, all pre-existing and posted above as follow-ups.
Gates: quality / Hydra Gates: 85 of 85 applicable gates ran; the one failure, gate-105 duplicate-page-refresh (a Vue page), fails identically on development, so it is not this PR's. Frontend Check (format) and Security (npm) are red on development too. PHPUnit and Newman were still running at the time of this review.