Fix HTTP session leaks during OAuth reauthentication
When an existing Gerrit session expires or the user
is signing out explicitly, the underlying HTTP session
remains active because of the JSESSIONID cookie being
preserved on the client. The situation was detected by
the OAuthWebFilter which was trying to resolve it by
invalidating the HTTP session explicitly.
By performing a detailed analysis of the HTTP sessions
and the GitHubLogin objects leaks, it is clear that
the explicit invalidation causes a "race" between the
client and the server where every single call creates
a new HTTP session object with a GitHubLogin associated
which would never be effectively used, because the
Gerrit login isn't completed and therefore the Gerrit
session is not finalised yet.
It is incorrect to expire the JSESSIONID if the
client still holds a cooking pointer to it, but the
existing session can be simply "cleaned up" by the
GitHub and Gerrit account information that aren't
known yet, so that no more objects are created and
leaked.
After the fix, there is exactly one GitHubLogin
object created per OAuth authentication phase vs.
tens of them which was the previous situation.
Bug: Issue 40015390
Change-Id: Ieff58fa48c9eb8cca6a90524fa616de9564767f5
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
index fdde648..da840bf 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
@@ -13,12 +13,16 @@
// limitations under the License.
package com.googlesource.gerrit.plugins.github.oauth;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;
public abstract class HttpSessionProvider<T> implements ScopedProvider<T> {
+ private final Supplier<String> singletonKey = Suppliers.memoize(() -> getClass().getName());
@Inject private Provider<T> provider;
@@ -32,19 +36,23 @@
@Override
public T get(final HttpServletRequest req) {
HttpSession session = req.getSession();
- String singletonKey = getClass().getName();
synchronized (this) {
@SuppressWarnings("unchecked")
- T instance = (T) session.getAttribute(singletonKey);
+ T instance = (T) session.getAttribute(singletonKey.get());
if (instance == null) {
instance = provider.get();
- session.setAttribute(singletonKey, instance);
+ session.setAttribute(singletonKey.get(), instance);
}
return instance;
}
}
+ public void clear(final HttpServletRequest req) {
+ Optional.ofNullable(req.getSession(false))
+ .ifPresent((s) -> s.removeAttribute(singletonKey.get()));
+ }
+
@Override
public HttpServletRequest getScopedRequest() {
return httpRequestProvider.get();
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
index 3c72141..0bca570 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
@@ -104,7 +104,7 @@
chain.doFilter(httpRequest, httpResponse);
}
} finally {
- HttpSession httpSession = httpRequest.getSession();
+ HttpSession httpSession = httpRequest.getSession(false);
if (gerritCookie != null && httpSession != null) {
String gerritCookieValue = gerritCookie.getValue();
String gerritSessionValue = (String) httpSession.getAttribute("GerritAccount");
@@ -112,7 +112,8 @@
if (gerritSessionValue == null) {
httpSession.setAttribute("GerritAccount", gerritCookieValue);
} else if (!gerritSessionValue.equals(gerritCookieValue)) {
- httpSession.invalidate();
+ httpSession.setAttribute("GerritAccount", null);
+ loginProvider.clear(httpRequest);
}
}
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
index 9c68dde..307a93b 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
@@ -19,5 +19,7 @@
public interface ScopedProvider<T> extends Provider<T> {
T get(HttpServletRequest request);
+ void clear(HttpServletRequest request);
+
HttpServletRequest getScopedRequest();
}
