docs: correct CSRF protection opt-in description#30
Open
koriym wants to merge 6 commits into
Open
Conversation
README claimed CSRF was only verified when #[CsrfProtection] was present, but SetAntiCsrfTrait alone is sufficient: the trait's #[Inject] setter populates $this->antiCsrf, postConstruct() calls enableAntiCsrf(), and AbstractForm::apply() throws CsrfViolationException on token mismatch regardless of the attribute (covered by AbstractFormTest::testAntiCsrfViolation). Document the two independent opt-in paths (per-form trait, per-action attribute) and fix the migrate-to-1.0 skill's "out of scope" note that implied SetAntiCsrfTrait-only forms silently stop enforcing CSRF in 1.0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR updates documentation across three files to clarify CSRF protection behavior in the form module. The changes explain that CSRF protection is opt-in, available through two independent mechanisms—per-form via ChangesCSRF Protection Documentation Clarification
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR corrects the project documentation around how CSRF protection is actually enabled/enforced in ray/web-form-module, aligning the READMEs and the bundled migration skill guidance with the current runtime behavior (notably AbstractForm::apply() and the existing tests).
Changes:
- Updated
README.md/README.JA.mdCSRF section to describe two independent CSRF opt-in paths (per-form trait vs. per-action attribute) and the resultingCsrfViolationExceptionbehavior. - Updated the migration skill (
.claude/.../SKILL.md) guidance to reflect the real “at-risk” migration scenario around the removedantiCsrf=trueoption.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| README.md | Rewrites CSRF protection explanation to reflect actual enforcement behavior and the two opt-in paths. |
| README.JA.md | Japanese equivalent of the CSRF documentation rewrite. |
| .claude/skills/migrate-to-1.0/SKILL.md | Adjusts migration guidance around antiCsrf=true and how CSRF enforcement is preserved in 1.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| independent paths: | ||
|
|
||
| - **Per-form**: add `use SetAntiCsrfTrait;` to the form. `AntiCsrfInterface` | ||
| is injected at construction time, the token field is added in |
Comment on lines
+145
to
+149
| CSRF protection is **opt-in** and can be enabled through either of two | ||
| independent paths: | ||
|
|
||
| - **Per-form**: add `use SetAntiCsrfTrait;` to the form. `AntiCsrfInterface` | ||
| is injected at construction time, the token field is added in |
Comment on lines
+130
to
+134
| CSRF対策は **opt-in** で、独立した 2 つの経路のいずれかで有効化できます。 | ||
|
|
||
| - **フォーム単位**: フォームに `use SetAntiCsrfTrait;` を追加します。 | ||
| DI で `AntiCsrfInterface` が注入され、`postConstruct()` でトークンフィールドが | ||
| 追加され、`apply()` の呼び出しごとにトークンが検証されます。 |
Comment on lines
+197
to
+203
| - Migrating `@FormValidation(antiCsrf=true)` callers that did **not** use | ||
| `SetAntiCsrfTrait` on the form. These relied on the removed `antiCsrf` | ||
| option to enable CSRF; after dropping the option they need | ||
| `#[CsrfProtection]` on the validated method (or `SetAntiCsrfTrait` on | ||
| the form) to keep enforcement. Flag these to the user so they can decide. | ||
| Forms that already use `SetAntiCsrfTrait` continue to enforce CSRF in 1.0 | ||
| via `AbstractForm::apply()` regardless of `#[CsrfProtection]`. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- Reword "injected at construction time" to clarify the trait uses an #[Inject] setter (method injection by Ray.Di), not constructor injection. - Split the README example into two minimal snippets — one per opt-in path — and note that combining both is redundant. The single combined example contradicted the "two independent paths" wording. - Drop the migrate-to-1.0 "out of scope" bullet that overlapped with the automated Step 1b rewrite. Replace it with a note that #[CsrfProtection] added by 1b is redundant when the form already uses SetAntiCsrfTrait, so users can choose a single declaration site.
Comment on lines
+174
to
+178
| // Per-form: declare CSRF on the form itself. | ||
| use Ray\WebFormModule\AbstractAuraForm; | ||
| use Ray\WebFormModule\SetAntiCsrfTrait; | ||
|
|
||
| class MyForm extends AbstractAuraForm |
Comment on lines
+159
to
+163
| // フォーム単位: フォーム自身で宣言。 | ||
| use Ray\WebFormModule\AbstractAuraForm; | ||
| use Ray\WebFormModule\SetAntiCsrfTrait; | ||
|
|
||
| class MyForm extends AbstractAuraForm |
Comment on lines
+197
to
+201
| - Deciding whether `#[CsrfProtection]` added by Step 1b is redundant. Forms | ||
| that already use `SetAntiCsrfTrait` enforce CSRF via `AbstractForm::apply()` | ||
| regardless of the attribute, so the attribute Step 1b inserts is harmless | ||
| but unnecessary. Flag these to the user so they can drop the attribute if | ||
| they prefer a single declaration site. |
- Split the README CSRF examples into two valid stand-alone code blocks
(per-action and per-form). The previous single block had `use` statements
after `class MyController`, which is not valid PHP in one file.
- Reword the Step 1b note ("methods without the attribute perform no CSRF
check") so it no longer contradicts the Out-of-scope note about
SetAntiCsrfTrait-only forms still enforcing CSRF via AbstractForm::apply().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.JA.md`:
- Line 163: The markdownlint MD046 warning indicates a fenced PHP code block (`
```php `) is used but the repo expects indented code blocks; replace the fenced
block marker (` ```php ` and the closing ```` ``` ````) with an indented code
block by prefixing each line of the code sample with four spaces (or a single
tab) where the fenced block starts, ensuring the content that was between `
```php ` and its closing fence remains intact and correctly indented so the
linter treats it as an indented code block.
- Around line 164-168: Replace the nonexistent base class AbstractAuraForm with
the correct AbstractForm: change the use statement from
Ray\WebFormModule\AbstractAuraForm to Ray\WebFormModule\AbstractForm and update
the class declaration so MyForm extends AbstractForm (leave SetAntiCsrfTrait
as-is); ensure you import AbstractForm and adjust the extends clause in the
MyForm class.
In `@README.md`:
- Line 178: The README contains a fenced PHP code block using ```php which
triggers markdownlint MD046 expecting indented code blocks; update the block by
converting the fenced PHP block (the ```php fence) into an indented code block
(prefix each code line with four spaces) or alternatively adjust the
repository's markdownlint config to allow fenced blocks for PHP, ensuring the
fenced ```php instance is replaced or made compliant with MD046.
- Around line 179-183: The README references a non-existent class
AbstractAuraForm; replace occurrences of AbstractAuraForm with AbstractForm by
updating the use statement (change use Ray\WebFormModule\AbstractAuraForm; to
use Ray\WebFormModule\AbstractForm;) and the class declaration (change class
MyForm extends AbstractAuraForm to class MyForm extends AbstractForm) so the
example matches the library API.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4db0596e-5966-46ae-8962-eae37fba59e5
📒 Files selected for processing (3)
.claude/skills/migrate-to-1.0/SKILL.mdREADME.JA.mdREADME.md
✅ Files skipped from review due to trivial changes (1)
- .claude/skills/migrate-to-1.0/SKILL.md
|
|
||
| フォーム単位 — フォーム自身で宣言: | ||
|
|
||
| ```php |
There was a problem hiding this comment.
markdownlint MD046 の警告を解消してください。
Line 163 のコードブロックが Expected: indented; Actual: fenced と報告されています。CI ルールに合わせてブロックスタイルを統一してください。
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 163-163: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.JA.md` at line 163, The markdownlint MD046 warning indicates a fenced
PHP code block (` ```php `) is used but the repo expects indented code blocks;
replace the fenced block marker (` ```php ` and the closing ```` ``` ````) with
an indented code block by prefixing each line of the code sample with four
spaces (or a single tab) where the fenced block starts, ensuring the content
that was between ` ```php ` and its closing fence remains intact and correctly
indented so the linter treats it as an indented code block.
|
|
||
| Per-form — declare CSRF on the form itself: | ||
|
|
||
| ```php |
There was a problem hiding this comment.
Address markdownlint MD046 on this new fenced block.
Line 178 is currently flagged (Expected: indented; Actual: fenced). If CI enforces this rule, convert this block style to match repository markdownlint configuration.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 178-178: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 178, The README contains a fenced PHP code block using
```php which triggers markdownlint MD046 expecting indented code blocks; update
the block by converting the fenced PHP block (the ```php fence) into an indented
code block (prefix each code line with four spaces) or alternatively adjust the
repository's markdownlint config to allow fenced blocks for PHP, ensuring the
fenced ```php instance is replaced or made compliant with MD046.
src/AbstractForm.php only declares AbstractForm; AbstractAuraForm does not exist in this package. The example was inherited from the original CSRF section and never compiled.
README.md and README.JA.md already use fenced code blocks throughout (20+ and 24+ occurrences). The default MD046 rule infers the style from whichever block appears first, so the fenced blocks were reported as errors. Pin the style to fenced to match the established convention.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 186-187: Add a blank line after the closing fenced code block
shown in the diff (the triple backticks that end the php example using
SetAntiCsrfTrait) so that the following prose ("You can provide your custom
`AntiCsrf` class...") is separated by one empty line; this fixes the
markdownlint MD031 error by ensuring there is an empty line between the fenced
code block and the subsequent paragraph.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dc2f4a5-3d1a-4313-a06d-e4e596d2e8bc
📒 Files selected for processing (3)
.markdownlint.jsonREADME.JA.mdREADME.md
✅ Files skipped from review due to trivial changes (2)
- .markdownlint.json
- README.JA.md
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Satisfies markdownlint MD031 (blanks-around-fences) for the fenced block introduced in the CSRF section.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The README sections on CSRF protection claimed that
SetAntiCsrfTraitonly wiresAntiCsrfInterfaceinto the form and that token verification requires#[CsrfProtection]:This is not what the code does.
SetAntiCsrfTrait::setAntiCsrf()is annotated with#[Inject], so DI populates$this->antiCsrfat construction.AbstractForm::postConstruct()then callsenableAntiCsrf()to add the token field, andAbstractForm::apply()throwsCsrfViolationExceptionon token mismatch — regardless of whether the controller method has#[CsrfProtection]. This is exercised byAbstractFormTest::testAntiCsrfViolation, which sets antiCsrf directly on the form (no interceptor, no attribute) and asserts the throw.So there are two independent opt-in paths and the README/skill should describe them as such.
Changes
README.md/README.JA.md: rewrite the "CSRF Protections" intro to describe the two independent paths (per-form trait vs. per-action attribute) and state that either causesapply()to throw on mismatch..claude/skills/migrate-to-1.0/SKILL.md: replace the "out of scope" bullet that said forms usingSetAntiCsrfTraitwithout#[CsrfProtection]"will silently stop enforcing CSRF in 1.0" — the actual at-risk case is the removed@FormValidation(antiCsrf=true)option when the form did not also use the trait.Test plan
use SetAntiCsrfTrait;+ no#[CsrfProtection]on controller) is consistent withAbstractFormTest::testAntiCsrfViolation.Generated by Claude Code
Summary by CodeRabbit