Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom strategy for handling invalid cookies #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@
<artifactId>assertj-core</artifactId>
<version>3.15.0</version>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>3.3.3</version>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson</groupId>
<artifactId>jackson-bom</artifactId>
Expand Down Expand Up @@ -152,6 +157,11 @@
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package com.innoq.spring.cookie.flash;

import com.innoq.spring.cookie.flash.codec.FlashMapListCodec;
import com.innoq.spring.cookie.flash.verification.CookieVerificationFailureHandler;
import com.innoq.spring.cookie.security.CookieValueSigner;
import org.springframework.util.Assert;
import org.springframework.web.servlet.FlashMap;
Expand All @@ -36,6 +38,8 @@ public final class CookieFlashMapManager extends AbstractFlashMapManager {
private final FlashMapListCodec codec;
private final CookieValueSigner signer;
private final String cookieName;
private CookieVerificationFailureHandler verificationFailureHandler =
CookieVerificationFailureHandler.ignoreFailures();

public CookieFlashMapManager(FlashMapListCodec codec,
CookieValueSigner signer) {
Expand All @@ -52,6 +56,12 @@ public CookieFlashMapManager(FlashMapListCodec codec,
this.cookieName = cookieName;
}

public void setVerificationFailureHandler(
CookieVerificationFailureHandler verificationFailureHandler) {
Assert.notNull(verificationFailureHandler, "CookieVerificationFailureHandler must not be null");
this.verificationFailureHandler = verificationFailureHandler;
}

@Override
protected List<FlashMap> retrieveFlashMaps(HttpServletRequest request) {
final Cookie cookie = getCookie(request, cookieName);
Expand Down Expand Up @@ -87,16 +97,14 @@ protected Object getFlashMapsMutex(HttpServletRequest request) {
private List<FlashMap> decode(String value) {
final String[] signatureAndPayload = reverse(value).split("--", 2);
if (signatureAndPayload.length != 2) {
// TODO logging
return null;
return verificationFailureHandler.onInvalidValue(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the semantics of this failure, i.e. what went wrong? And is is worth distinguishing between onInvalidValue() and onInvalidSignature()? In both cases, the content of the cookie was not as expected.

}

final String signature = reverse(signatureAndPayload[0]);
final String payload = reverse(signatureAndPayload[1]);

if (!isVerified(payload, signature)) {
// TODO logging
return null;
return verificationFailureHandler.onInvalidSignature(payload, signature);
}

return codec.decode(payload);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.innoq.spring.cookie.flash;
package com.innoq.spring.cookie.flash.codec;

import org.springframework.web.servlet.FlashMap;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;
import com.innoq.spring.cookie.flash.FlashMapListCodec;
import com.innoq.spring.cookie.flash.codec.FlashMapListCodec;
import org.springframework.util.Assert;
import org.springframework.web.servlet.FlashMap;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright 2018 innoQ Deutschland GmbH
*
* Licensed 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
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* 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 com.innoq.spring.cookie.flash.verification;

import org.springframework.web.servlet.FlashMap;

import java.util.List;

public interface CookieVerificationFailureHandler {

List<FlashMap> onInvalidValue(String value);

List<FlashMap> onInvalidSignature(String payload, String signature);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be legal to throw e.g. an IllegalStateException in case the signature is invalid? I assume that would lead to a 500 in Spring MVC, which would be good.


static CookieVerificationFailureHandler ignoreFailures() {
return IgnoreCookieVerificationFailureHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Copyright 2018 innoQ Deutschland GmbH
*
* Licensed 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
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* 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 com.innoq.spring.cookie.flash.verification;

import org.springframework.web.servlet.FlashMap;

import java.util.List;

enum IgnoreCookieVerificationFailureHandler
implements CookieVerificationFailureHandler {
INSTANCE;

@Override
public List<FlashMap> onInvalidValue(String value) {
return null;
}

@Override
public List<FlashMap> onInvalidSignature(String payload, String signature) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.innoq.spring.cookie.flash;

import com.innoq.spring.cookie.flash.codec.jackson.JacksonFlashMapListCodec;
import com.innoq.spring.cookie.flash.verification.CookieVerificationFailureHandler;
import com.innoq.spring.cookie.security.CookieValueSigner;
import org.junit.jupiter.api.Test;
import org.springframework.mock.web.MockHttpServletRequest;
Expand All @@ -30,6 +31,9 @@
import static java.util.Collections.emptyList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

class CookieFlashMapManagerTest {

Expand Down Expand Up @@ -65,6 +69,43 @@ void retrieveFlashMaps_withValidCookie_returnsFlashMaps() {
assertThat(flashMap.getTargetRequestPath()).isEqualTo("/foo");
}

@Test
void retrieveFlashMaps_withInvalidCookieValue_callsGivenVerificationFailureHandlerAndUsesItsReturnValue() {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
request.setCookies(new Cookie("flash", "ABCDEF"));

CookieVerificationFailureHandler verificationFailureHandler =
mock(CookieVerificationFailureHandler.class);
when(verificationFailureHandler.onInvalidValue("ABCDEF")).thenReturn(null);

sut.setVerificationFailureHandler(verificationFailureHandler);

List<FlashMap> flashMaps = sut.retrieveFlashMaps(request);

assertThat(flashMaps).isNull();
verify(verificationFailureHandler).onInvalidValue("ABCDEF");
}

@Test
void retrieveFlashMaps_withInvalidCookieValueSignatur_callsGivenVerificationFailureHandlerAndUsesItsReturnValue() {
String cookieValue = "abcd--efgh";

MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
request.setCookies(new Cookie("flash", cookieValue));

CookieVerificationFailureHandler verificationFailureHandler =
mock(CookieVerificationFailureHandler.class);
when(verificationFailureHandler.onInvalidSignature("abcd", "efgh"))
.thenReturn(null);

sut.setVerificationFailureHandler(verificationFailureHandler);

List<FlashMap> flashMaps = sut.retrieveFlashMaps(request);

assertThat(flashMaps).isNull();
verify(verificationFailureHandler).onInvalidSignature("abcd", "efgh");
}

@Test
void updateFlashMaps_withSingleFlashMap_writesCookie() {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
Expand Down