Merge pull request grpc#7897 from stanley-cheung/php-fix-per-rpc-creds

PHP: reject metadata keys that are not legal
lenmoyouziJiangjun · Aug 29, 2016 · 7acd1a7 · 7acd1a7
2 parents 6e34f81 + 129bca6
commit 7acd1a7
Show file tree

Hide file tree

Showing 8 changed files with 124 additions and 168 deletions.
diff --git a/package.xml b/package.xml
@@ -22,7 +22,7 @@
  </stability>
  <license>BSD</license>
  <notes>
-- TBD
+- Reject metadata keys which are not legal #7881
  </notes>
  <contents>
   <dir baseinstalldir="/" name="/">
@@ -1181,7 +1181,7 @@ Update to wrap gRPC C Core version 0.10.0
    <date>2016-08-22</date>
    <license>BSD</license>
    <notes>
-- TBD
+- Reject metadata keys which are not legal #7881
    </notes>
   </release>
  </changelog>

diff --git 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
@@ -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
@@ -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,
@@ -197,7 +206,12 @@ function updateAuthMetadataCallback($context)
     $methodName = $context->method_name;
     $auth_credentials = ApplicationDefaultCredentials::getCredentials();
 
-    return $auth_credentials->updateMetadata($metadata = [], $authUri);
+    $metadata = [];
+    $result = $auth_credentials->updateMetadata([], $authUri);
+    foreach ($result as $key => $value) {
+      $metadata[strtolower($key)] = $value;
+    }
+    return $metadata;
 }
 
 /**
@@ -242,7 +256,7 @@ function ($length) {
         $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');
 }
@@ -275,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());
 }
 
 /**
@@ -312,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());
 }
 
 /**
@@ -326,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());
 }
 
 /**
@@ -419,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),
@@ -435,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
@@ -132,4 +132,69 @@ public function testCreateFromPlugin()
         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