Fix PathTraversalCheck about query params

What does this MR do and why?

What does this MR fix?

The user search function may occasionally malfunction, failing to return the target user. This issue occurs on both the admin/users page and the member invitation page. It appears that the search keywords are not functioning as expected.

This bug has persisted for at least a year, but it’s hard to detect due to the strict conditions required to trigger it.

The cause of this bug

Gitlab::Middleware::PathTraversalCheck reads query parameters from request.GET and calls cleanup_query_parameters! to remove certain known-safe keys (such as search_query , listed in EXCLUDED_QUERY_PARAM_NAMES) before performing path traversal detection.

In some environments, another step may have already parsed the query string on the original env. In that case, request.GET can return a cached Hash that is shared and reused later in the request lifecycle. When cleanup_query_parameters! uses except! on that shared Hash, it mutates the cached object, causing downstream code (controllers, etc.) to see missing query params like search_query (e.g. on the admin/users search page).

References

Screenshots or screen recordings

Before After

How to set up and validate locally

  1. Simulate a scenario where a certain middleware reads the request params. This can absolutely happen in practice, but for simplicity's sake, you can just apply this diff to achieve the effect.
    diff --git a/lib/gitlab/middleware/request_context.rb b/lib/gitlab/middleware/request_context.rb
    index f609002007cd..7dfbaf344180 100644
    --- a/lib/gitlab/middleware/request_context.rb
    +++ b/lib/gitlab/middleware/request_context.rb
    @@ -9,6 +9,7 @@ def initialize(app)
    
          def call(env)
            request = ActionDispatch::Request.new(env)
    +        request.params
            Gitlab::RequestContext.start_request_context(request: request)
            Gitlab::RequestContext.start_thread_context
    
  2. Restart the rails-web service: gdk restart rails-web
  3. Try searching for users by keyword or state on admin/users page. You’ll notice that the desired results are never found.

MR acceptance checklist

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

Edited by Zhiyuan Lu

Merge request reports

Loading