Skip to content

Strip cookies for asset requests in development and test

Stan Hu requested to merge sh-strip-cookies into master

What does this MR do and why?

This helps fix some race conditions in feature specs that cause users to be logged out.

In production, all requests to /assets get served by Workhorse or a CDN.

However, in test and development, all /assets requests get directed to Rails since the -developmentMode Workhorse command-line flag is present. This has the unintended side effect of causing all responses for assets to return with a Set-Cookie header. The happens because the Rails ActionDispatch::Cookies middleware ensures that all responses have a Set-Cookie header if the request included one (https://github.com/rails/rails/blob/506462ab13755d9f024e1ddbfc8c58d73e7a1bce/actionpack/lib/action_dispatch/middleware/cookies.rb#L706-L716).

In spec/features/users/login_spec.rb, we observed this race condition:

  1. User loads /users/sign_in.
  2. This returns a Set-Cookie HTTP header of _gitlab_session=X.
  3. Many /assets are requested with _gitlab_session=X.
  4. User logs in before all assets are returned. The login returns _gitlab_session=Y.
  5. The requested /assets return with _gitlab_session=X, which causes the user to use the old cookie associated with an anonymous user.

This commit introduces a middleware that strips the Set-Cookie header from the request and response for /assets requests. This prevents this race condition from happening and emulates what happens in production.

Relates to #388049 (closed)

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

  1. Checkout this branch and run gdk restart rails-web.
  2. With an Incognito window, go to the /users/sign_in page.
  3. Pick any /assets request.
  4. Validate that all set-cookie headers are gone from the response even though Cookie is present.
  5. Validate that set-cookies headers are still present in /users/sign_in.
Edited by Stan Hu

Merge request reports

Loading