Skip to content

Defer CSRF counter measure ID validation - #403

Closed
lippserd wants to merge 1 commit into
mainfrom
defer-csrf-id-check
Closed

Defer CSRF counter measure ID validation#403
lippserd wants to merge 1 commit into
mainfrom
defer-csrf-id-check

Conversation

@lippserd

Copy link
Copy Markdown
Member

Require an ID only when request metadata cannot establish its origin and token-based validation is necessary. Safe and cross-site requests can then take their existing paths without a configured ID.

Require an ID only when request metadata cannot establish its origin and
token-based validation is necessary. Safe and cross-site requests can then
take their existing paths without a configured ID.
@lippserd
lippserd requested a review from jrauh01 July 31, 2026 13:06
@cla-bot cla-bot Bot added the cla/signed label Jul 31, 2026
@nilmerg
nilmerg requested review from TheSyscall and nilmerg July 31, 2026 14:20
@nilmerg

nilmerg commented Jul 31, 2026

Copy link
Copy Markdown
Member

Please provide an example where you need this. My gut's feeling isn't good here and I want the previous author's feedback as well.

@jrauh01 jrauh01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think it's an improvement on one hand to only check for an ID when it is really used, there is still the caveat that this could lead to a working form on the developer's browser, failing at runtime on the user's system.

}

if ($uniqueId === null) {
throw new Error('No CSRF counter measure ID set');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @throws to document this error.

};
}

private function makeFormWithoutId(bool $disabled = false): Form

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think you should remove the $disabled param and explicitly disable CSRF protection in the test itself via:

$form = $this->makeFormWithoutId();
$form->disableCsrfCounterMeasure();

It's only used once anyway, and this would make it clearer at the call site.

@nilmerg

nilmerg commented Aug 10, 2026

Copy link
Copy Markdown
Member

So, I talked about this with @jrauh01 and we concluded that this fully goes against what was originally triggered here.

The only way to get rid of the requirement to set an ID, is to remove support for it and rely on the header only.

But this isn't the goal of this proposal, so I'll close here.

@nilmerg nilmerg closed this Aug 10, 2026
@nilmerg
nilmerg deleted the defer-csrf-id-check branch August 10, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants