Skip to content

Use a constant in check_path_traversal!

David Fernandez requested to merge 10io-check-path-traversal-function-regex-as-constant into master

🚙 Context

While working on !123477 (merged), I noticed that check_path_traversal! is powered by a regex.

That regex is built every time the function is called. Given that the regex doesn't have any of the function parameters, we can use a constant here.

See !123477 (merged) for some numbers (row long url check path traversal with constant). The function execution time is about 2x faster with a constant.

Given how essential this function is, I think that this tiny change is worth it. Moreover, this change will absolutely not change the function behavior.

🤔 What does this MR do and why?

  • Introduce a constant for the regex used in check_path_traversal!.
  • While here, update the related specs with additional situations where this function should detect an issue.

We don't have a changelog here since this is more of a maintenancerefactor change. It introduces no changes for any user facing feature.

🖥 Screenshots or screen recordings

n/a

⚙ How to set up and validate locally

A rails console will do:

[1] pry(main)> Gitlab::PathTraversal.check_path_traversal!('foo/bar')
=> "foo/bar"

[2] pry(main)> Gitlab::PathTraversal.check_path_traversal!('foo/../bar')
Gitlab::PathTraversal::PathTraversalAttackError: Invalid path
from /Users/david/projects/gitlab-development-kit/gitlab/lib/gitlab/path_traversal.rb:28:in `check_path_traversal!'

[3] pry(main)> Gitlab::PathTraversal.check_path_traversal!('foo%2F%2E%2E%2Fbar')
Gitlab::PathTraversal::PathTraversalAttackError: Invalid path
from /Users/david/projects/gitlab-development-kit/gitlab/lib/gitlab/path_traversal.rb:28:in `check_path_traversal!'

[4] pry(main)> Gitlab::PathTraversal.check_path_traversal!('foo%252F%2E%2E%2Fbar')
Gitlab::Utils::DoubleEncodingError: path foo%252F%2E%2E%2Fbar is not allowed
from /Users/david/projects/gitlab-development-kit/gitlab/lib/gitlab/utils.rb:19:in `decode_path'

🏁 MR acceptance checklist

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

Merge request reports