Add configurable session cookie SameSite and Secure preferences (#1501)#1660
Merged
mambax7 merged 3 commits intoXOOPS:masterfrom Mar 15, 2026
Merged
Conversation
…S#1501) Fixes XOOPS#1501. Ports the session cookie preferences from the 259world development branch. The SameSite attribute was hardcoded to 'Lax' since PR XOOPS#1488 — now configurable via Admin > Preferences. Changes: - session.php: Read session_cookie_samesite and session_cookie_secure from $xoopsConfig with fallback to 'Lax'/auto-detect. Forces Secure=true when SameSite=None (browser requirement). - preferences.php: Add _MD_AM_SESSSAMESITE and _MD_AM_SESSSECURE language constants. - makedata.php: Add config rows for fresh installs (conf_id 136, 137) with Lax/Strict/None select options. - upgrade/index.php: Add addsessioncookieprefs upgrade task that inserts the two config rows and select options for existing installations.
WalkthroughAdds configurable session cookie attributes: SameSite and Secure. Installer data and upgrade logic add the new config entries and options; runtime session code reads and validates these settings to set cookie parameters; language strings for admin UI are included. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds configurable session-cookie attributes (SameSite and Secure) to XOOPS core so administrators can tune cookie behavior via System Preferences, with installer + upgrader support for 2.5.12.
Changes:
- Add new upgrade task to create
session_cookie_samesite/session_cookie_securepreferences (and options) during 2.5.11 → 2.5.12 upgrade. - Add new System Preferences language strings for the new settings.
- Update session cookie initialization to use configured SameSite/Secure values; update installer seed data to include the new config rows and options.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.5.12/index.php | Adds an upgrade task to insert the new session cookie preferences and SameSite options. |
| htdocs/modules/system/language/english/admin/preferences.php | Adds English labels/descriptions for the new preferences. |
| htdocs/kernel/session.php | Reads the new preferences to set session cookie samesite and secure attributes. |
| htdocs/install/include/makedata.php | Seeds new installs with the new config entries and SameSite options. |
Comment on lines
+200
to
+205
| // Insert SameSite preference | ||
| $db->exec("INSERT INTO `{$configTable}` (conf_modid, conf_catid, conf_name, conf_title, conf_value, conf_desc, conf_formtype, conf_valuetype, conf_order) VALUES (0, 1, 'session_cookie_samesite', '_MD_AM_SESSSAMESITE', 'Lax', '_MD_AM_SESSSAMESITE_DSC', 'select', 'text', 43)"); | ||
|
|
||
| // Insert Secure preference | ||
| $db->exec("INSERT INTO `{$configTable}` (conf_modid, conf_catid, conf_name, conf_title, conf_value, conf_desc, conf_formtype, conf_valuetype, conf_order) VALUES (0, 1, 'session_cookie_secure', '_MD_AM_SESSSECURE', '0', '_MD_AM_SESSSECURE_DSC', 'yesno', 'int', 44)"); | ||
|
|
Comment on lines
+200
to
+219
| // Insert SameSite preference | ||
| $db->exec("INSERT INTO `{$configTable}` (conf_modid, conf_catid, conf_name, conf_title, conf_value, conf_desc, conf_formtype, conf_valuetype, conf_order) VALUES (0, 1, 'session_cookie_samesite', '_MD_AM_SESSSAMESITE', 'Lax', '_MD_AM_SESSSAMESITE_DSC', 'select', 'text', 43)"); | ||
|
|
||
| // Insert Secure preference | ||
| $db->exec("INSERT INTO `{$configTable}` (conf_modid, conf_catid, conf_name, conf_title, conf_value, conf_desc, conf_formtype, conf_valuetype, conf_order) VALUES (0, 1, 'session_cookie_secure', '_MD_AM_SESSSECURE', '0', '_MD_AM_SESSSECURE_DSC', 'yesno', 'int', 44)"); | ||
|
|
||
| // Add select options for SameSite preference | ||
| $sql = "SELECT conf_id FROM `{$configTable}` WHERE conf_name = 'session_cookie_samesite'"; | ||
| $result = $db->query($sql); | ||
| if ($db->isResultSet($result) && ($result instanceof \mysqli_result)) { | ||
| $row = $db->fetchRow($result); | ||
| if ($row) { | ||
| $confId = (int) $row[0]; | ||
| $db->exec("INSERT INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('Lax', 'Lax', {$confId})"); | ||
| $db->exec("INSERT INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('Strict', 'Strict', {$confId})"); | ||
| $db->exec("INSERT INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('None', 'None', {$confId})"); | ||
| } | ||
| } | ||
|
|
||
| return true; |
Comment on lines
+179
to
+186
| $sql = 'SELECT COUNT(*) FROM `' . $GLOBALS['xoopsDB']->prefix('config') | ||
| . "` WHERE `conf_name` IN ('session_cookie_samesite', 'session_cookie_secure')"; | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| return false; | ||
| } | ||
| $row = $GLOBALS['xoopsDB']->fetchRow($result); | ||
| return $row && (int) $row[0] >= 2; |
Comment on lines
+71
to
+81
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| $secureFromConfig = isset($xoopsConfig['session_cookie_secure']) ? (bool) $xoopsConfig['session_cookie_secure'] : null; | ||
| $secure = is_bool($secureFromConfig) ? $secureFromConfig : (XOOPS_PROT === 'https://'); | ||
|
|
||
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/kernel/session.php`:
- Around line 75-77: The current logic lets a stored value like '0' in
$xoopsConfig['session_cookie_secure'] override HTTPS auto-detection, weakening
security; change the handling so only an explicit affirmative (e.g. string '1'
or boolean true) forces Secure=true, otherwise leave $secureFromConfig as null
so $secure falls back to XOOPS_PROT auto-detection. Update the assignment around
$secureFromConfig/$secure (variables $xoopsConfig['session_cookie_secure'],
$secureFromConfig, $secure and use XOOPS_PROT) to map only explicit true values
to true and treat any other stored value as “auto” (null) so HTTPS sites still
get Secure cookies by default; if you need full on/off/auto semantics implement
a tri-state config instead.
In `@upgrade/upd_2.5.11-to-2.5.12/index.php`:
- Around line 177-220: The migration currently always returns true and can leave
a half-applied state; update apply_addsessioncookieprefs() to be idempotent and
fail-fast: before inserting use existence checks (or INSERT IGNORE/ON DUPLICATE)
against $configTable to avoid duplicate conf_name inserts, verify each
$db->exec() and $db->query() call succeeds (and return false on any failure),
ensure after inserting the 'session_cookie_samesite' row you re-query
$configTable and validate $confId (using $db->isResultSet() and $db->fetchRow())
and then verify each $optionTable insert for SameSite options succeeds (return
false on failure), and finally only return true when all inserts/validations
complete successfully; reference apply_addsessioncookieprefs(),
check_addsessioncookieprefs(), $db, $configTable, $optionTable, $confId, exec(),
query(), isResultSet(), fetchRow().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f62a277-74a2-49eb-a285-7b2b850f1cc5
📒 Files selected for processing (4)
htdocs/install/include/makedata.phphtdocs/kernel/session.phphtdocs/modules/system/language/english/admin/preferences.phpupgrade/upd_2.5.11-to-2.5.12/index.php
htdocs/kernel/session.php
Outdated
Comment on lines
+75
to
+77
| $secureFromConfig = isset($xoopsConfig['session_cookie_secure']) ? (bool) $xoopsConfig['session_cookie_secure'] : null; | ||
| $secure = is_bool($secureFromConfig) ? $secureFromConfig : (XOOPS_PROT === 'https://'); | ||
|
|
There was a problem hiding this comment.
session_cookie_secure default handling weakens HTTPS cookie security.
At Line 76, config value '0' overrides HTTPS auto-detection to false. Because this PR seeds 'session_cookie_secure' = '0' in installer/upgrade, many HTTPS sites will emit non-Secure session cookies by default.
🔐 Suggested fix (preserve HTTPS-safe default; allow explicit force-on)
- $secureFromConfig = isset($xoopsConfig['session_cookie_secure']) ? (bool) $xoopsConfig['session_cookie_secure'] : null;
- $secure = is_bool($secureFromConfig) ? $secureFromConfig : (XOOPS_PROT === 'https://');
+ // Keep secure-by-default behavior on HTTPS, allow explicit force-on from config.
+ $secure = (XOOPS_PROT === 'https://');
+ if (isset($xoopsConfig['session_cookie_secure']) && (string)$xoopsConfig['session_cookie_secure'] === '1') {
+ $secure = true;
+ }If you need full explicit on/off/auto behavior, model this preference as a tri-state option instead of yes/no.
As per coding guidelines, htdocs/kernel/**/*.php requires special attention to security and backward compatibility in core kernel classes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/kernel/session.php` around lines 75 - 77, The current logic lets a
stored value like '0' in $xoopsConfig['session_cookie_secure'] override HTTPS
auto-detection, weakening security; change the handling so only an explicit
affirmative (e.g. string '1' or boolean true) forces Secure=true, otherwise
leave $secureFromConfig as null so $secure falls back to XOOPS_PROT
auto-detection. Update the assignment around $secureFromConfig/$secure
(variables $xoopsConfig['session_cookie_secure'], $secureFromConfig, $secure and
use XOOPS_PROT) to map only explicit true values to true and treat any other
stored value as “auto” (null) so HTTPS sites still get Secure cookies by
default; if you need full on/off/auto semantics implement a tri-state config
instead.
…handling Addresses P1 and P2 from Codex review on PR XOOPS#1660: P1 — Security regression in session.php: The (bool) cast on session_cookie_secure turned the DB default '0' into false, overriding HTTPS auto-detection. Now: Secure is auto-detected from XOOPS_PROT, and the preference can only force it ON (never OFF). SameSite=None still forces Secure=true. P2 — Upgrade task half-applied state: - check_addsessioncookieprefs() now verifies both config rows AND the 3 SameSite options exist - apply_addsessioncookieprefs() is idempotent: skips inserts if rows already exist, uses INSERT IGNORE for options, logs DB errors via $this->logs[], returns false on failure
Contributor
There was a problem hiding this comment.
Pull request overview
Adds configurable session-cookie security preferences to XOOPS 2.5.12, ensuring modern browser compatibility (SameSite + Secure) across both fresh installs and upgrades.
Changes:
- Add new system preferences for
session_cookie_samesiteandsession_cookie_secure(including installer defaults + SameSite options). - Apply these preferences when setting session cookie parameters in the core session handler.
- Add an upgrade task to backfill the new preferences into existing installations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.5.12/index.php | Adds an upgrade task to insert the new config rows + SameSite options. |
| htdocs/modules/system/language/english/admin/preferences.php | Adds English admin strings for the new preferences. |
| htdocs/kernel/session.php | Uses the new preferences to compute samesite and secure cookie params (including forcing Secure for SameSite=None). |
| htdocs/install/include/makedata.php | Seeds default config values and SameSite select options for new installs. |
| } | ||
| // Re-fetch the conf_id | ||
| $result = $db->query($sql); | ||
| $sameSiteRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; |
|
|
||
| if (!$optRow || (int) $optRow[0] < 3) { | ||
| foreach (['Lax', 'Strict', 'None'] as $opt) { | ||
| $db->exec("INSERT IGNORE INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('{$opt}', '{$opt}', {$confId})"); |
Comment on lines
+246
to
+252
| $sql = "SELECT COUNT(*) FROM `{$optionTable}` WHERE conf_id = {$confId}"; | ||
| $result = $db->query($sql); | ||
| $optRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; | ||
|
|
||
| if (!$optRow || (int) $optRow[0] < 3) { | ||
| foreach (['Lax', 'Strict', 'None'] as $opt) { | ||
| $db->exec("INSERT IGNORE INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('{$opt}', '{$opt}', {$confId})"); |
Comment on lines
+71
to
+79
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| // Auto-detect Secure from protocol; preference can only force it ON, never OFF | ||
| $secure = (XOOPS_PROT === 'https://'); | ||
| if (!empty($xoopsConfig['session_cookie_secure'])) { | ||
| $secure = true; | ||
| } |
Comment on lines
+71
to
+85
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| // Auto-detect Secure from protocol; preference can only force it ON, never OFF | ||
| $secure = (XOOPS_PROT === 'https://'); | ||
| if (!empty($xoopsConfig['session_cookie_secure'])) { | ||
| $secure = true; | ||
| } | ||
|
|
||
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; | ||
| } | ||
|
|
…nd duplicate prevention
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
upgrade/upd_2.5.11-to-2.5.12/index.php (2)
248-250:⚠️ Potential issue | 🟠 MajorHandle DELETE failure before reinserting options.
DELETE FROM configoptionresult is not checked. If it fails, migration can proceed and still return success in a partially failed state.🔧 Proposed fix
- $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}"); + if (!$db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}")) { + $this->logs[] = "Failed to reset SameSite options for conf_id {$confId}: " . $db->error(); + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.11-to-2.5.12/index.php` around lines 248 - 250, The DELETE that removes existing config options for SameSite uses $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}") but its result isn't checked; modify the migration to capture the return value/rowcount from $db->exec (or check $db->errorInfo()/lastError) after the DELETE for $confId (derived from $sameSiteRow) and if it indicates failure or zero rows when rows were expected, abort the migration (return/throw) and log or surface the DB error so reinsertion of options for $confId does not proceed on a failed delete.
195-203:⚠️ Potential issue | 🟠 Major
check_addsessioncookieprefs()can return true with wrong option values.The query only checks option count (
>= 3), so any three rows pass even ifLax/Strict/Noneare missing or corrupted. Tighten to required values withCOUNT(DISTINCT ...) = 3.🔧 Proposed fix
- $sql = "SELECT COUNT(*) FROM `" . $db->prefix('configoption') . "` co" + $sql = "SELECT COUNT(DISTINCT co.confop_value) FROM `" . $db->prefix('configoption') . "` co" . " INNER JOIN `" . $db->prefix('config') . "` c ON co.conf_id = c.conf_id" - . " WHERE c.conf_name = 'session_cookie_samesite' AND c.conf_modid = 0"; + . " WHERE c.conf_name = 'session_cookie_samesite' AND c.conf_modid = 0" + . " AND co.confop_value IN ('Lax', 'Strict', 'None')"; @@ - return $row && (int) $row[0] >= 3; + return $row && (int) $row[0] === 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.11-to-2.5.12/index.php` around lines 195 - 203, The check_addsessioncookieprefs() currently counts rows and can return true for any three rows; fix it by changing the SQL to verify the three required option values explicitly: use COUNT(DISTINCT co.conf_value) with a WHERE ... AND co.conf_value IN ('Lax','Strict','None') (or equivalent normalized values) and compare to = 3, then fetch the row as before; update the $sql string near the existing $sql/$db->query/$db->fetchRow usage so the function only returns true when all three distinct expected values exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/kernel/session.php`:
- Around line 69-92: Add unit tests for session cookie option resolution around
the $sameSite and $secure logic in session.php: write tests that (1) pass an
invalid session_cookie_samesite value and assert $sameSite falls back to 'Lax'
and is passed to session_set_cookie_params, (2) set session_cookie_samesite to
'None' and assert the resulting options have 'secure' === true, and (3) verify
HTTPS auto-detection by toggling XOOPS_PROT between 'https://' and 'http://'
with session_cookie_secure set to false/0 and asserting 'secure' is true only
when XOOPS_PROT === 'https://'; call the code path that builds $options and
inspect the array passed to session_set_cookie_params or provide a hook to
return options for assertion.
---
Duplicate comments:
In `@upgrade/upd_2.5.11-to-2.5.12/index.php`:
- Around line 248-250: The DELETE that removes existing config options for
SameSite uses $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id =
{$confId}") but its result isn't checked; modify the migration to capture the
return value/rowcount from $db->exec (or check $db->errorInfo()/lastError) after
the DELETE for $confId (derived from $sameSiteRow) and if it indicates failure
or zero rows when rows were expected, abort the migration (return/throw) and log
or surface the DB error so reinsertion of options for $confId does not proceed
on a failed delete.
- Around line 195-203: The check_addsessioncookieprefs() currently counts rows
and can return true for any three rows; fix it by changing the SQL to verify the
three required option values explicitly: use COUNT(DISTINCT co.conf_value) with
a WHERE ... AND co.conf_value IN ('Lax','Strict','None') (or equivalent
normalized values) and compare to = 3, then fetch the row as before; update the
$sql string near the existing $sql/$db->query/$db->fetchRow usage so the
function only returns true when all three distinct expected values exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d6989ef-5be1-4158-a484-c1d9ff23e387
📒 Files selected for processing (2)
htdocs/kernel/session.phpupgrade/upd_2.5.11-to-2.5.12/index.php
Comment on lines
+69
to
92
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| // Auto-detect Secure from protocol; preference can only force it ON, never OFF | ||
| $secure = (XOOPS_PROT === 'https://'); | ||
| if (!empty($xoopsConfig['session_cookie_secure'])) { | ||
| $secure = true; | ||
| } | ||
|
|
||
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; | ||
| } | ||
|
|
||
| $options = [ | ||
| 'lifetime' => $lifetime, | ||
| 'path' => '/', | ||
| 'domain' => $cookieDomain, | ||
| 'secure' => $secure, | ||
| 'httponly' => true, | ||
| 'samesite' => 'Lax', | ||
| 'samesite' => $sameSite, | ||
| ]; | ||
| session_set_cookie_params($options); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add regression tests for SameSite/Secure combinations.
Please add tests for: invalid SameSite fallback to Lax, None forcing Secure=true, and HTTPS auto-detection behavior when session_cookie_secure=0.
Based on learnings: New functionality must have corresponding unit tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/kernel/session.php` around lines 69 - 92, Add unit tests for session
cookie option resolution around the $sameSite and $secure logic in session.php:
write tests that (1) pass an invalid session_cookie_samesite value and assert
$sameSite falls back to 'Lax' and is passed to session_set_cookie_params, (2)
set session_cookie_samesite to 'None' and assert the resulting options have
'secure' === true, and (3) verify HTTPS auto-detection by toggling XOOPS_PROT
between 'https://' and 'http://' with session_cookie_secure set to false/0 and
asserting 'secure' is true only when XOOPS_PROT === 'https://'; call the code
path that builds $options and inspect the array passed to
session_set_cookie_params or provide a hook to return options for assertion.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds configurable session cookie attributes (SameSite + Secure) to XOOPS core, wiring them into new installs and the 2.5.11→2.5.12 upgrade path so admins can control cookie policy via system preferences.
Changes:
- Add an upgrade task to create
session_cookie_samesite/session_cookie_securepreferences (and SameSite options) for existing installs. - Add English admin preference strings for the new settings.
- Update the session handler to derive
samesite/securecookie params from preferences (including forcing Secure when SameSite=None). - Seed the new config rows/options during installation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.5.12/index.php | Adds upgrade task to insert the new config entries and SameSite options. |
| htdocs/modules/system/language/english/admin/preferences.php | Adds preference titles/descriptions for the new session cookie settings. |
| htdocs/kernel/session.php | Uses the new preferences to set session cookie parameters. |
| htdocs/install/include/makedata.php | Seeds the new config rows and configoption values on fresh installs. |
Comment on lines
+217
to
+240
| // Insert SameSite preference (skip if exists) | ||
| $sql = "SELECT conf_id FROM `{$configTable}` WHERE conf_name = 'session_cookie_samesite' AND conf_modid = 0"; | ||
| $result = $db->query($sql); | ||
| $sameSiteRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; | ||
|
|
||
| if (!$sameSiteRow) { | ||
| if (!$db->exec("INSERT INTO `{$configTable}` (conf_modid, conf_catid, conf_name, conf_title, conf_value, conf_desc, conf_formtype, conf_valuetype, conf_order) VALUES (0, 1, 'session_cookie_samesite', '_MD_AM_SESSSAMESITE', 'Lax', '_MD_AM_SESSSAMESITE_DSC', 'select', 'text', 43)")) { | ||
| $this->logs[] = 'Failed to insert session_cookie_samesite config: ' . $db->error(); | ||
| return false; | ||
| } | ||
| // Re-fetch the conf_id | ||
| $result = $db->query($sql); | ||
| $sameSiteRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; | ||
| if (!$sameSiteRow) { | ||
| $this->logs[] = 'Failed to retrieve session_cookie_samesite conf_id after insert'; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Insert Secure preference (skip if exists) | ||
| $sql = "SELECT conf_id FROM `{$configTable}` WHERE conf_name = 'session_cookie_secure' AND conf_modid = 0"; | ||
| $result = $db->query($sql); | ||
| $secureRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; | ||
|
|
|
|
||
| // Add select options for SameSite — delete and recreate to avoid duplicates | ||
| $confId = (int) $sameSiteRow[0]; | ||
| $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}"); |
| $confId = (int) $sameSiteRow[0]; | ||
| $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}"); | ||
| foreach (['Lax', 'Strict', 'None'] as $opt) { | ||
| if (!$db->exec("INSERT INTO `{$optionTable}` (confop_name, confop_value, conf_id) VALUES ('{$opt}', '{$opt}', {$confId})")) { |
Comment on lines
+69
to
91
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| // Auto-detect Secure from protocol; preference can only force it ON, never OFF | ||
| $secure = (XOOPS_PROT === 'https://'); | ||
| if (!empty($xoopsConfig['session_cookie_secure'])) { | ||
| $secure = true; | ||
| } | ||
|
|
||
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; | ||
| } | ||
|
|
||
| $options = [ | ||
| 'lifetime' => $lifetime, | ||
| 'path' => '/', | ||
| 'domain' => $cookieDomain, | ||
| 'secure' => $secure, | ||
| 'httponly' => true, | ||
| 'samesite' => 'Lax', | ||
| 'samesite' => $sameSite, | ||
| ]; |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds configurable session cookie attributes to XOOPS Core 2.5.12, wiring new preferences into installation defaults, the 2.5.11→2.5.12 upgrade path, and the runtime session cookie configuration.
Changes:
- Add new core preferences for
session_cookie_samesiteandsession_cookie_secure(installer seed + upgrade task). - Update session handler to apply SameSite/Secure cookie params based on preferences with sensible defaults.
- Add English admin preference labels/descriptions for the new settings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.5.12/index.php | Adds an upgrade task to create the new config rows/options for existing installations. |
| htdocs/modules/system/language/english/admin/preferences.php | Adds language constants for the new session cookie preferences. |
| htdocs/kernel/session.php | Applies SameSite/Secure cookie params from XOOPS preferences when configuring session cookies. |
| htdocs/install/include/makedata.php | Seeds fresh installs with the new config rows and SameSite select options. |
Comment on lines
+218
to
+220
| $sql = "SELECT conf_id FROM `{$configTable}` WHERE conf_name = 'session_cookie_samesite' AND conf_modid = 0"; | ||
| $result = $db->query($sql); | ||
| $sameSiteRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; |
Comment on lines
+237
to
+239
| $sql = "SELECT conf_id FROM `{$configTable}` WHERE conf_name = 'session_cookie_secure' AND conf_modid = 0"; | ||
| $result = $db->query($sql); | ||
| $secureRow = ($db->isResultSet($result) && ($result instanceof \mysqli_result)) ? $db->fetchRow($result) : false; |
Comment on lines
+194
to
+203
| // Check SameSite options exist (Lax, Strict, None) | ||
| $sql = "SELECT COUNT(*) FROM `" . $db->prefix('configoption') . "` co" | ||
| . " INNER JOIN `" . $db->prefix('config') . "` c ON co.conf_id = c.conf_id" | ||
| . " WHERE c.conf_name = 'session_cookie_samesite' AND c.conf_modid = 0"; | ||
| $result = $db->query($sql); | ||
| if (!$db->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| return false; | ||
| } | ||
| $row = $db->fetchRow($result); | ||
| return $row && (int) $row[0] >= 3; |
|
|
||
| // Add select options for SameSite — delete and recreate to avoid duplicates | ||
| $confId = (int) $sameSiteRow[0]; | ||
| $db->exec("DELETE FROM `{$optionTable}` WHERE conf_id = {$confId}"); |
Comment on lines
+79
to
+81
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; |
Comment on lines
+69
to
+82
| // Resolve SameSite and Secure from XOOPS config preferences | ||
| $sameSite = isset($xoopsConfig['session_cookie_samesite']) ? (string) $xoopsConfig['session_cookie_samesite'] : 'Lax'; | ||
| $sameSite = in_array($sameSite, ['Lax', 'Strict', 'None'], true) ? $sameSite : 'Lax'; | ||
|
|
||
| // Auto-detect Secure from protocol; preference can only force it ON, never OFF | ||
| $secure = (XOOPS_PROT === 'https://'); | ||
| if (!empty($xoopsConfig['session_cookie_secure'])) { | ||
| $secure = true; | ||
| } | ||
|
|
||
| // Browsers require Secure when SameSite=None | ||
| if ($sameSite === 'None') { | ||
| $secure = true; | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1660 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Fixes #1501. Ports the session cookie preferences from the 259world development branch. The SameSite attribute was hardcoded to 'Lax' since PR #1488 —
now configurable via Admin > Preferences.
Changes:
Summary by CodeRabbit
New Features
Documentation
Chores