implementing vulnerability remediation#87
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements security remediations to address two critical vulnerabilities: replacing the unsafe eval() function with json.loads() to prevent code injection, and converting string interpolation SQL queries to parameterized queries to prevent SQL injection attacks.
Key Changes:
- Replaced
eval()withjson.loads()for safe string-to-dict conversion - Implemented parameterized SQL queries with placeholder parameters (
?) - Refactored the conditional SQL query logic to use a single query with parameters
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| last_name = inputs_dict['LastName'] | ||
| if inputs_dict['MiddleName'] == "": | ||
| sqlQuery = f"""select * from [SalesLT].[Customer] WHERE FirstName='{inputs_dict['FirstName']}' and MiddleName is NULL and LastName='{inputs_dict['LastName']}'""" | ||
| middle_name = "NULL" |
There was a problem hiding this comment.
The NULL handling logic is incorrect. Setting middle_name = "NULL" (a string) won't work with the parameterized query. When MiddleName is empty, you should use None (Python's null value) instead of the string "NULL" to properly match SQL NULL values.
The original query used MiddleName is NULL SQL syntax for the NULL case, but the parameterized version uses MiddleName=?. When you pass None as the parameter value, it will correctly translate to SQL NULL and match NULL values in the database.
Recommended fix:
if inputs_dict['MiddleName'] == "":
middle_name = None
else:
middle_name = inputs_dict['MiddleName']| middle_name = "NULL" | |
| middle_name = None |
| else: | ||
| sqlQuery = f"""select * from [SalesLT].[Customer] WHERE FirstName='{inputs_dict['FirstName']}' and MiddleName='{inputs_dict['MiddleName']}' and LastName='{inputs_dict['LastName']}'""" | ||
| middle_name = inputs_dict['MiddleName'] | ||
| sqlQuery = f"""select * from [SalesLT].[Customer] WHERE FirstName=? and MiddleName=? and LastName=?""" |
There was a problem hiding this comment.
The SQL query logic for NULL handling has been changed incorrectly. The original code had two separate queries: one using MiddleName is NULL for empty middle names, and one using MiddleName='{value}' for non-empty values. The new parameterized query uses MiddleName=? for both cases, but SQL comparisons with NULL don't work the same way as regular equality checks.
To fix this while maintaining parameterized queries and security, you have two options:
Option 1: Use conditional SQL with IS NULL
if inputs_dict['MiddleName'] == "":
sqlQuery = """select * from [SalesLT].[Customer] WHERE FirstName=? and MiddleName IS NULL and LastName=?"""
cursor.execute(sqlQuery, (first_name, last_name))
else:
middle_name = inputs_dict['MiddleName']
sqlQuery = """select * from [SalesLT].[Customer] WHERE FirstName=? and MiddleName=? and LastName=?"""
cursor.execute(sqlQuery, (first_name, middle_name, last_name))Option 2: Use COALESCE or ISNULL in SQL
middle_name = inputs_dict['MiddleName'] if inputs_dict['MiddleName'] != "" else None
sqlQuery = """select * from [SalesLT].[Customer] WHERE FirstName=? and (MiddleName=? OR (MiddleName IS NULL AND ? IS NULL)) and LastName=?"""
cursor.execute(sqlQuery, (first_name, middle_name, middle_name, last_name))
Implemented remediations
As described in the ICM ticket and issue #86.
REMEDIATION
For eval() vulnerability, replace with:
For SQL injection, use parameterized queries: