Skip to content

fix(LoginController): call session_regenerate_id() before authentication to prevent session fixation - #2255

Merged
nodiscc merged 1 commit into
shaarli:masterfrom
nodiscc:fix/sessionRegenerateId-order
Aug 11, 2026
Merged

fix(LoginController): call session_regenerate_id() before authentication to prevent session fixation#2255
nodiscc merged 1 commit into
shaarli:masterfrom
nodiscc:fix/sessionRegenerateId-order

Conversation

@nodiscc

@nodiscc nodiscc commented Aug 10, 2026

Copy link
Copy Markdown
Member
  • PHP documentation (https://www.php.net/manual/en/features.session.security.management.php) mandates: "Session IDs must be regenerated when user privileges are elevated, such as after authenticating. session_regenerate_id() must be called prior to setting the authentication information to $_SESSION.'
  • Additionally, session_regenerate_id(true) and session_destroy() must never be called together for an active session.'
  • The old code called destroy() -> start() -> regenerateId(true), which:
  1. Violated the ordering requirement (regenerateId after auth)
  2. Combined regenerateId(true) with destroy() (forbidden by PHP docs)
  • Fix: call regenerateId(true) first (it saves session data and creates a new session ID), then set cookie parameters. No need for separate destroy() or start() calls.

Ref. #2213

…ion to prevent session fixation

PHP documentation mandates: 'Session IDs must be regenerated when user
privileges are elevated, such as after authenticating.
session_regenerate_id() must be called prior to setting the
authentication information to $_SESSION.'

Additionally, 'session_regenerate_id(true) and session_destroy() must
never be called together for an active session.'

The old code called destroy() -> start() -> regenerateId(true), which:
1. Violated the ordering requirement (regenerateId after auth)
2. Combined regenerateId(true) with destroy() (forbidden by PHP docs)

Fix: call regenerateId(true) first (it saves session data and creates
a new session ID), then set cookie parameters. No need for separate
destroy() or start() calls.
@nodiscc nodiscc added this to the 0.17.0 milestone Aug 10, 2026
@nodiscc nodiscc self-assigned this Aug 10, 2026
@nodiscc
nodiscc marked this pull request as ready for review August 10, 2026 19:08
@nodiscc
nodiscc merged commit 590ba1f into shaarli:master Aug 11, 2026
8 checks passed
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.

1 participant