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

Fix code scanning alert no. 2: Incomplete string escaping or encoding #497

Closed
wants to merge 1 commit into from
Closed

Fix code scanning alert no. 2: Incomplete string escaping or encoding #497

wants to merge 1 commit into from

Conversation

jpmckinney
Copy link
Member

Fixes https://github.com/open-contracting/spoonbill-web/security/code-scanning/2

To fix the problem, we need to ensure that all occurrences of the characters [ and ] in the name variable are properly escaped. This can be achieved by using the global flag (g) in the regular expressions. This way, all instances of the characters will be replaced, not just the first one.

We will modify the replace method calls on line 2 to use regular expressions with the global flag.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

@jpmckinney @github-advanced-security
Fix code scanning alert no. 2: Incomplete string escaping or encoding
6bcdea4 
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 4, 2024
View reviewed changes
frontend/src/utils/getQueryParam.js
@@ -1,5 +1,5 @@
export default (name) => {
name = name.replace(/[[]/, '\\[').replace(/[\]]/, '\\]');
name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to ensure that all special characters, including backslashes, are properly escaped in the input string. The best way to achieve this is by using a well-tested sanitization library, such as escape-string-regexp, which handles all necessary escaping. This approach ensures that the input string is correctly sanitized without introducing new vulnerabilities.

Suggested changeset 2
frontend/src/utils/getQueryParam.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/frontend/src/utils/getQueryParam.js b/frontend/src/utils/getQueryParam.js
--- a/frontend/src/utils/getQueryParam.js
+++ b/frontend/src/utils/getQueryParam.js
@@ -1,3 +1,5 @@
+import escapeStringRegexp from 'escape-string-regexp';
+
 export default (name) => {
-    name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');
+    name = escapeStringRegexp(name);
     const regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
EOF
@@ -1,3 +1,5 @@
import escapeStringRegexp from 'escape-string-regexp';

export default (name) => {
name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');
name = escapeStringRegexp(name);
const regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
frontend/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/frontend/package.json b/frontend/package.json
--- a/frontend/package.json
+++ b/frontend/package.json
@@ -15,3 +15,4 @@
     "vuetify": "^2.6.10",
-    "vuex": "^3.4.0"
+    "vuex": "^3.4.0",
+    "escape-string-regexp": "^5.0.0"
   },
EOF
@@ -15,3 +15,4 @@
"vuetify": "^2.6.10",
"vuex": "^3.4.0"
"vuex": "^3.4.0",
"escape-string-regexp": "^5.0.0"
},
This fix introduces these dependencies
Package Version Security advisories
escape-string-regexp (npm) 5.0.0 None
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
frontend/src/utils/getQueryParam.js
@@ -1,5 +1,5 @@
export default (name) => {
name = name.replace(/[[]/, '\\[').replace(/[\]]/, '\\]');
name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to ensure that all occurrences of the backslash character are properly escaped in the input string. This can be achieved by using a regular expression with the global flag (g) to replace all occurrences of the backslash character. Additionally, we should ensure that the existing replacements for square brackets are also using the global flag to replace all occurrences.

The best way to fix the problem without changing existing functionality is to update the name.replace method calls to include the global flag and add a replacement for backslashes. This will ensure that all occurrences of the specified characters are properly escaped.

Suggested changeset 1
frontend/src/utils/getQueryParam.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/frontend/src/utils/getQueryParam.js b/frontend/src/utils/getQueryParam.js
--- a/frontend/src/utils/getQueryParam.js
+++ b/frontend/src/utils/getQueryParam.js
@@ -1,3 +1,3 @@
 export default (name) => {
-    name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');
+    name = name.replace(/\\/g, '\\\\').replace(/\[/g, '\\[').replace(/\]/g, '\\]');
     const regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
EOF
@@ -1,3 +1,3 @@
export default (name) => {
name = name.replace(/\[/g, '\\[').replace(/\]/g, '\\]');
name = name.replace(/\\/g, '\\\\').replace(/\[/g, '\\[').replace(/\]/g, '\\]');
const regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
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
@jpmckinney jpmckinney closed this Dec 4, 2024
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
@jpmckinney