Skip to content

Minor updates for the path traversal middleware

David Fernandez requested to merge 413766-minor-updates 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.

For an additional safety net, the middleware is behind a feature flag. See the rollout issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415460.

⚙ What does this MR do and why?

This MR brings two small updates:

  1. We dup the env structure before using it. This comes from https://gitlab.com/gitlab-org/gitlab/-/issues/413766#note_1621784436 where we noticed that in https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/middleware/handle_malformed_strings.rb#L29-32 which is a similar middleware, there is a comment about modifying env by just reading its fields.
    • This problem seems to be more related with ActionDispatch::Request.new(env) than Rack::Request.new(env) but better be safe than sorry.
  2. if a path traversal is detected, we log it. This MR will add the http method in the fields logged. This is to help with debugging and analysis when looking at logs.

🖼 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/foo%2F..%2Fbar
  3. Check log/application_json.log

You will see these logs:

{"severity":"WARN","time":"2023-10-31T14:23:05.819Z","correlation_id":"01HE31VQ2DK3FT1PVK4PEKX66V","method":"GET","fullpath":"/foo%2F..%2Fbar","message":"Potential path traversal attempt detected","class_name":"Gitlab::Middleware::PathTraversalCheck"}

The http method is properly logged ✅

🛴 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 #413766

Edited by David Fernandez

Merge request reports