fix: user enumeration in guest registration endpoint (CVE-2025-59716)

[thesmartshadow]

This PR fixes an unauthenticated user enumeration issue in the guest registration endpoint
(/apps/guests/register/{email}/{token}).

Previously, the password form could reveal whether a pending guest account existed by returning
different error messages depending on whether the target user was a guest. This allowed account
enumeration.

Changes:

• Avoid account enumeration by returning a generic "The token is invalid" message for any invalid
  registration link (non-guest, missing token, token mismatch).
• Validate the provided token against the stored registerToken using hash_equals() (constant-time).
• Keep email format validation behavior unchanged.
• Also harden token comparison in the POST registration handler.

CVE: CVE-2025-59716
Reported by: Ali Firas (thesmartshadow)

[CLAassistant]

All committers have signed the CLA.

[thesmartshadow]

Thanks for the review!

It looks like only the coding-standard (phpcs) check is failing.
The change is intentionally minimal and security-focused, but I’m happy
to adjust formatting if you point out the specific style issue.

Thanks!

[thesmartshadow]

Hi, @phil-davis CI failure seems unrelated to this PR’s change (only touches lib/Controller/RegisterController.php).

Failing scenario:
webUIGuests/guests.feature:212
“check blocked domains set from command line for guests in webUI”
Expected blocked domains value “something.com,qwerty.org,example.gov” but got.

The following scenario (set via webUI then verify via occ) passes, so this looks like a timing/cache flake in webUI acceptance tests.
Could you please rerun the Drone pipeline (or advise if there’s a known flakiness workaround)?
Thanks!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[DeepDiver1975]

Verdict: fix correctly closes the primary enumeration. Two non-blocking gaps + missing changelog/test.

The leak: pre-fix showPasswordForm() returned No such guest user for a non-guest vs The token is invalid for a guest with a bad/missing token — an observable difference that enumerated accounts. The fix collapses every invalid case (non-guest, missing token, mismatch) to one generic The token is invalid and switches to constant-time hash_equals. Response is now uniform for existing vs non-existing accounts — enumeration via the GET form is closed. Good, minimal change.

Confirmed correct:

• hash_equals($registerToken, (string)$token) — arg order right (known/stored first, user-supplied second), both cast to string, guarded against empty args so no warning. (RegisterController.php ~L158)
• getUserValue defaults normalized to 0 / ''; DB lookups for isGuest and registerToken now run in every valid-email branch, so the two-query path is uniform.
• POST register() also hardened: a non-guest has no registerToken → $valid=false → generic error; setPassword only reached with a matching token. No regression vs base.

Gaps (non-blocking, worth a line each):

1. Residual timing delta: hash_equals runs only when the account exists AND has a token; for a non-existent account it's skipped. The response body is uniform (the real fix), so this is a microsecond-scale side-channel, not the described vector — but the PR text says it "validates the token using hash_equals (constant-time)", which slightly overstates uniformity. Fine to leave; just don't claim full timing-uniformity.
2. register() catch block still echoes $e->getMessage() into messages['password'] (RegisterController.php ~L245). Pre-existing and only reachable after a valid token, so not an enumeration vector — but it leaks internal exception text. Out of scope here; noting for awareness.

Requested changes:

• Changelog missing. owncloud/guests uses a plain CHANGELOG.md (no calens changelog/ dir). The ## [Unreleased] section is empty and this PR adds nothing. Please add under ## [Unreleased] a ### Fixed (create it) entry, e.g.:
  - [#661](https://github.com/owncloud/guests/pull/661) - fix: user enumeration in guest registration endpoint (CVE-2025-59716)
• No test. tests/unit has no RegisterControllerTest. A security fix that asserts a security property should be pinned by a test: same generic response + status for (a) non-guest email, (b) guest with wrong token, (c) unknown email — verifying no No such guest user message and identical output. Please add one so this can't regress silently.

CI is green. Nice, focused fix — adding the changelog entry and a regression test would make it complete.

🤖 Generated with Claude Code

[dj4oC]

Independently verified (not relying on the existing review's claims): fixes user enumeration (CVE-2025-59716 — A01:2021 Broken Access Control / CWE-204 Observable Response Discrepancy) in RegisterController::showPasswordForm() and register(). Pre-fix, a non-guest email produced No such guest user while a guest with a bad token produced The token is invalid — an oracle for account existence. Post-fix, every invalid case (non-guest, missing/empty token, mismatched token) collapses to the same generic The token is invalid message, and comparison uses hash_equals() instead of !==.

Findings

Security: Confirmed fix is correct — traced both showPasswordForm() and register() against getUserValue() default values (0/'') and the hash_equals($registerToken, (string)$token) argument order (stored value first, user-supplied second, both cast to string, guarded against empty-string args to avoid a hash_equals() warning). No remaining response-content divergence between existing/non-existing accounts.

Test coverage — blocking: tests/unit/ has no test for RegisterController at all (confirmed via get_files — this PR touches only lib/Controller/RegisterController.php, 0 test files). Per policy, a security fix needs a regression test asserting the security property itself, not just the happy path. Proposed test (matches this repo's existing convention in tests/unit/GroupBackendTest.php — Test\TestCase base, createMock):

<?php
namespace OCA\Guests\Tests\Unit\Controller;

use OCA\Guests\Controller\RegisterController;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Mail\IMailer;
use OCP\Security\ISecureRandom;
use Test\TestCase;

class RegisterControllerTest extends TestCase {
	private $controller;
	private $config;
	private $mailer;

	public function setUp(): void {
		parent::setUp();
		$this->config = $this->createMock(IConfig::class);
		$this->mailer = $this->createMock(IMailer::class);
		$this->mailer->method('validateMailAddress')->willReturn(true);
		$l10n = $this->createMock(IL10N::class);
		$l10n->method('t')->willReturnCallback(function ($text) {
			return $text;
		});
		$this->controller = new RegisterController(
			'guests',
			$this->createMock(IRequest::class),
			$this->createMock(IUserManager::class),
			$this->createMock(IGroupManager::class),
			$l10n,
			$this->config,
			$this->createMock(ISecureRandom::class),
			$this->mailer,
			$this->createMock(IURLGenerator::class)
		);
	}

	private function renderedMessages($email, $token) {
		$response = $this->controller->showPasswordForm($email, $token);
		return $response->getParams()['messages'];
	}

	public function testNonGuestAndBadTokenProduceIdenticalGenericMessage() {
		// non-guest, no token stored
		$this->config->method('getUserValue')->willReturn(0);
		$nonGuestMessages = $this->renderedMessages('nobody@example.com', 'anytoken');

		// guest, wrong token stored
		$this->config = $this->createMock(IConfig::class);
		$this->config->method('getUserValue')->willReturnMap([
			['guest@example.com', 'owncloud', 'isGuest', 0, true],
			['guest@example.com', 'guests', 'registerToken', '', 'correct-token'],
		]);
		$badTokenController = new RegisterController(
			'guests',
			$this->createMock(IRequest::class),
			$this->createMock(IUserManager::class),
			$this->createMock(IGroupManager::class),
			$this->createMock(IL10N::class),
			$this->config,
			$this->createMock(ISecureRandom::class),
			$this->mailer,
			$this->createMock(IURLGenerator::class)
		);
		$badTokenMessages = $badTokenController->showPasswordForm('guest@example.com', 'wrong-token');

		self::assertArrayNotHasKey('username', $nonGuestMessages);
		self::assertSame(['token'], \array_keys($nonGuestMessages));
		self::assertSame(\array_keys($nonGuestMessages), \array_keys($badTokenMessages->getParams()['messages']));
	}
}

(Adjust mocks to this repo's actual PHPUnit/mock conventions as needed — the point is asserting identical messages shape for non-guest vs. wrong-token vs. unknown-email, which is the actual security property being fixed.)

Documentation — blocking per compliance checklist: No changelog entry. CHANGELOG.md's ## [Unreleased] section is untouched. Please add:

### Fixed

- [#661](https://github.com/owncloud/guests/pull/661) - fix: user enumeration in guest registration endpoint (CVE-2025-59716)

Verdict

Changes requested — not for the security fix itself (which is correct), but the coverage and changelog gates aren't met yet.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code