Conversation
|
@sumitrathor1 done sir mene readme add kiya hoo taaki you know easy hoo check karne ke liye |
There was a problem hiding this comment.
Pull request overview
This PR hardens the application's security posture around sessions, CSRF, and authorization, addressing Issue #39 and related IDOR/CSRF concerns, and adds documentation plus browser-based test harnesses for verification.
Changes:
- Centralizes secure session management with
includes/helpers/session.phpand replaces directsession_start()calls across pages and APIs, including proper logout viadestroySession()and secure login viasetUserSession(). - Introduces CSRF protection helpers and wiring: tokens in forms/meta tags, automatic inclusion in
fetchJSON, and server-side validation on all key POST endpoints, plus ID and ownership/role helpers to mitigate IDOR. - Tightens configuration and database error responses, adds input validation helpers, and documents the session, CSRF, and IDOR implementations with dedicated markdown guides and in-browser test suites.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
senior-dashboard.php |
Switches to secureSessionStart() from the new session helper before enforcing senior role and rendering the dashboard. |
register.php |
Adds a hidden CSRF token field to the registration form, relying on header-included CSRF helpers. |
logout.php |
Migrates logout to the centralized destroySession() helper and redirect, but leaves an unreachable duplicate redirect block that should be removed. |
login.php |
Adds a hidden CSRF token field to the login form and leverages the updated header/CSRF setup. |
junior-dashboard.php |
Uses secureSessionStart() for the junior dashboard gatekeeping; contains a stale //session_start(); comment that can be removed. |
includes/helpers/validation.php |
Introduces an InputValidator class for email, password, text, name, role, and user category validation/sanitization for use in auth endpoints. |
includes/helpers/session.php |
Adds central secure session management (configuration, timeouts, fingerprinting, regeneration, login/logout helpers, and diagnostics). |
includes/helpers/csrf.php |
Implements CSRF token generation, storage, validation, and HTML/meta helpers plus a requireCSRFToken() guard for endpoints. |
includes/helpers/authorization.php |
Adds helpers for current user/role, ownership and existence checks, login/role guards, and ID validation to mitigate IDOR. |
includes/header.php |
Replaces raw session_start() with secureSessionStart(), loads CSRF helpers, and injects a CSRF meta tag for JS to consume. |
includes/create-post-modal.php |
Adds a hidden CSRF token field to the create-post modal form. |
idor-test.php |
New in-browser test suite for IDOR protections; currently uses plain session_start() instead of secureSessionStart(), so it does not fully reflect hardened session behavior. |
details.php |
Switches to secureSessionStart() from the session helper before loading DB/config and rendering details. |
csrf-test.php |
New in-browser test suite for CSRF protections; still uses session_start() rather than the central session helper. |
config/db.php |
Keeps DB connection logic but removes verbose debug data from JSON error responses, returning simpler, less-informative error payloads. |
config/config.php |
Refactors environment loading, centralizes IS_LOCAL, validates and exposes the Gemini API key more defensively, and adds security headers in non-local environments. |
assets/js/message.js |
Routes chatbot message sending through window.NearBy.fetchJSON, so CSRF tokens and uniform JSON error handling apply to assistant messages. |
assets/js/main.js |
Extends fetchJSON to read the CSRF token meta tag, attach tokens to headers/bodies/FormData for mutating requests, and keep JSON handling consistent. |
api/register.php |
Adds CSRF validation to the basic register endpoint before parsing and handling registration payloads. |
api/posts/list.php |
Ensures secureSessionStart() runs before listing posts, allowing logic to differentiate logged-in users (e.g., contact visibility). |
api/posts/create.php |
Adds CSRF validation and requireLogin() around post creation, and uses getCurrentUserId() from authorization helpers instead of raw $_SESSION. |
api/post_accommodation.php |
Wraps accommodation posting in secureSessionStart(), CSRF validation, and requireRole('senior'), and uses getCurrentUserId() to set the owner. |
api/message-assistant-send.php |
Secures assistant send endpoint with CSRF validation and requireLogin(), updates Gemini error messaging, and relies on the new config for API keys. |
api/message-assistant-history.php |
Uses the authorization helper to ensure the user is logged in and to scope chat history queries to the current user. |
api/login.php |
Adds CSRF validation and uses setUserSession() from the session helper for more secure login handling. |
api/fetch_accommodation_details.php |
Validates accommodation IDs with validateId() and returns 404 if the resource does not exist before querying details. |
api/contact/request.php |
Protects contact requests with secure session, CSRF validation, login requirement, ID validation, and a 404-on-missing-accommodation check. |
api/auth/register.php |
Intended to be a more robust registration endpoint using validators and CSRF, but currently contains a broken merge with duplicate PHP openings and duplicated mapping logic, which will cause a parse error and must be fixed. |
api/auth/login.php |
Adds validation helpers, CSRF protection, and setUserSession() to the auth login endpoint. |
api/auth/google-login.php |
Applies CSRF validation and secure session handling to the Google login flow, and uses setUserSession() after successful login. |
api/add_guidance.php |
Secures guidance creation with CSRF validation and requireRole('senior') while leaving the data model unchanged. |
api/accommodations/create.php |
Wraps accommodation creation in secure session, CSRF validation, and senior role checks, using getCurrentUserId() as the owner. |
SESSION_SECURITY.md |
Documents the new secure session strategy, its configuration, timeouts, fingerprinting, and migration guidelines for using secureSessionStart()/setUserSession()/destroySession(). |
IDOR_PROTECTION.md |
Describes the authorization helper design, ownership and existence checks, patterns for protected endpoints, and IDOR testing strategies. |
CSRF_PROTECTION.md |
Documents CSRF helper usage, token generation/validation, client integration (forms and AJAX), testing patterns, and maintenance guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $userType = $categoryMap[strtolower($userCategory)] ?? null; | ||
| if ($userType === null) { | ||
| echo json_encode(['success' => false, 'message' => 'Select a valid user category']); | ||
| exit; | ||
| } | ||
|
|
||
| $allowedDomain = '@mitsgwl.ac.in'; | ||
| $emailLower = strtolower($email); | ||
| if ($userType === 'student' && !str_ends_with($emailLower, $allowedDomain)) { | ||
| echo json_encode(['success' => false, 'message' => 'Students must use their @mitsgwl.ac.in email']); | ||
| exit; | ||
| } | ||
|
|
||
| if ($userType !== 'student') { | ||
| $role = 'senior'; | ||
| } | ||
|
|
There was a problem hiding this comment.
The user-category mapping and email-domain checks are duplicated (the $categoryMap / $userType / $allowedDomain logic appears twice), which adds unnecessary complexity and risk of divergence in future edits. Please consolidate this into a single block so the mapping and validation are defined only once.
| $userType = $categoryMap[strtolower($userCategory)] ?? null; | |
| if ($userType === null) { | |
| echo json_encode(['success' => false, 'message' => 'Select a valid user category']); | |
| exit; | |
| } | |
| $allowedDomain = '@mitsgwl.ac.in'; | |
| $emailLower = strtolower($email); | |
| if ($userType === 'student' && !str_ends_with($emailLower, $allowedDomain)) { | |
| echo json_encode(['success' => false, 'message' => 'Students must use their @mitsgwl.ac.in email']); | |
| exit; | |
| } | |
| if ($userType !== 'student') { | |
| $role = 'senior'; | |
| } |
| header('Location: login.php'); | ||
| exit; | ||
| header('Location: login.php'); | ||
| exit; |
There was a problem hiding this comment.
After calling destroySession() and redirecting to login.php with exit, the second header('Location: login.php'); exit; block is unreachable dead code. Please remove the duplicate redirect to avoid confusion and keep the logout flow clear.
| exit; | ||
| } | ||
|
|
||
| //session_start(); |
There was a problem hiding this comment.
This commented-out //session_start(); line is a leftover from the old session implementation and no longer serves any purpose now that secureSessionStart() is used. It can be safely removed to reduce noise and keep the session strategy unambiguous.
|
|
||
| <?php | ||
| if (session_status() === PHP_SESSION_NONE) { | ||
| session_start(); |
There was a problem hiding this comment.
This test page still uses a bare session_start() instead of the new secureSessionStart() helper, so its behavior does not reflect the hardened session configuration (timeout, fingerprinting, cookie flags) used elsewhere. For consistency with the documented session security guidelines, this should be updated to require the session helper and call secureSessionStart() instead.
| session_start(); | |
| if (function_exists('secureSessionStart')) { | |
| secureSessionStart(); | |
| } else { | |
| session_start(); | |
| } |
|
|
||
| <?php | ||
| if (session_status() === PHP_SESSION_NONE) { | ||
| session_start(); |
There was a problem hiding this comment.
This CSRF test page starts the session with session_start() rather than the centralized secureSessionStart() helper, so it bypasses the strengthened session settings and may not accurately exercise the real-session behavior. To keep tests aligned with production behavior, please switch this to use the session helper and secureSessionStart().
| session_start(); | |
| secureSessionStart(); |
|
@sumitrathor1 any update please merge |
|
@Ayaanshaikh12243 There is conflict in the file I request you to sync the new code then pls update your change |
|
@Ayaanshaikh12243 |
#39