You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/12/08 12:35:33 UTC
[GitHub] [knox] moresandeep opened a new pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
moresandeep opened a new pull request #391:
URL: https://github.com/apache/knox/pull/391
## What changes were proposed in this pull request?
Fix an issue where set-cookie responses from are broken when backend services send set-cookies with no spaces.
## How was this patch tested?
Locally tested
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #391:
URL: https://github.com/apache/knox/pull/391#discussion_r538431430
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultDispatch.java
##########
@@ -382,7 +382,14 @@ private String calculateResponseHeaderValue(Header headerToCheck, Map<String, Se
return ""; // we should exclude all -> there should not be any value added with this header
} else {
final String separator = SET_COOKIE.equalsIgnoreCase(headerNameToCheck) ? "; " : " ";
- Set<String> headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ Set<String> headerValuesToCheck;
+ if(headerToCheck.getName().equalsIgnoreCase(SET_COOKIE)) {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split(";")));
+ /* trim */
+ headerValuesToCheck = headerValuesToCheck.stream().map(String::trim).collect(Collectors.toSet());
+ } else {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ }
headerValuesToCheck = headerValuesToCheck.stream().map(h -> h.replaceAll(separator.trim(), "")).collect(Collectors.toSet());
Review comment:
Sorry i did not follow, in case of line 389 we need trimming for the case where cookies use spaces and ; together.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #391:
URL: https://github.com/apache/knox/pull/391
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #391:
URL: https://github.com/apache/knox/pull/391#discussion_r538440317
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultDispatch.java
##########
@@ -382,7 +382,14 @@ private String calculateResponseHeaderValue(Header headerToCheck, Map<String, Se
return ""; // we should exclude all -> there should not be any value added with this header
} else {
final String separator = SET_COOKIE.equalsIgnoreCase(headerNameToCheck) ? "; " : " ";
- Set<String> headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ Set<String> headerValuesToCheck;
+ if(headerToCheck.getName().equalsIgnoreCase(SET_COOKIE)) {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split(";")));
+ /* trim */
+ headerValuesToCheck = headerValuesToCheck.stream().map(String::trim).collect(Collectors.toSet());
+ } else {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ }
headerValuesToCheck = headerValuesToCheck.stream().map(h -> h.replaceAll(separator.trim(), "")).collect(Collectors.toSet());
Review comment:
I see it now, line #393 can be eliminated by trimming #384
I'll test the changes and update the patch, hopefully nothing breaks :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #391:
URL: https://github.com/apache/knox/pull/391#discussion_r538419292
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultDispatch.java
##########
@@ -382,7 +382,14 @@ private String calculateResponseHeaderValue(Header headerToCheck, Map<String, Se
return ""; // we should exclude all -> there should not be any value added with this header
} else {
final String separator = SET_COOKIE.equalsIgnoreCase(headerNameToCheck) ? "; " : " ";
- Set<String> headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ Set<String> headerValuesToCheck;
+ if(headerToCheck.getName().equalsIgnoreCase(SET_COOKIE)) {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split(";")));
+ /* trim */
+ headerValuesToCheck = headerValuesToCheck.stream().map(String::trim).collect(Collectors.toSet());
+ } else {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ }
headerValuesToCheck = headerValuesToCheck.stream().map(h -> h.replaceAll(separator.trim(), "")).collect(Collectors.toSet());
Review comment:
Why the need to trim the separator here? Why not set the value to the trimmed form in the first place?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #391: KNOX-2479 - Fix an issue where Knox munges set-cookie header
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #391:
URL: https://github.com/apache/knox/pull/391#discussion_r538778342
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultDispatch.java
##########
@@ -382,7 +382,14 @@ private String calculateResponseHeaderValue(Header headerToCheck, Map<String, Se
return ""; // we should exclude all -> there should not be any value added with this header
} else {
final String separator = SET_COOKIE.equalsIgnoreCase(headerNameToCheck) ? "; " : " ";
- Set<String> headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ Set<String> headerValuesToCheck;
+ if(headerToCheck.getName().equalsIgnoreCase(SET_COOKIE)) {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split(";")));
+ /* trim */
+ headerValuesToCheck = headerValuesToCheck.stream().map(String::trim).collect(Collectors.toSet());
+ } else {
+ headerValuesToCheck = new HashSet<>(Arrays.asList(headerToCheck.getValue().trim().split("\\s+")));
+ }
headerValuesToCheck = headerValuesToCheck.stream().map(h -> h.replaceAll(separator.trim(), "")).collect(Collectors.toSet());
Review comment:
Ok, I see separator gets used for the output as well. LGTM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org