Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update code to allow better static analysis #408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update code to allow better static analysis #408

wants to merge 1 commit into from
+83 −47

Conversation

holblin
Copy link

@holblin holblin commented Feb 12, 2025

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

holblin
holblin commented Feb 12, 2025
View reviewed changes
src/parse/index.ts
Comment on lines 154 to 160
/**
* Match `re` and return captures.
* Update position and css string. Return the matches
*/
function match(re: RegExp) {
const m = re.exec(css);
if (!m) {
return;
}
function processMatch(m: RegExpExecArray) {
const str = m[0];
updatePosition(str);
css = css.slice(str.length);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the bulk of the changes. The rest is just adaptation for this internal API change.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 12, 2025
View reviewed changes
src/parse/index.ts
Comment on lines +332 to +334
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
css,
);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
Loading
that depends on
library input
Loading
may run slow on strings starting with '(' and with many repetitions of '"'.
This
regular expression
Loading
that depends on
library input
Loading
may run slow on strings starting with '('' and with many repetitions of '''.
This
regular expression
Loading
that depends on
library input
Loading
may run slow on strings starting with '("' and with many repetitions of '"'.
src/parse/index.ts
);
let value = '';
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '(' and containing many repetitions of '""'.

Copilot Autofix AI 7 days ago

To fix the problem, we need to modify the regular expression to remove the ambiguity that causes exponential backtracking. Specifically, we can refactor the pattern to avoid nested quantifiers and ambiguous alternations. The goal is to ensure that each part of the regular expression has a clear and unambiguous match.

In this case, we can break down the regular expression into smaller, more manageable parts and use non-capturing groups to avoid ambiguity. We will replace the problematic part of the regular expression with a more efficient version.

Suggested changeset 1
src/parse/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/parse/index.ts b/src/parse/index.ts
--- a/src/parse/index.ts
+++ b/src/parse/index.ts
@@ -331,3 +331,3 @@
     const matchVal =
-      /^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
+      /^((?:'(?:\\'|[^'\\])*'|"(?:\\"|[^"\\])*"|\((?:'(?:\\'|[^'\\])*'|"(?:\\"|[^"\\])*"|[^)])*\)|[^};])+)/.exec(
         css,
EOF
@@ -331,3 +331,3 @@
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
/^((?:'(?:\\'|[^'\\])*'|"(?:\\"|[^"\\])*"|\((?:'(?:\\'|[^'\\])*'|"(?:\\"|[^"\\])*"|[^)])*\)|[^};])+)/.exec(
css,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/parse/index.ts
);
let value = '';
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '('' and containing many repetitions of ''''.

Copilot Autofix AI 7 days ago

To fix the problem, we need to remove the ambiguity in the regular expression that causes exponential backtracking. Specifically, we should modify the sub-expression (?:\\'|.)*? to ensure that it does not have overlapping matches. We can achieve this by explicitly excluding the single quote character from the . match using a negated character class.

  • Modify the regular expression on line 332 to replace (?:\\'|.)*? with (?:\\'|[^'])*?.
  • This change ensures that the . match does not include the single quote character, thus removing the ambiguity and preventing exponential backtracking.
Suggested changeset 1
src/parse/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/parse/index.ts b/src/parse/index.ts
--- a/src/parse/index.ts
+++ b/src/parse/index.ts
@@ -331,3 +331,3 @@
     const matchVal =
-      /^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
+      /^((?:'(?:\\'|[^'])*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|[^'])*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
         css,
EOF
@@ -331,3 +331,3 @@
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
/^((?:'(?:\\'|[^'])*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|[^'])*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
css,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/parse/index.ts
);
let value = '';
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '("' and containing many repetitions of '""'.

Copilot Autofix AI 7 days ago

To fix the problem, we need to modify the regular expression to remove the ambiguity that causes exponential backtracking. Specifically, we should replace the sub-expression (?:\\"|.)*? with a more efficient pattern that avoids ambiguity. One way to achieve this is to use a character class that excludes the characters we want to match separately, thus eliminating the need for nested quantifiers.

In this case, we can replace (?:\\"|.)*? with [^"\\]* to match any sequence of characters that are not double quotes or backslashes. This change will ensure that the regular expression performs efficiently without changing its functionality.

Suggested changeset 1
src/parse/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/parse/index.ts b/src/parse/index.ts
--- a/src/parse/index.ts
+++ b/src/parse/index.ts
@@ -331,3 +331,3 @@
     const matchVal =
-      /^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
+      /^((?:'(?:\\'|[^'])*?'|"(?:\\"|[^"])*?"|\((?:'(?:\\'|[^'])*?'|"(?:\\"|[^"])*?"|[^)])*?\)|[^};])+)/.exec(
         css,
EOF
@@ -331,3 +331,3 @@
const matchVal =
/^((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|\((?:'(?:\\'|.)*?'|"(?:\\"|.)*?"|[^)])*?\)|[^};])+)/.exec(
/^((?:'(?:\\'|[^'])*?'|"(?:\\"|[^"])*?"|\((?:'(?:\\'|[^'])*?'|"(?:\\"|[^"])*?"|[^)])*?\)|[^};])+)/.exec(
css,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/parse/index.ts
@@ -573,15 +603,16 @@
*/
function atcustommedia(): CssCustomMediaAST | void {
const pos = position();
const m = match(/^@custom-media\s+(--\S+)\s*([^{;\s][^{;]*);/);
const m = /^@custom-media\s+(--\S+)\s*([^{;\s][^{;]*);/.exec(css);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
Loading
that depends on
library input
Loading
may run slow on strings starting with '@custom-media --!!' and with many repetitions of '!!'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@holblin