issue #174: fixed key validation

jsteemann · jsteemann · commit f6b17452ec0a · 2014-08-29T19:05:39.000+02:00
diff --git a/lib/triagens/ArangoDb/Document.php b/lib/triagens/ArangoDb/Document.php
@@ -495,7 +495,7 @@ public function setInternalId($id)
         }
 
 
-        if (!preg_match('/^\w+\/\w+$/', $id)) {
+        if (!preg_match('/^[a-zA-Z0-9_-]{1,64}\/[a-zA-Z0-9_:-]{1,254}$/', $id)) {
             throw new ClientException('Invalid format for document id');
         }
 
@@ -519,7 +519,7 @@ public function setInternalKey($key)
             throw new ClientException('Should not update the key of an existing document');
         }
 
-        if (!preg_match('/^\w+$/', $key)) {
+        if (!preg_match('/^[a-zA-Z0-9_:-]{1,254}$/', $key)) {
             throw new ClientException('Invalid format for document key');
         }
 
diff --git a/lib/triagens/ArangoDb/DocumentHandler.php b/lib/triagens/ArangoDb/DocumentHandler.php
@@ -796,7 +796,7 @@ private function getDocumentId($document)
             $documentId = $document;
         }
 
-        if (!$documentId || !(is_string($documentId) || is_double($documentId) || is_int($documentId))) {
+        if (trim($documentId) === "" || !(is_string($documentId) || is_double($documentId) || is_int($documentId))) {
             throw new ClientException('Cannot alter a document without a document id');
         }
 
diff --git a/tests/DocumentBasicTest.php b/tests/DocumentBasicTest.php
@@ -104,6 +104,66 @@ public function testCreateAndDeleteDocumentUsingDefinedKey()
 
         $documentHandler->delete($document);
     }
+    
+    /**
+     * Try to create and delete a document with several keys
+     */
+    public function testCreateAndDeleteDocumentWithSeveralKeys()
+    {
+        $connection      = $this->connection;
+        $collection      = $this->collection;
+        $documentHandler = new DocumentHandler($connection);
+
+        $keys = array("foo", "bar", "bar:bar", "baz", "1", "0", "a-b-c", "a:b", "this-is-a-test", "FOO", "BAR", "Bar", "bAr");
+        foreach ($keys as $key) {
+          $document        = new Document();
+          $document->someAttribute = 'someValue';
+          $document->set('_key', $key);
+          $documentId = $documentHandler->add($collection->getName(), $document);
+
+          $resultingDocument = $documentHandler->get($collection->getName(), $documentId);
+
+          $resultingAttribute = $resultingDocument->someAttribute;
+          $resultingKey       = $resultingDocument->getKey();
+          $this->assertTrue(
+               $resultingAttribute === 'someValue',
+               'Resulting Attribute should be "someValue". It\'s :' . $resultingAttribute
+          );
+          $this->assertTrue(
+               $resultingKey === $key,
+               'Resulting Attribute should be "someValue". It\'s :' . $resultingKey
+          );
+
+          $documentHandler->delete($document);
+        }
+    }
+    
+    
+    /**
+     * Try to create a document with invalid keys
+     */
+    public function testCreateDocumentWithInvalidKeys()
+    {
+        $connection      = $this->connection;
+        $collection      = $this->collection;
+        $documentHandler = new DocumentHandler($connection);
+
+        $keys = array("", " ", " bar", "bar ", "/", "?", "abcdef gh", "abcxde&", "mötörhead", "this-key-will-be-too-long-to-be-processed-successfully-would-you-agree-with-me-sure-you-will-because-there-is-a-limit-of-254-characters-per-key-which-this-string-will-not-conform-to-if-you-are-still-reading-this-you-should-probably-do-something-else-right-now-REALLY");
+
+        foreach ($keys as $key) {
+          $document        = new Document();
+          $document->someAttribute = 'someValue';
+
+          $caught = false;
+          try {
+            $document->set('_key', $key);
+          } catch (\triagens\ArangoDb\ClientException $exception) {
+            $caught = true;
+          }
+
+          $this->assertTrue($caught, "expecting exception to be thrown for key ". $key);
+        }
+    }
 
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -495,7 +495,7 @@ public function setInternalId($id)`
`495`	`495`	`}`
`496`	`496`
`497`	`497`
`498`		`- if (!preg_match('/^\w+\/\w+$/', $id)) {`
	`498`	`+ if (!preg_match('/^[a-zA-Z0-9_-]{1,64}\/[a-zA-Z0-9_:-]{1,254}$/', $id)) {`
`499`	`499`	`throw new ClientException('Invalid format for document id');`
`500`	`500`	`}`
`501`	`501`
`@@ -519,7 +519,7 @@ public function setInternalKey($key)`
`519`	`519`	`throw new ClientException('Should not update the key of an existing document');`
`520`	`520`	`}`
`521`	`521`
`522`		`- if (!preg_match('/^\w+$/', $key)) {`
	`522`	`+ if (!preg_match('/^[a-zA-Z0-9_:-]{1,254}$/', $key)) {`
`523`	`523`	`throw new ClientException('Invalid format for document key');`
`524`	`524`	`}`
`525`	`525`
Original file line number	Diff line number	Diff line change
`@@ -796,7 +796,7 @@ private function getDocumentId($document)`
`796`	`796`	`$documentId = $document;`
`797`	`797`	`}`
`798`	`798`
`799`		`- if (!$documentId \|\| !(is_string($documentId) \|\| is_double($documentId) \|\| is_int($documentId))) {`
	`799`	`+ if (trim($documentId) === "" \|\| !(is_string($documentId) \|\| is_double($documentId) \|\| is_int($documentId))) {`
`800`	`800`	`throw new ClientException('Cannot alter a document without a document id');`
`801`	`801`	`}`
`802`	`802`