diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index cdc62d7a8d..6d0d9055ec 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -61,7 +61,7 @@ try { $workflows = array( new ConduitSSHWorkflow(), - + new DiffusionSSHMercurialServeWorkflow(), new DiffusionSSHGitUploadPackWorkflow(), new DiffusionSSHGitReceivePackWorkflow(), ); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c47a8e9513..d4b37d1218 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -543,6 +543,10 @@ phutil_register_library_map(array( 'DiffusionSSHGitReceivePackWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitReceivePackWorkflow.php', 'DiffusionSSHGitUploadPackWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitUploadPackWorkflow.php', 'DiffusionSSHGitWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitWorkflow.php', + 'DiffusionSSHMercurialServeWorkflow' => 'applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php', + 'DiffusionSSHMercurialWireClientProtocolChannel' => 'applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php', + 'DiffusionSSHMercurialWireTestCase' => 'applications/diffusion/ssh/__tests__/DiffusionSSHMercurialWireTestCase.php', + 'DiffusionSSHMercurialWorkflow' => 'applications/diffusion/ssh/DiffusionSSHMercurialWorkflow.php', 'DiffusionSSHWorkflow' => 'applications/diffusion/ssh/DiffusionSSHWorkflow.php', 'DiffusionServeController' => 'applications/diffusion/controller/DiffusionServeController.php', 'DiffusionSetPasswordPanel' => 'applications/diffusion/panel/DiffusionSetPasswordPanel.php', @@ -2808,6 +2812,10 @@ phutil_register_library_map(array( 'DiffusionSSHGitReceivePackWorkflow' => 'DiffusionSSHGitWorkflow', 'DiffusionSSHGitUploadPackWorkflow' => 'DiffusionSSHGitWorkflow', 'DiffusionSSHGitWorkflow' => 'DiffusionSSHWorkflow', + 'DiffusionSSHMercurialServeWorkflow' => 'DiffusionSSHMercurialWorkflow', + 'DiffusionSSHMercurialWireClientProtocolChannel' => 'PhutilProtocolChannel', + 'DiffusionSSHMercurialWireTestCase' => 'PhabricatorTestCase', + 'DiffusionSSHMercurialWorkflow' => 'DiffusionSSHWorkflow', 'DiffusionSSHWorkflow' => 'PhabricatorSSHWorkflow', 'DiffusionServeController' => 'DiffusionController', 'DiffusionSetPasswordPanel' => 'PhabricatorSettingsPanel', diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 288337e47c..6e78b89abc 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -228,40 +228,8 @@ final class DiffusionServeController extends DiffusionController { case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: $cmd = $request->getStr('cmd'); if ($cmd == 'batch') { - // For "batch" we get a "cmds" argument like - // - // heads ;known nodes= - // - // We need to examine the commands (here, "heads" and "known") to - // make sure they're all read-only. - - $args = $this->getMercurialArguments(); - $cmds = idx($args, 'cmds'); - if ($cmds) { - - // NOTE: Mercurial has some code to escape semicolons, but it does - // not actually function for command separation. For example, these - // two batch commands will produce completely different results (the - // former will run the lookup; the latter will fail with a parser - // error): - // - // lookup key=a:xb;lookup key=z* 0 - // lookup key=a:;b;lookup key=z* 0 - // ^ - // | - // +-- Note semicolon. - // - // So just split unconditionally. - - $cmds = explode(';', $cmds); - foreach ($cmds as $sub_cmd) { - $name = head(explode(' ', $sub_cmd, 2)); - if (!DiffusionMercurialWireProtocol::isReadOnlyCommand($name)) { - return false; - } - } - return true; - } + $cmds = idx($this->getMercurialArguments(), 'cmds'); + return DiffusionMercurialWireProtocol::isReadOnlyBatchCommand($cmds); } return DiffusionMercurialWireProtocol::isReadOnlyCommand($cmd); case PhabricatorRepositoryType::REPOSITORY_TYPE_SUBVERSION: diff --git a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php index 0f6a8ae74e..578196ff69 100644 --- a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php +++ b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php @@ -59,4 +59,44 @@ final class DiffusionMercurialWireProtocol { return isset($read_only[$command]); } + public static function isReadOnlyBatchCommand($cmds) { + if (!strlen($cmds)) { + // We expect a "batch" command to always have a "cmds" string, so err + // on the side of caution and throw if we don't get any data here. This + // either indicates a mangled command from the client or a programming + // error in our code. + throw new Exception("Expected nonempty 'cmds' specification!"); + } + + // For "batch" we get a "cmds" argument like: + // + // heads ;known nodes= + // + // We need to examine the commands (here, "heads" and "known") to make sure + // they're all read-only. + + // NOTE: Mercurial has some code to escape semicolons, but it does not + // actually function for command separation. For example, these two batch + // commands will produce completely different results (the former will run + // the lookup; the latter will fail with a parser error): + // + // lookup key=a:xb;lookup key=z* 0 + // lookup key=a:;b;lookup key=z* 0 + // ^ + // | + // +-- Note semicolon. + // + // So just split unconditionally. + + $cmds = explode(';', $cmds); + foreach ($cmds as $sub_cmd) { + $name = head(explode(' ', $sub_cmd, 2)); + if (!self::isReadOnlyCommand($name)) { + return false; + } + } + + return true; + } + } diff --git a/src/applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php new file mode 100644 index 0000000000..4e1dc64c81 --- /dev/null +++ b/src/applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php @@ -0,0 +1,106 @@ +setName('hg'); + $this->setArguments( + array( + array( + 'name' => 'repository', + 'short' => 'R', + 'param' => 'repo', + ), + array( + 'name' => 'stdio', + ), + array( + 'name' => 'command', + 'wildcard' => true, + ), + )); + } + + public function getRequestPath() { + return $this->getArgs()->getArg('repository'); + } + + protected function executeRepositoryOperations( + PhabricatorRepository $repository) { + + $args = $this->getArgs(); + + if (!$args->getArg('stdio')) { + throw new Exception("Expected `hg ... --stdio`!"); + } + + if ($args->getArg('command') !== array('serve')) { + throw new Exception("Expected `hg ... serve`!"); + } + + $future = new ExecFuture( + 'hg -R %s serve --stdio', + $repository->getLocalPath()); + + $io_channel = $this->getIOChannel(); + + $protocol_channel = new DiffusionSSHMercurialWireClientProtocolChannel( + $io_channel); + + $err = id($this->newPassthruCommand()) + ->setIOChannel($protocol_channel) + ->setCommandChannelFromExecFuture($future) + ->setWillWriteCallback(array($this, 'willWriteMessageCallback')) + ->execute(); + + // TODO: It's apparently technically possible to communicate errors to + // Mercurial over SSH by writing a special "\n\n-\n" string. However, + // my attempt to implement that resulted in Mercurial closing the socket and + // then hanging, without showing the error. This might be an issue on our + // side (we need to close our half of the socket?), or maybe the code + // for this in Mercurial doesn't actually work, or maybe something else + // is afoot. At some point, we should look into doing this more cleanly. + // For now, when we, e.g., reject writes for policy reasons, the user will + // see "abort: unexpected response: empty string" after the diagnostically + // useful, e.g., "remote: This repository is read-only over SSH." message. + + if (!$err && $this->didSeeWrite) { + $repository->writeStatusMessage( + PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE, + PhabricatorRepositoryStatusMessage::CODE_OKAY); + } + + return $err; + } + + public function willWriteMessageCallback( + PhabricatorSSHPassthruCommand $command, + $message) { + + $command = $message['command']; + + // Check if this is a readonly command. + + $is_readonly = false; + if ($command == 'batch') { + $cmds = idx($message['arguments'], 'cmds'); + if (DiffusionMercurialWireProtocol::isReadOnlyBatchCommand($cmds)) { + $is_readonly = true; + } + } else if (DiffusionMercurialWireProtocol::isReadOnlyCommand($command)) { + $is_readonly = true; + } + + if (!$is_readonly) { + $this->requireWriteAccess(); + $this->didSeeWrite = true; + } + + // If we're good, return the raw message data. + return $message['raw']; + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php b/src/applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php new file mode 100644 index 0000000000..ab0a62b85d --- /dev/null +++ b/src/applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php @@ -0,0 +1,217 @@ +command = ''; + $this->state = 'data-length'; + } else { + $this->state = 'command'; + } + $this->expectArgumentCount = null; + $this->expectBytes = null; + $this->command = null; + $this->argumentName = null; + $this->arguments = array(); + $this->raw = ''; + } + + private function readProtocolLine() { + $pos = strpos($this->buffer, "\n"); + + if ($pos === false) { + return null; + } + + $line = substr($this->buffer, 0, $pos); + + $this->raw .= $line."\n"; + $this->buffer = substr($this->buffer, $pos + 1); + + return $line; + } + + private function readProtocolBytes() { + if (strlen($this->buffer) < $this->expectBytes) { + return null; + } + + $bytes = substr($this->buffer, 0, $this->expectBytes); + $this->raw .= $bytes; + $this->buffer = substr($this->buffer, $this->expectBytes); + + return $bytes; + } + + private function newMessageAndResetState() { + $message = array( + 'command' => $this->command, + 'arguments' => $this->arguments, + 'raw' => $this->raw, + ); + $this->initializeState($this->command); + return $message; + } + + private function newDataMessage($bytes) { + $message = array( + 'command' => '', + 'raw' => strlen($bytes)."\n".$bytes, + ); + return $message; + } + + protected function decodeStream($data) { + $this->buffer .= $data; + + $out = array(); + $messages = array(); + + while (true) { + if ($this->state == 'command') { + $this->initializeState(); + + // We're reading a command. It looks like: + // + // + + $line = $this->readProtocolLine(); + if ($line === null) { + break; + } + + $this->command = $line; + $this->state = 'arguments'; + } else if ($this->state == 'arguments') { + + // Check if we're still waiting for arguments. + $args = DiffusionMercurialWireProtocol::getCommandArgs($this->command); + $have = array_select_keys($this->arguments, $args); + if (count($have) == count($args)) { + // We have all the arguments. Emit a message and read the next + // command. + $messages[] = $this->newMessageAndResetState(); + } else { + // We're still reading arguments. They can either look like: + // + // + // + // ... + // + // ...or like this: + // + // * + // + // + // ... + + $line = $this->readProtocolLine(); + if ($line === null) { + break; + } + + list($arg, $size) = explode(' ', $line, 2); + $size = (int)$size; + + if ($arg != '*') { + $this->expectBytes = $size; + $this->argumentName = $arg; + $this->state = 'value'; + } else { + $this->arguments['*'] = array(); + $this->expectArgumentCount = $size; + $this->state = 'argv'; + } + } + } else if ($this->state == 'value' || $this->state == 'argv-value') { + + // We're reading the value of an argument. We just need to wait for + // the right number of bytes to show up. + + $bytes = $this->readProtocolBytes(); + if ($bytes === null) { + break; + } + + if ($this->state == 'argv-value') { + $this->arguments['*'][$this->argumentName] = $bytes; + $this->state = 'argv'; + } else { + $this->arguments[$this->argumentName] = $bytes; + $this->state = 'arguments'; + } + + + } else if ($this->state == 'argv') { + + // We're reading a variable number of arguments. We need to wait for + // the arguments to arrive. + + if ($this->expectArgumentCount) { + $line = $this->readProtocolLine(); + if ($line === null) { + break; + } + + list($arg, $size) = explode(' ', $line, 2); + $size = (int)$size; + + $this->expectBytes = $size; + $this->argumentName = $arg; + $this->state = 'argv-value'; + + $this->expectArgumentCount--; + } else { + $this->state = 'arguments'; + } + } else if ($this->state == 'data-length') { + $line = $this->readProtocolLine(); + if ($line === null) { + break; + } + $this->expectBytes = (int)$line; + if (!$this->expectBytes) { + $messages[] = $this->newDataMessage(''); + $this->initializeState(); + } else { + $this->state = 'data-bytes'; + } + } else if ($this->state == 'data-bytes') { + $bytes = substr($this->buffer, 0, $this->expectBytes); + $this->buffer = substr($this->buffer, strlen($bytes)); + $this->expectBytes -= strlen($bytes); + + $messages[] = $this->newDataMessage($bytes); + + if (!$this->expectBytes) { + // We've finished reading this chunk, so go read the next chunk. + $this->state = 'data-length'; + } else { + // We're waiting for more data, and have read everything available + // to us so far. + break; + } + } else { + throw new Exception("Bad parser state '{$this->state}'!"); + } + } + + return $messages; + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionSSHMercurialWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHMercurialWorkflow.php new file mode 100644 index 0000000000..e48dcf5eb4 --- /dev/null +++ b/src/applications/diffusion/ssh/DiffusionSSHMercurialWorkflow.php @@ -0,0 +1,5 @@ +assertEqual(2, count($raw)); + $expect = json_decode($raw[1], true); + $this->assertEqual(true, is_array($expect), $file); + + $this->assertParserResult($expect, $raw[0], $file); + } + } + + private function assertParserResult(array $expect, $input, $file) { + list($x, $y) = PhutilSocketChannel::newChannelPair(); + $xp = new DiffusionSSHMercurialWireClientProtocolChannel($x); + + $y->write($input); + $y->flush(); + $y->closeWriteChannel(); + + $messages = array(); + for ($ii = 0; $ii < count($expect); $ii++) { + try { + $messages[] = $xp->waitForMessage(); + } catch (Exception $ex) { + // This is probably the parser not producing as many messages as + // we expect. Log the exception, but continue to the assertion below + // since that will often be easier to diagnose. + phlog($ex); + break; + } + } + + $this->assertEqual($expect, $messages, $file); + + // Now, make sure the channel doesn't have *more* messages than we expect. + // Specifically, it should throw when we try to read another message. + $caught = null; + try { + $xp->waitForMessage(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + ($caught instanceof Exception), + "No extra messages for '{$file}'."); + } + +} diff --git a/src/applications/diffusion/ssh/__tests__/hgwiredata/batch.txt b/src/applications/diffusion/ssh/__tests__/hgwiredata/batch.txt new file mode 100644 index 0000000000..86de58b959 --- /dev/null +++ b/src/applications/diffusion/ssh/__tests__/hgwiredata/batch.txt @@ -0,0 +1,16 @@ +batch +* 0 +cmds 19 +heads ;known nodes= +~~~~~~~~~~ +[ + { + "command" : "batch", + "arguments" : { + "*" : { + }, + "cmds" : "heads ;known nodes=" + }, + "raw" : "batch\n* 0\ncmds 19\nheads ;known nodes=" + } +] diff --git a/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities.txt b/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities.txt new file mode 100644 index 0000000000..9d3934b4fe --- /dev/null +++ b/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities.txt @@ -0,0 +1,10 @@ +capabilities + +~~~~~~~~~~ +[ + { + "command" : "capabilities", + "arguments" : [], + "raw" : "capabilities\n" + } +] diff --git a/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities2.txt b/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities2.txt new file mode 100644 index 0000000000..7ac0c71934 --- /dev/null +++ b/src/applications/diffusion/ssh/__tests__/hgwiredata/capabilities2.txt @@ -0,0 +1,16 @@ +capabilities +capabilities + +~~~~~~~~~~ +[ + { + "command" : "capabilities", + "arguments" : [], + "raw" : "capabilities\n" + }, + { + "command" : "capabilities", + "arguments" : [], + "raw" : "capabilities\n" + } +] diff --git a/src/applications/diffusion/ssh/__tests__/hgwiredata/getbundle.txt b/src/applications/diffusion/ssh/__tests__/hgwiredata/getbundle.txt new file mode 100644 index 0000000000..ff28bea22d --- /dev/null +++ b/src/applications/diffusion/ssh/__tests__/hgwiredata/getbundle.txt @@ -0,0 +1,18 @@ +getbundle +* 2 +common 40 +0000000000000000000000000000000000000000heads 122 +7cb27ad591d60500c020283b81c6467540218eda 1036b72db89a0451fa82fcd5462d903f591f0a3c 0b9d8290c4e067a0b91b43062ee9de392e8fae88 +~~~~~~~~~~ +[ + { + "command" : "getbundle", + "arguments" : { + "*" : { + "common" : "0000000000000000000000000000000000000000", + "heads" : "7cb27ad591d60500c020283b81c6467540218eda 1036b72db89a0451fa82fcd5462d903f591f0a3c 0b9d8290c4e067a0b91b43062ee9de392e8fae88" + } + }, + "raw" : "getbundle\n* 2\ncommon 40\n0000000000000000000000000000000000000000heads 122\n7cb27ad591d60500c020283b81c6467540218eda 1036b72db89a0451fa82fcd5462d903f591f0a3c 0b9d8290c4e067a0b91b43062ee9de392e8fae88" + } +] diff --git a/src/applications/diffusion/ssh/__tests__/hgwiredata/unbundle.txt b/src/applications/diffusion/ssh/__tests__/hgwiredata/unbundle.txt new file mode 100644 index 0000000000..8d4651e874 --- /dev/null +++ b/src/applications/diffusion/ssh/__tests__/hgwiredata/unbundle.txt @@ -0,0 +1,28 @@ +unbundle +heads 53 +686173686564 8022e00be6886fcf1be8f57f96c78aa924967f8320 +aaaaaaaaaaaaaaaaaaaa20 +bbbbbbbbbbbbbbbbbbbb0 + +~~~~~~~~~~ +[ + { + "command" : "unbundle", + "arguments" : { + "heads" : "686173686564 8022e00be6886fcf1be8f57f96c78aa924967f83" + }, + "raw" : "unbundle\nheads 53\n686173686564 8022e00be6886fcf1be8f57f96c78aa924967f83" + }, + { + "command" : "", + "raw" : "20\naaaaaaaaaaaaaaaaaaaa" + }, + { + "command" : "", + "raw" : "20\nbbbbbbbbbbbbbbbbbbbb" + }, + { + "command" : "", + "raw" : "0\n" + } +] diff --git a/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php b/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php index e26badae3a..0cf80bb73f 100644 --- a/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php +++ b/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php @@ -76,6 +76,13 @@ final class PhabricatorSSHPassthruCommand extends Phobject { public function writeErrorIOCallback(PhutilChannel $channel, $data) { $this->errorChannel->write($data); + + // TODO: Because of the way `waitForAny()` works, we degrade to a busy + // wait if we hand it a writable, write-only channel. We should handle this + // case better in `waitForAny()`. For now, just flush the error channel + // explicity after writing data over it. + + $this->errorChannel->flush(); } public function execute() { @@ -98,7 +105,9 @@ final class PhabricatorSSHPassthruCommand extends Phobject { $channels = array($command_channel, $io_channel, $error_channel); while (true) { - PhutilChannel::waitForAny($channels); + // TODO: See note in writeErrorIOCallback! + $wait = array($command_channel, $io_channel); + PhutilChannel::waitForAny($wait); $io_channel->update(); $command_channel->update(); @@ -107,21 +116,24 @@ final class PhabricatorSSHPassthruCommand extends Phobject { $done = !$command_channel->isOpen(); $in_message = $io_channel->read(); - $in_message = $this->willWriteData($in_message); if ($in_message !== null) { - $command_channel->write($in_message); + $in_message = $this->willWriteData($in_message); + if ($in_message !== null) { + $command_channel->write($in_message); + } } $out_message = $command_channel->read(); - $out_message = $this->willReadData($out_message); if ($out_message !== null) { - $io_channel->write($out_message); + $out_message = $this->willReadData($out_message); + if ($out_message !== null) { + $io_channel->write($out_message); + } } // If we have nothing left on stdin, close stdin on the subprocess. if (!$io_channel->isOpenForReading()) { - // TODO: This should probably be part of PhutilExecChannel? - $this->execFuture->write(''); + $command_channel->closeWriteChannel(); } if ($done) {