Code-review findings: imageUrls() SSRF, undeclared Change::$changeIndex, two concurrency races, a deprecated service locator

[ojura]

These five came up while I was tracking down the local-storage "Loading document" stall (fixed in #379). They're independent of that PR, so I'm filing them together rather than smuggling them in. Line references are against current master, and each one says whether I confirmed it directly or only read the mechanism off the code. A proposed patch (or, where the fix involves a real design choice, a proposed direction) is in the expandable block under each.

1. SSRF: imageUrls() fetches arbitrary client-supplied URLs (confirmed)

In lib/XHRCommand/OpenDocument.php, imageUrls() (around line 139) does fopen($inputUrl, 'r') where $inputUrl comes straight off the client command ($command['data']), with no check on scheme or host. So a client can point the server at internal services, the cloud metadata endpoint (169.254.169.254), or file:// paths and have it fetch them.

Suggested direction

I'm deliberately not dropping a finished diff here, because a half-good SSRF filter is worse than an obvious hole: it has to survive redirects, DNS rebinding, and IPv6, which is more than a one-liner. The baseline is to reject anything whose scheme is not http/https, resolve the host, and refuse it if any resolved address is private, loopback, or link-local (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) for each A/AAAA record). For real safety the fetch should also pin to the validated IP (so a second DNS lookup can't rebind) and disable redirect-following, which is easier with an HTTP client than with bare fopen. An allowlist of expected hosts, if the deployment can define one, sidesteps all of it.

2. Latent TypeError: Change::getChangeIndex() reads an undeclared property (confirmed)

In lib/Document/Change.php:61, getChangeIndex(): int returns $this->changeIndex, but the class only declares documentId, time, change, user, userOriginal, and neither the constructor nor fromRow() ever sets changeIndex. Any call would return null from an int-typed method (a TypeError), plus a dynamic-property deprecation on PHP 8.4+. Nothing calls it today, so it's latent, but ChangeStore does persist a change_index column, which makes the getter look intended-but-never-wired-up rather than simply dead.

Patch (declare and populate it from the row)

diff --git a/lib/Document/Change.php b/lib/Document/Change.php
--- a/lib/Document/Change.php
+++ b/lib/Document/Change.php
@@ -29,13 +29,15 @@ class Change {
 	private $change;
 	private $user;
 	private $userOriginal;
+	private $changeIndex;
 
-	public function __construct(int $documentId, int $time, string $change, string $user, string $userOriginal) {
+	public function __construct(int $documentId, int $time, string $change, string $user, string $userOriginal, int $changeIndex = 0) {
 		$this->documentId = $documentId;
 		$this->time = $time;
 		$this->change = $change;
 		$this->user = $user;
 		$this->userOriginal = $userOriginal;
+		$this->changeIndex = $changeIndex;
 	}
 
 	public function getDocumentId(): int {
@@ -78,7 +80,8 @@ class Change {
 			(int)$row['time'],
 			$row['change'],
 			$row['user'],
-			$row['user_original']
+			$row['user_original'],
+			(int)($row['change_index'] ?? 0)
 		);
 	}
 }

(If the getter is genuinely unused and not wanted, deleting it is the smaller change.)

3. Concurrency: colliding change_index under concurrent editors (mechanism clear, not reproduced)

In lib/Document/ChangeStore.php, addChangesForDocument() computes $changeIndex = getMaxChangeIndexForDocument($documentId) + 1 before it calls beginTransaction(), and nothing serializes that read against other writers. Two co-authors saving at the same moment can read the same max and write duplicate change_index values for one document.

Suggested direction

This one needs a schema change, so I'm not inlining a half-migration. The robust fix is a unique index on (document_id, change_index) plus a retry on the constraint violation, which also documents the invariant. Alternatively, take the max inside the transaction with SELECT ... FOR UPDATE (so concurrent writers block rather than race), or move to a per-document atomic counter. The migration is the part that needs care and a maintainer's call on the table, which is why this is a direction rather than a diff.

4. Concurrency: MemcacheIPCBackend can drop a message on publish (mechanism clear, not reproduced; memcache backend only)

In lib/IPC/MemcacheIPCBackend.php:49, pushMessage() does inc(write_key) and then, separately, set(message_$key). If a popMessage() lands in between, it atomically incs read_key to that same index, sees write_key >= read_key, and reads message_$key before it has been written, so it returns null while consuming the slot and the in-flight message is lost.

Suggested direction

The tempting one-line fix (write the payload before incrementing write_key) closes this writer/reader race but opens a writer/writer one: two writers reading the same write_key would compute the same next slot and one would clobber the other, whereas the current inc-first gives each writer a unique slot. So there isn't a clean reorder; a correct fix wants a genuinely atomic publish. Easiest is to lean on a primitive that has one (this is exactly why the Redis backend's rPush/blPop is race-free), e.g. append the message under the incremented key in a single atomic step, or guard the publish/consume pair with a short lock. The Redis and database backends take separate code paths and aren't affected, so this is memcache-only.

5. Deprecation: service locator in getCommandDispatcher() (confirmed)

In lib/Controller/DocumentController.php, getCommandDispatcher() resolves the command and idle handlers through \OC::$server->query($class), which is the deprecated service-locator pattern.

Patch (inject the container and use ContainerInterface::get)

diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php
--- a/lib/Controller/DocumentController.php
+++ b/lib/Controller/DocumentController.php
@@ -50,6 +50,7 @@ use OCP\AppFramework\Http\Attribute\PublicPage;
 use OCP\AppFramework\Http\Response;
 use OCP\IRequest;
 use OCP\IURLGenerator;
+use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 
 class DocumentController extends Controller {
@@ -77,6 +78,7 @@ class DocumentController extends Controller {
 	private $ipcFactory;
 	private $sessionManager;
 	private LoggerInterface $logger;
+	private ContainerInterface $container;
 
 	public function __construct(
 		$appName,
@@ -88,7 +90,8 @@ class DocumentController extends Controller {
 		WebVersion $webVersion,
 		IIPCFactory $ipcFactory,
 		SessionManager $sessionManager,
-		LoggerInterface $logger
+		LoggerInterface $logger,
+		ContainerInterface $container
 	) {
 		parent::__construct($appName, $request);
 
@@ -100,15 +103,16 @@ class DocumentController extends Controller {
 		$this->ipcFactory = $ipcFactory;
 		$this->sessionManager = $sessionManager;
 		$this->logger = $logger;
+		$this->container = $container;
 	}
 
 	private function getCommandDispatcher(): CommandDispatcher {
 		$dispatcher = new CommandDispatcher();
 		foreach (self::COMMAND_HANDLERS as $class) {
-			$dispatcher->addHandler(\OC::$server->query($class));
+			$dispatcher->addHandler($this->container->get($class));
 		}
 		foreach (self::IDLE_HANDLERS as $class) {
-			$dispatcher->addIdleHandler(\OC::$server->query($class));
+			$dispatcher->addIdleHandler($this->container->get($class));
 		}
 		return $dispatcher;
 	}

Constructor-injecting each handler directly would be cleaner still, just more invasive; this is the minimal swap off the deprecated static locator.