diff --git a/package.xml b/package.xml index 6e3aab7a769..7da3d48a073 100644 --- a/package.xml +++ b/package.xml @@ -22,7 +22,7 @@ BSD -- TBD +- Reject metadata keys which are not legal #7881 @@ -1173,7 +1173,7 @@ Update to wrap gRPC C Core version 0.10.0 2016-08-22 BSD -- TBD +- Reject metadata keys which are not legal #7881 diff --git a/src/php/ext/grpc/call.c b/src/php/ext/grpc/call.c index 66ca1513ed7..31c59fe5adc 100644 --- a/src/php/ext/grpc/call.c +++ b/src/php/ext/grpc/call.c @@ -164,6 +164,9 @@ bool create_metadata_array(zval *array, grpc_metadata_array *metadata) { if (key_type1 != HASH_KEY_IS_STRING) { return false; } + if (!grpc_header_key_is_legal(key1, strlen(key1))) { + return false; + } inner_array_hash = Z_ARRVAL_P(inner_array); PHP_GRPC_HASH_FOREACH_VAL_START(inner_array_hash, value) if (Z_TYPE_P(value) != IS_STRING) { diff --git a/src/php/ext/grpc/call_credentials.c b/src/php/ext/grpc/call_credentials.c index 6921a5df17b..25c92c91fec 100644 --- a/src/php/ext/grpc/call_credentials.c +++ b/src/php/ext/grpc/call_credentials.c @@ -192,24 +192,16 @@ void plugin_get_metadata(void *ptr, grpc_auth_metadata_context context, /* call the user callback function */ zend_call_function(state->fci, state->fci_cache TSRMLS_CC); - if (Z_TYPE_P(retval) != IS_ARRAY) { - zend_throw_exception(spl_ce_InvalidArgumentException, - "plugin callback must return metadata array", - 1 TSRMLS_CC); - return; - } - + grpc_status_code code = GRPC_STATUS_OK; grpc_metadata_array metadata; - if (!create_metadata_array(retval, &metadata)) { - zend_throw_exception(spl_ce_InvalidArgumentException, - "invalid metadata", 1 TSRMLS_CC); + + if (Z_TYPE_P(retval) != IS_ARRAY) { + code = GRPC_STATUS_INVALID_ARGUMENT; + } else if (!create_metadata_array(retval, &metadata)) { grpc_metadata_array_destroy(&metadata); - return; + code = GRPC_STATUS_INVALID_ARGUMENT; } - /* TODO: handle error */ - grpc_status_code code = GRPC_STATUS_OK; - /* Pass control back to core */ cb(user_data, metadata.metadata, metadata.count, code, NULL); } diff --git a/src/php/tests/interop/interop_client.php b/src/php/tests/interop/interop_client.php index 7f8354e236b..c94ba61296f 100755 --- a/src/php/tests/interop/interop_client.php +++ b/src/php/tests/interop/interop_client.php @@ -54,6 +54,15 @@ function hardAssert($value, $error_message) } } +function hardAssertIfStatusOk($status) +{ + if ($status->code !== Grpc\STATUS_OK) { + echo "Call did not complete successfully. Status object:\n"; + var_dump($status); + exit(1); + } +} + /** * Run the empty_unary test. * @@ -62,7 +71,7 @@ function hardAssert($value, $error_message) function emptyUnary($stub) { list($result, $status) = $stub->EmptyCall(new grpc\testing\EmptyMessage())->wait(); - hardAssert($status->code === Grpc\STATUS_OK, 'Call did not complete successfully'); + hardAssertIfStatusOk($status); hardAssert($result !== null, 'Call completed with a null response'); } @@ -105,7 +114,7 @@ function performLargeUnary($stub, $fillUsername = false, $fillOauthScope = false } list($result, $status) = $stub->UnaryCall($request, [], $options)->wait(); - hardAssert($status->code === Grpc\STATUS_OK, 'Call did not complete successfully'); + hardAssertIfStatusOk($status); hardAssert($result !== null, 'Call returned a null response'); $payload = $result->getPayload(); hardAssert($payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, @@ -247,7 +256,7 @@ function clientStreaming($stub) $call->write($request); } list($result, $status) = $call->wait(); - hardAssert($status->code === Grpc\STATUS_OK, 'Call did not complete successfully'); + hardAssertIfStatusOk($status); hardAssert($result->getAggregatedPayloadSize() === 74922, 'aggregated_payload_size was incorrect'); } @@ -280,8 +289,7 @@ function serverStreaming($stub) 'Response '.$i.' had the wrong length'); $i += 1; } - hardAssert($call->getStatus()->code === Grpc\STATUS_OK, - 'Call did not complete successfully'); + hardAssertIfStatusOk($call->getStatus()); } /** @@ -317,8 +325,7 @@ function pingPong($stub) } $call->writesDone(); hardAssert($call->read() === null, 'Server returned too many responses'); - hardAssert($call->getStatus()->code === Grpc\STATUS_OK, - 'Call did not complete successfully'); + hardAssertIfStatusOk($call->getStatus()); } /** @@ -331,8 +338,7 @@ function emptyStream($stub) $call = $stub->FullDuplexCall(); $call->writesDone(); hardAssert($call->read() === null, 'Server returned too many responses'); - hardAssert($call->getStatus()->code === Grpc\STATUS_OK, - 'Call did not complete successfully'); + hardAssertIfStatusOk($call->getStatus()); } /** @@ -424,8 +430,7 @@ function customMetadata($stub) 'Incorrect initial metadata value'); list($result, $status) = $call->wait(); - hardAssert($status->code === Grpc\STATUS_OK, - 'Call did not complete successfully'); + hardAssertIfStatusOk($status); $trailing_metadata = $call->getTrailingMetadata(); hardAssert(array_key_exists($ECHO_TRAILING_KEY, $trailing_metadata), @@ -440,8 +445,7 @@ function customMetadata($stub) $streaming_call->write($streaming_request); $streaming_call->writesDone(); - hardAssert($streaming_call->getStatus()->code === Grpc\STATUS_OK, - 'Call did not complete successfully'); + hardAssertIfStatusOk($streaming_call->getStatus()); $streaming_trailing_metadata = $streaming_call->getTrailingMetadata(); hardAssert(array_key_exists($ECHO_TRAILING_KEY, diff --git a/src/php/tests/unit_tests/CallCredentials2Test.php b/src/php/tests/unit_tests/CallCredentials2Test.php index a57e2b9b4ee..b3b98a22cae 100644 --- a/src/php/tests/unit_tests/CallCredentials2Test.php +++ b/src/php/tests/unit_tests/CallCredentials2Test.php @@ -132,4 +132,69 @@ class CallCredentials2Test extends PHPUnit_Framework_TestCase unset($call); unset($server_call); } + + public function invalidKeyCallbackFunc($context) + { + $this->assertTrue(is_string($context->service_url)); + $this->assertTrue(is_string($context->method_name)); + + return ['K1' => ['v1']]; + } + + public function testCallbackWithInvalidKey() + { + $deadline = Grpc\Timeval::infFuture(); + $status_text = 'xyz'; + $call = new Grpc\Call($this->channel, + '/abc/dummy_method', + $deadline, + $this->host_override); + + $call_credentials = Grpc\CallCredentials::createFromPlugin( + array($this, 'invalidKeyCallbackFunc')); + $call->setCredentials($call_credentials); + + $event = $call->startBatch([ + Grpc\OP_SEND_INITIAL_METADATA => [], + Grpc\OP_SEND_CLOSE_FROM_CLIENT => true, + Grpc\OP_RECV_STATUS_ON_CLIENT => true, + ]); + + $this->assertTrue($event->send_metadata); + $this->assertTrue($event->send_close); + $this->assertTrue($event->status->code == Grpc\STATUS_UNAUTHENTICATED); + } + + public function invalidReturnCallbackFunc($context) + { + $this->assertTrue(is_string($context->service_url)); + $this->assertTrue(is_string($context->method_name)); + + return "a string"; + } + + public function testCallbackWithInvalidReturnValue() + { + $deadline = Grpc\Timeval::infFuture(); + $status_text = 'xyz'; + $call = new Grpc\Call($this->channel, + '/abc/dummy_method', + $deadline, + $this->host_override); + + $call_credentials = Grpc\CallCredentials::createFromPlugin( + array($this, 'invalidReturnCallbackFunc')); + $call->setCredentials($call_credentials); + + $event = $call->startBatch([ + Grpc\OP_SEND_INITIAL_METADATA => [], + Grpc\OP_SEND_CLOSE_FROM_CLIENT => true, + Grpc\OP_RECV_STATUS_ON_CLIENT => true, + ]); + + $this->assertTrue($event->send_metadata); + $this->assertTrue($event->send_close); + $this->assertTrue($event->status->code == Grpc\STATUS_UNAUTHENTICATED); + } + } diff --git a/src/php/tests/unit_tests/CallCredentials3Test.php b/src/php/tests/unit_tests/CallCredentials3Test.php deleted file mode 100644 index 8f5e109bf5c..00000000000 --- a/src/php/tests/unit_tests/CallCredentials3Test.php +++ /dev/null @@ -1,135 +0,0 @@ -credentials = Grpc\ChannelCredentials::createSsl( - file_get_contents(dirname(__FILE__).'/../data/ca.pem')); - $server_credentials = Grpc\ServerCredentials::createSsl( - null, - file_get_contents(dirname(__FILE__).'/../data/server1.key'), - file_get_contents(dirname(__FILE__).'/../data/server1.pem')); - $this->server = new Grpc\Server(); - $this->port = $this->server->addSecureHttp2Port('0.0.0.0:0', - $server_credentials); - $this->server->start(); - $this->host_override = 'foo.test.google.fr'; - $this->channel = new Grpc\Channel( - 'localhost:'.$this->port, - [ - 'grpc.ssl_target_name_override' => $this->host_override, - 'grpc.default_authority' => $this->host_override, - 'credentials' => $this->credentials, - ] - ); - } - - public function tearDown() - { - unset($this->channel); - unset($this->server); - } - - public function callbackFunc($context) - { - $this->assertTrue(is_string($context->service_url)); - $this->assertTrue(is_string($context->method_name)); - - return ['k1' => ['v1'], 'k2' => ['v2']]; - } - - public function testCreateFromPlugin() - { - $deadline = Grpc\Timeval::infFuture(); - $status_text = 'xyz'; - $call = new Grpc\Call($this->channel, - '/abc/dummy_method', - $deadline, - $this->host_override); - - $call_credentials = Grpc\CallCredentials::createFromPlugin( - [$this, 'callbackFunc']); - $call->setCredentials($call_credentials); - - $event = $call->startBatch([ - Grpc\OP_SEND_INITIAL_METADATA => [], - Grpc\OP_SEND_CLOSE_FROM_CLIENT => true, - ]); - - $this->assertTrue($event->send_metadata); - $this->assertTrue($event->send_close); - - $event = $this->server->requestCall(); - - $this->assertTrue(is_array($event->metadata)); - $metadata = $event->metadata; - $this->assertTrue(array_key_exists('k1', $metadata)); - $this->assertTrue(array_key_exists('k2', $metadata)); - $this->assertSame($metadata['k1'], ['v1']); - $this->assertSame($metadata['k2'], ['v2']); - - $this->assertSame('/abc/dummy_method', $event->method); - $server_call = $event->call; - - $event = $server_call->startBatch([ - Grpc\OP_SEND_INITIAL_METADATA => [], - Grpc\OP_SEND_STATUS_FROM_SERVER => [ - 'metadata' => [], - 'code' => Grpc\STATUS_OK, - 'details' => $status_text, - ], - Grpc\OP_RECV_CLOSE_ON_SERVER => true, - ]); - - $this->assertTrue($event->send_metadata); - $this->assertTrue($event->send_status); - $this->assertFalse($event->cancelled); - - $event = $call->startBatch([ - Grpc\OP_RECV_INITIAL_METADATA => true, - Grpc\OP_RECV_STATUS_ON_CLIENT => true, - ]); - - $this->assertSame([], $event->metadata); - $status = $event->status; - $this->assertSame([], $status->metadata); - $this->assertSame(Grpc\STATUS_OK, $status->code); - $this->assertSame($status_text, $status->details); - - unset($call); - unset($server_call); - } -} diff --git a/src/php/tests/unit_tests/CallTest.php b/src/php/tests/unit_tests/CallTest.php index d736f515460..1205f0cd8eb 100644 --- a/src/php/tests/unit_tests/CallTest.php +++ b/src/php/tests/unit_tests/CallTest.php @@ -113,7 +113,7 @@ class CallTest extends PHPUnit_Framework_TestCase /** * @expectedException InvalidArgumentException */ - public function testInvalidMetadataKey() + public function testInvalidStartBatchKey() { $batch = [ 'invalid' => ['key1' => 'value1'], @@ -121,6 +121,28 @@ class CallTest extends PHPUnit_Framework_TestCase $result = $this->call->startBatch($batch); } + /** + * @expectedException InvalidArgumentException + */ + public function testInvalidMetadataStrKey() + { + $batch = [ + Grpc\OP_SEND_INITIAL_METADATA => ['Key' => ['value1', 'value2']], + ]; + $result = $this->call->startBatch($batch); + } + + /** + * @expectedException InvalidArgumentException + */ + public function testInvalidMetadataIntKey() + { + $batch = [ + Grpc\OP_SEND_INITIAL_METADATA => [1 => ['value1', 'value2']], + ]; + $result = $this->call->startBatch($batch); + } + /** * @expectedException InvalidArgumentException */ diff --git a/templates/package.xml.template b/templates/package.xml.template index 65fef1892fb..32ed3b633e8 100644 --- a/templates/package.xml.template +++ b/templates/package.xml.template @@ -24,7 +24,7 @@ BSD - - TBD + - Reject metadata keys which are not legal #7881 @@ -291,7 +291,7 @@ 2016-08-22 BSD - - TBD + - Reject metadata keys which are not legal #7881