Fix multiple decoding of parameter values (#108)

* refactor: add ContractTestBase to simplify creation of new E2E tests
* feat: add reproducer and fix for phone number issue

---------

Signed-off-by: Pascal Krause <[email protected]>

Author: pk-work

src/main/asciidoc/index.adoc

@@ -78,6 +78,25 @@ The {@link io.vertx.openapi.contract.OpenAPIContract} interface offers methods t
 {@link examples.ContractExamples#pathParameterOperationExample}
 ----
 
+=== Extensions
+
+Vert.x OpenAPI supports the extension `x-vertx-openapi-urldecode` to specify if a `cookie` or `header` parameter should
+be URL decoded. The default is `false`. `Query` parameters and `path` parameters gets always URL decoded.
+
+[source,yaml]
+----
+paths:
+  /forceEncoding:
+    get:
+      operationId: forceEncoding
+      parameters:
+        - in: header
+          name: headerWithEncodedContent
+          x-vertx-openapi-urldecode: true
+          schema:
+            type: string
+----
+
 == Validation
 
 Vert.x OpenAPI checks both whether the content is syntactically correct and whether it corresponds to the schema.

src/main/java/io/vertx/openapi/contract/Parameter.java

@@ -26,6 +26,8 @@
 @VertxGen
 public interface Parameter extends OpenAPIObject {
 
+  String EXTENSION_URLDECODE = "x-vertx-openapi-urldecode";
+
   /**
    * @return name of this parameter
    */

src/main/java/io/vertx/openapi/validation/RequestUtils.java

@@ -80,7 +80,7 @@ public static Future<ValidatableRequest> extract(HttpServerRequest request, Oper
           break;
         case PATH:
           int segment = findPathSegment(operation.getAbsoluteOpenAPIPath(), param.getName());
-          pathParams.put(param.getName(), extractPathParameters(request, segment));
+          pathParams.put(param.getName(), extractPathParameters(param, request, segment));
           break;
         case QUERY:
           query.put(param.getName(), extractQuery(request, param));
@@ -104,32 +104,37 @@ public static Future<ValidatableRequest> extract(HttpServerRequest request, Oper
 
   private static RequestParameter extractCookie(HttpServerRequest request, Parameter parameter) {
     Collection<String> cookies =
-        request.cookies(parameter.getName()).stream().map(Cookie::getValue).collect(Collectors.toList());
+        request.cookies(parameter.getName()).stream().map(Cookie::getValue).map(c -> urlDecodeIfRequired(parameter, c))
+            .collect(Collectors.toList());
     return joinFormValues(cookies, parameter, () -> {
       String explodedObject =
-          request.cookies().stream().map(c -> c.getName() + "=" + decodeUrl(c.getValue())).collect(joining("&"));
+          request.cookies().stream().map(c -> c.getName() + "=" + urlDecodeIfRequired(parameter, c.getValue()))
+              .collect(joining("&"));
       return new RequestParameterImpl(explodedObject);
     });
   }
 
   private static RequestParameter extractHeaders(HttpServerRequest request, Parameter parameter) {
     String headerValue = request.getHeader(parameter.getName());
-    return new RequestParameterImpl(decodeUrl(headerValue));
+    return new RequestParameterImpl(urlDecodeIfRequired(parameter, headerValue));
   }
 
-  private static RequestParameter extractPathParameters(HttpServerRequest request, int segment) {
+  private static RequestParameter extractPathParameters(Parameter param, HttpServerRequest request, int segment) {
     String[] pathSegments = request.path().substring(1).split("/");
     if (pathSegments.length < segment) {
       return EMPTY;
     }
     return new RequestParameterImpl(decodeUrl(pathSegments[segment - 1]));
   }
 
+  /**
+   * It seems that query parameters are always decoded, so we MUST NOT decode them again.
+   */
   private static RequestParameter extractQuery(HttpServerRequest request, Parameter parameter) {
     Collection<String> queryParams = request.params().getAll(parameter.getName());
     return joinFormValues(queryParams, parameter, () -> {
       String decodedQuery =
-          request.params().entries().stream().map(entry -> entry.getKey() + "=" + decodeUrl(entry.getValue()))
+          request.params().entries().stream().map(entry -> entry.getKey() + "=" + entry.getValue())
               .collect(joining("&"));
       return new RequestParameterImpl(decodedQuery);
     });
@@ -146,18 +151,18 @@ private static RequestParameter joinFormValues(Collection<String> formValues, Pa
         if (parameter.isExplode()) {
           return explodedObjectSupplier.get();
         } else {
-          return new RequestParameterImpl(decodeUrl(GET_FIRST_VALUE.apply(formValues)));
+          return new RequestParameterImpl(GET_FIRST_VALUE.apply(formValues));
         }
       case ARRAY:
         if (parameter.isExplode()) {
           String explodedString =
-              formValues.stream().map(fv -> parameter.getName() + "=" + decodeUrl(fv)).collect(joining("&"));
+              formValues.stream().map(fv -> parameter.getName() + "=" + fv).collect(joining("&"));
           return new RequestParameterImpl(explodedString);
         } else {
-          return new RequestParameterImpl(decodeUrl(GET_FIRST_VALUE.apply(formValues)));
+          return new RequestParameterImpl(GET_FIRST_VALUE.apply(formValues));
         }
       default:
-        return new RequestParameterImpl(decodeUrl(GET_FIRST_VALUE.apply(formValues)));
+        return new RequestParameterImpl(GET_FIRST_VALUE.apply(formValues));
     }
   }
 
@@ -167,7 +172,16 @@ public static int findPathSegment(String templatePath, String parameterName) {
     return (int) templatePath.subSequence(0, idx).chars().filter(c -> c == '/').count();
   }
 
-  static String decodeUrl(String encoded) {
+  static String urlDecodeIfRequired(Parameter param, String value) {
+    boolean requiresDecoding =
+        Boolean.TRUE.equals(param.getExtensions().getOrDefault(Parameter.EXTENSION_URLDECODE, false));
+    if (requiresDecoding) {
+      return decodeUrl(value);
+    }
+    return value;
+  }
+
+  private static String decodeUrl(String encoded) {
     try {
       return encoded == null ? null : URLDecoder.decode(encoded, StandardCharsets.UTF_8);
     } catch (Exception e) {

src/main/java/io/vertx/openapi/validation/transformer/ParameterTransformer.java

@@ -57,8 +57,7 @@ public Object transform(Parameter parameter, String rawValue) {
    * @return An {@link Object} holding the transformed value.
    */
   public Object transformPrimitive(Parameter parameter, String rawValue) {
-    boolean isString = STRING.equals(parameter.getSchemaType());
-    if (isString && rawValue.matches("\\w+")) {
+    if (STRING.equals(parameter.getSchemaType())) {
       return rawValue;
     }
 

src/test/java/io/vertx/tests/test/E2ETest.java

@@ -14,54 +14,27 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static io.vertx.tests.ResourceHelper.getRelatedTestResourcePath;
-import static io.vertx.tests.ResourceHelper.loadJson;
 
 import io.netty.handler.codec.http.HttpHeaderValues;
 import io.vertx.core.Future;
 import io.vertx.core.http.HttpClientRequest;
-import io.vertx.core.http.HttpClientResponse;
 import io.vertx.core.http.HttpHeaders;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.json.JsonArray;
 import io.vertx.core.json.JsonObject;
 import io.vertx.junit5.Timeout;
 import io.vertx.junit5.VertxTestContext;
-import io.vertx.openapi.contract.OpenAPIContract;
-import io.vertx.openapi.validation.RequestValidator;
-import io.vertx.openapi.validation.ResponseValidator;
 import io.vertx.openapi.validation.ValidatableResponse;
-import io.vertx.openapi.validation.ValidatedRequest;
-import io.vertx.tests.test.base.HttpServerTestBase;
+import io.vertx.tests.test.base.ContractTestBase;
 import java.nio.file.Path;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Consumer;
-import java.util.function.Function;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.parallel.Execution;
-import org.junit.jupiter.api.parallel.ExecutionMode;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
-// We need to enforce sequential execution, because the tests are manipulating fields
-@Execution(ExecutionMode.SAME_THREAD)
-class E2ETest extends HttpServerTestBase {
-
-  private OpenAPIContract contract;
-  private RequestValidator requestValidator;
-  private ResponseValidator responseValidator;
-
-  Future<Void> setupContract(String basePath, VertxTestContext testContext) {
-    JsonObject contractJson = loadJson(vertx, getRelatedTestResourcePath(E2ETest.class).resolve("petstore.json"));
-    // Modify base path
-    JsonObject server = contractJson.getJsonArray("servers").getJsonObject(0);
-    server.put("url", server.getString("url") + basePath);
-    return OpenAPIContract.from(vertx, contractJson).onComplete(testContext.succeeding(contract -> {
-      this.contract = contract;
-      this.requestValidator = RequestValidator.create(vertx, contract);
-      this.responseValidator = ResponseValidator.create(vertx, contract);
-    })).mapEmpty();
-  }
+class E2ETest extends ContractTestBase {
+  private Path CONTRACT_FILE = getRelatedTestResourcePath(E2ETest.class).resolve("petstore.json");
 
   @Timeout(value = 2, timeUnit = TimeUnit.SECONDS)
   @ParameterizedTest(name = "{index} Test with base path: {0}")
@@ -70,16 +43,16 @@ void testExtractPath(String basePath, VertxTestContext testContext) {
     int expectedPetId = 1;
     JsonObject expectedPet = new JsonObject().put("id", 1).put("name", "FooBar");
 
-    setupContract(basePath, testContext).compose(v -> createValidationHandler(req -> {
+    loadContract(CONTRACT_FILE, basePath, testContext).compose(v -> createServerWithRequestProcessor(req -> {
       testContext.verify(() -> {
         int petId = req.getPathParameters().get("petId").getInteger();
         assertThat(petId).isEqualTo(expectedPetId);
       });
       return ValidatableResponse.create(200, expectedPet.toBuffer(), HttpHeaderValues.APPLICATION_JSON.toString());
-    }, contract.operation("showPetById").getOperationId(), testContext)).compose(v -> {
+    }, "showPetById", testContext)).compose(v -> {
       String bp = basePath.endsWith("/") ? basePath.substring(0, basePath.length() - 1) : basePath;
       Future<HttpClientRequest> req = createRequest(HttpMethod.GET, bp + "/pets/" + expectedPetId);
-      return request(req, resp -> resp.body().onComplete(testContext.succeeding(
+      return sendAndVerifyRequest(req, resp -> resp.body().onComplete(testContext.succeeding(
           body -> testContext.verify(() -> {
             assertThat(resp.statusCode()).isEqualTo(200);
             assertThat(body.toJsonObject()).isEqualTo(expectedPet);
@@ -104,7 +77,7 @@ public void sendMultipartFormDataRequest(VertxTestContext testContext) {
             .put("email", "[email protected]")
             .put("phone", "5555555555"));
 
-    setupContract("", testContext).compose(v -> createValidationHandler(req -> {
+    loadContract(CONTRACT_FILE, testContext).compose(v -> createServerWithRequestProcessor(req -> {
       testContext.verify(() -> {
         assertThat(req.getBody()).isNotNull();
         JsonObject jsonReq = req.getBody().getJsonObject();
@@ -114,7 +87,7 @@ public void sendMultipartFormDataRequest(VertxTestContext testContext) {
         testContext.completeNow();
       });
       return ValidatableResponse.create(201);
-    }, contract.operation("uploadPet").getOperationId(), testContext))
+    }, "uploadPet", testContext))
         .compose(v -> createRequest(HttpMethod.POST, "/pets/upload"))
         .map(request -> request.putHeader(HttpHeaders.CONTENT_TYPE, "multipart/form-data; boundary=4ad8accc990e99c2"))
         .map(request -> request.putHeader(HttpHeaders.CONTENT_DISPOSITION, ""))
@@ -129,28 +102,13 @@ public void sendMultipartFormDataRequest(VertxTestContext testContext) {
   void testLessRestrictiveContentType(String requestContentType, VertxTestContext testContext) {
     JsonObject expectedPet = new JsonObject().put("id", 1).put("name", "FooBar");
 
-    setupContract("", testContext).compose(v -> createValidationHandler(req -> {
+    loadContract(CONTRACT_FILE, testContext).compose(v -> createServerWithRequestProcessor(req -> {
       testContext.completeNow();
       return ValidatableResponse.create(201);
-    }, contract.operation("createPets").getOperationId(), testContext))
+    }, "createPets", testContext))
         .compose(v -> createRequest(HttpMethod.POST, "/pets"))
         .map(request -> request.putHeader(HttpHeaders.CONTENT_TYPE, requestContentType))
         .compose(request -> request.send(expectedPet.toBuffer()))
         .onFailure(testContext::failNow);
   }
-
-  private Future<Void> request(Future<HttpClientRequest> request, Consumer<HttpClientResponse> verifier,
-      VertxTestContext testContext) {
-    return request.compose(HttpClientRequest::send)
-        .onSuccess(response -> testContext.verify(() -> verifier.accept(response))).onFailure(testContext::failNow)
-        .mapEmpty();
-  }
-
-  private Future<Void> createValidationHandler(Function<ValidatedRequest, ValidatableResponse> processor,
-      String operationId, VertxTestContext testContext) {
-    return createServer(request -> requestValidator.validate(request, operationId)
-        .map(processor).compose(validatableResponse -> responseValidator.validate(validatableResponse, operationId))
-        .compose(validatedResponse -> validatedResponse.send(request.response()))
-        .onFailure(testContext::failNow)).onFailure(testContext::failNow);
-  }
 }

src/test/java/io/vertx/tests/test/PhoneNumberTest.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2025, SAP SE
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+ * which is available at https://www.apache.org/licenses/LICENSE-2.0.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+ *
+ */
+
+package io.vertx.tests.test;
+
+import static com.google.common.truth.Truth.assertThat;
+import static io.vertx.core.http.HttpMethod.POST;
+import static io.vertx.tests.ResourceHelper.getRelatedTestResourcePath;
+
+import io.vertx.core.Future;
+import io.vertx.core.http.HttpClientRequest;
+import io.vertx.core.http.HttpClientResponse;
+import io.vertx.core.http.HttpHeaders;
+import io.vertx.junit5.Timeout;
+import io.vertx.junit5.VertxTestContext;
+import io.vertx.openapi.validation.ValidatableResponse;
+import io.vertx.openapi.validation.ValidatedRequest;
+import io.vertx.tests.test.base.ContractTestBase;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+class PhoneNumberTest extends ContractTestBase {
+  private Path CONTRACT_FILE = getRelatedTestResourcePath(PhoneNumberTest.class)
+      .resolve("contract_various_scenarios.yaml");
+
+  @Timeout(value = 2, timeUnit = TimeUnit.SECONDS)
+  @Test
+  @DisplayName("Test that query parameters don't get decoded twice and loose a '+' sign")
+  void testPhoneNumberInQuery(VertxTestContext testContext) {
+    String operationId = "phoneNumberInQuery";
+    String queryParam = "phoneNumber";
+    String queryValue = "+39 334 192 1829";
+    String encodedQueryValue = URLEncoder.encode(queryValue, StandardCharsets.UTF_8);
+    String queryString = queryParam + "=" + encodedQueryValue;
+
+    Function<ValidatedRequest, ValidatableResponse> requestProcessor = req -> {
+      assertThat(req.getQuery().get(queryParam).getString()).isEqualTo(queryValue);
+      return ValidatableResponse.create(200);
+    };
+
+    Consumer<HttpClientResponse> responseVerifier = resp -> testContext.verify(() -> {
+      assertThat(resp.statusCode()).isEqualTo(200);
+      testContext.completeNow();
+    });
+
+    loadContract(CONTRACT_FILE, testContext)
+        .compose(v -> createServerWithRequestProcessor(requestProcessor, operationId, testContext))
+        .compose(v -> {
+          Future<HttpClientRequest> req = createRequest(POST, "/phoneNumber?" + queryString);
+          return sendAndVerifyRequest(req, responseVerifier, testContext);
+        }).onFailure(testContext::failNow);
+  }
+
+  // @Timeout(value = 2, timeUnit = TimeUnit.SECONDS)
+  @Test
+  @DisplayName("Test that force encoding of header and cookies is respected")
+  void testForceEncodingHeaderAndCookies(VertxTestContext testContext) {
+    String operationId = "forceEncoding";
+    String headerEncoded = "headerEncoded";
+    String headerNotEncoded = "headerNotEncoded";
+    String cookieEncoded = "cookieEncoded";
+    String cookieNotEncoded = "cookieNotEncoded";
+    String encodedTestValue = "3%2C4%2C5";
+    String decodedTestValue = "3,4,5";
+
+    Function<ValidatedRequest, ValidatableResponse> requestProcessor = req -> {
+      assertThat(req.getCookies().get(cookieEncoded).getString()).isEqualTo(encodedTestValue);
+      assertThat(req.getCookies().get(cookieNotEncoded).getString()).isEqualTo(decodedTestValue);
+      assertThat(req.getHeaders().get(headerEncoded).getString()).isEqualTo(encodedTestValue);
+      assertThat(req.getHeaders().get(headerNotEncoded).getString()).isEqualTo(decodedTestValue);
+      return ValidatableResponse.create(200);
+    };
+
+    Consumer<HttpClientResponse> responseVerifier = resp -> testContext.verify(() -> {
+      assertThat(resp.statusCode()).isEqualTo(200);
+      testContext.completeNow();
+    });
+
+    loadContract(CONTRACT_FILE, testContext)
+        .compose(v -> createServerWithRequestProcessor(requestProcessor, operationId, testContext))
+        .compose(v -> {
+          Future<HttpClientRequest> req = createRequest(POST, "/phoneNumber", reqOpts -> {
+            reqOpts.putHeader(headerEncoded, encodedTestValue);
+            reqOpts.putHeader(headerNotEncoded, encodedTestValue);
+            reqOpts.putHeader(HttpHeaders.COOKIE, cookieEncoded + "=" + encodedTestValue + "; "
+                + cookieNotEncoded + "=" + encodedTestValue);
+          });
+
+          return sendAndVerifyRequest(req, responseVerifier, testContext);
+        }).onFailure(testContext::failNow);
+  }
+}