Skip to content

fix(review): #2719 review findings, round 1 - #2730

Open
WilcoLouwerse wants to merge 4 commits into
fix/endpoint-hyphenated-custom-auth-headerfrom
review/pr-2719-fixes
Open

WilcoLouwerse wants to merge 4 commits into
fix/endpoint-hyphenated-custom-auth-headerfrom
review/pr-2719-fixes

Conversation

@WilcoLouwerse

Copy link
Copy Markdown
Contributor

Fixes for the round-1 review of #2719 (#2719 (review)). Merge into fix/endpoint-hyphenated-custom-auth-header, then re-request the review; nothing here is merged by the reviewer.

Findings → commits

Finding Commit What changed
f2 🟡 the test never checks the bag shape that the fix depends on 3a10438e2 New test builds the headers bag with EndpointService::getHeaders() from a server array carrying HTTP_X_CORSA_SECRET and expects authorizeApiKey('s3cr3t', …).
f2 (pre-push) f9868cf50 The new test's docblock now names EndpointService::getHeaders() as the method it calls and states it has the same key map as FlowToken::getHeaders(), the production producer.
f3 🟢 fold the bag keys as well, so the lookup no longer depends on who built the bag 4f436304f processAuthenticationRule() folds - to _ in the bag keys too, comments updated, plus a test passing ['X-Corsa-Secret' => 's3cr3t'].
f4 🟢 the skip guard never fires, and its message is wrong 82a000ac2 Skip message reworded in EndpointServiceCustomHeaderAuthTest.php and EndpointServiceNcSessionRuleTest.php (plus that file's docblock claim) to say tests/bootstrap.php stubs OC\AppFramework\Http when Nextcloud is absent.

Left to the author (threads stay open)

  • The 10 pre-existing named-parameter phpcs findings in the original tests of EndpointServiceCustomHeaderAuthTest.php — outside these findings.

Tracked elsewhere

Checks run on this branch

  • phpunit -c phpunit-unit.xml tests/Unit/Service/EndpointServiceCustomHeaderAuthTest.php → OK (5 tests, 11 assertions)
  • phpunit -c phpunit-unit.xml tests/Unit/Service/EndpointServiceNcSessionRuleTest.php → OK (5 tests, 10 assertions)
  • phpunit -c phpunit-unit.xml tests/Unit/Service → 3124 tests, 1 failure, 2 skipped; base 90ba6d12e gives 3122 tests, the same 1 failure, 2 skipped. The failure is OpenRegisterCredentialBridgeOldOpenRegisterTest, whose subprocess fixture requires vendor/autoload.php inside the checkout, which the test worktree did not have.
  • Mutations on the f2 test, before f3: getHeaders() emitting hyphenated keys → red (the hand-built test stays green); pre-fix lookup strtolower(header) → red.
  • Mutations on the f2 test, with f3: the getHeaders() hyphen mutation → green, absorbed by the f3 bag-key fold by design; pre-fix lookup → red; getHeaders() without the HTTP_ prefix strip → red.
  • f3 control: lib change reversed with git apply -R → testAHyphenatedBagKeyMatchesTheConfiguredHeader red; re-applied → OK (5 tests, 11 assertions).
  • php -l on the three changed files → no syntax errors.
  • phpcs --standard=phpcs.xml on the changed files → no findings on added or changed lines (remaining findings are pre-existing).
  • run-hydra-gates.sh --scope-to-diff --base 90ba6d12e → exit 0; COVERAGE: 39 of 97 declared gates reported a result (58 not applicable to this repo/diff; 39 of 39 applicable gates ran).
  • git grep -n "does not stub" → empty.
  • pre-push adversarial check: two rounds, last returned no findings
  • not run: Newman/API tests, composer check:strict (phpstan/psalm), unit tests outside tests/Unit/Service — the worktree had no own vendor/ and no running Nextcloud instance.

🤖 Generated with Claude Code

WilcoLouwerse and others added 4 commits October 23, 2026 15:16
#2719 (comment)

The existing test hand-builds the headers bag, so it guards the lookup
but not its coupling to the bag's key shape. The new test builds the bag
with EndpointService::getHeaders() from a server array carrying
HTTP_X_CORSA_SECRET and passes it to processAuthenticationRule(). It goes
red when getHeaders() is mutated to emit hyphenated keys, and red with
the pre-fix lookup line; the hand-built test stays green under the
getHeaders() mutation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#2719 (comment)

processAuthenticationRule() now folds '-' to '_' in the headers bag keys
as well as in the configured header name, so the lookup no longer
depends on which producer built the bag (getHeaders() keys underscored,
a mapping rule or response headers may key hyphenated). The new test
passes ['X-Corsa-Secret' => 's3cr3t'] and goes red with the bag fold
reversed.

With this fold in place, a getHeaders() mutation that emits hyphenated
keys is absorbed by design; the getHeaders() test still goes red when
the HTTP_ prefix strip is removed or the pre-fix lookup line is restored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#2719 (comment)

tests/bootstrap.php loads tests/stubs/OC/AppFramework/Http.php whenever
Nextcloud is absent, so "which the unit bootstrap does not stub" was
false. Reworded the skip message in both copies and the docblock claim
in the pre-existing file. `git grep -n "does not stub"` now returns
nothing.

Files touched by the sweep:
- tests/Unit/Service/EndpointServiceCustomHeaderAuthTest.php
- tests/Unit/Service/EndpointServiceNcSessionRuleTest.php

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y uses

#2719 (comment)

The docblock said the bag came from the real producer; production feeds
the rule from FlowToken::getHeaders(). It now names
EndpointService::getHeaders() as the method the test calls and states it
has the same key map as FlowToken::getHeaders().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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