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

SHARD-1098 - Tickets #106

Merged
merged 14 commits into from
Dec 2, 2024
Merged

SHARD-1098 - Tickets #106

merged 14 commits into from
Dec 2, 2024

Conversation

BelfordZ
Copy link
Contributor

No description provided.

src/server.ts Fixed Show fixed Hide fixed
if (err) {
server.log.error(err)
process.exit(1)
}
Logger.mainLogger.debug('Archive-server has started.')
console.log(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${config.ARCHIVER_PORT}`)
Logger.mainLogger.info(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${config.ARCHIVER_PORT}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 22 days ago

To fix the log injection issue, we need to sanitize the user-provided input before logging it. Specifically, we should ensure that the config.ARCHIVER_PORT value does not contain any special characters or newlines that could be used for log injection. We can achieve this by using a regular expression to remove any unwanted characters from the input.

The best way to fix the problem without changing existing functionality is to sanitize the config.ARCHIVER_PORT value before using it in the log message. We will use the String.prototype.replace method to remove any newline characters from the input.

Suggested changeset 1
src/server.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/server.ts b/src/server.ts
--- a/src/server.ts
+++ b/src/server.ts
@@ -482,3 +482,4 @@
       }
-      Logger.mainLogger.info(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${config.ARCHIVER_PORT}`)
+      const sanitizedPort = config.ARCHIVER_PORT.replace(/\n|\r/g, "");
+      Logger.mainLogger.info(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${sanitizedPort}`)
       State.setActive()
EOF
@@ -482,3 +482,4 @@
}
Logger.mainLogger.info(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${config.ARCHIVER_PORT}`)
const sanitizedPort = config.ARCHIVER_PORT.replace(/\n|\r/g, "");
Logger.mainLogger.info(`Worker ${process.pid}: Archive-server is listening on http://0.0.0.0:${sanitizedPort}`)
State.setActive()
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
// The signature is valid
if (
!seen.has(sigs[i].owner) &&
allowedPubkeys[sigs[i].owner] &&

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.
shawnifill
shawnifill previously approved these changes Nov 29, 2024
src/server.ts Outdated Show resolved Hide resolved
shawnifill
shawnifill previously approved these changes Dec 2, 2024
src/Config.ts Outdated Show resolved Hide resolved
@BelfordZ BelfordZ merged commit 16c562b into dev Dec 2, 2024
5 of 7 checks passed
@BelfordZ BelfordZ deleted the tickets branch December 2, 2024 21:44
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.

2 participants