Fix advisory lock #8
Conversation
Also upgraded Go version and package dependencies.
Summary of ChangesHello @hi-rai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on maintenance, security, and robustness improvements. It updates the Go language version and several critical package dependencies to ensure the project benefits from the latest features and security patches. A significant change involves fixing a potential SQL injection vulnerability in a database query function. Additionally, it refines error handling logic in two functions and ensures that advisory locks are correctly applied within the scope of a database transaction, enhancing data integrity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with advisory locks, upgrades the Go version, and updates package dependencies. The primary fix in RunInLockedTx correctly ensures that the advisory lock is acquired within the transaction's context, which is a crucial correction for data consistency.
My review has identified a critical security concern in hyperbun.go. While an SQL injection vulnerability was partially addressed in ExistsBySQL by parameterizing the table name, the query string is still concatenated directly, leaving a significant SQL injection risk. The other changes are minor improvements to code clarity.
| if err := m.NewRaw("SELECT EXISTS(SELECT 1 from ? WHERE "+query+")", | ||
| append([]interface{}{bun.Ident(table)}, args...)...). | ||
| Scan(m.Context(), &exists); err != nil { |
There was a problem hiding this comment.
This change correctly parameterizes the table name to prevent SQL injection, which is a great improvement. However, a similar vulnerability still exists with the query parameter, which is directly concatenated into the SQL string. This can lead to SQL injection if the query string is constructed with untrusted input.
For example, a call like this would be vulnerable:
ExistsBySQL(db, "users", "username = '" + userInput + "'")While the function seems to intend for args to be used for parameterization, relying on developers to always do this correctly for the query string is risky. To fully mitigate this, the query string should also be built in a safe way, not through concatenation. A safer approach would be to use Bun's query builder instead of raw SQL strings.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Add golangci-lint config
Fixed advisory lock. Also upgraded Go version and package dependencies.