Resolve Incident 19090 - allow for special characters in commit diffs

What does this MR do and why?

Fixes Incident 19090.

Reviewer note This MR looks large but it mostly reintroduces changes from the original MR with additional logic to account for possible special characters.

In that incident, we saw Ultimate customers with secret push protection (SPP) enabled were having their pushes rejected with a 500 error when the commit included a special character, like the em-dash ("—") or the trademark symbol (™).

The error we saw was an Encoding::UndefinedConversionError when the code attempted to create an instance of Gitlab::SecretDetection::GRPC::ScanRequest::Payload with the data contained in DiffBlobs containing the special character. The data was encoded in ASCII-8BIT and the gRPC libraries we attempting to convert it to UTF-8 and failing. This did not seem to occur with SPP disabled, thus parsing the whole file.

In this MR we use force_encoding('UTF-8') on the diff data if it isn't already.

force_encoding simply marks the string as encoded as such but does no validation. String#encode attempts to transcode the string into the. desired encoding but in this situation that fails exactly like we saw on production from the gRPC object creation. Since the data appears to be coming from Gitaly as unicode but interpreted as being encoded as ASCII-8BIT, using force_encoding appears to work.

References

• Incident 19090.
• Incident follow-up
• Original MR

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

See the Steps to reproduce on the related issue.

Related to #512315 (closed)

changed milestone to %17.8

added Category:Secret Detection devopsapplication security testing groupsecret detection reproduced on GitLab.com sectionsec typebug workflowin dev labels

assigned to @eurie

added pipelinetier-1 label

Dependency change review report

Please wait to merge until below tasks are completed by @gitlab-com/gl-security/appsec

• Review metadata report and resolve this thread. Review guidelines are at handbook page

Click to view metadata analysis report

Modified Dependency: gitlab-secret_detection (0.14.2) Location: Gemfile.lock Version diffs

Checks passed: 3/5 ⚠️

• ℹ️️ Latest stable version: 0.15.0 released on: 2025-01-09. URL: https://rubygems.org/gems/gitlab-secret_detection
• ⚠️ Latest version is not in use.
• ✅ Total downloads: 107861
• ⚠️ Reverse gem dependencies: 0
• ✅ Total number of releases: 23
• ✅ Latest stable version age (months): 0
• ℹ️️ Gem source could not be located in GitHub.
• ℹ️️ Maintainer emails are private. Email domain check skipped.

⚠️ This automation is under testing, please leave your feedback in the issue.

mentioned in incident gitlab-com/gl-infra/production#19090 (closed)

added backend label

	8 Warnings
⚠️	This merge request is quite big (889 lines changed), please consider splitting it into multiple merge requests.
⚠️	95165aeb: The commit subject may not be longer than 72 characters. For more information, take a look at our Commit message guidelines.
⚠️	8b25c302: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
⚠️	d1c05753: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
⚠️	7302f670: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
⚠️	fee68c77: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
⚠️	b139eba6: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
⚠️	8c02ca8f: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

	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

Category	Reviewer	Maintainer
backend	@panoskanell (UTC+2, 7 hours ahead of author)	@hmehra (UTC+11, 16 hours ahead of author)
~"Secure::Secret Detection"	Reviewer review is optional for ~"Secure::Secret Detection"	@serenafang (UTC-6, 1 hour behind author)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

Rubygems

This merge request adds, or changes a Rubygems dependency. Please review the Gemfile guidelines.

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

Generated by 🚫 Danger

changed the description

removed backend label

resolved all threads

changed the description

changed the description

@serenafang can you do the backend review since you did it for the original MR?

requested review from @serenafang

mentioned in issue #512315 (closed)

added 1 commit

• a2b7531c - Removing special character from spec names to fix rspec junit formatter faiiling in pipelines

Compare with previous version

added backend label

changed milestone to %17.9

added missed:17.8 label

approved this merge request

added pipeline:mr-approved label

added pipelinetier-2 label and removed pipelinetier-1 label

Before you set this MR to auto-merge

This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

Before you set this MR to auto-merge, please check the following:

• You are the last maintainer of this merge request
• The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
• This pipeline is recent enough (created in the last 8 hours)

If all the criteria above apply, please set auto-merge for this merge request.

See pipeline tiers and merging a merge request for more details.

requested review from @ahmed.hemdan

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: ✅ test report for 9ed61288

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Verify      | 50     | 0      | 20      | 0     | 70    | ✅     |
| Plan        | 82     | 0      | 8       | 0     | 90    | ✅     |
| Create      | 138    | 0      | 20      | 0     | 158   | ✅     |
| Govern      | 80     | 0      | 12      | 0     | 92    | ✅     |
| Package     | 25     | 0      | 13      | 0     | 38    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| Secure      | 4      | 0      | 3       | 0     | 7     | ✅     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 430    | 0      | 123     | 0     | 553   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-cng: ✅ test report for 9ed61288

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
| Create      | 143    | 0      | 19      | 0     | 162   | ✅     |
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Govern      | 84     | 0      | 10      | 0     | 94    | ✅     |
| Package     | 30     | 0      | 14      | 0     | 44    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Verify      | 51     | 0      | 19      | 0     | 70    | ✅     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| Secure      | 2      | 0      | 5       | 0     | 7     | ✅     |
| Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 447    | 0      | 122     | 0     | 569   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-omnibus: ✅ test report for 9ed61288

expand test summary

+---------------------------------------------------------------------+
|                           suites summary                            |
+----------------+--------+--------+---------+-------+-------+--------+
|                | passed | failed | skipped | flaky | total | result |
+----------------+--------+--------+---------+-------+-------+--------+
| Data Stores    | 145    | 0      | 41      | 0     | 186   | ✅     |
| Package        | 106    | 0      | 52      | 0     | 158   | ✅     |
| Govern         | 355    | 0      | 32      | 0     | 387   | ✅     |
| Manage         | 31     | 0      | 44      | 0     | 75    | ✅     |
| Monitor        | 36     | 0      | 49      | 0     | 85    | ✅     |
| Ai-powered     | 1      | 0      | 8       | 0     | 9     | ✅     |
| Secure         | 20     | 0      | 9       | 0     | 29    | ✅     |
| Create         | 573    | 0      | 77      | 0     | 650   | ✅     |
| Verify         | 204    | 0      | 76      | 0     | 280   | ✅     |
| Release        | 20     | 0      | 4       | 0     | 24    | ✅     |
| Plan           | 329    | 0      | 32      | 0     | 361   | ✅     |
| GitLab Metrics | 2      | 0      | 1       | 0     | 3     | ✅     |
| Analytics      | 9      | 0      | 0       | 0     | 9     | ✅     |
| Fulfillment    | 10     | 0      | 28      | 0     | 38    | ✅     |
| Configure      | 1      | 0      | 12      | 0     | 13    | ✅     |
| Systems        | 6      | 0      | 1       | 0     | 7     | ✅     |
| ModelOps       | 0      | 0      | 4       | 0     | 4     | ➖     |
| Growth         | 0      | 0      | 8       | 0     | 8     | ➖     |
+----------------+--------+--------+---------+-------+-------+--------+
| Total          | 1848   | 0      | 478     | 0     | 2326  | ✅     |
+----------------+--------+--------+---------+-------+-------+--------+

added 1 commit

• 66e0358f - Change special character tests to test for secret detection too

Compare with previous version

reset approvals from @serenafang by pushing to the branch

mentioned in epic gitlab-org#14935 (closed)

mentioned in issue #494910

added 1 commit

• 6bb6c852 - Change special character tests to test for secret detection too

Compare with previous version

@eurie While I was catching up, I came across this MR. I've added some comments from my end, I hope that's alright. Let me know your thoughts.

mentioned in merge request gitlab-org/security-products/secret-detection/secret-detection-service!50 (closed)

added 1410 commits

• 6bb6c852...d4e76db8 - 1396 commits from branch master
• d4e76db8...fee68c77 - 4 earlier commits
• 1a36076d - Code review suggestions
• 7302f670 - Update SDS gem to 0.14.1
• 4d0a547a - Address undercoverage pipeline failure
• d1c05753 - Update SD gem to 0.14.2 to fix name collision
• 8b25c302 - Rebased with master
• 0737bf4e - Add shared specs and context to test special characters
• c5a919ac - Fixed shared examples and context for diffs
• 9916956e - Only convert payload data if it is not already UTF-8
• 95165aeb - Removing special character from spec names to fix rspec junit formatter faiiling in pipelines
• 91c7d501 - Change special character tests to test for secret detection too

Compare with previous version

added 1 commit

• 240d252c - Fix log output specs for scanner initialization

Compare with previous version

added 1 commit

• bd6bfa4e - Fix log output specs for scanner initialization

Compare with previous version

added 1 commit

• aea0f5de - Run the SDS request in its own thread

Compare with previous version

requested review from @ahmed.hemdan

@eurie Thanks for addressing my comments. I'm okay with addressing my non-blocking comments in future iterations.

I did a re-review and addded a couple more comments. I'd love your thoughts on them.

approved this merge request

added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-2 label

removed pipeline:run-e2e-omnibus-once label

requested review from @ahmed.hemdan and removed approval

added 1 commit

• b64e826e - Slight refactor around the new Thread block and settings

Compare with previous version

approved this merge request

resolved all threads

added 356 commits

• b64e826e...ae3637f9 - 355 commits from branch master
• 9ed61288 - Merge branch 'master' into '512315-fix-incident-19090-special-chars-in-diffs'

Compare with previous version

reset approvals from @ahmed.hemdan by pushing to the branch

approved this merge request

enabled automatic add to merge train when checks pass

started a merge train

merged

mentioned in commit 0e95568f

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

mentioned in issue #509035 (closed)