Skip to content

[ext/session] Validate session.cookie_samesite against allowed values#21647

Closed
jorgsowa wants to merge 1 commit intophp:masterfrom
jorgsowa:fix/session-samesite-validation
Closed

[ext/session] Validate session.cookie_samesite against allowed values#21647
jorgsowa wants to merge 1 commit intophp:masterfrom
jorgsowa:fix/session-samesite-validation

Conversation

@jorgsowa
Copy link
Copy Markdown
Contributor

@jorgsowa jorgsowa commented Apr 6, 2026

Summary

  • Adds OnUpdateSessionSameSite() INI handler that enforces the SameSite whitelist (Strict, Lax, None, or empty string)
  • Rejects any other value with E_WARNING and returns FAILURE, preventing arbitrary strings (including CRLF sequences) from being appended verbatim into the Set-Cookie header
  • php_setcookie() in ext/standard/head.c has a /* Should check value of SameSite? */ TODO comment for the same gap — this addresses it at the INI layer
  • session.cookie_path and session.cookie_domain already have equivalent validation in php_setcookie() at call time; session.cookie_partitioned is a boolean INI entry requiring no string validation

Test plan

  • ext/session/tests/session_cookie_samesite_validation.phpt — verifies invalid and CRLF-containing values are rejected, and that Strict/Lax/None/"" are all accepted

Add OnUpdateSessionSameSite() INI handler that enforces the SameSite
whitelist (Strict, Lax, None, or empty string), rejecting any other
value with E_WARNING. Previously OnUpdateSessionStr() accepted arbitrary
strings including CRLF sequences, which were appended verbatim into the
Set-Cookie header. The php_setcookie() function has a TODO comment for
the same gap; this addresses it at the INI layer.
@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 6, 2026

Why close the PR?

@jorgsowa
Copy link
Copy Markdown
Contributor Author

jorgsowa commented Apr 6, 2026

This is AI slop. I didn't check carefully the content before submitting PR. I want to do it properly. Sorry.

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.

2 participants