-
Notifications
You must be signed in to change notification settings - Fork 0
compare to semgrep #64
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
base: dast-test1
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,22 @@ | ||
--- | ||
name: Linters | ||
|
||
name: Security Checks | ||
on: | ||
workflow_dispatch: | ||
pull_request: | ||
push: | ||
|
||
jobs: | ||
"dast-test": | ||
runs-on: ubuntu-latest | ||
container: python:3.11 | ||
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
|
||
- name: Install dependencies | ||
run: cat results.sarif && exit 1 | ||
semgrep: | ||
runs-on: ubuntu-latest | ||
container: | ||
image: returntocorp/semgrep:latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
|
||
- name: Run Semgrep | ||
env: | ||
SEMGREP_RULES: >- | ||
p/security-audit | ||
p/owasp-top-ten | ||
p/javascript | ||
p/python | ||
run: semgrep ci | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1 +1,67 @@ | ||||||||||||||
foobar | ||||||||||||||
from flask import Flask, request, jsonify, Response | ||||||||||||||
import sqlite3 | ||||||||||||||
|
||||||||||||||
app = Flask(__name__) | ||||||||||||||
|
||||||||||||||
# Database file | ||||||||||||||
DATABASE = 'app.db' | ||||||||||||||
|
||||||||||||||
def get_db_connection(): | ||||||||||||||
conn = sqlite3.connect(DATABASE) | ||||||||||||||
return conn | ||||||||||||||
Comment on lines
+9
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling to database connection The database connection lacks error handling which could lead to unhandled exceptions. def get_db_connection():
- conn = sqlite3.connect(DATABASE)
- return conn
+ try:
+ conn = sqlite3.connect(DATABASE)
+ return conn
+ except sqlite3.Error as e:
+ app.logger.error(f"Database connection error: {e}")
+ raise
|
||||||||||||||
|
||||||||||||||
def init_db(): | ||||||||||||||
# Create table and insert data | ||||||||||||||
conn = get_db_connection() | ||||||||||||||
cursor = conn.cursor() | ||||||||||||||
# Check if table already exists to prevent overwriting | ||||||||||||||
cursor.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='users';") | ||||||||||||||
if cursor.fetchone() is None: | ||||||||||||||
cursor.execute('CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT, age INTEGER);') | ||||||||||||||
cursor.execute("INSERT INTO users (name, age) VALUES ('Alice', 30), ('Bob', 25), ('Charlie', 35);") | ||||||||||||||
conn.commit() | ||||||||||||||
conn.close() | ||||||||||||||
|
||||||||||||||
@app.route('/users', methods=['GET']) | ||||||||||||||
def get_user(): | ||||||||||||||
name = request.args.get('name') | ||||||||||||||
conn = get_db_connection() | ||||||||||||||
cursor = conn.cursor() | ||||||||||||||
|
||||||||||||||
# Vulnerable SQL Query from raw string concatenation | ||||||||||||||
query = f"SELECT * FROM users WHERE name = '{name}'" | ||||||||||||||
cursor.execute(query) | ||||||||||||||
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Fix SQL injection vulnerability immediately The current implementation is vulnerable to SQL injection attacks. The commented-out parameterized query must be used instead. - # Vulnerable SQL Query from raw string concatenation
- query = f"SELECT * FROM users WHERE name = '{name}'"
- cursor.execute(query)
+ # Safe SQL Query using parameterized queries
+ query = "SELECT * FROM users WHERE name = ?"
+ cursor.execute(query, (name,)) 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: Security Checks[error] 27-33: SQL Injection vulnerability detected: User-controlled data from request is passed to 'execute()' through raw string concatenation. Use django's QuerySets with parameterized queries instead. [error] 32-32: Tainted SQL string: User input is used to manually construct a SQL query, risking SQL injection. Use parameterized queries or an ORM instead. |
||||||||||||||
|
||||||||||||||
# # Fixed SQL Query using parameterized queries | ||||||||||||||
# query = "SELECT * FROM users WHERE name = ?" | ||||||||||||||
# cursor.execute(query, (name,)) | ||||||||||||||
|
||||||||||||||
user = cursor.fetchone() | ||||||||||||||
conn.close() | ||||||||||||||
if user: | ||||||||||||||
return jsonify({"id": user[0], "name": user[1], "age": user[2]}) | ||||||||||||||
else: | ||||||||||||||
return jsonify({"error": "User not found"}), 404 | ||||||||||||||
|
||||||||||||||
@app.route('/.env', methods=['GET']) | ||||||||||||||
def get_env(): | ||||||||||||||
env_content = """ | ||||||||||||||
DB_NAME=crapi | ||||||||||||||
DB_USER=crapi | ||||||||||||||
DB_PASSWORD=crapi | ||||||||||||||
DB_HOST=postgresdb | ||||||||||||||
DB_PORT=5432 | ||||||||||||||
SERVER_PORT=8080 | ||||||||||||||
MONGO_DB_HOST=mongodb | ||||||||||||||
MONGO_DB_PORT=27017 | ||||||||||||||
MONGO_DB_USER=crapi | ||||||||||||||
MONGO_DB_PASSWORD=crapi | ||||||||||||||
MONGO_DB_NAME=crapi | ||||||||||||||
""" | ||||||||||||||
return Response(env_content, headers={ | ||||||||||||||
"Content-Disposition": "attachment; filename=env" | ||||||||||||||
}) | ||||||||||||||
Comment on lines
+46
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Remove endpoint exposing sensitive configuration The This entire endpoint should be removed immediately. Environment variables should be securely managed through proper configuration management systems, not exposed via API endpoints. |
||||||||||||||
|
||||||||||||||
if __name__ == '__main__': | ||||||||||||||
init_db() # Initialize the database and populate it | ||||||||||||||
app.run(host="0.0.0.0", debug=True) | ||||||||||||||
Comment on lines
+65
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Fix insecure deployment settings Multiple security issues in the application startup:
if __name__ == '__main__':
init_db() # Initialize the database and populate it
- app.run(host="0.0.0.0", debug=True)
+ debug_mode = os.getenv('FLASK_DEBUG', 'False').lower() == 'true'
+ app.run(host="127.0.0.1", debug=debug_mode)
🧰 Tools🪛 GitHub Actions: Security Checks[error] 67-67: Security risk: Flask app running with host='0.0.0.0' could expose the server publicly. [error] 67-67: Debug mode enabled: Flask app running with debug=True, which can leak sensitive information. This should not be used in production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making the database path configurable and secure
Hardcoding the database path could pose security risks. Consider: