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

BLUE-273 Worker processed debug log flag #84

Merged
merged 1 commit into from
Sep 18, 2024
Merged

BLUE-273 Worker processed debug log flag #84

merged 1 commit into from
Sep 18, 2024

Conversation

jairajdev
Copy link
Contributor

No description provided.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 18, 2024 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit b79d692)

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

Debug Log Condition
The condition to check config.workerProcessesDebugLog before logging debug information is repeated multiple times. Consider refactoring this to a helper function to improve code maintainability and reduce redundancy.

Config Structure
The addition of workerProcessesDebugLog directly in the Config interface and its default setting in config object might need further validation to ensure it aligns with all configurations and does not introduce any side effects.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 18, 2024
View reviewed changes
src/primary-process/index.ts
@@ -47,7 +47,8 @@
worker.kill()
}
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
if (config.workerProcessesDebugLog)
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI about 1 month ago

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

Suggested changeset 1
src/primary-process/index.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/primary-process/index.ts b/src/primary-process/index.ts
--- a/src/primary-process/index.ts
+++ b/src/primary-process/index.ts
@@ -49,4 +49,6 @@
     if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
-      if (config.workerProcessesDebugLog)
-        console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
+      if (config.workerProcessesDebugLog) {
+        const sanitizedLimit = config.receiptLoadTrakerLimit.toString().replace(/\n|\r/g, "");
+        console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`);
+      }
       // Kill the extra workers from the end of the array
EOF
@@ -49,4 +49,6 @@
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
if (config.workerProcessesDebugLog)
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
if (config.workerProcessesDebugLog) {
const sanitizedLimit = config.receiptLoadTrakerLimit.toString().replace(/\n|\r/g, "");
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`);
}
// Kill the extra workers from the end of the array
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
achal-singh
achal-singh previously approved these changes Sep 18, 2024
View reviewed changes
@jairajdev jairajdev dismissed achal-singh’s stale review September 18, 2024 19:40

The merge-base changed after approval.

devendra-shardeum
devendra-shardeum requested changes Sep 18, 2024
View reviewed changes
Copy link

@devendra-shardeum devendra-shardeum left a comment

Choose a reason for hiding this comment

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

I think src/worker-process/index.ts should also have the flags around the ( console.log / console.error .. etc. ) .. right now i can see that only being done for src/primary-process/index.ts

@mhanson-github
Copy link
Contributor

I think src/worker-process/index.ts should also have the flags around the ( console.log / console.error .. etc. ) .. right now i can see that only being done for src/primary-process/index.ts

As the other file was out of scope for what Jai thought we needed, I will in principle agree with you, but I think we should create a separate ticket for the additional flag wrapping. Will have a task created

@github-actions GitHub Actions
Copy link

Persistent review updated to latest commit b79d692

devendra-shardeum
devendra-shardeum approved these changes Sep 18, 2024
View reviewed changes
Copy link

@devendra-shardeum devendra-shardeum left a comment

Choose a reason for hiding this comment

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

marking it as approved since the requested changes moved to a new ticket.. https://linear.app/shm/issue/BLUE-275/additional-flag-wrapping-in-archiver-logs

@mhanson-github mhanson-github merged commit 9f303da into dev Sep 18, 2024
10 of 12 checks passed
@mhanson-github mhanson-github deleted the workerProcessedDebugLog-flag branch September 18, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devendra-shardeum devendra-shardeum devendra-shardeum approved these changes

@achal-singh achal-singh achal-singh left review comments

Assignees

@jairajdev jairajdev

Labels
Review effort [1-5]: 3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jairajdev @mhanson-github @achal-singh @devendra-shardeum