Conversation
bus coordinator system for rajpravas - coordinator login and bulk upl…
There was a problem hiding this comment.
Code Review
This pull request introduces a coordinator dashboard, coordinator login, and travel bus management system, including bus creation, passenger assignment, and bulk upload features. The review feedback highlights several critical issues: a runtime TypeError in travelBusDetails.js due to a missing DOM element, a UI-database sync issue in fetchUpcomingBookings.js when reassignment is cancelled, a potential date comparison failure in fetchUpcomingBookings.js, and a Cross-Site Scripting (XSS) vulnerability in coordinatorDashboard.js caused by rendering unescaped user input via innerHTML.
| document | ||
| .getElementById( | ||
| 'bulkUploadPassengers' | ||
| ) | ||
| .addEventListener( | ||
| 'click', | ||
| () => { | ||
|
|
||
| document | ||
| .getElementById( | ||
| 'bulkUploadInput' | ||
| ) | ||
| .click(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
The element with ID bulkUploadPassengers does not exist in travelBusDetails.html. Attempting to call .addEventListener on null will throw a TypeError at runtime, which halts the execution of the rest of the DOMContentLoaded script. This prevents subsequent event listeners (like the one for bulkUploadInput) from being registered.
You should use optional chaining (?.) to prevent the script from crashing if the element is missing.
| document | |
| .getElementById( | |
| 'bulkUploadPassengers' | |
| ) | |
| .addEventListener( | |
| 'click', | |
| () => { | |
| document | |
| .getElementById( | |
| 'bulkUploadInput' | |
| ) | |
| .click(); | |
| } | |
| ); | |
| document | |
| .getElementById( | |
| 'bulkUploadPassengers' | |
| ) | |
| ?.addEventListener( | |
| 'click', | |
| () => { | |
| document | |
| .getElementById( | |
| 'bulkUploadInput' | |
| ) | |
| .click(); | |
| } | |
| ); |
| if (!increaseCapacity) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the user declines to increase the capacity of the matching bus, the function returns early. However, the original transaction update (which removed the passenger from their old bus) has already succeeded on the server. Returning early here leaves the modal open and prevents the table from refreshing, leaving the UI out of sync with the database.
Even if the reassignment fails or is cancelled, you must alert the user that the original update succeeded, close the modal, and refresh the table.
if (!increaseCapacity) {
alert('Passenger removed from previous bus, but not reassigned due to capacity limits.');
document.getElementById('transactionModal').style.display = 'none';
document.getElementById('reportForm').dispatchEvent(new Event('submit'));
return;
}| if ( | ||
| bus.event_date !== booking.date | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Directly comparing bus.event_date and booking.date using strict inequality (!==) can fail if one is an ISO timestamp (e.g., 2023-10-27T00:00:00.000Z) and the other is a plain date string (e.g., 2023-10-27). To ensure correctness, normalize both date strings to YYYY-MM-DD before comparing.
| if ( | |
| bus.event_date !== booking.date | |
| ) { | |
| return; | |
| } | |
| const busDate = bus.event_date ? bus.event_date.split('T')[0] : ''; | |
| const bookingDate = booking.date ? booking.date.split('T')[0] : ''; | |
| if (busDate !== bookingDate) { | |
| return; | |
| } |
| const row = | ||
| document.createElement( | ||
| 'tr' | ||
| ); | ||
|
|
||
| row.innerHTML = ` |
There was a problem hiding this comment.
Using .innerHTML with unescaped user-controlled fields like passenger.name, passenger.comments, and passenger.luggage introduces a Cross-Site Scripting (XSS) vulnerability. If a user inputs malicious HTML/JS into their comments or name, it will execute in the coordinator's browser.
Consider creating a helper function to escape HTML characters, and wrap all dynamic passenger fields (like passenger.name, passenger.comments, passenger.luggage) in escapeHTML(...) inside the template literal.
| const row = | |
| document.createElement( | |
| 'tr' | |
| ); | |
| row.innerHTML = ` | |
| const escapeHTML = str => String(str || '').replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | |
| const row = | |
| document.createElement( | |
| 'tr' | |
| ); | |
| row.innerHTML = ` |
No description provided.