(knox) 28/37: KNOX-3414: build a well-formed success redirect when redirect_uri has a query (review finding M7)

[sm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

smolnar82 pushed a commit to branch knox_idf
in repository https://gitbox.apache.org/repos/asf/knox.git

commit 5827bb196b3e4d082dd6d1dd6fdb545b51985497
Author: Sandor Molnar <sm...@apache.org>
AuthorDate: Tue Aug 11 23:36:59 2026 +0200

    KNOX-3414: build a well-formed success redirect when redirect_uri has a query (review finding M7)
    
    redirectToAuthSuccess appended "?code=...&state=..." unconditionally. When the
    registered redirect_uri already carried a query string (e.g.
    https://app.example/cb?ui=dark), the result had two "?" separators, so the
    client parsed neither code nor state and the authorization-code flow silently
    failed for those clients.
    
    Extract buildSuccessRedirect, which uses "&" when the redirect_uri already
    contains a query string and "?" otherwise, preserving any pre-existing params.
    
    Covered by AuthorizeResourceSuccessRedirectTest.
    
    Co-Authored-By: Claude Opus 4.8 <no...@anthropic.com>
---
 .../gateway/service/knoxidf/AuthorizeResource.java | 23 ++++++--
 .../AuthorizeResourceSuccessRedirectTest.java      | 67 ++++++++++++++++++++++
 2 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java
index 4b6d7ec7a..84c213c95 100644
--- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java
+++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java
@@ -263,15 +263,28 @@ public class AuthorizeResource extends PasscodeTokenResourceBase {
     }
 
     private Response redirectToAuthSuccess(final AuthorizeRequestMetadata authorizeRequestMetadata, final String code) {
-        final String redirectLocation;
         try {
-            redirectLocation = authorizeRequestMetadata.getRedirectUri()
-                    + "?code=" + URLEncoder.encode(code, UTF_8)
-                    + "&state=" + URLEncoder.encode(authorizeRequestMetadata.getState(), UTF_8);
+            final String redirectLocation = buildSuccessRedirect(
+                    authorizeRequestMetadata.getRedirectUri(), code, authorizeRequestMetadata.getState());
+            return Response.seeOther(URI.create(redirectLocation)).build();
         } catch (UnsupportedEncodingException e) {
             throw new RuntimeException(e); //This should never happen with UTF-8
         }
-        return Response.seeOther(URI.create(redirectLocation)).build();
+    }
+
+    /**
+     * Appends the {@code code} and {@code state} authorization-response params to the client's
+     * registered redirect_uri. Uses {@code &} as the separator when the redirect_uri already carries
+     * a query string and {@code ?} otherwise, so a registered URI such as
+     * {@code https://app.example/cb?ui=1} produces {@code ...?ui=1&code=...&state=...} rather than a
+     * malformed second {@code ?} that the client would fail to parse. Package-private for testing.
+     */
+    static String buildSuccessRedirect(final String redirectUri, final String code, final String state)
+            throws UnsupportedEncodingException {
+        final String separator = redirectUri.contains("?") ? "&" : "?";
+        return redirectUri
+                + separator + "code=" + URLEncoder.encode(code, UTF_8)
+                + "&state=" + URLEncoder.encode(state, UTF_8);
     }
 
     @GET
diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceSuccessRedirectTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceSuccessRedirectTest.java
new file mode 100644
index 000000000..30da0eb25
--- /dev/null
+++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceSuccessRedirectTest.java
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.knox.gateway.service.knoxidf;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.net.URI;
+import java.util.List;
+
+import org.apache.http.NameValuePair;
+import org.apache.http.client.utils.URLEncodedUtils;
+import org.junit.Test;
+
+/**
+ * Verifies the authorization-code success redirect stays well-formed when the registered
+ * redirect_uri already carries a query string (review finding M7). Appending {@code ?code=...}
+ * unconditionally produced a second {@code ?}, so the client parsed neither code nor state.
+ */
+public class AuthorizeResourceSuccessRedirectTest {
+
+  private static List<NameValuePair> queryParams(final String location) {
+    return URLEncodedUtils.parse(URI.create(location), java.nio.charset.StandardCharsets.UTF_8);
+  }
+
+  private static String param(final List<NameValuePair> params, final String name) {
+    return params.stream().filter(p -> p.getName().equals(name)).map(NameValuePair::getValue)
+        .findFirst().orElse(null);
+  }
+
+  @Test
+  public void testRedirectUriWithoutQueryUsesQuestionMark() throws Exception {
+    final String location = AuthorizeResource.buildSuccessRedirect(
+        "https://app.example/cb", "the code", "the state");
+    assertTrue("A redirect_uri without a query must start its params with '?'.",
+        location.startsWith("https://app.example/cb?"));
+    final List<NameValuePair> params = queryParams(location);
+    assertEquals("the code", param(params, "code"));
+    assertEquals("the state", param(params, "state"));
+  }
+
+  @Test
+  public void testRedirectUriWithExistingQueryUsesAmpersand() throws Exception {
+    final String location = AuthorizeResource.buildSuccessRedirect(
+        "https://app.example/cb?ui=dark", "the code", "the state");
+    // Exactly one '?' -- the code/state must be appended with '&', not a second '?'.
+    assertEquals("There must be exactly one query separator.", 1, location.chars().filter(c -> c == '?').count());
+    final List<NameValuePair> params = queryParams(location);
+    assertEquals("The pre-existing query param must survive.", "dark", param(params, "ui"));
+    assertEquals("the code", param(params, "code"));
+    assertEquals("the state", param(params, "state"));
+  }
+}