Repository navigation
Strip the NOT_SERIALIZED marker in the session meta getters - #242
Conversation
…ters
HordeSession stores strings with a leading NOT_SERIALIZED byte ("\0").
getScoped() removes it, but getAuthenticatedUser(), getAuthId(),
getBrowserFingerprint() and getRemoteAddress() read $this->data raw and
returned the marker with the value, e.g. "\0jdoe" instead of "jdoe".
Every caller uses the result as a user id for preference lookups (topbar,
sidebar, access keys, view mode, theme), so on modern-stack pages no user
preference was ever found and the server defaults applied instead.
Read them through getScoped() like any other scoped value.
There was a problem hiding this comment.
🟢 Approval recommended
The changes correctly reuse existing decoding logic, with only a non-blocking regression-test suggestion remaining.
1 open finding
What changed in this PR
Corrects session metadata decoding so modern-stack preference lookups receive user IDs without the storage marker.
Changes:
- Routes four string metadata getters through
getScoped(). - Leaves integer timestamp handling unchanged.
| File | Description |
|---|---|
src/Session/HordeSession.php |
Decodes stored user IDs, browser fingerprints, and remote addresses through the existing scoped reader. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| public function getAuthenticatedUser(): ?string | ||
| { | ||
| $value = $this->data['horde']['auth/userId'] ?? null; | ||
| $value = $this->getScoped('horde', 'auth/userId'); |
There was a problem hiding this comment.
Agreed, the existing tests seed unprefixed strings and passed with the bug. Added regression coverage in b6d6c18: a data provider over the four getters, values written through setScoped(), checked on the writing session and after a toPayload() round trip.
PHPUnit isn't available on my setup, so I replayed the same assertions in a standalone script against the installed dependencies: all 8 cases fail with the original class (each getter returns the value with a leading \0) and pass with the fix. CI will have the final word.
The existing getter tests seed the session with unprefixed strings, so they passed with the NOT_SERIALIZED marker leaking out. Write the values through setScoped() instead, and check each getter both on the writing session and after a round trip through toPayload().
HordeSessionstores string values with a leadingNOT_SERIALIZEDbyte(
"\0").getScoped()strips it, but the four session-meta getters read$this->datadirectly and return the marker along with the value:getAuthenticatedUser()/getAuthId()"\0jdoe"instead of"jdoe"getBrowserFingerprint(),getRemoteAddress()\0Impact
Every caller in Core uses the result as the user id for a preference lookup
(
TopbarBuilder,SidebarBuilder,AccessKeyTrackerFactory,ViewModeConfigurator,RenderingModeResolver,DesktopChromeRendererFactory).With the marker, no stored preference is ever found on modern-stack pages
and the server defaults silently apply instead — the user's theme, the
access-key preference, and so on.
Found while testing #233: probing the theme resolution in a real request
showed it querying preferences for
"\0pfard".Change
Read these four values through
getScoped('horde', …), like any otherscoped value.
getAuthTimestamp()is left alone: integers are stored raw.None of the callers compares these values to raw session data or uses them
for authentication decisions, so removing the marker cannot change who is
considered logged in.
Testing
On a FRAMEWORK_6_0 install, with the fix applied:
preference is read again (
accesskeys.jsgets loaded), and the user'stheme is resolved instead of the server default (together with the
companion theme-assets change);
Login and logout were not specifically exercised.