diff --git a/apps/files_external/lib/Lib/Storage/SFTPReflection.php b/apps/files_external/lib/Lib/Storage/SFTPReflection.php index 830d28351dd24..a60c290867374 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReflection.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReflection.php @@ -32,7 +32,7 @@ trait SFTPReflection { /** * Invoke a method that is private in phpseclib v3. */ - private function invokeSftp(SFTP $sftp, string $method, array $arguments = []): mixed { + protected function invokeSftp(SFTP $sftp, string $method, array $arguments = []): mixed { self::$sftpReflectionMethods[$method] ??= new \ReflectionMethod(SFTP::class, $method); return self::$sftpReflectionMethods[$method]->invokeArgs($sftp, $arguments); } @@ -40,7 +40,7 @@ private function invokeSftp(SFTP $sftp, string $method, array $arguments = []): /** * Read a property that is private/protected in phpseclib v3. */ - private function getSftpProperty(SFTP $sftp, string $property): mixed { + protected function getSftpProperty(SFTP $sftp, string $property): mixed { self::$sftpReflectionProperties[$property] ??= new \ReflectionProperty(SFTP::class, $property); return self::$sftpReflectionProperties[$property]->getValue($sftp); } diff --git a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php index d9e63650b2ab4..9eb4dc67e31d1 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php @@ -37,6 +37,27 @@ class SFTPWriteStream implements File { private string $path; + /** Payload budget for a single SSH_FXP_WRITE packet */ + private int $packetSize = 0; + + /** Number of SSH_FXP_WRITE packets sent but not yet acknowledged */ + private int $outstanding = 0; + + /** Mirrors phpseclib's NET_SFTP_UPLOAD_QUEUE_SIZE */ + private const int QUEUE_SIZE = 1024; + + /** 2^32, used to split a 64-bit file offset into its high and low 32-bit words */ + private const int UINT32_MODULUS = 2 ** 32; + + /** Default SFTP payload size (32 KiB) used when the negotiated maximum is unavailable */ + private const int DEFAULT_MAX_PACKET_SIZE = 1 << 15; + + /** Bytes reserved in an SSH_FXP_WRITE packet for the handle-length, offset and data-length fields */ + private const int WRITE_PACKET_OVERHEAD = 25; + + /** Length of the 32-bit "string" length prefix preceding the handle in an SSH_FXP_HANDLE response */ + private const int STRING_LENGTH_PREFIX = 4; + public static function register($protocol = 'sftpwrite') { if (in_array($protocol, stream_get_wrappers(), true)) { return false; @@ -77,11 +98,10 @@ public function stream_open($path, $mode, $options, &$opened_path) { } $remote_file = $this->sftp->realpath($path); - - $this->path = $remote_file; if ($remote_file === false) { return false; } + $this->path = $remote_file; $packet = pack('Na*N2', strlen($remote_file), $remote_file, NET_SFTP_OPEN_WRITE | NET_SFTP_OPEN_CREATE | NET_SFTP_OPEN_TRUNCATE, 0); try { @@ -93,7 +113,7 @@ public function stream_open($path, $mode, $options, &$opened_path) { $response = $this->invokeSftp($this->sftp, 'get_sftp_packet'); switch ($this->getSftpProperty($this->sftp, 'packet_type')) { case NET_SFTP_HANDLE: - $this->handle = substr($response, 4); + $this->handle = substr($response, self::STRING_LENGTH_PREFIX); break; case NET_SFTP_STATUS: // presumably SSH_FX_NO_SUCH_FILE or SSH_FX_PERMISSION_DENIED $this->invokeSftp($this->sftp, 'logError', [$response]); @@ -103,6 +123,11 @@ public function stream_open($path, $mode, $options, &$opened_path) { return false; } + // Size each SSH_FXP_WRITE to the negotiated maximum packet, leaving room + // for the handle and the packet header (mirrors phpseclib's SFTP::put()). + $maxPacket = $this->getSftpProperty($this->sftp, 'max_sftp_packet') ?: self::DEFAULT_MAX_PACKET_SIZE; + $this->packetSize = max(1, $maxPacket - strlen($this->handle) - self::WRITE_PACKET_OVERHEAD); + return true; } @@ -128,15 +153,55 @@ public function stream_write($data) { $this->buffer .= $data; - if (strlen($this->buffer) > 64 * 1024) { - if (!$this->stream_flush()) { + // Send full packets as soon as enough data is buffered, without waiting + // for each acknowledgement, so the upload stays pipelined. + while (strlen($this->buffer) >= $this->packetSize) { + if (!$this->sendChunk(substr($this->buffer, 0, $this->packetSize))) { return false; } + $this->buffer = substr($this->buffer, $this->packetSize); } return $written; } + /** + * Send a single SSH_FXP_WRITE packet without waiting for its response. + * + * Acknowledgements are only drained once the queue is full or the stream is + * flushed/closed. Blocking on every packet would turn each write into a full + * round trip, which is what made the previous implementation slow. + */ + private function sendChunk(string $chunk): bool { + $size = strlen($chunk); + $packet = pack('Na*N3a*', strlen($this->handle), $this->handle, $this->internalPosition / self::UINT32_MODULUS, $this->internalPosition, $size, $chunk); + try { + $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_WRITE, $packet]); + } catch (\Throwable) { + return false; + } + $this->internalPosition += $size; + $this->outstanding++; + + if ($this->outstanding >= self::QUEUE_SIZE) { + return $this->drainResponses(); + } + + return true; + } + + /** + * Read the acknowledgements for all packets sent since the last drain. + */ + private function drainResponses(): bool { + if ($this->outstanding === 0) { + return true; + } + $result = $this->invokeSftp($this->sftp, 'read_put_responses', [$this->outstanding]); + $this->outstanding = 0; + return $result; + } + #[\Override] public function stream_set_option($option, $arg1, $arg2) { return false; @@ -159,17 +224,15 @@ public function stream_lock($operation) { #[\Override] public function stream_flush() { - $size = strlen($this->buffer); - $packet = pack('Na*N3a*', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size, $this->buffer); - try { - $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_WRITE, $packet]); - } catch (\Throwable) { - return false; + // Flush the trailing partial packet, then wait for all outstanding writes. + if ($this->buffer !== '') { + if (!$this->sendChunk($this->buffer)) { + return false; + } + $this->buffer = ''; } - $this->internalPosition += $size; - $this->buffer = ''; - return $this->invokeSftp($this->sftp, 'read_put_responses', [1]); + return $this->drainResponses(); } #[\Override] diff --git a/apps/files_external/tests/Storage/SFTPWriteStreamTest.php b/apps/files_external/tests/Storage/SFTPWriteStreamTest.php new file mode 100644 index 0000000000000..19453b907cd94 --- /dev/null +++ b/apps/files_external/tests/Storage/SFTPWriteStreamTest.php @@ -0,0 +1,374 @@ +createMock(SFTP::class); + $sftp->method('realpath')->willReturnCallback(fn ($p) => $state->realpath ?? $p); + $sftp->method('touch')->willReturnCallback(function (...$args) use ($state): bool { + $state->touchArgs = $args; + return true; + }); + + $stream = $this->getMockBuilder(SFTPWriteStream::class) + ->onlyMethods(['invokeSftp', 'getSftpProperty']) + ->getMock(); + + $stream->method('getSftpProperty')->willReturnCallback(fn ($s, $prop) => match ($prop) { + 'bitmap' => $state->bitmap, + 'packet_type' => $state->packetType, + 'max_sftp_packet' => $state->maxPacket, + default => null, + }); + + $stream->method('invokeSftp')->willReturnCallback(function ($s, $method, $args = []) use ($state) { + switch ($method) { + case 'send_sftp_packet': + $state->sent[] = ['type' => $args[0], 'packet' => $args[1]]; + if ($state->sendThrowAfter !== null && count($state->sent) > $state->sendThrowAfter) { + throw new \RuntimeException('send failed'); + } + return null; + case 'get_sftp_packet': + $resp = array_shift($state->getResponses); + $state->packetType = $resp['type']; + return $resp['data']; + case 'read_put_responses': + $state->readPut[] = $args[0]; + return $state->readPutReturn; + case 'close_handle': + $state->closeHandleCalled = true; + return $state->closeHandleReturn; + case 'logError': + $state->logError[] = $args[0]; + return null; + } + return null; + }); + + $stream->context = stream_context_create(['sftp' => ['session' => $sftp]]); + return $stream; + } + + /** + * Default "happy path" fake-server state: logged in, OPEN returns a handle. + */ + private function defaultState(int $maxPacket = 32768, string $handle = 'handle'): \stdClass { + $state = new \stdClass(); + $state->bitmap = SSH2::MASK_LOGIN; + $state->packetType = null; + $state->maxPacket = $maxPacket; + $state->realpath = null; + $state->sent = []; + $state->getResponses = [['type' => NET_SFTP_HANDLE, 'data' => pack('N', strlen($handle)) . $handle]]; + $state->readPut = []; + $state->readPutReturn = true; + $state->closeHandleCalled = false; + $state->closeHandleReturn = true; + $state->logError = []; + $state->sendThrowAfter = null; + $state->touchArgs = null; + return $state; + } + + /** @return list */ + private function writePackets(\stdClass $state): array { + return array_values(array_filter($state->sent, fn ($p) => $p['type'] === NET_SFTP_WRITE)); + } + + private function assertWritePacket(string $packet, string $handle, int $offset, string $data): void { + $handleLen = unpack('N', substr($packet, 0, 4))[1]; + $pos = 4; + $this->assertSame($handle, substr($packet, $pos, $handleLen)); + $pos += $handleLen; + $offHigh = unpack('N', substr($packet, $pos, 4))[1]; + $offLow = unpack('N', substr($packet, $pos + 4, 4))[1]; + $dataLen = unpack('N', substr($packet, $pos + 8, 4))[1]; + $pos += 12; + $this->assertSame($offset, $offHigh * (2 ** 32) + $offLow, 'offset'); + $this->assertSame($data, substr($packet, $pos, $dataLen), 'data'); + } + + private function openStream(\stdClass $state): SFTPWriteStream&MockObject { + $stream = $this->buildStream($state); + $opened = ''; + $this->assertTrue($stream->stream_open('sftpwrite://upload/file.txt', 'w', 0, $opened)); + return $stream; + } + + // --- register() --------------------------------------------------------- + + public function testRegisterAndDuplicate(): void { + $proto = 'sftpwritetest' . bin2hex(random_bytes(4)); + $this->assertNotFalse(SFTPWriteStream::register($proto)); + $this->assertFalse(SFTPWriteStream::register($proto)); + } + + // --- loadContext -------------------------------------------------------- + + public function testStreamOpenThrowsWhenContextOptionMissing(): void { + $stream = $this->getMockBuilder(SFTPWriteStream::class) + ->onlyMethods(['invokeSftp', 'getSftpProperty']) + ->getMock(); + $stream->context = stream_context_create([]); + + $this->expectException(\BadMethodCallException::class); + $opened = ''; + $stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened); + } + + public function testStreamOpenThrowsWhenSessionMissing(): void { + $stream = $this->getMockBuilder(SFTPWriteStream::class) + ->onlyMethods(['invokeSftp', 'getSftpProperty']) + ->getMock(); + $stream->context = stream_context_create(['sftp' => ['size' => 1]]); + + $this->expectException(\BadMethodCallException::class); + $opened = ''; + $stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened); + } + + // --- stream_open -------------------------------------------------------- + + public function testStreamOpenSuccessSendsOpenPacket(): void { + $state = $this->defaultState(); + $this->openStream($state); + + $this->assertCount(1, $state->sent); + $this->assertSame(NET_SFTP_OPEN, $state->sent[0]['type']); + } + + public function testStreamOpenFailsWhenNotLoggedIn(): void { + $state = $this->defaultState(); + $state->bitmap = 0; + $stream = $this->buildStream($state); + + $opened = ''; + $this->assertFalse($stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened)); + $this->assertSame([], $state->sent); + } + + public function testStreamOpenFailsWhenRealpathFails(): void { + $state = $this->defaultState(); + $state->realpath = false; + $stream = $this->buildStream($state); + + $opened = ''; + $this->assertFalse($stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened)); + $this->assertSame([], $state->sent); + } + + public function testStreamOpenReturnsFalseWhenSendThrows(): void { + $state = $this->defaultState(); + $state->sendThrowAfter = 0; // throw on the first packet (the OPEN) + $stream = $this->buildStream($state); + + $opened = ''; + $this->assertFalse($stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened)); + } + + public function testStreamOpenLogsErrorOnStatusResponse(): void { + $state = $this->defaultState(); + $state->getResponses = [['type' => NET_SFTP_STATUS, 'data' => 'permission denied']]; + $stream = $this->buildStream($state); + + $opened = ''; + $this->assertFalse($stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened)); + $this->assertSame(['permission denied'], $state->logError); + } + + public function testStreamOpenReturnsFalseOnUnexpectedResponse(): void { + $state = $this->defaultState(); + $state->getResponses = [['type' => 0x7f, 'data' => 'garbage']]; + $stream = $this->buildStream($state); + + set_error_handler(static fn () => true); // swallow the expected E_USER_NOTICE + try { + $opened = ''; + $this->assertFalse($stream->stream_open('sftpwrite://upload/f', 'w', 0, $opened)); + } finally { + restore_error_handler(); + } + } + + // --- stream_write / pipelining ----------------------------------------- + + public function testStreamWriteBuffersBelowPacketSize(): void { + $state = $this->defaultState(); // packetSize = 32768 - 6 - 25 + $stream = $this->openStream($state); + + $this->assertSame(5, $stream->stream_write('hello')); + $this->assertSame([], $this->writePackets($state)); + $this->assertSame(5, $stream->stream_tell()); + } + + public function testStreamWriteSendsFullPacketsWithCorrectFraming(): void { + $state = $this->defaultState(40); // packetSize = 40 - 6 - 25 = 9 + $stream = $this->openStream($state); + + // 13 bytes -> one full 9-byte packet, 4 bytes buffered + $this->assertSame(13, $stream->stream_write('0123456789ABC')); + $writes = $this->writePackets($state); + $this->assertCount(1, $writes); + $this->assertWritePacket($writes[0]['packet'], 'handle', 0, '012345678'); + + // flushing sends the buffered remainder at the advanced offset + $this->assertTrue($stream->stream_flush()); + $writes = $this->writePackets($state); + $this->assertCount(2, $writes); + $this->assertWritePacket($writes[1]['packet'], 'handle', 9, '9ABC'); + } + + public function testWritesAreNotAcknowledgedUntilFlush(): void { + $state = $this->defaultState(40); // packetSize = 9 + $stream = $this->openStream($state); + + $stream->stream_write(str_repeat('x', 9 * 5)); // five full packets + $this->assertCount(5, $this->writePackets($state)); + $this->assertSame([], $state->readPut, 'no ACKs drained mid-stream'); + + $this->assertTrue($stream->stream_flush()); + $this->assertSame([5], $state->readPut, 'all five drained at flush'); + } + + public function testAcknowledgementsDrainedWhenQueueIsFull(): void { + $state = $this->defaultState(32); // packetSize = max(1, 32 - 6 - 25) = 1 + $stream = $this->openStream($state); + + // QUEUE_SIZE (1024) one-byte packets must trigger exactly one mid-stream drain + $stream->stream_write(str_repeat('y', 1024)); + $this->assertSame([1024], $state->readPut); + } + + public function testStreamWriteReturnsFalseWhenSendFails(): void { + $state = $this->defaultState(40); // packetSize = 9 + $stream = $this->openStream($state); + + $state->sendThrowAfter = 1; // OPEN already sent (1); next send (a WRITE) throws + $this->assertFalse($stream->stream_write(str_repeat('z', 9))); + } + + public function testHighOffsetIsSplitIntoTwoWords(): void { + $state = $this->defaultState(); + $stream = $this->openStream($state); + + $stream->stream_write('tail'); // buffered (below packet size) + // Pretend we are already past the 4 GiB boundary to exercise UINT32_MODULUS. + $prop = new \ReflectionProperty(SFTPWriteStream::class, 'internalPosition'); + $prop->setValue($stream, (2 ** 32) + 5); + + $this->assertTrue($stream->stream_flush()); + $writes = $this->writePackets($state); + $this->assertWritePacket(end($writes)['packet'], 'handle', (2 ** 32) + 5, 'tail'); + } + + // --- stream_flush ------------------------------------------------------- + + public function testStreamFlushWithoutDataOrOutstandingSucceeds(): void { + $state = $this->defaultState(); + $stream = $this->openStream($state); + + $this->assertTrue($stream->stream_flush()); + $this->assertSame([], $state->readPut); + $this->assertSame([], $this->writePackets($state)); + } + + public function testStreamFlushReturnsFalseWhenSendFails(): void { + $state = $this->defaultState(40); // packetSize = 9 + $stream = $this->openStream($state); + + $stream->stream_write(str_repeat('a', 5)); // 5 bytes buffered, nothing sent yet + $state->sendThrowAfter = 1; // OPEN already sent; the flush's WRITE will throw + $this->assertFalse($stream->stream_flush()); + } + + public function testStreamFlushReturnsFalseWhenDrainFails(): void { + $state = $this->defaultState(40); + $state->readPutReturn = false; + $stream = $this->openStream($state); + + $stream->stream_write(str_repeat('a', 9)); // one outstanding packet + $this->assertFalse($stream->stream_flush()); + } + + // --- stream_close ------------------------------------------------------- + + public function testStreamCloseFlushesClosesHandleAndTouches(): void { + $state = $this->defaultState(40); + $stream = $this->openStream($state); + + $stream->stream_write(str_repeat('b', 5)); // buffered remainder + $this->assertTrue($stream->stream_close()); + + $this->assertTrue($state->closeHandleCalled); + $this->assertSame([1], $state->readPut, 'remainder flushed then drained'); + $this->assertNotNull($state->touchArgs); + } + + public function testStreamCloseReturnsFalseWhenCloseHandleFails(): void { + $state = $this->defaultState(); + $state->closeHandleReturn = false; + $stream = $this->openStream($state); + + $this->assertFalse($stream->stream_close()); + $this->assertNull($state->touchArgs, 'touch not attempted after failed close'); + } + + // --- stream_tell -------------------------------------------------------- + + public function testStreamTellTracksBytesWritten(): void { + $state = $this->defaultState(); + $stream = $this->openStream($state); + + $stream->stream_write('abc'); + $stream->stream_write('de'); + $this->assertSame(5, $stream->stream_tell()); + } + + // --- read-only / no-op stream operations -------------------------------- + + public function testUnsupportedOperationsReturnFalse(): void { + $stream = $this->buildStream($this->defaultState()); + + $this->assertFalse($stream->stream_seek(0)); + $this->assertFalse($stream->stream_read(10)); + $this->assertFalse($stream->stream_set_option(0, 0, 0)); + $this->assertFalse($stream->stream_truncate(0)); + $this->assertFalse($stream->stream_stat()); + $this->assertFalse($stream->stream_lock(LOCK_EX)); + $this->assertFalse($stream->stream_eof()); + } +}