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(); }