Harden security (#47)

* Harden security

* Code style fixes

Author: M-arcus

src/CXml/Authentication/SimpleSharedSecretAuthenticator.php

@@ -16,7 +16,8 @@ public function __construct(private string $sharedSecret)
 
     public function authenticate(Header $header, Context $context): void
     {
-        if ($this->sharedSecret !== $header->sender->credential->getSharedSecret()) {
+        // use hash_equals() for constant-time comparison to prevent timing attacks
+        if (!hash_equals($this->sharedSecret, (string)$header->sender->credential->getSharedSecret())) {
             throw new CXmlAuthenticationInvalidException($header->sender->credential);
         }
     }

src/CXml/Builder/OrderRequestBuilder.php

@@ -125,7 +125,6 @@ public static function fromPunchOutOrderMessage(
             $orderDate,
             $currency,
             $language,
-            null,
         );
 
         $orb->setShipTo($punchOutOrderMessage->punchOutOrderMessageHeader->getShipTo());

src/CXml/Credential/Registry.php

@@ -58,7 +58,8 @@ public function authenticate(Header $header, Context $context): void
             $senderCredential->identity,
         );
 
-        if ($baseCredential->getSharedSecret() !== $senderCredential->getSharedSecret()) {
+        // use hash_equals() for constant-time comparison to prevent timing attacks
+        if (!hash_equals((string)$baseCredential->getSharedSecret(), (string)$senderCredential->getSharedSecret())) {
             throw new CXmlAuthenticationInvalidException($senderCredential);
         }
     }

src/CXml/Endpoint.php

@@ -32,13 +32,13 @@ public function __construct(
      */
     public function parseAndProcessStringAsCXml(string $xml, ?Context $context = null): ?CXml
     {
-        $this->logger->info('Processing incoming CXml message', ['xml' => $xml]);
+        $this->logger->info('Processing incoming CXml message', ['xml' => $this->removeSharedSecret($xml)]);
 
         // validate
         try {
             $this->dtdValidator->validateAgainstDtd($xml);
         } catch (CXmlInvalidException $cXmlInvalidException) {
-            $this->logger->error('Incoming CXml was invalid (via DTD)', ['xml' => $xml]);
+            $this->logger->error('Incoming CXml was invalid (via DTD)', ['xml' => $this->removeSharedSecret($xml)]);
 
             throw $cXmlInvalidException;
         }
@@ -47,7 +47,7 @@ public function parseAndProcessStringAsCXml(string $xml, ?Context $context = nul
         try {
             $cxml = $this->serializer->deserialize($xml);
         } catch (RuntimeException $runtimeException) {
-            $this->logger->error('Error while deserializing xml to CXml: ' . $runtimeException->getMessage(), ['xml' => $xml]);
+            $this->logger->error('Error while deserializing xml to CXml: ' . $runtimeException->getMessage(), ['xml' => $this->removeSharedSecret($xml)]);
 
             throw new CXmlInvalidException('Error while deserializing xml: ' . $runtimeException->getMessage(), $xml, $runtimeException);
         }
@@ -56,13 +56,18 @@ public function parseAndProcessStringAsCXml(string $xml, ?Context $context = nul
         try {
             $result = $this->processor->process($cxml, $context);
         } catch (CXmlException $cXmlException) {
-            $this->logger->error('Error while processing valid CXml: ' . $cXmlException->getMessage(), ['xml' => $xml]);
+            $this->logger->error('Error while processing valid CXml: ' . $cXmlException->getMessage(), ['xml' => $this->removeSharedSecret($xml)]);
 
             throw $cXmlException;
         }
 
-        $this->logger->info('Success after processing incoming CXml message', ['xml' => $xml]);
+        $this->logger->info('Success after processing incoming CXml message', ['xml' => $this->removeSharedSecret($xml)]);
 
         return $result;
     }
+
+    private function removeSharedSecret(string $xml): string
+    {
+        return (string)preg_replace('/<SharedSecret>.*?<\/SharedSecret>/s', '<SharedSecret>***REDACTED***</SharedSecret>', $xml);
+    }
 }

src/CXml/Model/BusinessPartner.php

@@ -11,6 +11,7 @@
 class BusinessPartner
 {
     use IdReferencesTrait;
+
     final public const ROLE_SOLD_TO = 'soldTo';
 
     final public const ROLE_SHIP_FROM = 'shipFrom';

src/CXml/Model/Response/ProfileResponse.php

@@ -26,7 +26,7 @@ class ProfileResponse implements ResponsePayloadInterface
     /**
      * @var Transaction[]
      */
-    #[Serializer\XmlList(inline: true, entry: 'Transaction')]
+    #[Serializer\XmlList(entry: 'Transaction', inline: true)]
     #[Serializer\Type('array<CXml\Model\Transaction>')]
     private array $transactions = [];
 

src/CXml/Validation/DtdValidator.php

@@ -44,7 +44,8 @@ public function validateAgainstDtd(string $xml): void
         $internalErrors = libxml_use_internal_errors(true);
 
         $old = new DOMDocument();
-        $old->loadXML($xml);
+        // disable network access for security reasons
+        $old->loadXML($xml, LIBXML_NONET);
 
         $this->validateAgainstMultipleDtd($this->pathToDtds, $old);
 

src/CXml/Validation/Exception/CXmlInvalidException.php

@@ -34,7 +34,7 @@ public static function fromLibXmlError($libXmlError, string $xml): self
 
         return new self(
             $message,
-            $xml,
+            (string)preg_replace('/<SharedSecret>.*?<\/SharedSecret>/s', '<SharedSecret>***REDACTED***</SharedSecret>', $xml),
         );
     }
 }

tests/CXmlTest/Handling/HandlerTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace CXmlTest\Handling;
 
 use CXml\Authentication\SimpleSharedSecretAuthenticator;

tests/CXmlTest/Model/PunchoutOrderMessageAdvancedPricingTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace CXmlTest\Model;
 
 use CXml\Builder;