Percent-encode the offer carried in the credential offer URI

Author: cicnavi

docs/8-api.md

@@ -134,7 +134,7 @@ Response:
 
 ```json
 {
-    "credential_offer_uri": "openid-credential-offer://?credential_offer={\"credential_issuer\":\"https:\\/\\/idp.mivanci.incubator.hexaa.eu\",\"credential_configuration_ids\":[\"ResearchAndScholarshipCredentialDcSdJwt\"],\"grants\":{\"authorization_code\":{\"issuer_state\":\"30616b68fa26b00c5a6391faffc02e4e4fd9b0023fd6a3aa29ec754e2f5e2871\"}}}"
+    "credential_offer_uri": "openid-credential-offer://?credential_offer=%7B%22credential_issuer%22%3A%22https%3A%2F%2Fidp.mivanci.incubator.hexaa.eu%22%2C%22credential_configuration_ids%22%3A%5B%22ResearchAndScholarshipCredentialDcSdJwt%22%5D%2C%22grants%22%3A%7B%22authorization_code%22%3A%7B%22issuer_state%22%3A%2230616b68fa26b00c5a6391faffc02e4e4fd9b0023fd6a3aa29ec754e2f5e2871%22%7D%7D%7D"
 }
 
 ```
@@ -168,7 +168,7 @@ Response:
 
 ```json
 {
-    "credential_offer_uri": "openid-credential-offer://?credential_offer={\"credential_issuer\":\"https:\\/\\/idp.mivanci.incubator.hexaa.eu\",\"credential_configuration_ids\":[\"ResearchAndScholarshipCredentialDcSdJwt\"],\"grants\":{\"urn:ietf:params:oauth:grant-type:pre-authorized_code\":{\"pre-authorized_code\":\"_ffcdf6d86cd564c300346351dce0b4ccb2fde304e2\",\"tx_code\":{\"input_mode\":\"numeric\",\"length\":4,\"description\":\"Please provide the one-time code that was sent to e-mail testuser@example.com\"}}}}"
+    "credential_offer_uri": "openid-credential-offer://?credential_offer=%7B%22credential_issuer%22%3A%22https%3A%2F%2Fidp.mivanci.incubator.hexaa.eu%22%2C%22credential_configuration_ids%22%3A%5B%22ResearchAndScholarshipCredentialDcSdJwt%22%5D%2C%22grants%22%3A%7B%22urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Apre-authorized_code%22%3A%7B%22pre-authorized_code%22%3A%22_ffcdf6d86cd564c300346351dce0b4ccb2fde304e2%22%2C%22tx_code%22%3A%7B%22input_mode%22%3A%22numeric%22%2C%22length%22%3A4%2C%22description%22%3A%22Please%20provide%20the%20one-time%20code%20that%20was%20sent%20to%20e-mail%20testuser%40example.com%22%7D%7D%7D%7D"
 }
 ```
 

src/Factories/CredentialOfferUriFactory.php

@@ -50,6 +50,7 @@ public function __construct(
     /**
      * @param string[] $credentialConfigurationIds
      * @throws \SimpleSAML\OpenID\Exceptions\OpenIdException
+     * @throws \JsonException
      */
     public function buildForAuthorization(
         array $credentialConfigurationIds,
@@ -89,19 +90,13 @@ public function buildForAuthorization(
             ],
         );
 
-        $credentialOfferValue = $credentialOffer->jsonSerialize();
-        $parameterName = ParametersEnum::CredentialOfferUri->value;
-        if (is_array($credentialOfferValue)) {
-            $parameterName = ParametersEnum::CredentialOffer->value;
-            $credentialOfferValue = json_encode($credentialOfferValue);
-        }
-
-        return "openid-credential-offer://?$parameterName=$credentialOfferValue";
+        return $this->buildUri($credentialOffer->jsonSerialize());
     }
 
     /**
      * @param string[] $credentialConfigurationIds
      * @throws \SimpleSAML\OpenID\Exceptions\OpenIdException
+     * @throws \JsonException
      */
     public function buildPreAuthorized(
         array $credentialConfigurationIds,
@@ -232,14 +227,34 @@ public function buildPreAuthorized(
             $this->sendTxCodeByEmail($txCode, $userEmail);
         }
 
-        $credentialOfferValue = $credentialOffer->jsonSerialize();
-        $parameterName = ParametersEnum::CredentialOfferUri->value;
-        if (is_array($credentialOfferValue)) {
+        return $this->buildUri($credentialOffer->jsonSerialize());
+    }
+
+    /**
+     * Build the offer URI a wallet is sent to, carrying the offer either by value or by reference.
+     *
+     * The offer travels as a query parameter, so its value has to be percent encoded. A by-reference
+     * offer is a URL which may carry a query string of its own, and a by-value offer is JSON, full of
+     * characters a query cannot hold literally. Appended raw, an '&' in either would split the value
+     * into a second parameter and a '#' would truncate it into a fragment.
+     *
+     * @param string|mixed[] $credentialOffer A URI to the offer, or the offer parameters themselves.
+     * @throws \JsonException
+     */
+    protected function buildUri(string|array $credentialOffer): string
+    {
+        if (is_array($credentialOffer)) {
             $parameterName = ParametersEnum::CredentialOffer->value;
-            $credentialOfferValue = json_encode($credentialOfferValue);
+            $parameterValue = json_encode($credentialOffer, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES);
+        } else {
+            $parameterName = ParametersEnum::CredentialOfferUri->value;
+            $parameterValue = $credentialOffer;
         }
 
-        return "openid-credential-offer://?$parameterName=$credentialOfferValue";
+        return 'openid-credential-offer://?' . http_build_query(
+            [$parameterName => $parameterValue],
+            encoding_type: PHP_QUERY_RFC3986,
+        );
     }
 
     /**

tests/unit/src/Factories/CredentialOfferUriFactoryTest.php

@@ -10,6 +10,7 @@
 use PHPUnit\Framework\Attributes\UsesClass;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use ReflectionMethod;
 use RuntimeException;
 use SimpleSAML\Module\oidc\Bridges\SspBridge;
 use SimpleSAML\Module\oidc\Bridges\SspBridge\Utils;
@@ -98,6 +99,43 @@ public function testFallbackUserIdentifierDoesNotLogAttributesOrExceptionDetails
         $this->assertStringNotContainsString($sensitiveExceptionValue, $logs);
     }
 
+    public function testByValueOfferSurvivesQueryParsing(): void
+    {
+        // Appended raw, the '&' would split the offer into a second query parameter and the '#' would
+        // truncate it into a fragment, so a wallet would never see the whole issuer.
+        $issuer = 'https://issuer.example.org/vci?tenant=a&region=b#frag';
+
+        $credentialOfferUri = $this->factory(
+            $this->createMock(LoggerService::class),
+            $this->createMock(UserIdentifierResolver::class),
+            issuer: $issuer,
+        )->buildForAuthorization(['credential-configuration']);
+
+        $parameters = $this->parseOfferUriQuery($credentialOfferUri);
+
+        $this->assertSame(['credential_offer'], array_keys($parameters));
+        $offer = json_decode($parameters['credential_offer'], true, 512, JSON_THROW_ON_ERROR);
+        $this->assertSame($issuer, $offer['credential_issuer']);
+    }
+
+    public function testByReferenceOfferSurvivesQueryParsing(): void
+    {
+        // An offer passed by reference is a URL which may carry a query string of its own.
+        $offerUri = 'https://issuer.example.org/offers/1?tenant=a&format=jwt#frag';
+
+        $factory = $this->factory(
+            $this->createMock(LoggerService::class),
+            $this->createMock(UserIdentifierResolver::class),
+        );
+
+        $parameters = $this->parseOfferUriQuery(
+            (new ReflectionMethod($factory, 'buildUri'))->invoke($factory, $offerUri),
+        );
+
+        $this->assertSame(['credential_offer_uri'], array_keys($parameters));
+        $this->assertSame($offerUri, $parameters['credential_offer_uri']);
+    }
+
     public function testBuildTxCodeGeneratesFourDigitNumericCode(): void
     {
         $txCode = $this->factory(
@@ -116,14 +154,15 @@ private function factory(
         ?ClientRepository $clientRepository = null,
         ?UserRepository $userRepository = null,
         ?UserEntityFactory $userEntityFactory = null,
+        string $issuer = 'https://issuer.example.org',
     ): CredentialOfferUriFactory {
         $moduleConfig = $this->createMock(ModuleConfig::class);
         $moduleConfig->method('getVciCredentialConfigurationIdsSupported')
             ->willReturn(['credential-configuration']);
         $moduleConfig->method('getUserIdentifierAttributes')->willReturn(['uid']);
         $moduleConfig->method('getDefaultUsersEmailAttributeName')->willReturn('mail');
         $moduleConfig->method('getAuthCodeDuration')->willReturn(new DateInterval('PT10M'));
-        $moduleConfig->method('getIssuer')->willReturn('https://issuer.example.org');
+        $moduleConfig->method('getIssuer')->willReturn($issuer);
 
         $random = $this->createMock(Random::class);
         $random->method('generateID')->willReturn('pre-authorized-code-secret');
@@ -149,6 +188,23 @@ private function factory(
         );
     }
 
+    /**
+     * Parse the query of an offer URI back into parameters. parse_url() rejects the
+     * openid-credential-offer:// scheme outright, so the prefix is stripped by hand.
+     *
+     * @return array<string, string>
+     */
+    private function parseOfferUriQuery(string $credentialOfferUri): array
+    {
+        $prefix = 'openid-credential-offer://?';
+        $this->assertStringStartsWith($prefix, $credentialOfferUri);
+
+        parse_str(substr($credentialOfferUri, strlen($prefix)), $parameters);
+
+        /** @var array<string, string> $parameters */
+        return $parameters;
+    }
+
     private function captureLogs(LoggerService&MockObject $logger, string $level): void
     {
         $logger->method($level)->willReturnCallback(