Skip to content

Do not regenerate session on every login form render - #20463

Open
arturod67 wants to merge 1 commit into
phpmyadmin:masterfrom
YorkHost-fr:fix-session-regeneration-login
Open

Do not regenerate session on every login form render#20463
arturod67 wants to merge 1 commit into
phpmyadmin:masterfrom
YorkHost-fr:fix-session-regeneration-login

Conversation

@arturod67

@arturod67 arturod67 commented Aug 31, 2026

Copy link
Copy Markdown

Fixes #20462. See the issue for full analysis and a server-side reproduction with two curl requests. Approach pre-validated by @kamil-tekiela in the issue thread. Session fixation protection on successful login (AuthenticationCookie) is unchanged; logout/re-login still rotates the session id (verified on a live deployment).

@arturod67
Do not regenerate session on every login form render
3392687 
Session::secure() was called unconditionally before showLoginForm(),
so every unauthenticated GET (browser prefetch, the /messages route
loaded by the login page, parallel tabs) invalidated the previous
session. The set_session hidden field of an already-rendered form
then no longer matched the session cookie, and the POST failed with
MismatchedSessionId.

Session fixation protection is preserved: Session::secure() is still
called on successful login in AuthenticationCookie::authenticate().
Only generate a token here when none exists yet.

Fixes phpmyadmin#20462

Signed-off-by: Arturo DERGAL - YorkHost <144963931+arturod67@users.noreply.github.com>
@arturod67 arturod67 mentioned this pull request Aug 31, 2026
Open
@arturod67

arturod67 commented Sep 28, 2026

Copy link
Copy Markdown
Author

any update on this?

@williamdes

williamdes commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Merci for this PR !
Could you check if it can be applied on 5.2 versions ?

@codecov

codecov Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.93%. Comparing base (becc5b9) to head (3392687).
⚠️ Report is 159 commits behind head on master.

Files with missing lines Patch % Lines
src/Plugins/AuthenticationPlugin.php 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #20463      +/-   ##
============================================
- Coverage     63.93%   63.93%   -0.01%     
- Complexity    16091    16092       +1     
============================================
  Files           677      677              
  Lines         57853    57854       +1     
============================================
  Hits          36987    36987              
- Misses        20866    20867       +1
Flag Coverage Δ
dbase-extension 63.88% <0.00%> (+0.02%) ⬆️
unit-8.2-ubuntu-latest 63.86% <0.00%> (-0.03%) ⬇️
unit-8.3-ubuntu-latest 63.87% <0.00%> (-0.02%) ⬇️
unit-8.4-ubuntu-latest 63.89% <0.00%> (-0.01%) ⬇️
unit-8.5-ubuntu-latest 63.89% <0.00%> (-0.01%) ⬇️
unit-8.6-ubuntu-latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arturod67

arturod67 commented Sep 30, 2026

Copy link
Copy Markdown
Author

Thanks @williamdes! Yes, 5.2 is affected by the same bug and the fix applies there too.

On QA_5_2, the same code path exists in libraries/classes/Plugins/AuthenticationPlugin.php (L254-258): Session::secure() (which calls session_regenerate_id(true)) runs unconditionally before showLoginForm(). The set_session hidden field (templates/login/form.twig) and the check that throws "Failed to set session cookie…" (libraries/classes/Common.php L497) are also the same. Session fixation protection on successful login is present too (AuthenticationCookie.php L326), so dropping the regeneration on form render is just as safe on 5.2.

The patch doesn't cherry-pick as-is: the file path differs, and Session::getToken() doesn't exist in 5.2. The equivalent change there would be:

/* Show login form (this exits) */
if (! $success) {
    /* Generate session token if missing; regeneration happens on successful login */
    if (empty($_SESSION[' PMA_token '])) {
        Session::secure();
    }

    $this->showLoginForm();
}

If you'd like, I can open a separate PR against QA_5_2 with this backport, or retarget this one so it gets merged up to master. Let me know which you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Unconditional Session::secure() before login form invalidates session on every GET, causing MismatchedSessionId in browsers

2 participants

@arturod67 @williamdes