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

fix: current-cycle-hash resp type issue #35

Merged
merged 6 commits into from
Jun 17, 2024
Merged

fix: current-cycle-hash resp type issue #35

merged 6 commits into from
Jun 17, 2024

Conversation

tanuj-shardeum
Copy link
Contributor

No description provided.

Copy link

linear bot commented Jun 17, 2024

BLUE-117 syncV2 is breaking in archiver due to 'current-cycle-hash'

ISSUE SUMMARY:

The current-cycle-hash endpoint https://github.com/shardeum/shardus-core/blob/1fa5f1c32f4b1044ea4dbb217a4ebb23a0596eb6/src/p2p/SyncV2/routes.ts#L52) seems to be changed recently. Basically, the response of it appears to be now as text , which seemed to be json before.

const res = await get(url)

  if (res.ok) {
    return (await res.json()) as T
  } else {
    throw new Error(`get failed with status ${res.statusText}`)
  }

I had to change the above code in the archiver

const res = await get(url)
in the as following to get it working as a temp solution.

  const res = await get(url)

  if (res.ok) {
    let result
    if (url.includes('current-cycle-hash')) result = await res.text()
    else  result = await res.json()
    return (result) as T
  } else {
    throw new Error(`get failed with status ${res.statusText}`)
  }

<<TODO: Replace this with a short summary of the issue.>>


ISSUE REPRO STEPS:

<HINT: Add steps to list as-needed. If interaction is complex, add screenshots or a Slack screen-capture video (just drag and drop)>

  1. <<TODO: Replace with repro step Bump the npm_and_yarn group across 1 directory with 3 updates #1>>
  2. <<TODO: Replace with repro step Challenge receipt #2>>
  3. Observe <<TODO: Describe unintended behavior.>>

EXPECTED RESULT:

<<TODO: Replace this with your expected results.>>


PULL REQUESTS:

<HINT: If your fix requires changes in multiple repos, add the following info per-repository.>

<<TODO: Enter Repository Name>>

Pull Request Link: <<TODO: Insert PR-LINK>>

GPT Review Link: <<TODO: Insert GPT-Review-Link>>

Jenkins Test Link: <<TODO: Insert Jenkins Test Job Link>>


ADDITIONAL INSTRUCTIONS:

<HINT: Add any additional instructions needed for the assignee. If you have specific requirements for how the task should be implemented or fixed, enter them or link them here.>

<<TODO: Insert additional instructions for assignee.>>

jairajdev
jairajdev previously approved these changes Jun 17, 2024
)
if (savedReceiptsCount > 0)
Logger.mainLogger.debug(
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`

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 creating new log entries by injecting newline characters into the input.

In the file src/API.ts, we need to sanitize the gossipPayload variable before passing it to the Collector.validateGossipData and Collector.processGossipData methods. We can do this by replacing all newline characters in the gossipPayload string with an empty string.

In the file src/Data/Collector.ts, we need to sanitize the getCurrentCycleCounter() function call before logging it. We can do this by converting the result of the function call to a string and then replacing all newline characters with an empty string.

Suggested changeset 2
src/Data/Collector.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/Collector.ts b/src/Data/Collector.ts
--- a/src/Data/Collector.ts
+++ b/src/Data/Collector.ts
@@ -1254,6 +1254,9 @@
   }
-  if (savedReceiptsCount > 0)
+  if (savedReceiptsCount > 0) {
+    // Sanitize getCurrentCycleCounter() by replacing newline characters
+    const sanitizedCycleCounter = String(getCurrentCycleCounter()).replace(/\n|\r/g, "")
     Logger.mainLogger.debug(
-      `Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`
+      `Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${sanitizedCycleCounter}`
     )
+  }
 }
EOF
@@ -1254,6 +1254,9 @@
}
if (savedReceiptsCount > 0)
if (savedReceiptsCount > 0) {
// Sanitize getCurrentCycleCounter() by replacing newline characters
const sanitizedCycleCounter = String(getCurrentCycleCounter()).replace(/\n|\r/g, "")
Logger.mainLogger.debug(
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${sanitizedCycleCounter}`
)
}
}
src/API.ts
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/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -838,3 +838,5 @@
   server.post('/gossip-data', async (_request: GossipDataRequest, reply) => {
-    const gossipPayload = _request.body
+    let gossipPayload = _request.body
+    // Sanitize gossipPayload by replacing newline characters
+    gossipPayload = JSON.parse(JSON.stringify(gossipPayload).replace(/\\n|\\r/g, ""))
     if (config.VERBOSE)
EOF
@@ -838,3 +838,5 @@
server.post('/gossip-data', async (_request: GossipDataRequest, reply) => {
const gossipPayload = _request.body
let gossipPayload = _request.body
// Sanitize gossipPayload by replacing newline characters
gossipPayload = JSON.parse(JSON.stringify(gossipPayload).replace(/\\n|\\r/g, ""))
if (config.VERBOSE)
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
)
if (savedOriginalTxsCount > 0)
Logger.mainLogger.debug(
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`

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, 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 by providing input with newline characters.

In the file src/API.ts, we need to sanitize the gossipPayload variable before logging it. We can do this by replacing all newline characters in the gossipPayload variable with an empty string.

In the file src/Data/Collector.ts, we need to sanitize the getCurrentCycleCounter() function before logging it. We can do this by converting the function's return value to a string and then replacing all newline characters in the string with an empty string.

Suggested changeset 2
src/Data/Collector.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/Collector.ts b/src/Data/Collector.ts
--- a/src/Data/Collector.ts
+++ b/src/Data/Collector.ts
@@ -1272,3 +1272,3 @@
     Logger.mainLogger.debug(
-      `Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`
+      `Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter().toString().replace(/\n|\r/g, "")}`
     )
EOF
@@ -1272,3 +1272,3 @@
Logger.mainLogger.debug(
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter().toString().replace(/\n|\r/g, "")}`
)
src/API.ts
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/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -840,3 +840,3 @@
     if (config.VERBOSE)
-      Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload))
+      Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload).replace(/\n|\r/g, ""))
     const result = Collector.validateGossipData(gossipPayload)
EOF
@@ -840,3 +840,3 @@
if (config.VERBOSE)
Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload))
Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload).replace(/\n|\r/g, ""))
const result = Collector.validateGossipData(gossipPayload)
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
@afostr afostr merged commit 13cfc2e into dev Jun 17, 2024
1 check passed
@mhanson-github mhanson-github deleted the BLUE-117 branch September 5, 2024 02:54
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.

3 participants