Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Session/HordeSession.php
Original file line number Diff line number Diff line change
Expand Up @@ -306,28 +306,28 @@ public function clearScopeWithPrefixes(string $app, array $prefixes): void

public function getAuthenticatedUser(): ?string
{
$value = $this->data['horde']['auth/userId'] ?? null;
$value = $this->getScoped('horde', 'auth/userId');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return is_string($value) ? $value : null;
}

public function getAuthId(): ?string
{
$value = $this->data['horde']['auth/authId'] ?? null;
$value = $this->getScoped('horde', 'auth/authId');

return is_string($value) ? $value : null;
}

public function getBrowserFingerprint(): ?string
{
$value = $this->data['horde']['auth/browser'] ?? null;
$value = $this->getScoped('horde', 'auth/browser');

return is_string($value) ? $value : null;
}

public function getRemoteAddress(): ?string
{
$value = $this->data['horde']['auth/remoteAddr'] ?? null;
$value = $this->getScoped('horde', 'auth/remoteAddr');

return is_string($value) ? $value : null;
}
Expand Down
42 changes: 42 additions & 0 deletions test/Unit/Session/HordeSessionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Horde\SessionHandler\Session;
use Horde\SessionHandler\SessionId;
use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\TestCase;
use Horde_Pack;
Expand Down Expand Up @@ -221,6 +222,47 @@ public function testGetRemoteAddress(): void
self::assertSame('192.168.1.1', $session->getRemoteAddress());
}

/**
* Strings written through setScoped() carry the NOT_SERIALIZED marker
* on the wire. The meta getters must return them without it, both on
* the writing session and after a round trip through toPayload().
* The tests above seed $data with unprefixed strings, so they did not
* catch the marker leaking out.
*
* @return array<string, array{string, string, string}>
*/
public static function metaGetterProvider(): array
{
return [
'authenticated user' => ['auth/userId', 'getAuthenticatedUser', 'alice'],
'auth id' => ['auth/authId', 'getAuthId', 'alice@example.com'],
'browser fingerprint' => ['auth/browser', 'getBrowserFingerprint', 'Mozilla/5.0'],
'remote address' => ['auth/remoteAddr', 'getRemoteAddress', '192.168.1.1'],
];
}

#[Test]
#[DataProvider('metaGetterProvider')]
public function testMetaGetterStripsStorageMarker(string $key, string $getter, string $value): void
{
$session = $this->createSession();
$session->setScoped('horde', $key, $value);

self::assertSame($value, $session->{$getter}());
}

#[Test]
#[DataProvider('metaGetterProvider')]
public function testMetaGetterStripsStorageMarkerAfterPayloadRoundTrip(string $key, string $getter, string $value): void
{
$writer = $this->createSession();
$writer->setScoped('horde', $key, $value);

$reader = $this->createSession($writer->toPayload());

self::assertSame($value, $reader->{$getter}());
}

#[Test]
public function testGetAuthTimestamp(): void
{
Expand Down
Loading