-
Notifications
You must be signed in to change notification settings - Fork 0
fix(migration): Single quotes not escaped #11
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(migration): Single quotes not escaped #11
Conversation
Changes migration 9 to escape the `externalName` for single quotes
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.
Pull request overview
This PR fixes a SQL injection vulnerability in migration 9 by escaping single quotes in the externalName variable before concatenating it into an SQL UPDATE statement. The migration updates the longlived_name column in the search_config table using data from the Generic database's ImqsMetaTable.
Key Changes:
- Added single quote escaping for
externalNamein migration 9 to prevent SQL syntax errors - Updated Dockerfile comment to reference
latestinstead ofmastertag
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/db.go | Adds single quote escaping for externalName before SQL concatenation in migration 9 |
| Dockerfile | Updates Docker build comment to use latest tag instead of master |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/db.go
Outdated
| externalName = strings.Replace(externalName, `'`, `''`, -1) | ||
|
|
||
| migrationSQL += `UPDATE search_config SET longlived_name = '` + externalName + `' WHERE tablename = '` + internalName + `'; ` |
Copilot
AI
Nov 25, 2025
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.
The codebase already has a utility function escapeSQLLiteral (defined at lines 65-67) that performs the same single quote escaping. Consider using escapeSQLLiteral(nil, externalName) instead of manually calling strings.Replace. This would make the code more consistent and maintainable.
Note: The function returns the value wrapped in quotes, so you would need to adjust line 230 to use it without adding extra quotes.
| externalName = strings.Replace(externalName, `'`, `''`, -1) | |
| migrationSQL += `UPDATE search_config SET longlived_name = '` + externalName + `' WHERE tablename = '` + internalName + `'; ` | |
| escapedExternalName := escapeSQLLiteral(nil, externalName) | |
| migrationSQL += `UPDATE search_config SET longlived_name = ` + escapedExternalName + ` WHERE tablename = '` + internalName + `'; ` |
server/db.go
Outdated
| } | ||
|
|
||
| // Escape single quotes in names | ||
| externalName = strings.Replace(externalName, `'`, `''`, -1) |
Copilot
AI
Nov 25, 2025
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.
The internalName variable is also concatenated into the SQL query without escaping. If internalName contains single quotes, this could lead to SQL injection or syntax errors. You should escape internalName as well, similar to what was done for externalName on line 228.
| externalName = strings.Replace(externalName, `'`, `''`, -1) | |
| externalName = strings.Replace(externalName, `'`, `''`, -1) | |
| internalName = strings.Replace(internalName, `'`, `''`, -1) |
| @@ -1,4 +1,4 @@ | |||
| # docker build -t imqs/search:master --build-arg SSH_KEY="`cat ~/.ssh/id_rsa`" . | |||
| # docker build -t imqs/search:latest --build-arg SSH_KEY="`cat ~/.ssh/id_rsa`" . | |||
Copilot
AI
Nov 25, 2025
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.
This change from master to latest appears unrelated to the PR's stated purpose of fixing single quote escaping in migration 9. If this change is intentional, it should be documented in the PR description or made in a separate PR to maintain clear separation of concerns.
| # docker build -t imqs/search:latest --build-arg SSH_KEY="`cat ~/.ssh/id_rsa`" . | |
| # docker build -t imqs/search:master --build-arg SSH_KEY="`cat ~/.ssh/id_rsa`" . |
anzelIMQS
left a comment
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.
LGTM, +1 with Copi's comments
FledermausDevin
left a comment
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.
LGTM
Changes migration 9 to escape the `externalName` for single quotes Refs: ASG-5083
Changes migration 9 to escape the `externalName` for single quotes Refs: ASG-5083
Changes migration 9 to escape the
externalNamefor single quotesRefs: ASG-5083