Skip to content

Keep fragment identifier when performing an OAuth redirect

Sean McGivern requested to merge fix-oauth-redirect-fragment into master

When we request a GitLab Pages page with access control enabled, something like this happens:

  1. 302 to Pages's /auth endpoint.
  2. 302 to GitLab's /oauth/authorize endpoint, where GitLab is an OAuth provider.
  3. 200 with JS redirection code to redirect back to Pages's /auth endpoint.
  4. 302 to Pages's /auth endpoint again, but this time on the specific Pages domain in question.
  5. 200 on the originally-requested page.

There can be intermediate steps here - particularly if the user is not signed in to GitLab already - but that's the gist. This broke fragment identifiers (the part of the URL after the #). The server doesn't receive these, but most browsers preserve them on a server-side redirect: https://stackoverflow.com/a/5283739

Our 200 in step 3 was breaking that, however, because it's not a 3XX redirect. Instead we return a minimal HTML page with some JavaScript to perform the redirect, and a link if the user doesn't have JavaScript enabled. This is to work around a previous security issue: #300308 (closed)

This commit changes the JS redirection code to include the fragment identifier, if present, and so avoid breaking things in the common case. This doesn't just apply to Pages, but to any other use of GitLab as an OAuth provider. It does not attempt to fix the HTML link as the only way the client can access the fragment programatically is through JS, and the link is only useful if JS is disabled.

Out of an abundance of caution, we only include the fragment if it is comprised of word characters, dashes, and underscores. All other characters are, for now, excluded.

Testing

  1. Set up access control for Pages locally: https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/doc/howto/pages.md
  2. Create a private Pages site.
  3. Try to access that site with a fragment identifier. In this example, I'm using http://root.pages.gdk.test:3010/pages/#hi

Before:

Before

After:

After

Edited by Sean McGivern

Merge request reports