utsav feedback form#267
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 feature that enables users to provide feedback for Utsav events. It establishes the necessary backend infrastructure, including a new database table and Sequelize model, along with API endpoints and controller logic to manage the submission and validation of this feedback. The system is designed to capture detailed user input on various aspects of the Utsav experience. 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
|
![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 introduces a new feature for collecting feedback on 'utsav' events, involving a new database migration, model, controller logic, and API routes. However, it presents critical security concerns regarding access control. The feedback validation and submission endpoints rely on client-provided identifiers (cardno) instead of authenticated user session data, which could lead to IDOR and Broken Access Control vulnerabilities. These issues require remediation by utilizing req.user and ensuring valid bookings. Beyond security, a critical issue was identified where an incorrect error class (APIError instead of ApiError) is used, potentially causing a server crash. There are also several medium-severity issues related to code style, dead code, and opportunities to enhance the robustness of the request validation logic.
| } = req.body; | ||
|
|
||
| if (!cardno || !utsav_id) { | ||
| throw new APIError(400, 'cardno and utsav_id are required'); |
There was a problem hiding this comment.
The code uses APIError, but this is not defined or imported. The file does import ApiError at the top, which should be used instead. This will cause a ReferenceError and crash the request. This same issue exists on lines 341 and 349.
| throw new APIError(400, 'cardno and utsav_id are required'); | |
| throw new ApiError(400, 'cardno and utsav_id are required'); |
| }); | ||
|
|
||
| if (existing) { | ||
| throw new APIError(409, 'Feedback already submitted'); |
There was a problem hiding this comment.
The code uses APIError, but this is not defined or imported. The file does import ApiError at the top, which should be used instead to prevent a ReferenceError.
throw new ApiError(409, 'Feedback already submitted');| const { | ||
| cardno, | ||
| utsav_id, | ||
| mumukshu_name, | ||
| accommodation_type, | ||
| room_number, | ||
|
|
||
| accommodation_rating, | ||
| qr_rating, | ||
| food_rating, | ||
| program_rating, | ||
| volunteer_rating, | ||
| infrastructure_rating, | ||
| decor_rating, | ||
| internal_transport_rating, | ||
| raj_pravas_rating, | ||
| sparsh_rating, | ||
| av_rating, | ||
|
|
||
| loved_most, | ||
| improvement_suggestions, | ||
| } = req.body; |
There was a problem hiding this comment.
The submitUtsavFeedback function retrieves cardno from req.body instead of using the authenticated user's card number. This allows an attacker to submit feedback on behalf of any user. Additionally, the function should verify that the user actually has a valid booking for the event before allowing feedback submission. Currently, it only checks if feedback was already submitted, but not if the user was authorized to submit it in the first place (i.e., by having a completed booking).
const { cardno } = req.user;
const {
utsav_id,
mumukshu_name,
accommodation_type,
room_number,
accommodation_rating,
qr_rating,
food_rating,
program_rating,
volunteer_rating,
infrastructure_rating,
decor_rating,
internal_transport_rating,
raj_pravas_rating,
sparsh_rating,
av_rating,
loved_most,
improvement_suggestions,
} = req.body;| /* -------------------------------------------------------------------------- */ | ||
|
|
||
| export const validateUtsavFeedback = async (req, res) => { | ||
| const { utsavid, cardno } = req.query; |
There was a problem hiding this comment.
The validateUtsavFeedback function retrieves cardno from req.query instead of using the authenticated user's card number. This allows any authenticated user to check the booking status and feedback existence of any other user by providing their cardno in the query string. This is an Insecure Direct Object Reference (IDOR) vulnerability. You should use req.user.cardno which is populated by the validateCard middleware.
| const { utsavid, cardno } = req.query; | |
| const { utsavid } = req.query; | |
| const { cardno } = req.user; |
| }; | ||
|
|
||
|
|
||
| import { Op } from 'sequelize'; |
There was a problem hiding this comment.
Imports should be grouped at the top of the file for better readability and maintainability. Please move this import to the top of the file with the other imports.
| // import db from '../../models/index.js'; | ||
| // import APIError from '../../utils/APIError.js'; | ||
|
|
||
| // const { UtsavFeedback, UtsavBooking } = db; |
There was a problem hiding this comment.
This commented-out code appears to be a leftover from development. It should be removed to keep the code clean and reduce clutter.
| const requiredFields = [ | ||
| mumukshu_name, | ||
| accommodation_type, | ||
| room_number, | ||
| accommodation_rating, | ||
| qr_rating, | ||
| food_rating, | ||
| program_rating, | ||
| volunteer_rating, | ||
| infrastructure_rating, | ||
| decor_rating, | ||
| internal_transport_rating, | ||
| raj_pravas_rating, | ||
| sparsh_rating, | ||
| av_rating, | ||
| loved_most, | ||
| improvement_suggestions, | ||
| ]; | ||
|
|
||
| if (requiredFields.some((f) => f === null || f === undefined || f === '')) { | ||
| throw new APIError(400, 'All feedback fields are required'); | ||
| } |
There was a problem hiding this comment.
The current validation for required fields is a bit verbose and fragile. If a new field is added or removed, it has to be updated in multiple places (destructuring and the requiredFields array). A more maintainable approach is to define the list of required field names and iterate over them to check req.body. This also allows for more specific error messages.
const requiredFieldNames = [
'mumukshu_name',
'accommodation_type',
'room_number',
'accommodation_rating',
'qr_rating',
'food_rating',
'program_rating',
'volunteer_rating',
'infrastructure_rating',
'decor_rating',
'internal_transport_rating',
'raj_pravas_rating',
'sparsh_rating',
'av_rating',
'loved_most',
'improvement_suggestions',
];
for (const field of requiredFieldNames) {
const value = req.body[field];
if (value === null || value === undefined || value === '') {
throw new ApiError(400, `Missing or empty required field: ${field}`);
}
}| import { | ||
| RESEARCH_CENTRE, | ||
| STATUS_CLOSED, | ||
| STATUS_OPEN | ||
| } from '../config/constants.js'; |
There was a problem hiding this comment.
These constants (RESEARCH_CENTRE, STATUS_CLOSED, STATUS_OPEN) are imported but not used in this file. They should be removed to keep the code clean.
| STATUS_CLOSED, | ||
| STATUS_OPEN | ||
| } from '../config/constants.js'; | ||
| // import { UtsavFeedback } from './associations.js'; |
There was a problem hiding this comment.
This commented-out import appears to be a leftover from development and should be removed.
…igration, associations, and removing unnecessary fields.
3 participants
No description provided.