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 <[email protected]> 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 <[email protected]> --- .../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")); + } +}
