-
Notifications
You must be signed in to change notification settings - Fork 0
frontend for ticketing system - ignore css #103
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
base: dev
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @vvshk, 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 introduces a new, fully functional administrative interface for managing support tickets. It enables administrators to efficiently oversee and respond to customer inquiries through a dedicated web page with filtering options, detailed ticket views, and messaging capabilities. The underlying code also lays the groundwork for real-time updates by integrating Socket.IO. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
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.
Code Review
This pull request introduces the frontend for a ticketing system. While the feature is a good addition, the current implementation has several critical security vulnerabilities and other issues that must be addressed before merging. My review focuses on XSS vulnerabilities from improper use of innerHTML, the use of an outdated and insecure version of jQuery, lack of error handling for API requests, and a configuration issue that would break the application in a production environment. Additionally, there's a significant opportunity to improve efficiency by using WebSockets instead of polling, for which dependencies have already been added.
| checkRoleAccess(['superAdmin', 'maintenanceAdmin']); | ||
| </script> | ||
|
|
||
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script> |
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 version of jQuery being used (1.11.2) is very old and has known security vulnerabilities, including Cross-Site Scripting (XSS). It is critical to update to a modern, secure version of jQuery (e.g., 3.x series) to protect the application and its users.
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script> | |
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script> |
| div.innerHTML = ` | ||
| <p>${msg.message}</p> | ||
| <span>${msg.sender_type} • ${new Date( | ||
| msg.createdAt | ||
| ).toLocaleString()}</span> | ||
| `; |
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.
Setting innerHTML with user-generated message content (msg.message) is a critical security vulnerability. This allows for Cross-Site Scripting (XSS) attacks, where a malicious user can inject scripts that will be executed in the admin's browser. Always use textContent to render user-provided content as plain text.
<p></p>
<span>${msg.sender_type} • ${new Date(
msg.createdAt
).toLocaleString()}</span>
`;
div.querySelector('p').textContent = msg.message;| const baseUrl = 'http://127.0.0.1:3000/api/v1'; | ||
| // const baseUrl = 'https://aashray.vitraagvigyaan.org/api/v1'; |
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 baseUrl is configured to point to a local development server (http://127.0.0.1:3000). If this change is merged and deployed to production, the application will fail to connect to the backend API, rendering it unusable. This must be reverted to the production URL.
| const baseUrl = 'http://127.0.0.1:3000/api/v1'; | |
| // const baseUrl = 'https://aashray.vitraagvigyaan.org/api/v1'; | |
| // const baseUrl = 'http://127.0.0.1:3000/api/v1'; | |
| const baseUrl = 'https://aashray.vitraagvigyaan.org/api/v1'; |
| const res = await fetch( | ||
| `${CONFIG.basePath}/tickets?${params.toString()}`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${sessionStorage.getItem('token')}` | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| const result = await res.json(); | ||
| renderTicketTable(result.data || []); |
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 fetch call lacks error handling. If the network request fails or the server returns an error status (e.g., 500), the promise will reject and cause an unhandled exception, which could crash the application. It's important to wrap asynchronous operations in try...catch blocks and check the response status with res.ok.
This should be applied to all fetch calls in this file (loadTicketDetails, sendReply, closeTicket).
try {
const res = await fetch(
`${CONFIG.basePath}/tickets?${params.toString()}`,
{
headers: {
Authorization: `Bearer ${sessionStorage.getItem('token')}`
}
}
);
if (!res.ok) {
console.error('Failed to fetch tickets:', res.status, res.statusText);
// Consider showing an error message to the user
return;
}
const result = await res.json();
renderTicketTable(result.data || []);
} catch (error) {
console.error('Error fetching tickets:', error);
// Consider showing an error message to the user
}| tr.innerHTML = ` | ||
| <td>${dot}${t.id}</td> | ||
| <td>${t.issued_by}</td> | ||
| <td>${t.service}</td> | ||
| <td>${t.status}</td> | ||
| <td>${new Date(t.createdAt).toLocaleString()}</td> | ||
| <td> | ||
| <button onclick="openTicket('${t.id}')">View</button> | ||
| </td> | ||
| `; |
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.
Directly setting innerHTML with data received from an API (t.id, t.issued_by, etc.) creates a Cross-Site Scripting (XSS) vulnerability. If any of these fields contain malicious user-provided data, it could execute scripts in the browser. It is much safer to create DOM elements programmatically and set their textContent property.
Example of a safer approach:
const tr = document.createElement('tr');
const createCell = (text) => {
const td = document.createElement('td');
td.textContent = text;
return td;
};
const idCell = document.createElement('td');
idCell.innerHTML = dot; // 'dot' is safe as it's generated in the code
idCell.appendChild(document.createTextNode(t.id));
const actionCell = document.createElement('td');
const viewButton = document.createElement('button');
viewButton.textContent = 'View';
viewButton.addEventListener('click', () => openTicket(t.id));
actionCell.appendChild(viewButton);
tr.append(
idCell,
createCell(t.issued_by),
createCell(t.service),
createCell(t.status),
createCell(new Date(t.createdAt).toLocaleString()),
actionCell
);
tbody.appendChild(tr);| document.getElementById('ticketMeta').innerHTML = ` | ||
| <p><b>Service:</b> ${ticket.service}</p> | ||
| <p><b>Issued By:</b> ${ticket.issued_by}</p> | ||
| <p><b>Description:</b> ${ticket.description}</p> | ||
| `; |
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.
Using innerHTML to set content from API data (ticket.service, ticket.issued_by, ticket.description) is unsafe and can lead to Cross-Site Scripting (XSS) vulnerabilities. Malicious scripts in these fields would be executed. Use textContent for any data that should be treated as plain text.
const meta = document.getElementById('ticketMeta');
meta.innerHTML = ''; // Clear previous content
const createMetaLine = (label, value) => {
const p = document.createElement('p');
const b = document.createElement('b');
b.textContent = `${label}: `;
p.appendChild(b);
p.appendChild(document.createTextNode(value));
return p;
};
meta.appendChild(createMetaLine('Service', ticket.service));
meta.appendChild(createMetaLine('Issued By', ticket.issued_by));
meta.appendChild(createMetaLine('Description', ticket.description));| function startAutoRefresh() { | ||
| stopAutoRefresh(); | ||
| refreshInterval = setInterval(loadTicketDetails, 60000); | ||
| } | ||
|
|
||
| function stopAutoRefresh() { | ||
| if (refreshInterval) { | ||
| clearInterval(refreshInterval); | ||
| refreshInterval = null; | ||
| } | ||
| } | ||
|
|
||
| function startTicketListRefresh() { | ||
| stopTicketListRefresh(); | ||
| ticketListInterval = setInterval(fetchTickets, 5000); | ||
| } | ||
|
|
||
| function stopTicketListRefresh() { | ||
| if (ticketListInterval) { | ||
| clearInterval(ticketListInterval); | ||
| ticketListInterval = null; | ||
| } | ||
| } |
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 application uses polling via setInterval to check for updates. This is inefficient, causing unnecessary network traffic and server load, and introduces delays in UI updates. Since socket.io dependencies have been added to package.json, it is strongly recommended to refactor this implementation to use WebSockets for real-time, event-driven updates. This will be more performant and provide a better user experience.
| <h2>Support Tickets</h2> | ||
|
|
||
| <!-- Filters --> | ||
| <div style="margin-bottom: 10px"> |
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.
| <option value="maintenance">Maintenance</option> | ||
| </select> | ||
|
|
||
| <button onclick="fetchTickets()">Apply</button> |
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.
Using onclick attributes directly in HTML mixes markup with application logic, which can make the code harder to maintain and debug. It's a better practice to attach event listeners from your JavaScript file using addEventListener. This improves separation of concerns.
For example, you could give this button an ID and attach the listener in tickets.js:
HTML:
<button id="apply-filters-btn">Apply</button>JavaScript (tickets.js):
document.getElementById('apply-filters-btn').addEventListener('click', fetchTickets);This should be applied to all buttons with onclick handlers in this file.
| </div> | ||
|
|
||
| <!-- Tickets Table --> | ||
| <table border="1" width="100%" id="ticketTable"> |
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 border and width attributes are presentational and are considered deprecated in modern HTML. All styling should be handled via CSS to ensure a clean separation of structure and presentation. While the PR title says to ignore CSS, using semantic HTML is important for maintainability.
| <table border="1" width="100%" id="ticketTable"> | |
| <table width="100%" id="ticketTable"> |
No description provided.