Shard 1426 by devendra-shardeum · Pull Request #128 · shardeum/archiver

devendra-shardeum · 2025-01-15T16:00:26Z

Added prepared statements everywhere in archiver
now it's faster and secured against sql injections

…pared statment for better code readability, higher performance sql injection proof queries to db

src/dbstore/accounts.ts

+  // Log the result count if verbose logging is enabled
  if (config.VERBOSE) {
-    Logger.mainLogger.debug('Account accounts', accounts ? accounts.length : accounts, 'skip', skip)
+    Logger.mainLogger.debug('Account accounts', accounts.length, 'skip', skip, 'limit', limit);


To fix the problem, we need to sanitize the skip parameter before logging it. This can be done by removing any newline characters from the skip parameter using String.prototype.replace. This ensures that the log entry cannot be manipulated by injecting special characters.

In the file src/dbstore/accounts.ts, sanitize the skip parameter before logging it.

Add a utility function to sanitize user input by removing newline characters.

Use this utility function to sanitize the skip parameter before logging it.

src/dbstore/cycles.ts

+
    if (config.VERBOSE) {
-      Logger.mainLogger.debug('Updated cycle for counter', cycle.cycleRecord.counter, cycle.cycleMarker)
+      Logger.mainLogger.debug('Updated cycle for counter', cycle.counter, marker);


To fix the log injection issue, we need to sanitize the cycle.counter value before logging it. Specifically, we should remove any newline characters from the cycle.counter value to prevent log injection attacks. This can be done using the String.prototype.replace method.

src/dbstore/cycles.ts

+
    if (config.VERBOSE) {
-      Logger.mainLogger.debug('Updated cycle for counter', cycle.cycleRecord.counter, cycle.cycleMarker)
+      Logger.mainLogger.debug('Updated cycle for counter', cycle.counter, marker);


To fix the log injection issue, we need to sanitize the marker variable before logging it. This can be done by removing any newline characters from the marker string using String.prototype.replace. This ensures that user input cannot inject new log entries.

src/dbstore/cycles.ts

-    Logger.mainLogger.error(e)
-    Logger.mainLogger.error('Unable to update Cycle', cycle.cycleMarker)
+    Logger.mainLogger.error(e);
+    Logger.mainLogger.error('Unable to update Cycle', marker);


To fix the log injection issue, we need to sanitize the marker variable before logging it. This can be done by removing any newline characters from the marker string. We will use the String.prototype.replace method to achieve this. The changes will be made in the updateCycle function in the src/dbstore/cycles.ts file.

src/dbstore/receipts.ts

    }
+
+    if (config.VERBOSE) {
+      Logger.mainLogger.debug('Receipt receipts', receipts.length, 'skip', skip);


To fix the problem, we need to sanitize the skip parameter before logging it. This can be done by converting the skip parameter to a string and removing any newline characters. This ensures that the log entry cannot be manipulated by injecting special characters.

In the queryReceipts function, sanitize the skip parameter before logging it.

Use String.prototype.replace to remove any newline characters from the skip parameter.

Ensure that the sanitized skip parameter is used in the log statement.

src/dbstore/transactions.ts

+        'Transaction transactions',
+        transactions ? transactions.length : transactions,
+        'skip',
+        skip


To fix the log injection issue, we need to sanitize the skip parameter before logging it. Specifically, we should remove any newline characters from the skip parameter to prevent log injection. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

urnotsam · 2025-02-05T16:50:59Z

src/Data/AccountDataProvider.ts

-      lastUpdateNeeded = true
    }
+  } catch (e) {
+    Logger.mainLogger.error(e);


Add more info to this error message than just the error itself

urnotsam · 2025-02-05T16:51:43Z

src/Data/AccountDataProvider.ts

+
+
+
+// /**


remove commented out code if no longer needed

urnotsam · 2025-02-05T16:54:50Z

src/dbstore/accounts.ts

+      data: DeSerializeFromJsonString(dbAccount.data),
+    }));
+  } catch (e) {
+    Logger.mainLogger.error(e);


Add a more detailed error message

urnotsam · 2025-02-05T16:55:03Z

src/dbstore/accounts.ts

+      data: DeSerializeFromJsonString(dbAccount.data),
+    }));
+  } catch (e) {
+    Logger.mainLogger.error(e);


add a more detailed error message

urnotsam · 2025-02-05T16:56:22Z

src/dbstore/cycles.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return []
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:56:44Z

src/dbstore/cycles.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return []
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:56:55Z

src/dbstore/cycles.ts

+    return count;
  } catch (e) {
-    Logger.mainLogger.error(e)
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:58:38Z

src/dbstore/originalTxsData.ts

+    return result ? result['COUNT(*)'] || 0 : 0;
  } catch (e) {
-    console.log(e)
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:58:59Z

src/dbstore/originalTxsData.ts

+    });
  } catch (e) {
-    console.log(e)
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:59:11Z

src/dbstore/originalTxsData.ts

+    return originalTxData as OriginalTxData | null;
  } catch (e) {
-    console.log(e)
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:59:22Z

src/dbstore/originalTxsData.ts

+      }
    }
+  } catch (e) {
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T16:59:35Z

src/dbstore/originalTxsData.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return null
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T17:04:26Z

src/dbstore/processedTxs.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return null
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T17:04:37Z

src/dbstore/processedTxs.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return null
+    Logger.mainLogger.error(e);


Add more detailed error message

urnotsam · 2025-02-05T17:05:37Z

src/dbstore/receipts.ts

  } catch (e) {
-    Logger.mainLogger.error(e)
-    return null
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam · 2025-02-05T17:06:11Z

src/dbstore/receipts.ts

    }
  } catch (e) {
-    console.log(e)
+    Logger.mainLogger.error(e);


add more detailed error message

urnotsam

looks good. mostly missing detailed error messages in catches but since these existed previously I wont hold up this PR for it

devendra-shardeum added 6 commits January 15, 2025 05:52

added prepared statements manager for standardising how we handle pre…

12deda1

…pared statment for better code readability, higher performance sql injection proof queries to db

made the prepared statement manager even better

24500e5

added prepared statements for cycles database

09efac8

added prepared statements for receipts database

5fe4424

added prepared statements for originalTxsData database

51ac271

added prepared statements in all the files

907f97e

devendra-shardeum requested review from S0naliThakur, aniketdivekar, mgthuramoemyint and mssabr01 January 15, 2025 16:00

github-advanced-security bot found potential problems Jan 15, 2025

View reviewed changes

urnotsam reviewed Feb 5, 2025

View reviewed changes

src/Data/AccountDataProvider.ts

// /**

Copy link

Contributor

urnotsam Feb 5, 2025

Choose a reason for hiding this comment

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

remove commented out code if no longer needed

urnotsam reviewed Feb 5, 2025

View reviewed changes

src/dbstore/originalTxsData.ts

});

} catch (e) {

console.log(e)

Logger.mainLogger.error(e);

Copy link

Contributor

urnotsam Feb 5, 2025

Choose a reason for hiding this comment

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

add more detailed error message

urnotsam reviewed Feb 5, 2025

View reviewed changes

src/dbstore/originalTxsData.ts

}

}

} catch (e) {

Logger.mainLogger.error(e);

Copy link

Contributor

urnotsam Feb 5, 2025

Choose a reason for hiding this comment

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

add more detailed error message

urnotsam reviewed Feb 5, 2025

View reviewed changes

src/dbstore/receipts.ts

}

} catch (e) {

console.log(e)

Logger.mainLogger.error(e);

Copy link

Contributor

urnotsam Feb 5, 2025

Choose a reason for hiding this comment

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

add more detailed error message

urnotsam approved these changes Feb 5, 2025

View reviewed changes

mhanson-github force-pushed the dev branch from e875823 to 330bf6f Compare April 7, 2025 04:26

mhanson-github force-pushed the dev branch from dcf1122 to 053fd38 Compare April 26, 2025 04:34

mhanson-github force-pushed the dev branch from 8f3b44a to edc0f4d Compare May 13, 2025 05:52

@@ -233,3 +233,4 @@
               if (config.VERBOSE) {
-                Logger.mainLogger.debug('Account accounts', accounts.length, 'skip', skip, 'limit', limit);
+                const sanitizedSkip = String(skip).replace(/\n|\r/g, "");
+                Logger.mainLogger.debug('Account accounts', accounts.length, 'skip', sanitizedSkip, 'limit', limit);
               }

@@ -90,3 +90,4 @@
                 if (config.VERBOSE) {
-                  Logger.mainLogger.debug('Updated cycle for counter', cycle.counter, marker);
+                  const sanitizedCounter = String(cycle.counter).replace(/\n|\r/g, "");
+                  Logger.mainLogger.debug('Updated cycle for counter', sanitizedCounter, marker);
                 }

@@ -90,3 +90,4 @@
                 if (config.VERBOSE) {
-                  Logger.mainLogger.debug('Updated cycle for counter', cycle.counter, marker);
+                  const sanitizedMarker = marker.replace(/\n|\r/g, "");
+                  Logger.mainLogger.debug('Updated cycle for counter', cycle.counter, sanitizedMarker);
                 }

@@ -94,3 +94,4 @@
                 Logger.mainLogger.error(e);
-                Logger.mainLogger.error('Unable to update Cycle', marker);
+                const sanitizedMarker = marker.replace(/\n|\r/g, "");
+                Logger.mainLogger.error('Unable to update Cycle', sanitizedMarker);
               }

@@ -267,3 +267,4 @@
                 if (config.VERBOSE) {
-                  Logger.mainLogger.debug('Receipt receipts', receipts.length, 'skip', skip);
+                  const sanitizedSkip = String(skip).replace(/\n|\r/g, "");
+                  Logger.mainLogger.debug('Receipt receipts', receipts.length, 'skip', sanitizedSkip);
                 }

Conversation

devendra-shardeum commented Jan 15, 2025

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urnotsam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants