fix(pay-slip)[backend]: resolve TOCTOU race condition in CreatePaySlip#45
fix(pay-slip)[backend]: resolve TOCTOU race condition in CreatePaySlip#45MohamadNazik wants to merge 1 commit intoLSFLK:mainfrom
Conversation
Summary of ChangesHello, 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 addresses a critical Time-of-Check-Time-of-Use (TOCTOU) race condition in the CreatePaySlip functionality. By introducing robust transaction management and row-level locking at the database and service layers, it ensures that concurrent requests for the same pay slip record are handled atomically, preventing the creation of duplicate entries and maintaining data integrity. The changes streamline the handler logic while significantly enhancing the reliability of pay slip creation. Highlights
Changelog
Activity
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 effectively resolves the Time-of-Check-Time-of-Use (TOCTOU) race condition in CreatePaySlip by introducing an atomic upsert operation using a database transaction and row-level locking. However, a high-severity Denial of Service (DoS) vulnerability was identified in the database layer's transaction handling due to the use of a global mutex, which can lead to deadlocks and performance bottlenecks, especially during connection failures. Addressing this is crucial for application stability and scalability. Additionally, there are opportunities to enhance performance, maintain API consistency, and improve data integrity.
| func (db *Database) Begin() (*sql.Tx, error) { | ||
| db.mu.Lock() | ||
| defer db.mu.Unlock() | ||
| return db.Conn.Begin() | ||
| } |
There was a problem hiding this comment.
The current implementation of Begin (and other database methods) acquires a global mutex db.mu and holds it for the entire duration of the database operation. This introduces a high-severity Denial of Service (DoS) vulnerability. If a database operation blocks, it can prevent the background pinger from reconnecting, leading to deadlocks and application unavailability. This also severely bottlenecks performance by serializing all database access, despite *sql.DB being thread-safe.
The mutex should only protect access to the db.Conn pointer, not the entire database operation. A sync.RWMutex could also be considered for finer-grained control, allowing concurrent reads while protecting writes during reconnection. The provided code suggestion demonstrates how to release the mutex before calling conn.Begin().
| func (db *Database) Begin() (*sql.Tx, error) { | |
| db.mu.Lock() | |
| defer db.mu.Unlock() | |
| return db.Conn.Begin() | |
| } | |
| func (db *Database) Begin() (*sql.Tx, error) { | |
| db.mu.Lock() | |
| conn := db.Conn | |
| db.mu.Unlock() | |
| return conn.Begin() | |
| } |
| result, err := h.PaySlipService.UpsertPaySlip(ps) | ||
| if err != nil { | ||
| http.Error(w, "Failed to save pay slip", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| jsonResponse(w, http.StatusCreated, ps) | ||
| jsonResponse(w, http.StatusCreated, result) |
There was a problem hiding this comment.
The previous implementation returned http.StatusOK for updates and http.StatusCreated for new records. This change now always returns http.StatusCreated, which is a subtle change in the API's behavior that might affect clients.
To preserve the original behavior, I suggest modifying UpsertPaySlip to return a boolean indicating whether a new record was created. The handler can then use this to set the appropriate status code.
For example, if UpsertPaySlip is changed to return (*models.PaySlip, bool, error):
| result, err := h.PaySlipService.UpsertPaySlip(ps) | |
| if err != nil { | |
| http.Error(w, "Failed to save pay slip", http.StatusInternalServerError) | |
| return | |
| } | |
| jsonResponse(w, http.StatusCreated, ps) | |
| jsonResponse(w, http.StatusCreated, result) | |
| result, created, err := h.PaySlipService.UpsertPaySlip(ps) | |
| if err != nil { | |
| http.Error(w, "Failed to save pay slip", http.StatusInternalServerError) | |
| return | |
| } | |
| statusCode := http.StatusOK | |
| if created { | |
| statusCode = http.StatusCreated | |
| } | |
| jsonResponse(w, statusCode, result) |
| // 1. Lock existing row or potential slot using SELECT ... FOR UPDATE | ||
| // Note: In MySQL, if the record doesn't exist, this might not lock anything unless there's an index. | ||
| // However, we have an index on (user_id, month, year) [via idx_payslips_user_date, though it's partial or we should rely on unique constraint if it existed]. | ||
| // Actually, the primary way to prevent duplicates is row-level locking on existing OR gap locking if applicable. | ||
| // A more robust way in MySQL without a UNIQUE constraint is to lock the user record or a mutex table, | ||
| // but SELECT ... FOR UPDATE on the specific criteria is the standard approach for "upsert-gate". |
There was a problem hiding this comment.
These comments correctly identify that SELECT ... FOR UPDATE is a standard approach to prevent race conditions. However, the most robust and performant solution to prevent duplicates is to enforce uniqueness at the database level.
I recommend adding a UNIQUE constraint on (user_id, month, year) in the pay_slips table via a new migration:
ALTER TABLE pay_slips ADD UNIQUE INDEX uq_payslips_user_date (user_id, month, year);With this constraint in place, you could simplify this function significantly by using MySQL's INSERT ... ON DUPLICATE KEY UPDATE syntax. This is atomic and more efficient than a transaction with SELECT ... FOR UPDATE, and it would eliminate the need for manual transaction management for this operation.
1 participant
Summary
This PR resolves a Time-of-Check-Time-of-Use (TOCTOU) race condition in the
CreatePaySliphandler. The previous implementation performed an existence check followed by an insert or update in separate, non-atomic service calls, which allowed concurrent requests for the same user/month/year to result in duplicate records.The Fix
UpsertPaySlipmethod that:SELECT ... FOR UPDATE) to block concurrent requests for the same record until the transaction completes.Verification