Raise if path isn't a string
What does this MR do and why?
Related to https://gitlab.com/gitlab-org/gitlab/-/issues/371994
Kicking off a quick MR to see if specs fail. I will clean it up with specs depending on how much stuff breaks (if at all!)
Screenshots or screen recordings
N/A
How to set up and validate locally
From the rails console
[1] pry(main)> attack = [ ".." , "..", "..", "..", "..", "..", "..", "..", "..", "..", "etc", "passwd"]
=> ["..", "..", "..", "..", "..", "..", "..", "..", "..", "..", "etc", "passwd"]
[2] pry(main)> Gitlab::Utils.check_path_traversal! attack
Gitlab::Utils::PathTraversalAttackError: Invalid path
from /home/dcouture/gdk/canonical/gitlab/lib/gitlab/utils.rb:20:in `check_path_traversal!'
Previously this did not raise
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.
Merge request reports
Activity
assigned to @dcouture
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @rymai
,@rspeicher
,@aqualls
,@vsizov
,@reprazent
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
- A deleted user
added backend label
1 Warning Please add a merge request subtype to this merge request. 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 Harsimar Sandhu ( @harsimarsandhu
) (UTC+5.5, 9.5 hours ahead of@dcouture
)Douglas Barbosa Alexandre ( @dbalexandre
) (UTC-3, 1 hour ahead of@dcouture
)Application Security Reviewer review is optional for Application Security Costel Maxim ( @cmaxim
) (UTC+3, 7 hours ahead of@dcouture
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. 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
Danger- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@dcouture - please see the following guidance and update this merge request.1 Error, 1 Warning Please add typebug typefeature, or typemaintenance label to this merge request. Please add a subtype label to this merge request. If you have added a type label and do not feel the purpose of this merge request matches one of the subtypes labels, please resolve this discussion.
Edited by 🤖 GitLab Bot 🤖
Allure report
allure-report-publisher
generated test report!review-qa-blocking:
test report for 3020e2d8expand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Create | 28 | 0 | 1 | 3 | 29 | ❗ | | Plan | 47 | 0 | 1 | 0 | 48 | ✅ | | Manage | 46 | 0 | 3 | 12 | 49 | ❗ | | Secure | 2 | 0 | 0 | 0 | 2 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Verify | 12 | 0 | 1 | 2 | 13 | ❗ | | Protect | 2 | 0 | 0 | 0 | 2 | ✅ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 146 | 0 | 9 | 17 | 155 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
- Resolved by Doug Stull
I tried running one of the specs and saw that it was failing because we were passing in a
Gitlab::HashedPath
object.I guess we could call
to_s
for these objects
added 5625 commits
-
3020e2d8...34679d04 - 5624 commits from branch
master
- 97a4b6d6 - Raise if path isn't a string
-
3020e2d8...34679d04 - 5624 commits from branch
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @rymai
,@dbalexandre
,@rspeicher
,@mayra-cabrera
,@vsizov
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender Botchanged milestone to %15.5
added feature featureenhancement security securitybotignore typefeature + 1 deleted label
removed featureenhancement label
added typebug label and removed feature typefeature labels
removed securitybotignore label
added 62 commits
-
0036263e...5f4cc918 - 61 commits from branch
master
- 6d36dce7 - Raise if path isn't a string
-
0036263e...5f4cc918 - 61 commits from branch
added 1 commit
- 06ed5ef9 - Paths that aren't strings are considered invalid
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- Resolved by Doug Stull
There was an issue with the review app job so I rebased
@john-mason can you review this MR please? Thanks!
- Resolved by Dominic Couture
@nmalcolm can you do the security review please? It's required because this changes the path traversal check. Thanks!
requested review from @nmalcolm
requested review from @dstull and removed review request for @john-mason
@john-mason
, 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:
removed review request for @dstull
enabled an automatic merge when the pipeline for 6daa5f16 succeeds
- Resolved by Doug Stull
Rebasing, the
rspec foss-impact
failures seem unrelated.
added 575 commits
-
06ed5ef9...1c0abcb5 - 574 commits from branch
master
- c01de7f3 - Paths that aren't strings are considered invalid
-
06ed5ef9...1c0abcb5 - 574 commits from branch
requested review from @dstull
enabled an automatic merge when the pipeline for ff876672 succeeds
added 413 commits
-
c01de7f3...bd007e39 - 412 commits from branch
master
- eda6a8bb - Paths that aren't strings are considered invalid
-
c01de7f3...bd007e39 - 412 commits from branch
enabled an automatic merge when the pipeline for 269176ba succeeds
mentioned in issue #377433 (closed)
mentioned in merge request !100793 (merged)
removed review request for @dstull
added 434 commits
-
eda6a8bb...5a890861 - 433 commits from branch
master
- 6619a62f - Paths that aren't strings are considered invalid
-
eda6a8bb...5a890861 - 433 commits from branch
added 297 commits
-
6619a62f...c627d72c - 296 commits from branch
master
- 84d7cacf - Paths that aren't strings are considered invalid
-
6619a62f...c627d72c - 296 commits from branch
requested review from @dstull
changed milestone to %15.6
enabled an automatic merge when the pipeline for 29f022e5 succeeds
mentioned in commit 7c230637
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1646 (merged)
added releasedpublished label and removed releasedcandidate label