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.

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

GOLD-101 stringify migration #25

Merged
merged 13 commits into from
Jun 7, 2024
Merged

GOLD-101 stringify migration #25

merged 13 commits into from
Jun 7, 2024

Conversation

kgmyatthu
Copy link
Contributor

No description provided.

Copy link

linear bot commented May 20, 2024

@kgmyatthu kgmyatthu requested a review from arhamj May 20, 2024 08:38
@arhamj
Copy link
Contributor

arhamj commented May 21, 2024

Changes LGTM but we will have to wait for shardus/types changes to be merged and released.

src/Data/Data.ts Dismissed Show dismissed Hide dismissed
src/Data/Data.ts Dismissed Show dismissed Hide dismissed
nestedCountersInstance.countEvent('archiver', 'clear_contact_timeout')
}
if (config.VERBOSE)
console.log('DATA', sender.nodeInfo.publicKey, sender.nodeInfo.ip, sender.nodeInfo.port)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is sender.nodeInfo.port which is logged on line 248 in the file src/Data/Data.ts.

We can sanitize the data by removing any newline characters from it. This can be done using the replace method in JavaScript. The replace method can take a regular expression as the first argument and a replacement string as the second argument. We can use the regular expression /\n|\r/g to match any newline characters and replace them with an empty string.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -246,4 +246,6 @@
 
-      if (config.VERBOSE)
-        console.log('DATA', sender.nodeInfo.publicKey, sender.nodeInfo.ip, sender.nodeInfo.port)
+      if (config.VERBOSE) {
+        let sanitizedPort = sender.nodeInfo.port.replace(/\n|\r/g, "");
+        console.log('DATA', sender.nodeInfo.publicKey, sender.nodeInfo.ip, sanitizedPort)
+      }
 
EOF
@@ -246,4 +246,6 @@

if (config.VERBOSE)
console.log('DATA', sender.nodeInfo.publicKey, sender.nodeInfo.ip, sender.nodeInfo.port)
if (config.VERBOSE) {
let sanitizedPort = sender.nodeInfo.port.replace(/\n|\r/g, "");
console.log('DATA', sender.nodeInfo.publicKey, sender.nodeInfo.ip, sanitizedPort)
}

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
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port,

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where a malicious user can manipulate the log entries by providing input with special characters that are interpreted when the log output is displayed.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is sender.nodeInfo.port which is logged on line 256 in the file src/Data/Data.ts.

We can sanitize the data by removing any newline characters from it. This can be done using the replace method in JavaScript. The replace method searches a string for a specified value, or a regular expression, and returns a new string where the specified values are replaced. We can use a regular expression to match any newline characters and replace them with an empty string.

The regular expression for matching newline characters is /\n|\r/g. Here, \n matches a newline character, \r matches a carriage return character, | is a logical OR operator which matches the pattern before or the pattern after the operator, and g is a flag that indicates a global search which means find all matches rather than stopping after the first match.

So, the code to sanitize sender.nodeInfo.port would be sender.nodeInfo.port.replace(/\n|\r/g, "").

We need to make this change on line 256 in the file src/Data/Data.ts.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -255,3 +255,3 @@
             sender.nodeInfo.ip,
-            sender.nodeInfo.port,
+            sender.nodeInfo.port.replace(/\n|\r/g, ""),
             newData.responses.ORIGINAL_TX_DATA.length
EOF
@@ -255,3 +255,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port,
sender.nodeInfo.port.replace(/\n|\r/g, ""),
newData.responses.ORIGINAL_TX_DATA.length
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
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port,

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is sender.nodeInfo.port which is logged on line 271 in the file src/Data/Data.ts.

The best way to sanitize this data is to remove any newline characters from it. This can be done using the replace method in JavaScript. The replace method can take a regular expression as its first argument and a replacement string as its second argument. We can use the regular expression /\n|\r/g to match any newline characters and replace them with an empty string.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -270,3 +270,3 @@
             sender.nodeInfo.ip,
-            sender.nodeInfo.port,
+            sender.nodeInfo.port.replace(/\n|\r/g, ""),
             newData.responses.RECEIPT.length
EOF
@@ -270,3 +270,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port,
sender.nodeInfo.port.replace(/\n|\r/g, ""),
newData.responses.RECEIPT.length
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
console.log(
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where a malicious user can manipulate the log entries by providing input with characters that are interpreted when the log output is displayed.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, we can use the String.prototype.replace method to remove any newline characters from the user-provided data. This will prevent the user from injecting new log entries.

The specific lines to change are in the file src/Data/Data.ts. We need to sanitize sender.nodeInfo.port before logging it on lines 289 and 295.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -288,3 +288,3 @@
           sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sender.nodeInfo.port.replace(/\n|\r/g, "")
         )
@@ -294,3 +294,3 @@
           sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sender.nodeInfo.port.replace(/\n|\r/g, "")
         )
EOF
@@ -288,3 +288,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port.replace(/\n|\r/g, "")
)
@@ -294,3 +294,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port.replace(/\n|\r/g, "")
)
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
)
Logger.mainLogger.debug(
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, we can use the String.prototype.replace method to remove any newline characters from the user-provided data. This will prevent the attacker from injecting new log entries.

We need to make changes in the file src/Data/Data.ts. Specifically, we need to sanitize sender.nodeInfo.publicKey, sender.nodeInfo.ip, and sender.nodeInfo.port before logging them.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -284,7 +284,10 @@
       if (newData.responses && newData.responses.ACCOUNT) {
+        let sanitizedPublicKey = sender.nodeInfo.publicKey.replace(/\n|\r/g, "");
+        let sanitizedIp = sender.nodeInfo.ip.replace(/\n|\r/g, "");
+        let sanitizedPort = sender.nodeInfo.port.replace(/\n|\r/g, "");
         console.log(
           'RECEIVED ACCOUNTS DATA',
-          sender.nodeInfo.publicKey,
-          sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sanitizedPublicKey,
+          sanitizedIp,
+          sanitizedPort
         )
@@ -292,5 +295,5 @@
           'RECEIVED ACCOUNTS DATA',
-          sender.nodeInfo.publicKey,
-          sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sanitizedPublicKey,
+          sanitizedIp,
+          sanitizedPort
         )
EOF
@@ -284,7 +284,10 @@
if (newData.responses && newData.responses.ACCOUNT) {
let sanitizedPublicKey = sender.nodeInfo.publicKey.replace(/\n|\r/g, "");
let sanitizedIp = sender.nodeInfo.ip.replace(/\n|\r/g, "");
let sanitizedPort = sender.nodeInfo.port.replace(/\n|\r/g, "");
console.log(
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port
sanitizedPublicKey,
sanitizedIp,
sanitizedPort
)
@@ -292,5 +295,5 @@
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port
sanitizedPublicKey,
sanitizedIp,
sanitizedPort
)
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
Logger.mainLogger.debug(
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
'RECEIVED ACCOUNTS DATA',
sender.nodeInfo.publicKey,
sender.nodeInfo.ip,
sender.nodeInfo.port

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, we need to sanitize sender.nodeInfo.port before logging it in the console.log and Logger.mainLogger.debug statements.

We can sanitize the data by removing any newline characters (\n and \r) from the user-provided data. This can be done using the replace method of the string, which can replace all occurrences of a substring with another string.

The specific lines to change are 289 and 295 in the file src/Data/Data.ts.

Suggested changeset 1
src/Data/Data.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/Data/Data.ts b/src/Data/Data.ts
--- a/src/Data/Data.ts
+++ b/src/Data/Data.ts
@@ -288,3 +288,3 @@
           sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sender.nodeInfo.port.replace(/\n|\r/g, "")
         )
@@ -294,3 +294,3 @@
           sender.nodeInfo.ip,
-          sender.nodeInfo.port
+          sender.nodeInfo.port.replace(/\n|\r/g, "")
         )
EOF
@@ -288,3 +288,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port.replace(/\n|\r/g, "")
)
@@ -294,3 +294,3 @@
sender.nodeInfo.ip,
sender.nodeInfo.port
sender.nodeInfo.port.replace(/\n|\r/g, "")
)
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
try {
return StringUtils.safeJsonParse(text)
} catch (parseError) {
console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is a URL and a parse error message. We can sanitize these by removing any newline characters (\n and \r) that they may contain. This can be done using the replace method of the String object in JavaScript.

The specific lines to change are in the file src/P2P.ts, specifically line 95 where the user-provided data is logged. We need to replace this line with a version that sanitizes the url and parseError variables before logging them.

No new methods, imports, or definitions are needed to implement these changes.

Suggested changeset 1
src/P2P.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/P2P.ts b/src/P2P.ts
--- a/src/P2P.ts
+++ b/src/P2P.ts
@@ -94,3 +94,3 @@
       } catch (parseError) {
-        console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
+        console.warn(`getJson failed: invalid JSON response url: ${url.replace(/\n|\r/g, "")} parseError: ${parseError.replace(/\n|\r/g, "")}`)
         return null
EOF
@@ -94,3 +94,3 @@
} catch (parseError) {
console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
console.warn(`getJson failed: invalid JSON response url: ${url.replace(/\n|\r/g, "")} parseError: ${parseError.replace(/\n|\r/g, "")}`)
return null
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
try {
return StringUtils.safeJsonParse(text)
} catch (parseError) {
console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is a URL, so we need to ensure that it does not contain any characters that can be interpreted when the log output is displayed.

We can use the encodeURI function to encode the URL before logging it. This function encodes special characters, except: , / ? : @ & = + $ #. This should be sufficient to prevent log injection attacks in this case.

The changes need to be made in the file src/P2P.ts, specifically on line 122 where the URL is logged.

Suggested changeset 1
src/P2P.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/P2P.ts b/src/P2P.ts
--- a/src/P2P.ts
+++ b/src/P2P.ts
@@ -121,3 +121,3 @@
       } catch (parseError) {
-        console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
+        console.warn(`getJson failed: invalid JSON response url: ${encodeURI(url)} parseError: ${parseError}`)
         return null
EOF
@@ -121,3 +121,3 @@
} catch (parseError) {
console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
console.warn(`getJson failed: invalid JSON response url: ${encodeURI(url)} parseError: ${parseError}`)
return null
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
} else {
console.warn('getJson failed: got bad response')
console.warn(url)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
Log entry depends on a
user-provided value
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing input that includes special characters interpreted by the log viewer.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, the user-provided data is a URL, so we need to ensure that it does not contain any characters that could be interpreted by the log viewer.

We can use the encodeURI function to encode the URL before logging it. This function encodes special characters, except: , / ? : @ & = + $ #. This should be sufficient to prevent log injection attacks in this case.

The changes need to be made in the file src/P2P.ts, specifically in the function getJson on lines 122 and 127 where the url variable is logged.

Suggested changeset 1
src/P2P.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/P2P.ts b/src/P2P.ts
--- a/src/P2P.ts
+++ b/src/P2P.ts
@@ -121,3 +121,3 @@
       } catch (parseError) {
-        console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
+        console.warn(`getJson failed: invalid JSON response url: ${encodeURI(url)} parseError: ${parseError}`)
         return null
@@ -126,3 +126,3 @@
       console.warn('getJson failed: got bad response')
-      console.warn(url)
+      console.warn(encodeURI(url))
       console.warn(res.headers)
EOF
@@ -121,3 +121,3 @@
} catch (parseError) {
console.warn(`getJson failed: invalid JSON response url: ${url} parseError: ${parseError}`)
console.warn(`getJson failed: invalid JSON response url: ${encodeURI(url)} parseError: ${parseError}`)
return null
@@ -126,3 +126,3 @@
console.warn('getJson failed: got bad response')
console.warn(url)
console.warn(encodeURI(url))
console.warn(res.headers)
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
arhamj
arhamj previously approved these changes Jun 5, 2024
@arhamj arhamj merged commit 30ef2fa into dev Jun 7, 2024
2 checks passed
@mhanson-github mhanson-github deleted the stringify-migrations branch August 30, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants