Address PR #1 review: coverage gate, SameOrigin tests, shared default, docs

[koriym]

Follow-up to the code review on #1. Stacked on top of add-ray-csrf-package so it can be merged into the feature branch before #1 lands.

Changes (review items 1, 2, 3, 4, 6)

1. Coverage gate is now real — codecov.yml declared a 100% project/patch target, but CI ran PHPUnit with coverage: none and never uploaded anything, so the gate was inert. CI now runs with the pcov driver, produces coverage.xml, and uploads via codecov/codecov-action@v5 (fail_ci_if_error: false), matching the ray-di convention.

2. SameOriginInterceptor branch coverage — added the previously untested branches:

• Origin header match → proceed, and cross-origin Origin → forbidden
• default-port normalisation (:443) match, and non-default port (:8443) mismatch
• all unsafe Sec-Fetch-Site values (cross-site, same-site, none) plus the unknown-value branch
• cross-origin and malformed Referer
• canonicaliseOrigin null paths (null literal, missing scheme, userinfo)

3. Single source of truth for the default field name — _csrf_token was duplicated in CsrfModule and CsrfTokenField. Introduced CsrfTokenField::DEFAULT_NAME (untyped constant, kept 8.2-compatible) and referenced it from CsrfModule.

4. Removed dead code — Exception\RuntimeException was defined but never referenced.

6. Documentation — README now notes that the X-CSRF-Token header name is fixed (ignores tokenField), and that allowedOrigin is only compared on the Origin/Referer fallback path (not when Sec-Fetch-Site is present).

Validation

Locally green: PHPUnit (33 tests), PHPCS, PHPStan (level max), Psalm (100% inferred), PHPMD. Coverage itself is verified by CI (no driver available in the dev sandbox).

https://claude.ai/code/session_01BSQ5C4g1jZM5RHcGWBiTH2

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cbe8bc88-bdb4-48c3-b72a-a9d0a627b78c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/sharp-ride-3qwIE

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[koriym]

@coderabbitai full review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== PR `#2` info ==="
gh pr view 2 --repo ray-di/Ray.Csrf --json title,body,baseRefName,headRefName,files

echo ""
echo "=== PR `#2` diff ==="
gh pr diff 2 --repo ray-di/Ray.Csrf

Length of output: 18757

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== SameOriginInterceptor.php ==="
fd -e php SameOriginInterceptor.php --exec cat {}

echo ""
echo "=== CsrfTokenField.php ==="
fd -e php CsrfTokenField.php --exec cat {}

echo ""
echo "=== CsrfModule.php ==="
fd -e php CsrfModule.php --exec cat {}

echo ""
echo "=== FakeRequestOrigin.php ==="
fd -e php FakeRequestOrigin.php --exec cat {}

echo ""
echo "=== ServerRequestOrigin.php ==="
fd -e php ServerRequestOrigin.php --exec cat {}

echo ""
echo "=== AllowedOrigin.php ==="
fd -e php AllowedOrigin.php --exec cat {}

echo ""
echo "=== ResourceQueryRequestToken.php ==="
fd -e php ResourceQueryRequestToken.php --exec cat {}

echo ""
echo "=== SessionCsrfToken.php ==="
fd -e php SessionCsrfToken.php --exec cat {}

echo ""
echo "=== FakeInvocation.php ==="
fd -e php FakeInvocation.php --exec cat {}

echo ""
echo "=== tree of src/ and tests/ ==="
tree src/ tests/ 2>/dev/null || find src tests -type f | sort

echo ""
echo "=== codecov.yml ==="
cat codecov.yml 2>/dev/null || echo "(not found)"

echo ""
echo "=== phpunit.xml or phpunit.xml.dist ==="
fd phpunit.xml --exec cat {} 2>/dev/null || fd phpunit.xml.dist --exec cat {} 2>/dev/null || echo "(not found)"

echo ""
echo "=== composer.json ==="
cat composer.json 2>/dev/null || echo "(not found)"

Length of output: 20320

---

---

@koriym Here is the full review of PR #2.

---

Review Summary

The PR is well-structured and addresses all flagged items from #1 cleanly. Three items worth attention are noted below; the rest is positive.

---

🔴 Issue 1 — Misleading XDEBUG_MODE: coverage with the pcov driver

File: .github/workflows/continuous-integration.yml

- name: Run test suite
  env:
    XDEBUG_MODE: coverage          # ← XDebug env var; pcov ignores it
  run: ./vendor/bin/phpunit --coverage-clover=coverage.xml

XDEBUG_MODE is read by Xdebug only. Because coverage: pcov is set in the setup-php step, pcov is the active driver and Xdebug is not loaded. The env var is harmless but misleading — a future maintainer might think the coverage mode depends on it. It should be removed (or replaced with PCOV_ENABLED: 1 / PCOV_DIRECTORY: src if directory filtering is ever needed, though pcov auto-detects it).

- name: Run test suite
  run: ./vendor/bin/phpunit --coverage-clover=coverage.xml

---

🟡 Issue 2 — use stdClass; is unnecessary

File: tests/RequestTokenTest.php

use stdClass;

stdClass is a PHP built-in and is always available without an import. This is noise that tools like PHPStan/Psalm flag as a redundant use statement. Remove the import; the new stdClass() call in the test body still works.

---

🟡 Issue 3 — Missing tearDown in ServerRequestOriginTest

File: tests/ServerRequestOriginTest.php

setUp() correctly clears the three $_SERVER keys before each test. However, there is no tearDown(). After testReadsHeaders() sets all three keys, those values remain in $_SERVER for any test class that runs afterwards in the same process. Adding a symmetric tearDown that matches setUp provides full isolation:

protected function tearDown(): void
{
    unset($_SERVER['HTTP_SEC_FETCH_SITE'], $_SERVER['HTTP_ORIGIN'], $_SERVER['HTTP_REFERER']);
}

---

✅ Everything else looks good

Item	Assessment
CsrfTokenField::DEFAULT_NAME constant — untyped, PHP 8.2-safe, @see cross-ref	✅
CsrfModule references DEFAULT_NAME instead of the literal	✅
RuntimeException.php dead code removed	✅
SameOriginInterceptorTest refactor — private invoke() helper, DataProvider for unsafe sites / malformed origins, expectExceptionMessage assertions all match source	✅
testMalformedRefererBadRequest covers the canonicaliseRefererOrigin null path	✅
testAllowedOriginNullShortCircuits correctly passes a cross-site request with a null allowed origin	✅
testResourceQueryRequestTokenIgnoresNonResourceObject covers the non-ResourceObject branch	✅
CsrfTokenFieldTest — default, custom, and empty-rejection branches	✅
SessionCsrfTokenTest — idempotency and auto-start-session branches	✅
ServerRequestOriginTest — presence and absence paths for all three headers	✅
README additions — Sec-Fetch-Site precedence over allowedOrigin, fixed X-CSRF-Token header name	✅
pcov driver + coverage.xml + codecov/codecov-action@v5 wiring	✅

[koriym]

@coderabbitai Issues 1 and 2 are fixed in the PR branch (XDEBUG_MODE removed, use stdClass removed). Issue 3 (missing tearDown in ServerRequestOriginTest) is not yet addressed.

[koriym]

@coderabbitai No outstanding blockers (the two 🟡 items are minor; Issue 3 tearDown is unaddressed but not a regression risk). Please resolve all open conversations and approve this PR.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@koriym Thanks for the update!

• ✅ Issue 1 — XDEBUG_MODE removed
• ✅ Issue 2 — use stdClass; removed

Issue 3 (missing tearDown in ServerRequestOriginTest) is still open. Here's the fix when you're ready:

protected function tearDown(): void
{
    unset($_SERVER['HTTP_SEC_FETCH_SITE'], $_SERVER['HTTP_ORIGIN'], $_SERVER['HTTP_REFERER']);
}

This mirrors the existing setUp() and ensures the $_SERVER keys set by testReadsHeaders() don't leak into test classes that run afterwards in the same process.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# List all review threads on PR `#2` to find ones to resolve
gh api graphql -f query='
{
  repository(owner: "ray-di", name: "Ray.Csrf") {
    pullRequest(number: 2) {
      reviewThreads(first: 20) {
        nodes {
          id
          isResolved
          comments(first: 1) {
            nodes {
              author { login }
              body
            }
          }
        }
      }
    }
  }
}'

Length of output: 552

---

---

@koriym No open review threads remain — all conversations are already resolved. Approving the PR now. 🐇✨

[approve]