Skip to content

Remove OAuth paths from protected paths rate limit

Markus Koller requested to merge 345554-oauth-protected-paths into master

What does this MR do and why?

These paths were added in the 13.3.3 security release (https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/757) so they would get throttled by the "protected paths" rate limit, as a protection against brute-force attacks.

This rate limit turned out to be to strict for normal OAuth usage (https://gitlab.com/gitlab-org/gitlab/-/issues/345554), so we're removing the paths again and instead let them get throttled by the general rate limit for unauthenticated requests, which didn't exist yet when we made the original change, and should still offer sufficient protection.

This configuration change was already applied manually on gitlab.com and verified to be working as expected. (https://gitlab.com/gitlab-com/gl-infra/production/-/issues/6000)

Migration output

$ rails db:migrate:up VERSION=20211215182006
== 20211215182006 UpdateApplicationSettingsProtectedPaths: migrating ==========
-- change_column_default(:application_settings, :protected_paths, ["/users/password", "/users/sign_in", "/api/v3/session.json", "/api/v3/session", "/api/v4/session.json", "/api/v4/session", "/users", "/users/confirmation", "/unsubscribes/", "/import/github/personal_access_token", "/admin/session"])
   -> 0.2358s
== 20211215182006 UpdateApplicationSettingsProtectedPaths: migrated (1.1885s) =

$ rails db:migrate:down VERSION=20211215182006
== 20211215182006 UpdateApplicationSettingsProtectedPaths: reverting ==========
-- change_column_default(:application_settings, :protected_paths, ["/users/password", "/users/sign_in", "/api/v3/session.json", "/api/v3/session", "/api/v4/session.json", "/api/v4/session", "/users", "/users/confirmation", "/unsubscribes/", "/import/github/personal_access_token", "/admin/session", "/oauth/authorize", "/oauth/token"])
   -> 0.2351s
== 20211215182006 UpdateApplicationSettingsProtectedPaths: reverted (0.8404s) =

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #345554

Edited by Markus Koller

Merge request reports