Skip to content

Exclude global search path from the path traversal check middleware

David Fernandez requested to merge 413766-exclude-global-search-path into master

🌳 Context

In Rack middleware for path traversal checks (!123477 - merged), we introduced a rack middleware with this idea of checking the request path, if a path traversal is detected, reject the request. See #413766+ for more details.

Since middlewares are executed for all requests, we are not starting with the "reject the request" part (yet). Instead, we simply log the path traversal attempt and additionally we measure the execution time of that check. This is the simulation phase.

During the verifications on staging, we noticed that we could flag valid requests to global search (/search) as path traversal attempts. After a discussion with groupglobal search, we even realized that with the new search engine (currently in beta), the search parameter could be a regex. In regex, we could easily have . or .. or /../ that will be detected as path traversals attempts.

We decided to take the most simple solution here and simply disable the middleware for those paths. This MR implements that exclusion.

🤔 What does this MR do and why?

  • Update the middleware so that all paths starting with /search are excluded from its consideration (and will never be rejected).
  • Update the related specs.

The entire middleware is behind a feature flag. Currently, not enabled on gitlab.com and not enabled by default.

🖼 Screenshots or screen recordings

None

How to set up and validate locally

  1. In a rails console, enable the middleware and enable the execution time logging:
Feature.enable(:check_path_traversal_middleware)
  1. Start the server.
  2. Access http://gdk.test:8000/search?search=foo%2F..%2Fbar
  3. Check log/application_json.log

1️⃣ On master

You will see these logs:

{"severity":"WARN","time":"2023-10-13T08:16:04.927Z","correlation_id":"01HCM1PR9Q03RRX5V1BBJJ8H7Q","message":"Potential path traversal attempt detected","path":"/search?search=foo/../bar"}
{"severity":"WARN","time":"2023-10-13T08:16:04.928Z","correlation_id":"01HCM1PR9Q03RRX5V1BBJJ8H7Q","fullpath":"/search?search=foo%2F..%2Fbar","message":"Potential path traversal attempt detected","class_name":"Gitlab::Middleware::PathTraversalCheck"}

That's the middleware detecting a path traversal attempt and logging it.

2️⃣ with this MR

You will see:

(nothing 😸 )

That proves that the middleware was completely skipped, hence no logs.

🛵 MR acceptance checklist

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

Edited by David Fernandez

Merge request reports