Skip to content

Commit 9f8f7df

Browse files
bridgerdierValentin Brückelrrayst
committed
Improve Error Messages Related To Session Data (#1842)
* Use 302 in all OAuth2 Redirects, adjusted tests accordingly * distinguish 'CSRF token mismatch' causes. --------- Co-authored-by: Valentin Brückel <brueckel@predic8.de> Co-authored-by: Tobias Polley <mail@tobias-polley.de>
1 parent 4723038 commit 9f8f7df

File tree

6 files changed

+54
-13
lines changed

6 files changed

+54
-13
lines changed

‎core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@
4141

4242
public class OAuth2CallbackRequestHandler {
4343
private static final Logger log = LoggerFactory.getLogger(OAuth2CallbackRequestHandler.class);
44+
public static final String MEMBRANE_MISSING_SESSION = "Missing session.";
45+
public static final String MEMBRANE_CSRF_TOKEN_MISSING_IN_SESSION = "CSRF token missing in session.";
46+
public static final String MEMBRANE_CSRF_TOKEN_MISMATCH = "CSRF token mismatch.";
4447

4548
LogHelper logHelper = new LogHelper();
4649
private URIFactory uriFactory;
@@ -87,7 +90,22 @@ public boolean handleRequest(Exchange exc, Session session) throws Exception {
8790
String stateFromUri = getSecurityTokenFromState(state2);
8891

8992
if (!csrfTokenMatches(session, stateFromUri)) {
90-
throw new RuntimeException("CSRF token mismatch.");
93+
if (session.isNew()) {
94+
throw new OAuth2Exception(
95+
"MEMBRANE_MISSING_SESSION",
96+
MEMBRANE_MISSING_SESSION,
97+
Response.badRequest().body(MEMBRANE_MISSING_SESSION).build());
98+
} else if (!session.get().containsKey(ParamNames.STATE)) {
99+
throw new OAuth2Exception(
100+
"MEMBRANE_CSRF_TOKEN_MISSING_IN_SESSION",
101+
MEMBRANE_CSRF_TOKEN_MISSING_IN_SESSION,
102+
Response.badRequest().body(MEMBRANE_CSRF_TOKEN_MISSING_IN_SESSION).build());
103+
}else {
104+
throw new OAuth2Exception(
105+
"MEMBRANE_CSRF_TOKEN_MISMATCH",
106+
MEMBRANE_CSRF_TOKEN_MISMATCH,
107+
Response.badRequest().body(MEMBRANE_CSRF_TOKEN_MISMATCH).build());
108+
}
91109
}
92110

93111
// state in session can be "merged" -> save the selected state in session overwriting the possibly merged value

‎core/src/main/java/com/predic8/membrane/core/interceptor/session/InMemorySessionManager.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import com.predic8.membrane.core.*;
2020
import com.predic8.membrane.core.exchange.*;
2121
import com.predic8.membrane.core.http.*;
22+
import org.slf4j.Logger;
23+
import org.slf4j.LoggerFactory;
2224

2325
import java.time.*;
2426
import java.util.*;
@@ -28,6 +30,8 @@
2830
@MCElement(name = "inMemorySessionManager2")
2931
public class InMemorySessionManager extends SessionManager {
3032

33+
private static final Logger log = LoggerFactory.getLogger(InMemorySessionManager.class);
34+
3135
final static String ID_NAME = "_in_memory_session_id";
3236
Cache<String, Session> sessions;
3337

‎core/src/main/java/com/predic8/membrane/core/interceptor/session/Session.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@
2525

2626
public class Session {
2727

28+
@JsonIgnore // we don't want this utility method to show up in JSON representations
29+
public boolean isNew() {
30+
if (isDirty)
31+
return false;
32+
if (content.size() != 1)
33+
return false;
34+
if (!content.containsKey(INTERNAL_PREFIX + AUTHORIZATION_LEVEL))
35+
return false;
36+
return AuthorizationLevel.ANONYMOUS.name().equals(content.get(INTERNAL_PREFIX + AUTHORIZATION_LEVEL));
37+
}
38+
2839
public enum AuthorizationLevel{
2940
ANONYMOUS,
3041
VERIFIED

‎core/src/main/java/com/predic8/membrane/core/interceptor/session/SessionManager.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ private void createDefaultResponseIfNeeded(Exchange exc) {
139139
exc.setResponse(Response.ok().build());
140140
}
141141

142-
143142
private void handleSetCookieHeaderForResponse(Exchange exc, Session session) throws Exception {
144143
Optional<Object> originalCookieValueAtBeginning = Optional.ofNullable(exc.getProperty(SESSION_COOKIE_ORIGINAL));
145144

@@ -211,7 +210,7 @@ private void removeRedundantExpireCookieIfRefreshed(Exchange exc, Map<String, Li
211210
.filter(e -> e.getValue().size() > 1)
212211
.filter(e -> e.getValue().stream().filter(s -> s.contains(VALUE_TO_EXPIRE_SESSION_IN_BROWSER)).count() == 1)
213212
.forEach(e -> {
214-
setCookieHeaders.get(e.getKey()).remove(e.getValue());
213+
setCookieHeaders.get(e.getKey()).remove(e.getValue()); // TODO does this actually do anything?
215214
exc.getResponse().getHeader().remove(getAllRelevantSetCookieHeaders(exc)
216215
.filter(hf -> hf.getValue().contains(VALUE_TO_EXPIRE_SESSION_IN_BROWSER))
217216
.findFirst().get());
@@ -230,8 +229,7 @@ private void setCookieForCurrentSession(Exchange exc, String currentSessionCooki
230229
log.warn("Cookie is larger than 4093 bytes, this will not work some browsers.");
231230
String setCookieValue = currentSessionCookieValue
232231
+ ";" + String.join(";", createCookieAttributes(exc));
233-
exc.getResponse().getHeader()
234-
.add(Header.SET_COOKIE, setCookieValue);
232+
exc.getResponse().getHeader().add(Header.SET_COOKIE, setCookieValue);
235233
}
236234

237235
private void setCookieForExpiredSessions(Exchange exc, String currentSessionCookieValue) {
@@ -263,8 +261,9 @@ private List<String> expireCookies(Exchange exc, List<String> invalidCookies) {
263261

264262
protected Session getSessionInternal(Exchange exc) {
265263
exc.setProperty(SESSION_COOKIE_ORIGINAL,null);
266-
if (getCookieHeader(exc) == null)
264+
if (getCookieHeader(exc) == null) {
267265
return new Session(usernameKeyName, new HashMap<>());
266+
}
268267

269268
Map<String, Map<String, Object>> validCookiesAsListOfMaps = convertValidCookiesToAttributes(exc);
270269
Session session = new Session(usernameKeyName, mergeCookies(new ArrayList<>(validCookiesAsListOfMaps.values())));
@@ -341,7 +340,11 @@ public List<String> createInvalidationAttributes(Exchange exc) {
341340

342341

343342
protected Stream<String> getCookies(Exchange exc) {
344-
return exc.getRequest().getHeader().getValues(new HeaderName(COOKIE)).stream().map(s -> s.getValue().split(";")).flatMap(Arrays::stream).map(String::trim);
343+
return exc.getRequest().getHeader().getValues(new HeaderName(COOKIE))
344+
.stream()
345+
.map(s -> s.getValue().split(";"))
346+
.flatMap(Arrays::stream)
347+
.map(String::trim);
345348
}
346349

347350
public void removeSession(Exchange exc) {

‎core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/OAuth2ResourceTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import java.util.concurrent.atomic.*;
3636

3737
import static com.predic8.membrane.core.http.MimeType.*;
38+
import static com.predic8.membrane.core.http.Request.get;
39+
import static com.predic8.membrane.core.interceptor.oauth2client.rf.OAuth2CallbackRequestHandler.MEMBRANE_MISSING_SESSION;
3840
import static org.junit.jupiter.api.Assertions.*;
3941

4042
public abstract class OAuth2ResourceTest {
@@ -181,11 +183,9 @@ public Outcome handleRequest(Exchange exc) throws Exception {
181183

182184
browser.clearCookies(); // send the auth link to some helpless (other) user
183185

184-
excCallResource = browser.apply(new Request.Builder().get("http://localhost:" + serverPort + ref.get()).buildExchange());
185-
186-
assertEquals(400, excCallResource.getResponse().getStatusCode());
187-
188-
assertTrue(excCallResource.getResponse().getBodyAsStringDecoded().contains("CSRF"));
186+
var response = browser.apply(get("http://localhost:" + serverPort + ref.get()).buildExchange()).getResponse();
187+
assertEquals(400, response.getStatusCode());
188+
assertTrue(response.getBodyAsStringDecoded().contains(MEMBRANE_MISSING_SESSION));
189189
}
190190

191191
@Test

‎core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTest.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@
3131
import java.util.concurrent.atomic.*;
3232

3333
import static com.predic8.membrane.core.http.Header.*;
34+
import static com.predic8.membrane.core.http.Request.get;
35+
import static com.predic8.membrane.core.interceptor.oauth2client.rf.OAuth2CallbackRequestHandler.MEMBRANE_MISSING_SESSION;
36+
import static com.predic8.membrane.core.util.URLParamUtil.DuplicateKeyOrInvalidFormStrategy.ERROR;
37+
import static com.predic8.membrane.core.util.URLParamUtil.parseQueryString;
3438
import static org.junit.jupiter.api.Assertions.*;
3539

3640
public abstract class OAuth2ResourceB2CTest {
@@ -162,7 +166,8 @@ public Outcome handleRequest(Exchange exc) throws Exception {
162166

163167
assertEquals(400, excCallResource.getResponse().getStatusCode());
164168

165-
assertTrue(excCallResource.getResponse().getBodyAsStringDecoded().contains("CSRF"));
169+
String response = excCallResource.getResponse().getBodyAsStringDecoded();
170+
assertTrue(response.contains(MEMBRANE_MISSING_SESSION));
166171
}
167172

168173
@Test

0 commit comments

Comments
 (0)