1904761 - Sonarqube Report shows "Remove this control character." in pdf.js 4.3.136

Status: RESOLVED INVALID

(Firefox :: PDF Viewer, defect)

Description

[anirudhtt]

Attachment: Screenshot 2024-06-26 122100.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36

Steps to reproduce:

While running sonarqube report on latest pdf.js version 4.3.136, following medium issue - Remove this control character.

Actual results:

"Remove this control character" Issues reported on following lines inside pdf.mjs:
Line No: 758
return decoded.replaceAll(/\x1b[^\x1b]*(?:\x1b|$)/g, "");

Line No: 9084:
if (!/^[\x00-\xFF]+$/.test(value)) {

Line No: 9165:
if (!value.startsWith("=?") || /[\x00-\x19\x80-\xff]/.test(value)) {

Line No: 9168:
return value.replaceAll(/=?([\w-])?([QqBb])?((?:[^?]|?(?!=)))?=/g, function (matches, charset, encoding, text) {

Expected results:

Report shoulod not contain these reliability issues. Attaching screenshot for the same. Report is taken on Sonarqube Version 10.5.0.89998

Please suggest justification/provide fix for the same.

Comment 1

[Andrew McCreight [:mccr8]]

What does this mean? Why is this a security issue?

Comment 2

[Andrew McCreight [:mccr8]]

It looks like the eslint rule no-control-regex was added in bug 1370240, which sounds like the same kind of thing, but maybe it doesn't apply to PDFjs.

Comment 3

[Calixte Denizet (:calixte)]

Sorry but what's the problem with control chars in regex especially when they're coded with their hex value (i.e. \x**) ?
pdf.js has to deal with a lot of different data and using regex is a way easier and optimal when compared with any other solutions.

Comment 4

[anirudhtt]

(In reply to Andrew McCreight [:mccr8] from comment #2)

It looks like the eslint rule no-control-regex was added in bug 1370240, which sounds like the same kind of thing, but maybe it doesn't apply to PDFjs.

Yes, sonarqube is complianing about this rule only. We just now upgraded pdf.js which we use in our pdf viewer product to the latest version. As per our policy, sonarqube is one of the tools we run on our code along with all the third party items used which includes pdf.js. If you can share your input/justification that this is not a cause for concern, we can flag this as a false positive in our application if anyone raises it.

Comment 5

[Jonas Jenwald [:Snuffleupagus]]

Please keep in mind that all code analysers will have false positives, and it'll thus be necessary for the user (of the analyser) to carefully evaluate the results and not just trust them as fact.
Note that "SonarQube" is not being used by the PDF.js project and it thus seems fair to expect that you at least attempt to validate the results yourself, before asking others to do that work for you.
(Here things also become more difficult, and thus time consuming, since you didn't link to actual source-code but only provided code-snippets.)

Looking briefly at the cases mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1904761#c0 all of them seem to be intended and correct, since changing the regular expressions would break the actual functionality of the code.
This can be inferred by reading not just the relevant lines but also the surrounding code, related comments, and (if necessary) by looking at the commit history.