Skip to content
Snippets Groups Projects

Rack middleware for path traversal checks

All threads resolved!

:shield: What if?

What if we check all requests received by the rails backend for path traversals?

The idea is to have something along the components that handle web requests, if possible as early as possible. That component would simply check the path that is accessed and run this function that will check for path traversals.

This would add another protection layer against path traversals located on accessed urls (like gitlab.com/api/v4/foo%2F..%2Fbar).

One good candidate for such component is a rails middleware. This MR explores that option.

Related to: #413766+

:rocket: Deployment plan

We can't possibly deploy a change that impacts all web requests without some safety net. In addition, it would be good to measure in advance that impact. We're adding another handling in the web request. That handling is not free and will cost cpu time. That time will be added to the request latency time.

Before accepting the tradeoff, it would be nice to have a glimpse of what are exactly the terms of the tradeoff.

As such, we suggest deploying this change behind a feature flag and the following plan:

  1. Simulation phase (:point_left: this MR)
    • feature flag enabled for a low-med percentage of requests, e.g. Feature.enable_percentage_of_time(:my_feature_3, 5)
    • All requests are allowed.
    • Depending on the feature flag, the path traversals checks are executed. Any failures in those checks are logged.
    • While we're here, log the amount of time spent to execute the checks.
  2. Analysis phase
    • Gather the logs from the previous phase. Analyze if the request is a genuine one (one that we expect) or not.
    • If it's a valid request, investigate how to ignore it or make it skip the checks.
    • Iterate until no further logs are collected for valid requests.
    • Analyze the results of the amount of time spent on checks.
  3. Enforce phase
    • Start rejecting requests that don't pass the checks. Deploy this with a feature flag.
    • Let it run for a full milestone on .com.
    • Let it run for a full milestone on self-managed.
  4. Cleanup phase
    • If no feedback or issues are reported from the previous phase, remove the feature flag and logging.

This MR represents phase (1.): simulation. In other words, no request is declined with this change. We're merely logging the activity of the middleware.

:thinking: What does this MR do and why?

  • Add a middleware to check path traversals on requests.
  • Add the related specs.

:tv: Screenshots or screen recordings

You know... this a rails middleware. So, no UI changes :smile_cat:

So below to see the middleware in action

:gear: How to set up and validate locally

  1. Enable the feature flag:
    Feature.enable(:middleware_path_traversal)

Try different urls and check the application_json.log

url application_json.log
curl http://gdk.test:8000/foo/bar {"severity":"WARN","time":"2023-06-14T10:00:37.397Z","correlation_id":"01H2WNK6NG48YEMFVY9Z7TEMG6","duration_s":0.00089,"class_name":"Gitlab::Middleware::PathTraversalCheck"}
curl http://gdk.test:8000/foo/../bar {"severity":"WARN","time":"2023-06-14T11:56:52.031Z","correlation_id":"01H2WW81TXMJQHVFHTN7V144NJ","duration_s":0.00131,"class_name":"Gitlab::Middleware::PathTraversalCheck"}
curl http://gdk.test:8000/foo%2F../bar {"severity":"WARN","time":"2023-06-14T11:57:50.778Z","correlation_id":"01H2WW9V9GEJ2J3MN28VJNA9MW","fullpath":"/foo/%2F../bar","message":"Potential path traversal attempt detected","duration_s":0.00203,"class_name":"Gitlab::Middleware::PathTraversalCheck"}
curl "http://gdk.test:8000/foo/bar?q=%2F%2E%2E%2Ftest" {"severity":"WARN","time":"2023-06-14T11:58:35.362Z","correlation_id":"01H2WWB6TRM27HJX5BRTTVKQXA","fullpath":"/foo/bar?q=%2F%2E%2E%2Ftest","message":"Potential path traversal attempt detected","duration_s":0.00226,"class_name":"Gitlab::Middleware::PathTraversalCheck"}
curl "http://gdk.test:8000/foo%2F%2E%2E%2Fbar" {"severity":"WARN","time":"2023-06-14T11:59:43.408Z","correlation_id":"01H2WWD9938KKBR52MJBEXNSSE","fullpath":"/foo%2F%2E%2E%2Fbar","message":"Potential path traversal attempt detected","duration_s":0.00222,"class_name":"Gitlab::Middleware::PathTraversalCheck"}

† for this case, it seems that Rack itself will refuse to parse the fullpath as foo/../bar. Instead, the path used will be /bar which, from the middleware point of view, is a valid url (no path traversals).

MR acceptance checklist

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

:crystal_ball: Other considerations

:racehorse: Why not workhorse?

Excellent question :thumbsup:

Workhorse would be the ideal location to reject such invalid requests. This way, the rails backend doesn't even receive it at all (= rails resources saved).

However, given the reach of this change (all requests), we absolutely need some safety nets. feature flag s are those safety nets. In addition, we already have a function that check for path traversals in rails and that is known to be working as expected. In workhorse, we would need to find or create such function. We could be introducing risky changes (like rejecting a request that doesn't have any path traversal).

All things considered, I think it's better to start with a rails middleware. If that approach turns out to be a good one but it's consuming too many resources, we can look to port the solution to workhorse.

:bar_chart: Get me some numbers

In order to know how much time we're going to spend in checking for path traversals, I thought that running some benchmarks would be useful.

Here is the file I used. Pardon the code quality, I created this within 5 minutes and I just wanted the results. While at it, I explored some other implementation options for the check path traversal function to check if we could improve the execution time.

For the scenarios, I used 3 different urls: a short one, a medium one (~1000 chars) and a long one (~20 000 chars). The medium and long one have a path traversal attempt.

`script/benchmarks/path_traversal.rb`
# frozen_string_literal: true

require 'benchmark/ips'
require_relative "../../config/environment"

SHORT_URL = "foo%2Fbar"
LONG_URL = "#{'a' * 10_000}%2F%2E%2E%2F#{'b' * 10_000}"
MEDIUM_URL = "#{'a' * 1000}%2F%2E%2E"

RE2_REGEX = RE2::Regexp.new('(^(\.{1,2})$|^\.\.[/\\\\]|[/\\\\]\.\.$|[/\\\\]\.\.[/\\\\])').freeze
SIMPLIFIED_REGEX = %r{(/{0,1}\.\./{0,1})}.freeze
TWO_DOTS = '..'
EXISTING_PATH_TRAVERSAL_REGEX = %r{(\A(\.{1,2})\z|\A\.\.[/\\]|[/\\]\.\.\z|[/\\]\.\.[/\\]|\n)}.freeze

def check_path_traversal_with_constant!(path)
  return unless path

  path = path.to_s if path.is_a?(Gitlab::HashedPath)
  raise PathTraversalAttackError, 'Invalid path' unless path.is_a?(String)

  path = ::Gitlab::Utils.decode_path(path)

  if path.match?(EXISTING_PATH_TRAVERSAL_REGEX)
    logger.warn(message: "Potential path traversal attempt detected", path: path.to_s)
    raise PathTraversalAttackError, 'Invalid path'
  end

  path
end


Benchmark.ips do |x|
  x.report("short url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(SHORT_URL)
  rescue
  end
  x.report("short url check path traversal with constant") do
    check_path_traversal_with_constant!(SHORT_URL)
  rescue
  end
  x.report("short url re2") do
    RE2_REGEX.match(CGI.unescape(SHORT_URL))
  end
  x.report("short url include?") do
    CGI.unescape(SHORT_URL).include?(TWO_DOTS)
  end
  x.report("short url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(SHORT_URL))
  end

  x.compare!
end

Benchmark.ips do |x|
  x.report("medium url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(MEDIUM_URL)
  rescue
  end
  x.report("medium url check path traversal with constant") do
    check_path_traversal_with_constant!(MEDIUM_URL)
  rescue
  end
  x.report("medium url re2") do
    RE2_REGEX.match(CGI.unescape(MEDIUM_URL))
  end
  x.report("medium url include?") do
    CGI.unescape(MEDIUM_URL).include?(TWO_DOTS)
  end
  x.report("medium url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(MEDIUM_URL))
  end

  x.compare!
end


Benchmark.ips do |x|
  x.report("long url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(LONG_URL)
  rescue
  end
  x.report("long url check path traversal with constant") do
    check_path_traversal_with_constant!(LONG_URL)
  rescue
  end
  x.report("long url re2") do
    RE2_REGEX.match(CGI.unescape(LONG_URL))
  end
  x.report("long url include?") do
    CGI.unescape(LONG_URL).include?(TWO_DOTS)
  end
  x.report("long url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(LONG_URL))
  end

  x.compare!
end


Benchmark.bmbm do |x|
  x.report("short url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(SHORT_URL)
  rescue
  end
  x.report("short url check path traversal with constant") do
    check_path_traversal_with_constant!(SHORT_URL)
  rescue
  end
  x.report("short url re2") do
    RE2_REGEX.match(CGI.unescape(SHORT_URL))
  end
  x.report("short url include?") do
    CGI.unescape(SHORT_URL).include?(TWO_DOTS)
  end
  x.report("short url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(SHORT_URL))
  end
end

Benchmark.bmbm do |x|
  x.report("medium url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(MEDIUM_URL)
  rescue
  end
  x.report("medium url check path traversal with constant") do
    check_path_traversal_with_constant!(MEDIUM_URL)
  rescue
  end
  x.report("medium url re2") do
    RE2_REGEX.match(CGI.unescape(MEDIUM_URL))
  end
  x.report("medium url include?") do
    CGI.unescape(MEDIUM_URL).include?(TWO_DOTS)
  end
  x.report("medium url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(MEDIUM_URL))
  end
end


Benchmark.bmbm do |x|
  x.report("long url existing check path traversal") do
    Gitlab::PathTraversal.check_path_traversal!(LONG_URL)
  rescue
  end
  x.report("long url check path traversal with constant") do
    check_path_traversal_with_constant!(LONG_URL)
  rescue
  end
  x.report("long url re2") do
    RE2_REGEX.match(CGI.unescape(LONG_URL))
  end
  x.report("long url include?") do
    CGI.unescape(LONG_URL).include?(TWO_DOTS)
  end
  x.report("long url simplified regex") do
    SIMPLIFIED_REGEX.match?(CGI.unescape(LONG_URL))
  end
end

Here are the results I get:

results
$ ruby script/benchmarks/path_traversal.rb
Warming up --------------------------------------
short url existing check path traversal
                       131.495k i/100ms
short url check path traversal with constant
                       135.288k i/100ms
       short url re2   188.348k i/100ms
  short url include?   374.376k i/100ms
short url simplified regex
                       358.905k i/100ms
Calculating -------------------------------------
short url existing check path traversal
                          1.307M (± 2.9%) i/s -      6.575M in   5.036125s
short url check path traversal with constant
                          1.303M (± 3.5%) i/s -      6.629M in   5.094431s
       short url re2      1.838M (±17.5%) i/s -      8.664M in   5.073499s
  short url include?      3.607M (± 3.8%) i/s -     18.344M in   5.093497s
short url simplified regex
                          3.399M (± 3.4%) i/s -     17.227M in   5.074337s

Comparison:
  short url include?:  3606763.6 i/s
short url simplified regex:  3398969.6 i/s - same-ish: difference falls within error
       short url re2:  1838392.3 i/s - 1.96x  slower
short url existing check path traversal:  1306655.8 i/s - 2.76x  slower
short url check path traversal with constant:  1302862.5 i/s - 2.77x  slower

Warming up --------------------------------------
medium url existing check path traversal
                        31.000  i/100ms
medium url check path traversal with constant
                         2.805k i/100ms
      medium url re2    26.154k i/100ms
 medium url include?    48.485k i/100ms
medium url simplified regex
                        51.226k i/100ms
Calculating -------------------------------------
medium url existing check path traversal
                          6.534k (±46.9%) i/s -      4.061k in   5.284083s
medium url check path traversal with constant
                         29.154k (± 3.6%) i/s -    145.860k in   5.009454s
      medium url re2    247.030k (±18.7%) i/s -      1.125M in   5.006283s
 medium url include?    477.863k (±18.6%) i/s -      2.182M in   5.030746s
medium url simplified regex
                        485.396k (±18.5%) i/s -      2.254M in   5.090959s

Comparison:
medium url simplified regex:   485395.5 i/s
 medium url include?:   477862.6 i/s - same-ish: difference falls within error
      medium url re2:   247030.0 i/s - 1.96x  slower
medium url check path traversal with constant:    29153.9 i/s - 16.65x  slower
medium url existing check path traversal:     6534.4 i/s - 74.28x  slower

Warming up --------------------------------------
long url existing check path traversal
                        29.000  i/100ms
long url check path traversal with constant
                       399.000  i/100ms
        long url re2     2.367k i/100ms
   long url include?     3.518k i/100ms
long url simplified regex
                         3.936k i/100ms
Calculating -------------------------------------
long url existing check path traversal
                          2.426k (±36.5%) i/s -      3.799k in   5.005201s
long url check path traversal with constant
                          4.017k (±10.1%) i/s -     19.551k in   5.015698s
        long url re2     22.342k (±18.1%) i/s -    104.148k in   5.061759s
   long url include?     35.546k (± 1.9%) i/s -    179.418k in   5.049406s
long url simplified regex
                         39.282k (± 2.5%) i/s -    196.800k in   5.013185s

Comparison:
long url simplified regex:    39281.7 i/s
   long url include?:    35545.8 i/s - 1.11x  slower
        long url re2:    22341.9 i/s - 1.76x  slower
long url check path traversal with constant:     4016.9 i/s - 9.78x  slower
long url existing check path traversal:     2425.6 i/s - 16.19x  slower

Rehearsal --------------------------------------------------------------------------------
short url existing check path traversal        0.000040   0.000024   0.000064 (  0.000015)
short url check path traversal with constant   0.000030   0.000025   0.000055 (  0.000042)
short url re2                                  0.000012   0.000009   0.000021 (  0.000010)
short url include?                             0.000004   0.000001   0.000005 (  0.000005)
short url simplified regex                     0.000003   0.000001   0.000004 (  0.000004)
----------------------------------------------------------------------- total: 0.000149sec

                                                   user     system      total        real
short url existing check path traversal        0.000017   0.000034   0.000051 (  0.000015)
short url check path traversal with constant   0.000015   0.000002   0.000017 (  0.000013)
short url re2                                  0.000017   0.000035   0.000052 (  0.000014)
short url include?                             0.000013   0.000002   0.000015 (  0.000011)
short url simplified regex                     0.000009   0.000019   0.000028 (  0.000007)
Rehearsal ---------------------------------------------------------------------------------
medium url existing check path traversal        0.000223   0.000169   0.000392 (  0.000279)
medium url check path traversal with constant   0.000050   0.000010   0.000060 (  0.000048)
medium url re2                                  0.000039   0.000024   0.000063 (  0.000048)
medium url include?                             0.000017   0.000011   0.000028 (  0.000026)
medium url simplified regex                     0.000006   0.000000   0.000006 (  0.000006)
------------------------------------------------------------------------ total: 0.000549sec

                                                    user     system      total        real
medium url existing check path traversal        0.000232   0.000191   0.000423 (  0.000288)
medium url check path traversal with constant   0.000070   0.000038   0.000108 (  0.000069)
medium url re2                                  0.000028   0.000049   0.000077 (  0.000025)
medium url include?                             0.000013   0.000002   0.000015 (  0.000011)
medium url simplified regex                     0.000014   0.000028   0.000042 (  0.000011)
Rehearsal -------------------------------------------------------------------------------
long url existing check path traversal        0.000460   0.000146   0.000606 (  0.000497)
long url check path traversal with constant   0.000266   0.000012   0.000278 (  0.000265)
long url re2                                  0.000052   0.000008   0.000060 (  0.000053)
long url include?                             0.000034   0.000012   0.000046 (  0.000034)
long url simplified regex                     0.000026   0.000009   0.000035 (  0.000028)
---------------------------------------------------------------------- total: 0.001025sec

                                                  user     system      total        real
long url existing check path traversal        0.000459   0.000197   0.000656 (  0.000516)
long url check path traversal with constant   0.000276   0.000061   0.000337 (  0.000279)
long url re2                                  0.000073   0.000029   0.000102 (  0.000070)
long url include?                             0.000039   0.000050   0.000089 (  0.000036)
long url simplified regex                     0.000039   0.000035   0.000074 (  0.000037)

Alright, here is what I can say reading the above:

  • Obviously, the longer the url, the longer the execution is.
  • The existing function is already quite fast. On long urls, it took less than 1ms to execute.
  • Putting the existing regex as a constant and update the existing function is an interesting option. Almost 2x faster on long urls.
  • There is room for improvement with alternative implementations.
    • Among them, the simplified regex (simplified as we would expect to use it only on urls) and the .include? approaches are crazy :zap: . About 15+x faster.
    • The problem with alternative implementations is that we introduce a risk of not properly covering all the cases.

In conclusion here, we have improvements available but given the already good performance of the existing function, I think it's not worth it (nor the risks involved of using a different implementation).

The only improvement we could consider here is moving the regex to a constant. This would have 0 impact on the function's behavior. Done in Use a constant in check_path_traversal! (!123661 - merged). :smile_cat:

Edited by David Fernandez

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Contributor
    1 Message
    :book: CHANGELOG missing:

    If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend Rad Batnag current availability (@radbatnag) (UTC+8, 6 hours ahead of @10io) Thong Kuah current availability (@tkuah) (UTC+13, 11 hours ahead of @10io)
    Application Security Reviewer review is optional for Application Security Dominic Couture current availability (@dcouture) (UTC+1, 1 hour behind @10io)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for 62a31af0

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Plan        | 51     | 0      | 0       | 0     | 51    | ✅     |
    | Verify      | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Manage      | 12     | 0      | 1       | 0     | 13    | ✅     |
    | Create      | 37     | 0      | 1       | 0     | 38    | ✅     |
    | Data Stores | 18     | 0      | 2       | 1     | 20    | ❗     |
    | Govern      | 33     | 0      | 1       | 0     | 34    | ✅     |
    | Package     | 0      | 0      | 1       | 0     | 1     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 159    | 0      | 6       | 1     | 165   | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa: :heavy_minus_sign: test report for 62a31af0

    expand test summary
    +--------------------------------------------------------------+
    |                        suites summary                        |
    +---------+--------+--------+---------+-------+-------+--------+
    |         | passed | failed | skipped | flaky | total | result |
    +---------+--------+--------+---------+-------+-------+--------+
    | Package | 0      | 0      | 1       | 0     | 1     | ➖     |
    +---------+--------+--------+---------+-------+-------+--------+
    | Total   | 0      | 0      | 1       | 0     | 1     | ➖     |
    +---------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for 62a31af0

    expand test summary
    +--------------------------------------------------------------+
    |                        suites summary                        |
    +---------+--------+--------+---------+-------+-------+--------+
    |         | passed | failed | skipped | flaky | total | result |
    +---------+--------+--------+---------+-------+-------+--------+
    | Package | 68     | 0      | 8       | 0     | 76    | ✅     |
    +---------+--------+--------+---------+-------+-------+--------+
    | Total   | 68     | 0      | 8       | 0     | 76    | ✅     |
    +---------+--------+--------+---------+-------+-------+--------+
  • David Fernandez added 1 commit

    added 1 commit

    Compare with previous version

  • David Fernandez added 1 commit

    added 1 commit

    • 51d0854b - Supports the double encoding error

    Compare with previous version

  • David Fernandez changed the description

    changed the description

  • David Fernandez marked this merge request as ready

    marked this merge request as ready

  • David Fernandez changed title from Draft: Resolve "Introduce a rack middleware for path traversal checks" to Rack middleware for path traversal checks

    changed title from Draft: Resolve "Introduce a rack middleware for path traversal checks" to Rack middleware for path traversal checks

  • David Fernandez marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • David Fernandez
  • David Fernandez
  • David Fernandez mentioned in merge request !123661 (merged)

    mentioned in merge request !123661 (merged)

  • David Fernandez changed the description

    changed the description

  • added security typemaintenance labels and removed typefeature label

  • David Fernandez added 1 commit

    added 1 commit

    • e38f930f - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez changed the description

    changed the description

  • David Fernandez
  • Sylvester Chin requested review from @schin1

    requested review from @schin1

  • Sylvester Chin
  • Sylvester Chin removed review request for @schin1

    removed review request for @schin1

  • David Fernandez added 1 commit

    added 1 commit

    • 95ba07f5 - Apply backend reviewer feedback

    Compare with previous version

  • Contributor

    :warning: @10io Some end-to-end (E2E) tests have been selected based on the stage label on this MR. If not run already, please run the e2e:package-and-test-ee job in the qa stage and review the results before merging this MR. (E2E tests are not run automatically on some MRs due to runner resource constraints.)

    If you would like to run all e2e tests, please apply the pipeline:run-all-e2e label and restart the pipeline.

    Once done, please apply the :white_check_mark: emoji on this comment.

    For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.

  • David Fernandez changed the description

    changed the description

  • David Fernandez requested review from @schin1

    requested review from @schin1

    • Resolved by Thong Kuah

      @10io Same as you, one thing I wondered about was susceptibility to Denial of Service via regex backtracking. https://docs.gitlab.com/ee/development/secure_coding_guidelines.html#denial-of-service-redos--catastrophic-backtracking

      PATH_TRAVERSAL_REGEX = %r{(\A(\.{1,2})\z|\A\.\.[/\\]|[/\\]\.\.\z|[/\\]\.\.[/\\]|\n)}

      But that regex is fine! No backtracking there.


      Another thing I wondered was legit URL paths that might get blocked by PATH_TRAVERSAL_REGEX, particularly the first and last part of that regex:

      • http://example.com/. would be blocked: that seems fine :shrug:
      • http://example.com/.. would be blocked: that seems fine :shrug:
      • http://example.com/... would not be blocked: that seems fine :shrug:
      • `http://example.com/.well-known/ would not be blocked :thumbsup:
      • http://example.com/fooNEWLINEGOESHERE would be blocked by matching \n, that seems fine too. Not sure how/why a URL path would have a newline in it.

      :warning: Then I wondered about the double encoding stuff, and I think this might a usability issue worth worrying about:

      > ::Gitlab::Utils.decode_path("/api/v4/issues?search=Release %2525")
      Gitlab::Utils::DoubleEncodingError: path /api/v4/issues?search=Release %2525 is not allowed

      We use % for milestones, and a milestone number might look like double encoding. Milestone 2525 would be %2525. :cry: So depending on how we represent that around the app we might hit a problem.

      • Creating a milestone via the UI seems fine; it's form-encoded
      • Search seems fine too as it uses milestone_title: https://gitlab.com/nmalcolm/test/-/issues/?sort=created_date&state=opened&milestone_title=2525&first_page_size=20

      I'd hope running the full spec suite would pick up any other potential failures :fingers_crossed: WDYT @10io ?

  • Nick Malcolm changed the description

    changed the description

    • Resolved by Thong Kuah

      :gear: How to set up and validate locally

      @10io The steps work for me! Due to the amount of extra logs this writes, I've updated the description to explicitly mention this should be a low-percentage based rollout. I think we need to remove the logging before this gets above 50% enabled, for example. In my testing toggling the flag, a "single" request in a browser to a Project home page made 10 log entries.

      Change back if you disagree!

  • Sylvester Chin approved this merge request

    approved this merge request

  • :wave: @schin1, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • Sylvester Chin removed review request for @schin1

    removed review request for @schin1

  • David Fernandez changed the description

    changed the description

  • David Fernandez added 1 commit

    added 1 commit

    • a0099a26 - Apply app sec review feedback

    Compare with previous version

  • David Fernandez requested review from @nmalcolm

    requested review from @nmalcolm

  • David Fernandez added 1960 commits

    added 1960 commits

    Compare with previous version

  • David Fernandez added 1 commit

    added 1 commit

    • d6b114b1 - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez requested review from @tkuah

    requested review from @tkuah

  • Nick Malcolm approved this merge request

    approved this merge request

  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Sylvester Chin mentioned in merge request !123762 (merged)

    mentioned in merge request !123762 (merged)

  • David Fernandez added 1 commit

    added 1 commit

    • 60e00831 - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez added 2338 commits

    added 2338 commits

    Compare with previous version

  • David Fernandez added 1 commit

    added 1 commit

    • 549191f0 - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez added 1 commit

    added 1 commit

    • 1ccad161 - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez requested review from @tkuah

    requested review from @tkuah

  • David Fernandez
  • Thong Kuah changed milestone to %16.3

    changed milestone to %16.3

  • Thong Kuah changed milestone to %16.4

    changed milestone to %16.4

  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • David Fernandez added 7709 commits

    added 7709 commits

    Compare with previous version

  • David Fernandez added 1 commit

    added 1 commit

    • 74908b3a - Add the path traversal check middleware

    Compare with previous version

  • added workflowin review label and removed workflowblocked label

  • David Fernandez requested review from @tkuah

    requested review from @tkuah

  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Thong Kuah approved this merge request

    approved this merge request

  • David Fernandez changed milestone to %16.5

    changed milestone to %16.5

  • David Fernandez added 1 commit

    added 1 commit

    • 1e0579d1 - Add the path traversal check middleware

    Compare with previous version

  • David Fernandez requested review from @tkuah

    requested review from @tkuah

  • Thong Kuah
  • David Fernandez changed the description

    changed the description

  • David Fernandez added 4758 commits

    added 4758 commits

    Compare with previous version

  • Thong Kuah approved this merge request

    approved this merge request

  • Nick Malcolm approved this merge request

    approved this merge request

  • Dmitry Gruzd resolved all threads

    resolved all threads

  • Dmitry Gruzd enabled an automatic merge when the pipeline for 4c16e38d succeeds

    enabled an automatic merge when the pipeline for 4c16e38d succeeds

  • merged

  • Dmitry Gruzd mentioned in commit 3fd5ba77

    mentioned in commit 3fd5ba77

  • added workflowstaging label and removed workflowcanary label

  • David Fernandez mentioned in merge request !133465 (merged)

    mentioned in merge request !133465 (merged)

  • David Fernandez mentioned in merge request !134054 (merged)

    mentioned in merge request !134054 (merged)

  • David Fernandez mentioned in merge request !135592 (merged)

    mentioned in merge request !135592 (merged)

  • David Fernandez mentioned in merge request !138368 (merged)

    mentioned in merge request !138368 (merged)

  • David Fernandez mentioned in merge request !149537 (merged)

    mentioned in merge request !149537 (merged)

  • Please register or sign in to reply
    Loading