Rack middleware for path traversal checks
![:shield: :shield:](/-/emojis/4/shield.png)
What if?
![:shield: :shield:](/-/emojis/4/shield.png)
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: :rocket:](/-/emojis/4/rocket.png)
Deployment plan
![:rocket: :rocket:](/-/emojis/4/rocket.png)
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:
- Simulation phase (
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.
-
feature flag enabled for a low-med percentage of requests, e.g.
- 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.
- 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.
- 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: :thinking:](/-/emojis/4/thinking.png)
What does this MR do and why?
![:thinking: :thinking:](/-/emojis/4/thinking.png)
- Add a middleware to check path traversals on requests.
- Gated behind a feature flag. Rollout issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415460.
- When enabled, the middleware runs the path traversal check and log any request that fails it.
- In other words, the middleware added by this MR will never block or reject any request.
- Add the related specs.
![:tv: :tv:](/-/emojis/4/tv.png)
Screenshots or screen recordings
![:tv: :tv:](/-/emojis/4/tv.png)
You know... this a rails middleware. So, no UI changes
So below to see the middleware in action
![:gear: :gear:](/-/emojis/4/gear.png)
How to set up and validate locally
![:gear: :gear:](/-/emojis/4/gear.png)
- 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.
-
I have evaluated the MR acceptance checklist for this MR.
![:crystal_ball: :crystal_ball:](/-/emojis/4/crystal_ball.png)
Other considerations
![:crystal_ball: :crystal_ball:](/-/emojis/4/crystal_ball.png)
![:racehorse: :racehorse:](/-/emojis/4/racehorse.png)
Why not workhorse?
![:racehorse: :racehorse:](/-/emojis/4/racehorse.png)
Excellent question
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: :bar_chart:](/-/emojis/4/bar_chart.png)
Get me some numbers
![:bar_chart: :bar_chart:](/-/emojis/4/bar_chart.png)
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 . About15+x
faster. - The problem with alternative implementations is that we introduce a risk of not properly covering all the cases.
- Among them, the simplified regex (simplified as we would expect to use it only on urls) and the
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).
Merge request reports
Activity
changed milestone to %Backlog
added backend security typemaintenance labels
assigned to @10io
removed security label
added featureenhancement typefeature labels and removed typemaintenance label
- A deleted user
added feature flag grouppackage registry labels
- Resolved by David Fernandez
added devopspackage sectionci labels
1 Message 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 (
@radbatnag
) (UTC+8, 6 hours ahead of@10io
)Thong Kuah (
@tkuah
) (UTC+13, 11 hours ahead of@10io
)Application Security Reviewer review is optional for Application Security Dominic Couture (
@dcouture
) (UTC+1, 1 hour behind@10io
)Please check reviewer's status!
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
danger-review
job that generated this comment.Generated by
Dangermentioned in commit gitlab-org-sandbox/gitlab-jh-validation@caae218e
Allure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 62a31af0expand 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:
test report for 62a31af0expand 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:
test report for 62a31af0expand test summary
+--------------------------------------------------------------+ | suites summary | +---------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +---------+--------+--------+---------+-------+-------+--------+ | Package | 68 | 0 | 8 | 0 | 76 | ✅ | +---------+--------+--------+---------+-------+-------+--------+ | Total | 68 | 0 | 8 | 0 | 76 | ✅ | +---------+--------+--------+---------+-------+-------+--------+
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@f911bb7a
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@5b97ebbe
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@223a5014
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- Resolved by Dmitry Gruzd
- Resolved by Dmitry Gruzd
- Resolved by Sylvester Chin
mentioned in merge request !123661 (merged)
added security typemaintenance labels and removed typefeature label
removed featureenhancement label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@7e249d2c
- Resolved by Thong Kuah
- Resolved by Sylvester Chin
requested review from @schin1
- Resolved by Thong Kuah
- Resolved by Thong Kuah
@10io this MR looks good overall! I've left a suggestion and a few questions. Back to you!
removed review request for @schin1
@10io Some end-to-end (E2E) tests have been selected based on the stage label on this MR. If not run already, please run thee2e:package-and-test-ee
job in theqa
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
emoji on this comment.For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@59485744
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 -
http://example.com/..
would be blocked: that seems fine -
http://example.com/...
would not be blocked: that seems fine - `http://example.com/.well-known/ would not be blocked
-
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.
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
. 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
WDYT @10io ? -
- Resolved by Thong Kuah
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!
@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:
added pipeline:mr-approved label
removed review request for @schin1
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@ce2e19d0
requested review from @nmalcolm
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@8c500280
added 1960 commits
-
a0099a26...923cc8ac - 1956 commits from branch
master
- c4913fc7 - Add the path traversal check middleware
- 3f6b7629 - Apply backend reviewer feedback
- 5a1c4b1c - Apply app sec review feedback
- 8e064efb - Add option to disable the double encoding check
Toggle commit list-
a0099a26...923cc8ac - 1956 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@52347f1c
requested review from @tkuah
- Resolved by David Fernandez
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by Dmitry Gruzd
Thanks @10io !
This is very nice work, and well thought out as well. I have some minor suggestions only.
removed review request for @tkuah
mentioned in merge request !123762 (merged)
added 2338 commits
-
60e00831...064e33d3 - 2337 commits from branch
master
- e4e509f6 - Add the path traversal check middleware
-
60e00831...064e33d3 - 2337 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@eb70c560
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@e69c4f52
requested review from @tkuah
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@a523de5e
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@4fa3ef3f
- Resolved by Dmitry Gruzd
changed milestone to %16.3
changed milestone to %16.4
removed review request for @tkuah
added workflowblocked label
added 7709 commits
-
1ccad161...7c5df9ca - 7708 commits from branch
master
- 01a7dcc4 - Add the path traversal check middleware
-
1ccad161...7c5df9ca - 7708 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@301caeba
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@db658cfc
added workflowin review label and removed workflowblocked label
requested review from @tkuah
removed review request for @tkuah
changed milestone to %16.5
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@25db7bc3
requested review from @tkuah
- Resolved by Dmitry Gruzd
added 4758 commits
-
1e0579d1...2d391e0d - 4757 commits from branch
master
- 62a31af0 - Add the path traversal check middleware
-
1e0579d1...2d391e0d - 4757 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@3881776a
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@4c16e38d
enabled an automatic merge when the pipeline for 4c16e38d succeeds
mentioned in commit 3fd5ba77
added workflowstaging-canary label and removed workflowin review label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
mentioned in merge request !133465 (merged)
added releasedcandidate label
mentioned in merge request !134054 (merged)
added releasedpublished label and removed releasedcandidate label
mentioned in merge request !135592 (merged)
mentioned in merge request !138368 (merged)
mentioned in issue gitlab-org/quality/triage-reports#15709 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16015 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16477 (closed)
mentioned in issue gitlab-org/quality/triage-reports#17011 (closed)
mentioned in merge request !149537 (merged)
mentioned in issue gitlab-org/quality/triage-reports#17460 (closed)
mentioned in issue gitlab-org/quality/triage-reports#17920 (closed)
mentioned in issue gitlab-org/quality/triage-reports#18453 (closed)
mentioned in issue gitlab-org/quality/triage-reports#18932 (closed)
mentioned in issue gitlab-org/quality/triage-reports#19386 (closed)
mentioned in issue gitlab-org/quality/triage-reports#20606 (closed)
mentioned in issue gitlab-org/quality/triage-reports#20972 (closed)
mentioned in issue gitlab-org/quality/triage-reports#21538 (closed)
mentioned in issue gitlab-org/quality/triage-reports#22037