Update --extra-index-url usage in repository webpages
What does this MR do and why?
A fix for Issue #384253 (closed).
This MR changes --extra-index-url to --index-url in order to prevent users from using the first command without understanding its security implications. Since using -extra-index-url can lead to dependency confusion (for further details see: https://hackerone.com/reports/1681275), this change increases users' security while using GitLab.
Furthermore, forwarding of requests for unknown packages to pypi.org should be disabled by default. This setting can have dangerous side effects and should not be enabled by default. Administrators that understand the consequences could still enable the feature for their instance. Even though the option to disable "PyPi Forwarding" is described in the documentation, it is a feature that can only be configured with a "Premium" subscription. Users with a "Free" subscription cannot disable "PyPi Forwarding". Therefore to strengthen security, the GitLab Team might consider adding this option to the "Free" tier of GitLab.
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
Hey @usdResponsibleDisclosure!
Thank you for your contribution to GitLab. Please refer to the contribution flow documentation for a quick overview of the process, and the merge request (MR) guidelines for the detailed process.
When you're ready for a first review, post
@gitlab-bot ready
. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with@gitlab-bot ready @user1 @user2
.At any time, if you need help, feel free to post
@gitlab-bot help
or initiate a mentor session on Discord. Read more on how to get help.To enable automated checks on your MR, please configure Danger for your fork.
You can comment
@gitlab-bot label <label1> <label2>
to add labels to your MR. Please see the list of allowed labels in thelabel
command documentation.This message was generated automatically. You're welcome to improve it.
added Community contribution workflowin dev labels
assigned to @usdResponsibleDisclosure
Important: This MR may be part of the running Hackathon! Check out the Hackathon page to see if it is eligible for a chance to win a prize!
This message was generated automatically. You're welcome to improve it.
added Hackathon label
added 1st contribution label
mentioned in issue gitlab-org/quality/triage-reports#10194 (closed)
added groupsource code maintenancerefactor labels
added typemaintenance label
added devopscreate sectiondev labels
@gitlab-bot ready
added workflowready for review label and removed workflowin dev label
@bwill
, this Community contribution is ready for review.- Do you have capacity and domain expertise to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
- Add the workflowin dev label if the merge request needs action from the author. This message was generated automatically. You're welcome to improve it.
requested review from @bwill
added frontend label
@usdResponsibleDisclosure Thanks for the MR and the very thorough HackerOne report. Your current changes are updating a test snapshot. This will not change how the text appears in the UI. In order to do that, we need to make these changes:
diff --git a/app/assets/javascripts/packages_and_registries/package_registry/components/details/pypi_installation.vue b/app/assets/javascripts/packages_and_registries/package_registry/components/details/pypi_installation.vue index dd58f28a2626..fdc6e75c932f 100644 --- a/app/assets/javascripts/packages_and_registries/package_registry/components/details/pypi_installation.vue +++ b/app/assets/javascripts/packages_and_registries/package_registry/components/details/pypi_installation.vue @@ -30,7 +30,7 @@ export default { computed: { pypiPipCommand() { // eslint-disable-next-line @gitlab/require-i18n-strings - return `pip install ${this.packageEntity.name} --extra-index-url ${this.packageEntity.pypiUrl}`; + return `pip install ${this.packageEntity.name} --index-url ${this.packageEntity.pypiUrl}`; }, pypiSetupCommand() { return `[gitlab] diff --git a/spec/frontend/packages_and_registries/package_registry/components/details/pypi_installation_spec.js b/spec/frontend/packages_and_registries/package_registry/components/details/pypi_installation_spec.js index 20acb0872e55..4a27f8011dfe 100644 --- a/spec/frontend/packages_and_registries/package_registry/components/details/pypi_installation_spec.js +++ b/spec/frontend/packages_and_registries/package_registry/components/details/pypi_installation_spec.js @@ -16,7 +16,7 @@ const packageEntity = { ...packageData(), packageType: PACKAGE_TYPE_PYPI }; describe('PypiInstallation', () => { let wrapper; - const pipCommandStr = `pip install @gitlab-org/package-15 --extra-index-url ${packageEntity.pypiUrl}`; + const pipCommandStr = `pip install @gitlab-org/package-15 --index-url ${packageEntity.pypiUrl}`; const pypiSetupStr = `[gitlab] repository = ${packageEntity.pypiSetupUrl} username = __token__
Once you've made the changes, do
@gitlab-bot ready @bwill
and we should be able to get this merged pretty quickly.Also, I wondered if we should explain the security risk in the documentation, but it seems that has already been done: !55085 (merged)
added workflowin dev label and removed workflowready for review label
removed review request for @bwill
added 1 commit
- b1b22b95 - Fix --extra-index-url usage in spec/frontend/
- Resolved by Himanshu Kapoor
@gitlab-bot ready @bwill Thank you for your feedback on this MR, hopefully I've applied the changes correctly this time.
In order to strenghten security on Gitlab in addition to the changes in this MR, we would ask the Gitlab team to consider making the option to disable "PyPi Forwarding" available to all users, regardless of using Gitlab Premium or Free Tiers.
requested review from @bwill
- Resolved by Himanshu Kapoor
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 frontend Artur Fedorov (
@arfedoro
) (UTC+1)Himanshu Kapoor (
@himkp
) (UTC+1)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
added grouppackage registry label and removed groupsource code label
added devopspackage sectionops labels and removed devopscreate sectiondev labels
changed milestone to %15.7
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits a343bd3e and 344bd529
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.53 MB 3.53 MB - -0.0 % mainChunk 1.95 MB 1.95 MB - 0.0 %
Note: We do not have exact data for a343bd3e. So we have used data from: bac7c3db.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for 344bd529expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Manage | 34 | 0 | 3 | 1 | 37 | ❗ | | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Verify | 12 | 0 | 1 | 0 | 13 | ✅ | | Govern | 24 | 0 | 5 | 0 | 29 | ✅ | | Framework sanity | 9 | 0 | 1 | 0 | 10 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 156 | 0 | 13 | 1 | 169 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
removed review request for @bwill
- Resolved by usdResponsibleDisclosure
@bwill Sorry for not getting around to fixing the commit message issues sooner, but hopefully everything is according to your guidelines now.
requested review from @bwill
removed review request for @bwill
- Resolved by Himanshu Kapoor
@bwill Just re-added the change to snapshots, hopefully it'll work this time. Thank you!
changed milestone to %15.8
requested review from @himkp
- Resolved by Himanshu Kapoor
@bwill
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, please start a new pipeline before merging.
For more info, please refer to the following links:
added pipeline:mr-approved label
@usdResponsibleDisclosure, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:
- React with a
or a on this comment to describe your experience. - Create a new comment starting with
@gitlab-bot feedback
below, and leave any additional feedback you have for us in the comment.
Interested in learning more tips and tricks to solve your next challenge faster? Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.
Thanks for your help!
This message was generated automatically. You're welcome to improve it.
- React with a
mentioned in commit 742d1cf5
added workflowstaging-canary label and removed workflowin dev 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
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1829 (merged)
added releasedpublished label and removed releasedcandidate label
added linked-issue label