Skip to content

Rack path traversal: exclude more query parameters

David Fernandez requested to merge 466563-ignore-commit-message-and-content into master

🔭 Context

Similar to Rack path traversal: exclude the note and body ... (!155279 - merged).

We are slowing rolling out the Rack middleware for path traversal checks. See https://gitlab.com/groups/gitlab-org/-/epics/13437+.

The middleware is only logging attempts. It is behind a feature flag.

During the roll out, we noticed that we could have an issue with the API requests that create commits. That API has a commit_message and content parameter that could contain strings that would be detected as path traversals even though they are valid requests.

We will need to excluded these from the Rack path traversal checker. That's what this MR does. The related issue is https://gitlab.com/gitlab-org/gitlab/-/issues/466563+.

⚙ What does this MR do and why?

  • Exclude the commit_message and content query parameters in the check path traversal middleware.

Note that specs were not updated because the excluded query parameter names are in the constant and we have a spec that will loop on that constant to assert the behavior.

The entire middleware is behind a feature flag. At the time of this writing, we're still rolling it out.

MR acceptance checklist

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

✅

🦄 Screenshots or screen recordings

🤷 (No UI changes)

⚗ How to set up and validate locally

  1. Enable the middleware: Feature.enable(:check_path_traversal_middleware)
  2. Have a project ready with the git repository initialized.
  3. Have a PAT ready.

1️⃣ On master 

$ curl -vvv --request POST --header 'PRIVATE-TOKEN: <pat>' "http://gdk.test:8000/api/v4/projects/<project_id>/repository/files/test2%2Ftest.txt?branch=main&commit_message=foo%2F..%2Fbar&content=this+is+a+test"

$ curl -vvv --request POST --header 'PRIVATE-TOKEN: <pat>' "http://gdk.test:8000/api/v4/projects/<project_id>/repository/files/test3%2Ftest.txt?branch=main&commit_message=test&content=foo%2F..%2Fbar"

Look at log/application_json.log:

{"severity":"WARN","time":"2024-06-11T13:47:14.751Z","correlation_id":"01J03RV1RMJRZ6129Q8D20BFGD","meta.caller_id":"POST /api/:version/projects/:id/repository/files/:file_path","meta.remote_ip":"172.16.123.1","meta.feature_category":"source_code_management","meta.user":"root","meta.user_id":1,"meta.project":"root/create-commits-api","meta.root_namespace":"root","meta.client_id":"user/1","method":"POST","fullpath":"/api/v4/projects/331/repository/files/test2%2Ftest.txt?branch=main\u0026commit_message=foo%2F..%2Fbar\u0026content=this+is+a+test","message":"Potential path traversal attempt detected","status":201,"class_name":"Gitlab::Middleware::PathTraversalCheck"}
{"severity":"WARN","time":"2024-06-11T13:47:50.157Z","correlation_id":"01J03RW4FNW9E7GKBCWCC49Q7A","meta.caller_id":"POST /api/:version/projects/:id/repository/files/:file_path","meta.remote_ip":"172.16.123.1","meta.feature_category":"source_code_management","meta.user":"root","meta.user_id":1,"meta.project":"root/create-commits-api","meta.root_namespace":"root","meta.client_id":"user/1","method":"POST","fullpath":"/api/v4/projects/331/repository/files/test3%2Ftest.txt?branch=main\u0026commit_message=test\u0026content=foo%2F..%2Fbar","message":"Potential path traversal attempt detected","status":201,"class_name":"Gitlab::Middleware::PathTraversalCheck"}

Two attempts have been caught.

2️⃣ With this MR

(Restart gdk when switching git branches. Middleware classes are loaded at boot time and are not updated even if the underlying ruby file has changed.)

$ curl -vvv --request POST --header 'PRIVATE-TOKEN: <pat>' "http://gdk.test:8000/api/v4/projects/<project_id>/repository/files/test4%2Ftest.txt?branch=main&commit_message=foo%2F..%2Fbar&content=this+is+a+test"

$ curl -vvv --request POST --header 'PRIVATE-TOKEN: <pat>' "http://gdk.test:8000/api/v4/projects/<project_id>/repository/files/test5%2Ftest.txt?branch=main&commit_message=test&content=foo%2F..%2Fbar"

Look at log/application_json.log:

(empty)

The middleware excluded these query parameters properly 🎉

Edited by David Fernandez

Merge request reports