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 code scanning alert no. 8: Database query built from user-controlled sources #2375

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fiftin
Copy link
Collaborator

@fiftin fiftin commented Sep 28, 2024

Fixes https://github.com/semaphoreui/semaphore/security/code-scanning/8

To fix the problem, we need to ensure that user-controlled values are safely embedded into the SQL query. This can be achieved by using parameterized queries or prepared statements. Specifically, we should avoid directly concatenating the orderColumn and orderDirection into the query string. Instead, we can use the squirrel library's methods to safely construct the query.

  1. Modify the getObjectsByReferrer function to use parameterized queries for the ORDER BY clause.
  2. Ensure that the orderColumn and orderDirection values are safely embedded into the query using the squirrel.Expr method.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…led sources

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@@ -498,7 +498,7 @@
}

if orderColumn != "" {
q = q.OrderBy("pe." + orderColumn + " " + orderDirection)
q = q.OrderBy(squirrel.Expr("pe." + orderColumn + " " + orderDirection))

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query depends on a
user-provided value
.

Copilot Autofix AI about 2 months ago

To fix the problem, we should avoid using string concatenation to build SQL queries with user-provided values. Instead, we should use parameterized queries or the squirrel library's built-in methods to safely include these values in the query.

  • Replace the string concatenation in the OrderBy clause with a safer method.
  • Use squirrel.Expr with placeholders and arguments to safely include the orderColumn and orderDirection values.
Suggested changeset 1
db/sql/SqlDb.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/db/sql/SqlDb.go b/db/sql/SqlDb.go
--- a/db/sql/SqlDb.go
+++ b/db/sql/SqlDb.go
@@ -500,3 +500,3 @@
 	if orderColumn != "" {
-		q = q.OrderBy(squirrel.Expr("pe." + orderColumn + " " + orderDirection))
+		q = q.OrderBy(squirrel.Expr("pe.? ?", squirrel.Expr(orderColumn), orderDirection))
 	}
EOF
@@ -500,3 +500,3 @@
if orderColumn != "" {
q = q.OrderBy(squirrel.Expr("pe." + orderColumn + " " + orderDirection))
q = q.OrderBy(squirrel.Expr("pe.? ?", squirrel.Expr(orderColumn), orderDirection))
}
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant