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

BLUE-280: Add Publisher for Transaction Digest to IPFS via Web3.Storage #86

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

achal-singh
Copy link
Contributor

@achal-singh achal-singh commented Sep 24, 2024

Summary

  • The Transaction DIgest Cron Server in the Archiver creates Transaction Digests every 10 cycles.
  • This PR adds an ipfsPublisher (as a part of the Transaction DIgest Cron Server) to upload/publish the same Transaction Digest to the IPFS Network via the Web3.Storage provider.

Linear Task

Merging Notes

It'd be good to merge BLUE-283's PR into BLUE-280 PR first before merging BLUE-280 to dev.

Testing Notes

  1. Before starting with the tests, make sure you register on the web3.storage portal with your email address and subscribe to their free-tier plan (that gives 5 GB of free storage).
  2. Access your account on https://console.web3.storage and create a Space, set any name as per your preference. At the top of the screen you'll see a Root DID string (starts with did:key:...) of your Space.
  3. Copy and paste the same email ID and Root DID of your space to the root_did and admin_email fields in the archiver-config.json file respectively and set the enableSavingToWeb3Storage flag to true.
  4. Run npm run txDigestCronServer OR pm2 start --name txDigestCron npm -- run txDigestCronServer to launch the txDigestCronServer process.

@achal-singh achal-singh self-assigned this Sep 24, 2024
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Error Handling
The error handling in uploadDigestToIPFS function (line 55-60) might not provide sufficient information for debugging. Consider enhancing the error messages to include more specific details about the failure context.

Logging Practice
The use of console.log and console.error for logging throughout the ipfsPublisher.ts file is inconsistent with the use of mainLogger in other parts of the application. It is recommended to use mainLogger for uniformity and better control over logging levels and outputs.

Possible Bug
The condition in uploadDigestToIPFS function (line 29) seems to be incorrect. The function returns early if isUploadPossible() is false, which is the opposite of the expected behavior where it should proceed if the upload is possible.

src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
package.json Outdated Show resolved Hide resolved
src/Config.ts Outdated Show resolved Hide resolved
src/Config.ts Outdated Show resolved Hide resolved
try {
const plan = await account.plan.get()
if (!plan.ok) {
console.error(`❌ ${admin_email} does not have an active plan subscribed on Web3.Storage.`)

Choose a reason for hiding this comment

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

we should not log email id. let's mask it using industry standard logic or remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can mask it for sure, although it can't be exploited, I consoled it initially just for testing purpose.

src/txDigester/txDigestFunctions.ts Show resolved Hide resolved
isPublisherActive = false
} catch (error) {
isPublisherActive = false
console.error(`❌ Error while Uploading Digest (w/ Hash: ${data.hash}) to IPFS: `)

Choose a reason for hiding this comment

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

lets log cycle range also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included the hash only to keep the log line short and the hash makes it easier to search in a DB (if someone wants to quickly look it up since its just 1 value).
I'll add in start and end cycles too.

src/txDigester/ipfsPublisher.ts Show resolved Hide resolved
isPublisherActive = true
console.log(`Uploading TX Digest for Cycle Range ${data.cycleStart} to ${data.cycleEnd}`)
await client.setCurrentSpace(rootDID as Web3StorageRootDID)
console.log(`Uploading Data to Root-DID: ${rootDID}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 3 months ago

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

Suggested changeset 1
src/txDigester/ipfsPublisher.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/txDigester/ipfsPublisher.ts b/src/txDigester/ipfsPublisher.ts
--- a/src/txDigester/ipfsPublisher.ts
+++ b/src/txDigester/ipfsPublisher.ts
@@ -39,3 +39,4 @@
     await client.setCurrentSpace(rootDID as Web3StorageRootDID)
-    console.log(`Uploading Data to Root-DID: ${rootDID}`)
+    const sanitizedRootDID = rootDID.replace(/\n|\r/g, "")
+    console.log(`Uploading Data to Root-DID: ${sanitizedRootDID}`)
 
EOF
@@ -39,3 +39,4 @@
await client.setCurrentSpace(rootDID as Web3StorageRootDID)
console.log(`Uploading Data to Root-DID: ${rootDID}`)
const sanitizedRootDID = rootDID.replace(/\n|\r/g, "")
console.log(`Uploading Data to Root-DID: ${sanitizedRootDID}`)

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/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
try {
isPublisherActive = true
console.log('Initialising IPFS publisher...')
if (!adminEmail || !rootDID) {

Check failure

Code scanning / CodeQL

User-controlled bypass of security check High

This condition guards a sensitive
action
, but a
user-provided value
controls it.
This condition guards a sensitive
action
, but a
user-provided value
controls it.
try {
isPublisherActive = true
console.log('Initialising IPFS publisher...')
if (!adminEmail || !rootDID) {

Check failure

Code scanning / CodeQL

User-controlled bypass of security check High

This condition guards a sensitive
action
, but a
user-provided value
controls it.
This condition guards a sensitive
action
, but a
user-provided value
controls it.
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
src/txDigester/ipfsPublisher.ts Fixed Show fixed Hide fixed
@achal-singh achal-singh force-pushed the BLUE-280 branch 3 times, most recently from f6fc2c2 to f56041b Compare September 25, 2024 11:57
}
client = await W3SClient.create()

console.log(`Logging into Web3.Storage with Account: ${maskedEmail(adminEmail)}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 3 months ago

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

Suggested changeset 1
src/txDigester/ipfsPublisher.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/txDigester/ipfsPublisher.ts b/src/txDigester/ipfsPublisher.ts
--- a/src/txDigester/ipfsPublisher.ts
+++ b/src/txDigester/ipfsPublisher.ts
@@ -78,2 +78,6 @@
 
+const sanitizeEmail = (email: string): string => {
+  return email.replace(/\n|\r/g, "");
+}
+
 const maskedEmail = (email: string): string => {
@@ -94,3 +98,3 @@
 
-    console.log(`Logging into Web3.Storage with Account: ${maskedEmail(adminEmail)}`)
+    console.log(`Logging into Web3.Storage with Account: ${maskedEmail(sanitizeEmail(adminEmail))}`)
     if (!isAuthWithEmail(adminEmail as Web3StorageAdminEmail)) {
EOF
@@ -78,2 +78,6 @@

const sanitizeEmail = (email: string): string => {
return email.replace(/\n|\r/g, "");
}

const maskedEmail = (email: string): string => {
@@ -94,3 +98,3 @@

console.log(`Logging into Web3.Storage with Account: ${maskedEmail(adminEmail)}`)
console.log(`Logging into Web3.Storage with Account: ${maskedEmail(sanitizeEmail(adminEmail))}`)
if (!isAuthWithEmail(adminEmail as Web3StorageAdminEmail)) {
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(`Logging into Web3.Storage with Account: ${maskedEmail(adminEmail)}`)
if (!isAuthWithEmail(adminEmail as Web3StorageAdminEmail)) {
console.log(`⏳ Owner of ${adminEmail} needs to approve the Web3.Storage Auth request to proceed.`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
}

await client.setCurrentSpace(rootDID as Web3StorageRootDID)
console.log(`Current Space set to DID: ${rootDID}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 3 months ago

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

Suggested changeset 1
src/txDigester/ipfsPublisher.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/txDigester/ipfsPublisher.ts b/src/txDigester/ipfsPublisher.ts
--- a/src/txDigester/ipfsPublisher.ts
+++ b/src/txDigester/ipfsPublisher.ts
@@ -102,3 +102,4 @@
     await client.setCurrentSpace(rootDID as Web3StorageRootDID)
-    console.log(`Current Space set to DID: ${rootDID}`)
+    const sanitizedRootDID = rootDID.replace(/\n|\r/g, "")
+    console.log(`Current Space set to DID: ${sanitizedRootDID}`)
     isPublisherActive = false
EOF
@@ -102,3 +102,4 @@
await client.setCurrentSpace(rootDID as Web3StorageRootDID)
console.log(`Current Space set to DID: ${rootDID}`)
const sanitizedRootDID = rootDID.replace(/\n|\r/g, "")
console.log(`Current Space set to DID: ${sanitizedRootDID}`)
isPublisherActive = false
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants