DOMPurify: Disallow form tag by default
Implements https://gitlab.com/gitlab-org/gitlab/-/issues/370314.
What does this MR do and why?
This MR forbids the <form>
and in <input>
DOMPurify
& v-safe-html
. This is being done to prevent possible injection attacks.
To learn more about the security part of it, see related issue.
This change is feature flag controlled with a new flag dompurify_advance_filter
.
Note: The input tags will be forbidden with #383333.
Screenshots or screen recordings
No visual changes for users.
How to set up and validate locally
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 @djadmin
- A deleted user
added frontend label
2 Warnings ⚠ featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
⚠ This merge request changed files with disabled eslint rules. Please consider fixing them. 1 Message 📖 CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Disabled eslint rules
The following files have disabled
eslint
rules. Please consider fixing them:spec/frontend/lib/dompurify_spec.js
Run the following command for more details
node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \ 'spec/frontend/lib/dompurify_spec.js'
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 Stanislav Lashmanov ( @slashmanov
) (UTC+4, 1.5 hours behind@djadmin
)Andrew Fontaine ( @afontaine
) (UTC-5, 10.5 hours behind@djadmin
)Application Security Reviewer review is optional for Application Security Costel Maxim ( @cmaxim
) (UTC+2, 3.5 hours behind@djadmin
)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.
👋 @djadmin - please see the following guidance and update this merge request.1 Error ❌ Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
changed milestone to %15.7
added featureenhancement security typefeature workflowin dev labels
added pipeline:run-all-rspec label
Allure report
allure-report-publisher
generated test report!e2e-review-qa:
❗ test report for 9d9d9a1fexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Manage | 39 | 0 | 4 | 2 | 43 | ❗ | | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Create | 28 | 0 | 1 | 1 | 29 | ❗ | | Govern | 10 | 0 | 5 | 1 | 15 | ❗ | | Verify | 12 | 0 | 1 | 0 | 13 | ✅ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 147 | 0 | 15 | 4 | 162 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
❗ test report for 405c7aebexpand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Verify | 82 | 0 | 15 | 0 | 97 | ✅ | | Secure | 9 | 0 | 3 | 1 | 12 | ❗ | | Create | 279 | 0 | 9 | 2 | 288 | ❗ | | Manage | 125 | 0 | 24 | 3 | 149 | ❗ | | Fulfillment | 4 | 0 | 26 | 0 | 30 | ✅ | | Govern | 76 | 0 | 0 | 0 | 76 | ✅ | | Release | 11 | 0 | 0 | 0 | 11 | ✅ | | Plan | 112 | 0 | 0 | 0 | 112 | ✅ | | Configure | 0 | 0 | 6 | 0 | 6 | ➖ | | Package | 0 | 0 | 5 | 0 | 5 | ➖ | | Version sanity check | 0 | 0 | 2 | 2 | 2 | ➖ | | Analytics | 3 | 0 | 0 | 0 | 3 | ✅ | | ModelOps | 0 | 0 | 2 | 0 | 2 | ➖ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 701 | 0 | 92 | 8 | 793 | ❗ | +----------------------+--------+--------+---------+-------+-------+--------+
- A deleted user
added backend feature flag groupdynamic analysis labels
added devopssecure sectionsec labels
removed backend devopssecure feature flag groupdynamic analysis sectionsec labels
Setting label groupdynamic analysis based on
@djadmin
's group.