[
https://issues.apache.org/jira/browse/TOMEE-4584?focusedWorklogId=1006173&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1006173
]
ASF GitHub Bot logged work on TOMEE-4584:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 19/Feb/26 16:11
Start Date: 19/Feb/26 16:11
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2472:
URL: https://github.com/apache/tomee/pull/2472#discussion_r2828759723
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
+ String[] values = parameterMap.get(name);
+ return values == null || values.length == 0 ? null : values[0];
+ }
+
+ @Override
+ public Map<String, String[]> getParameterMap() {
+ return parameterMap;
Review Comment:
The parameterMap field can be null when deserializing old JSON, but is
returned directly without any defensive handling. Consider returning
Collections.emptyMap() when parameterMap is null to prevent
NullPointerExceptions in calling code, consistent with the defensive approach
used for headers.
```suggestion
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
return Collections.enumeration(params.keySet());
}
@Override
public String[] getParameterValues(String name) {
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
return params.get(name);
}
@Override
public String getParameter(String name) {
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
String[] values = params.get(name);
return values == null || values.length == 0 ? null :
values[0];
}
@Override
public Map<String, String[]> getParameterMap() {
return parameterMap != null ? parameterMap :
Collections.emptyMap();
```
##########
tomee/tomee-security/src/test/java/org/apache/tomee/security/http/SavedRequestTest.java:
##########
@@ -71,6 +73,10 @@ public void deserialization() throws Exception {
assertEquals("PATCH", request.getMethod());
assertEquals("foo=bar", request.getQueryString());
assertEquals("http://example.com/foo";, request.getUrl());
+ assertNotNull(request.getParameterMap());
+ assertNotNull(request.getParameterMap().get("foo"));
+ assertEquals(1, request.getParameterMap().get("foo").length);
+ assertEquals("bar", request.getParameterMap().get("foo")[0]);
}
@Test
Review Comment:
Add a test case for backward compatibility to verify that deserializing JSON
without the parameterMap field (from older versions) doesn't throw exceptions.
This is important since SavedRequest instances are stored in sessions and may
persist across deployments. The test should verify that old JSON strings
lacking parameterMap can still be deserialized and that the parameter-related
methods return appropriate defaults (e.g., empty collections) instead of
throwing NullPointerExceptions.
```suggestion
@Test
public void deserializationWithoutParameterMap() throws Exception {
// JSON produced by older versions that did not include the
parameterMap field
String json =
"{\"cookies\":[{\"name\":\"first\",\"value\":\"val1\",\"attributes\":{}},{\"name\":\"second\",\"value\":\"val2\",\"attributes\":{}}],\"headers\":{\"header1\":[\"h1val1\",\"h1val2\"],\"header2\":[\"h2val1\"]},\"method\":\"PATCH\",\"queryString\":\"foo=bar\",\"url\":\"http://example.com/foo\"}";;
SavedRequest request = SavedRequest.fromJson(json);
assertNotNull(request);
assertEquals(2, request.getCookies().length);
assertEquals("first", request.getCookies()[0].getName());
assertEquals("val1", request.getCookies()[0].getValue());
assertEquals("second", request.getCookies()[1].getName());
assertEquals("val2", request.getCookies()[1].getValue());
assertEquals(2, request.getHeaders().size());
assertEquals(List.of("h1val1", "h1val2"),
request.getHeaders().get("header1"));
assertEquals(List.of("h2val1"), request.getHeaders().get("header2"));
assertEquals("PATCH", request.getMethod());
assertEquals("foo=bar", request.getQueryString());
assertEquals("http://example.com/foo";, request.getUrl());
// parameterMap should be initialized to a safe default (e.g., empty
map) rather than null
assertNotNull(request.getParameterMap());
assertTrue(request.getParameterMap().isEmpty());
}
@Test
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
Review Comment:
The parameterMap field is accessed without null checking. This will cause a
NullPointerException when deserializing JSON from older versions that don't
include the parameterMap field. The headers field follows a null-safe pattern
(see lines 98, 103, 112) that should be applied here as well. Consider
returning Collections.emptyEnumeration() when parameterMap is null.
```suggestion
return parameterMap != null
? Collections.enumeration(parameterMap.keySet())
: Collections.emptyEnumeration();
}
@Override
public String[] getParameterValues(String name) {
return parameterMap != null ? parameterMap.get(name) : null;
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
Review Comment:
The parameterMap field is accessed without null checking. When parameterMap
is null (e.g., when deserializing old JSON), calling parameterMap.get(name)
will throw a NullPointerException. Add a null check: if parameterMap is null,
return null directly.
```suggestion
public Enumeration<String> getParameterNames() {
if (parameterMap == null) {
return Collections.emptyEnumeration();
}
return Collections.enumeration(parameterMap.keySet());
}
@Override
public String[] getParameterValues(String name) {
if (parameterMap == null) {
return null;
}
return parameterMap.get(name);
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
+ String[] values = parameterMap.get(name);
+ return values == null || values.length == 0 ? null : values[0];
+ }
+
+ @Override
+ public Map<String, String[]> getParameterMap() {
+ return parameterMap;
Review Comment:
The parameterMap field is accessed without null checking. When parameterMap
is null (e.g., when deserializing old JSON), this will return null directly.
Consider adding a null check similar to the pattern used for headers (lines
103, 112) to return null when parameterMap is null or an empty array when the
parameter doesn't exist.
```suggestion
return parameterMap != null
? Collections.enumeration(parameterMap.keySet())
: Collections.emptyEnumeration();
}
@Override
public String[] getParameterValues(String name) {
if (parameterMap == null) {
return null;
}
String[] values = parameterMap.get(name);
return values != null ? values : new String[0];
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
String[] values = parameterMap.get(name);
return values == null || values.length == 0 ? null :
values[0];
}
@Override
public Map<String, String[]> getParameterMap() {
return parameterMap != null ? parameterMap :
Collections.emptyMap();
```
Issue Time Tracking
-------------------
Worklog Id: (was: 1006173)
Time Spent: 20m (was: 10m)
> OpenIdAuthenticationMechanism redirectToOriginalResource does not restore
> parameterMap
> --------------------------------------------------------------------------------------
>
> Key: TOMEE-4584
> URL: https://issues.apache.org/jira/browse/TOMEE-4584
> Project: TomEE
> Issue Type: Bug
> Affects Versions: 10.1.4
> Reporter: Markus Jung
> Assignee: Markus Jung
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> See spec/javadoc of OpenIdAuthenticationMechanismDefinition:
>
> {code:java}
> /**
> * Optional. Automatically redirects the caller (the end-user) from
> * the redirect URI defined by the <code>redirectURI</code> attribute
> * to the resource the end-user originally requested in a "login to continue"
> * scenario.
> *
> * <p>
> * After arriving at the original requested resource, the runtime restores
> * the request as it originally happened, including cookies, headers, the
> * request method and the request parameters in the same way as done when
> * using the {@link LoginToContinue} feature.
> *
> * @return
> */
> boolean redirectToOriginalResource() default false; {code}
>
> The following methods do not work as intended with redirectToOriginalResource:
> * getParameterNames()
> * getParameterValues(String name)
> * getParameter(String name)
> * getParameterMap()
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
