Skip to content

Dev#136

Open
vvshk wants to merge 18 commits into
mainfrom
dev
Open

Dev#136
vvshk wants to merge 18 commits into
mainfrom
dev

Conversation

@vvshk
Copy link
Copy Markdown
Collaborator

@vvshk vvshk commented May 25, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +54 to +68
document
.getElementById(
'bulkUploadPassengers'
)
.addEventListener(
'click',
() => {

document
.getElementById(
'bulkUploadInput'
)
.click();
}
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
document
.getElementById(
'bulkUploadPassengers'
)
.addEventListener(
'click',
() => {
document
.getElementById(
'bulkUploadInput'
)
.click();
}
);
document
.getElementById(
'bulkUploadPassengers'
)
?.addEventListener(
'click',
() => {
document
.getElementById(
'bulkUploadInput'
)
.click();
}
);

Comment on lines +744 to +746
if (!increaseCapacity) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
            }

Comment on lines +494 to +498
if (
bus.event_date !== booking.date
) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;
}

Comment on lines +482 to +487
const row =
document.createElement(
'tr'
);

row.innerHTML = `
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
const row =
document.createElement(
'tr'
);
row.innerHTML = `
const escapeHTML = str => String(str || '').replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
const row =
document.createElement(
'tr'
);
row.innerHTML = `

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant