Open
Conversation
Contributor
Author
|
@Shalini828 done this was imp |
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #40 by implementing comprehensive security measures to prevent IDOR (Insecure Direct Object References) vulnerabilities and CSRF (Cross-Site Request Forgery) attacks across the NearBy application.
Changes:
- Implemented CSRF protection with token generation, validation, and integration across all POST endpoints
- Added authorization helpers for ownership verification, role-based access control, and resource validation
- Created input validation helpers with email, password, name, and category validation
- Enhanced security configuration with API key validation, reduced error disclosure, and security headers
- Added comprehensive documentation and test suites for both CSRF and IDOR protection
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| includes/helpers/csrf.php | New CSRF protection helper with token generation and validation functions |
| includes/helpers/authorization.php | New authorization helper with ownership verification and access control |
| includes/helpers/validation.php | New input validation helper for sanitizing and validating user input |
| api/posts/create.php | Added CSRF validation and replaced manual auth checks with helper functions |
| api/accommodations/create.php | Added CSRF and authorization checks using helper functions |
| api/add_guidance.php | Added CSRF and role-based access control |
| api/contact/request.php | Added CSRF, authentication, and resource existence validation |
| api/message-assistant-send.php | Added CSRF validation with custom error handling |
| api/message-assistant-history.php | Replaced manual auth check with requireLogin helper |
| api/login.php | Added CSRF protection to login endpoint |
| api/register.php | Added CSRF protection to registration endpoint |
| api/auth/login.php | Added CSRF protection and input validation |
| api/auth/register.php | Added CSRF protection and comprehensive input validation |
| api/auth/google-login.php | Added CSRF protection |
| api/post_accommodation.php | Added CSRF and authorization checks |
| api/fetch_accommodation_details.php | Added ID validation and resource existence checks |
| config/config.php | Added API key validation, environment detection, and security headers |
| config/db.php | Removed detailed error messages to prevent information disclosure |
| includes/header.php | Added CSRF helper include and meta tag for JavaScript access |
| includes/create-post-modal.php | Added CSRF token field to post creation form |
| login.php | Added CSRF token field to login form |
| register.php | Added CSRF token field to registration form |
| assets/js/main.js | Enhanced fetchJSON to automatically include CSRF tokens |
| assets/js/message.js | Updated to use centralized fetchJSON with CSRF support |
| csrf-test.php | Test suite for CSRF protection |
| idor-test.php | Test suite for IDOR protection |
| CSRF_PROTECTION.md | Comprehensive documentation for CSRF implementation |
| IDOR_PROTECTION.md | Comprehensive documentation for IDOR implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+64
to
+65
| // Validate API key format | ||
| if (!$key || !preg_match('/^AIza[0-9A-Za-z_-]{35}$/', $key)) { |
There was a problem hiding this comment.
The regex pattern for validating the Gemini API key may be too strict. The pattern assumes exactly 35 characters after 'AIza', but Gemini API keys can vary in length. If a valid API key doesn't match this exact pattern, it will be rejected and set to null, causing API calls to fail. Consider either removing this validation or making it more flexible to accommodate different key lengths.
Suggested change
| // Validate API key format | |
| if (!$key || !preg_match('/^AIza[0-9A-Za-z_-]{35}$/', $key)) { | |
| // Validate API key format (allow variable length after "AIza") | |
| if (!$key || !preg_match('/^AIza[0-9A-Za-z_-]+$/', $key)) { |
Comment on lines
+1
to
+369
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>IDOR Protection Test Suite</title> | ||
| <style> | ||
| body { font-family: Arial, sans-serif; max-width: 1000px; margin: 50px auto; padding: 20px; } | ||
| .test-section { background: #f5f5f5; padding: 20px; margin: 20px 0; border-radius: 8px; } | ||
| .test-case { background: #fff; padding: 15px; margin: 10px 0; border-left: 4px solid #007bff; } | ||
| .success { color: #28a745; font-weight: bold; } | ||
| .error { color: #dc3545; font-weight: bold; } | ||
| .warning { color: #ffc107; font-weight: bold; } | ||
| button { padding: 10px 20px; margin: 10px 5px; cursor: pointer; background: #007bff; color: white; border: none; border-radius: 4px; } | ||
| button:hover { background: #0056b3; } | ||
| button:disabled { background: #ccc; cursor: not-allowed; } | ||
| pre { background: #fff; padding: 15px; overflow-x: auto; border: 1px solid #ddd; margin: 10px 0; font-size: 12px; } | ||
| .status-indicator { display: inline-block; width: 12px; height: 12px; border-radius: 50%; margin-right: 8px; } | ||
| .status-success { background: #28a745; } | ||
| .status-error { background: #dc3545; } | ||
| .status-pending { background: #ffc107; } | ||
| h1 { color: #333; } | ||
| h2 { color: #666; margin-top: 0; } | ||
| .info-box { background: #e7f3ff; padding: 15px; border-left: 4px solid #2196F3; margin: 20px 0; } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h1>🛡️ IDOR Protection Test Suite</h1> | ||
| <p>This page tests the IDOR (Insecure Direct Object References) protection implementation.</p> | ||
|
|
||
| <?php | ||
| if (session_status() === PHP_SESSION_NONE) { | ||
| session_start(); | ||
| } | ||
|
|
||
| $isLoggedIn = isset($_SESSION['user']['id']); | ||
| $userRole = $_SESSION['user']['role'] ?? 'none'; | ||
| $userId = $_SESSION['user']['id'] ?? 'N/A'; | ||
| ?> | ||
|
|
||
| <div class="info-box"> | ||
| <strong>Current Session:</strong><br> | ||
| Logged In: <?= $isLoggedIn ? '✓ Yes' : '✗ No' ?><br> | ||
| <?php if ($isLoggedIn): ?> | ||
| User ID: <?= htmlspecialchars($userId) ?><br> | ||
| Role: <?= htmlspecialchars($userRole) ?> | ||
| <?php else: ?> | ||
| <em>Please log in to test authenticated endpoints</em> | ||
| <?php endif; ?> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 1: Authentication Check</h2> | ||
| <p>Tests that endpoints requiring authentication reject unauthenticated users.</p> | ||
| <div class="test-case"> | ||
| <h3>1.1 Access Protected Endpoint (Create Post)</h3> | ||
| <button onclick="testAuthenticationRequired()">Run Test</button> | ||
| <div id="result-auth"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 2: Role-Based Access Control</h2> | ||
| <p>Tests that role-restricted endpoints reject users without required role.</p> | ||
| <div class="test-case"> | ||
| <h3>2.1 Junior User Creating Accommodation (Should Fail)</h3> | ||
| <button onclick="testRoleRestriction()">Run Test</button> | ||
| <div id="result-role"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 3: ID Validation</h2> | ||
| <p>Tests that invalid IDs are properly rejected.</p> | ||
| <div class="test-case"> | ||
| <h3>3.1 Non-Numeric ID</h3> | ||
| <button onclick="testInvalidId('abc')">Test with 'abc'</button> | ||
| <button onclick="testInvalidId('--1')">Test with '--1'</button> | ||
| <button onclick="testInvalidId('')">Test with empty</button> | ||
| <div id="result-invalidid"></div> | ||
| </div> | ||
| <div class="test-case"> | ||
| <h3>3.2 SQL Injection Attempt</h3> | ||
| <button onclick="testSQLInjection()">Run Test</button> | ||
| <div id="result-sqli"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 4: Resource Existence Verification</h2> | ||
| <p>Tests that non-existent resources return proper 404 errors.</p> | ||
| <div class="test-case"> | ||
| <h3>4.1 Access Non-Existent Accommodation</h3> | ||
| <button onclick="testNonExistentResource()">Run Test</button> | ||
| <div id="result-notfound"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 5: User Data Isolation</h2> | ||
| <p>Tests that users can only access their own data.</p> | ||
| <div class="test-case"> | ||
| <h3>5.1 Chat History (Should Only Return Own Messages)</h3> | ||
| <button onclick="testDataIsolation()">Run Test</button> | ||
| <div id="result-isolation"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>Test 6: Valid Operations</h2> | ||
| <p>Tests that legitimate operations work correctly.</p> | ||
| <div class="test-case"> | ||
| <h3>6.1 View Existing Accommodation</h3> | ||
| <input type="number" id="validAccId" placeholder="Enter accommodation ID" value="1"> | ||
| <button onclick="testValidOperation()">Run Test</button> | ||
| <div id="result-valid"></div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <script> | ||
| function displayResult(elementId, status, title, message, details = null) { | ||
| const element = document.getElementById(elementId); | ||
| const statusClass = status === 'success' ? 'status-success' : | ||
| status === 'error' ? 'status-error' : 'status-pending'; | ||
| const textClass = status === 'success' ? 'success' : | ||
| status === 'error' ? 'error' : 'warning'; | ||
|
|
||
| let html = ` | ||
| <p> | ||
| <span class="status-indicator ${statusClass}"></span> | ||
| <span class="${textClass}">${title}</span> | ||
| </p> | ||
| <p>${message}</p> | ||
| `; | ||
|
|
||
| if (details) { | ||
| html += `<pre>${JSON.stringify(details, null, 2)}</pre>`; | ||
| } | ||
|
|
||
| element.innerHTML = html; | ||
| } | ||
|
|
||
| async function testAuthenticationRequired() { | ||
| displayResult('result-auth', 'pending', 'Testing...', 'Sending request without authentication'); | ||
|
|
||
| try { | ||
| const response = await fetch('api/posts/create.php', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| post_category: 'room', | ||
| location: 'Test', | ||
| description: 'Test', | ||
| contact_phone: '1234567890' | ||
| }) | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 401 || response.status === 403) { | ||
| displayResult('result-auth', 'success', | ||
| '✓ PASS: Authentication Required', | ||
| 'Endpoint correctly rejected unauthenticated/unauthorized request', | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-auth', 'error', | ||
| '✗ FAIL: No Authentication Check', | ||
| 'Endpoint should require authentication but did not', | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-auth', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testRoleRestriction() { | ||
| displayResult('result-role', 'pending', 'Testing...', 'Attempting role-restricted operation'); | ||
|
|
||
| try { | ||
| const response = await fetch('api/accommodations/create.php', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| title: 'Test Room', | ||
| type: 'PG', | ||
| allowed_for: 'Male', | ||
| rent: '5000', | ||
| location: 'Test', | ||
| description: 'Test' | ||
| }) | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| const userRole = '<?= $userRole ?>'; | ||
|
|
||
| if (userRole === 'junior' && response.status === 403) { | ||
| displayResult('result-role', 'success', | ||
| '✓ PASS: Role Restriction Working', | ||
| 'Junior user correctly denied access to senior-only operation', | ||
| data | ||
| ); | ||
| } else if (userRole === 'senior' && response.status === 200) { | ||
| displayResult('result-role', 'success', | ||
| '✓ PASS: Senior Access Granted', | ||
| 'Senior user correctly allowed to create accommodation', | ||
| data | ||
| ); | ||
| } else if (response.status === 401 || response.status === 403) { | ||
| displayResult('result-role', 'success', | ||
| '✓ PASS: Unauthorized Access Denied', | ||
| 'Non-authenticated or non-authorized user denied access', | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-role', 'warning', | ||
| '⚠ Check Status', | ||
| `Unexpected status: ${response.status}. Check if role validation is working.`, | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-role', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testInvalidId(invalidId) { | ||
| displayResult('result-invalidid', 'pending', 'Testing...', `Sending invalid ID: "${invalidId}"`); | ||
|
|
||
| try { | ||
| const response = await fetch(`api/fetch_accommodation_details.php?id=${encodeURIComponent(invalidId)}`); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 400) { | ||
| displayResult('result-invalidid', 'success', | ||
| '✓ PASS: Invalid ID Rejected', | ||
| `Endpoint correctly rejected invalid ID "${invalidId}"`, | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-invalidid', 'error', | ||
| '✗ FAIL: Invalid ID Not Caught', | ||
| `Endpoint should reject invalid ID "${invalidId}" but didn't`, | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-invalidid', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testSQLInjection() { | ||
| const sqlInjectionAttempt = "1' OR '1'='1"; | ||
| displayResult('result-sqli', 'pending', 'Testing...', 'Attempting SQL injection'); | ||
|
|
||
| try { | ||
| const response = await fetch(`api/fetch_accommodation_details.php?id=${encodeURIComponent(sqlInjectionAttempt)}`); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 400) { | ||
| displayResult('result-sqli', 'success', | ||
| '✓ PASS: SQL Injection Prevented', | ||
| 'ID validation prevented SQL injection attempt', | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-sqli', 'error', | ||
| '✗ FAIL: Potential SQL Injection', | ||
| 'SQL injection attempt was not properly blocked', | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-sqli', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testNonExistentResource() { | ||
| const nonExistentId = 999999; | ||
| displayResult('result-notfound', 'pending', 'Testing...', 'Accessing non-existent resource'); | ||
|
|
||
| try { | ||
| const response = await fetch(`api/fetch_accommodation_details.php?id=${nonExistentId}`); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 404) { | ||
| displayResult('result-notfound', 'success', | ||
| '✓ PASS: Proper 404 Response', | ||
| 'Non-existent resource correctly returned 404', | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-notfound', 'warning', | ||
| '⚠ Unexpected Response', | ||
| `Expected 404 but got ${response.status}. This might be OK if resource actually exists.`, | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-notfound', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testDataIsolation() { | ||
| displayResult('result-isolation', 'pending', 'Testing...', 'Fetching chat history'); | ||
|
|
||
| try { | ||
| const response = await fetch('api/message-assistant-history.php'); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 401) { | ||
| displayResult('result-isolation', 'success', | ||
| '✓ PASS: Authentication Required', | ||
| 'Chat history correctly requires authentication', | ||
| data | ||
| ); | ||
| } else if (response.status === 200 && data.success) { | ||
| displayResult('result-isolation', 'success', | ||
| '✓ PASS: Data Retrieved', | ||
| 'Chat history returned (verify it only contains your messages in database)', | ||
| { message_count: data.messages?.length || 0, note: 'Check database to verify user_id matches session' } | ||
| ); | ||
| } else { | ||
| displayResult('result-isolation', 'warning', | ||
| '⚠ Check Response', | ||
| 'Unexpected response', | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-isolation', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
|
|
||
| async function testValidOperation() { | ||
| const accId = document.getElementById('validAccId').value; | ||
| displayResult('result-valid', 'pending', 'Testing...', 'Fetching accommodation details'); | ||
|
|
||
| try { | ||
| const response = await fetch(`api/fetch_accommodation_details.php?id=${accId}`); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 200 && data.success) { | ||
| displayResult('result-valid', 'success', | ||
| '✓ PASS: Valid Operation Works', | ||
| 'Legitimate request succeeded', | ||
| { title: data.data?.title, type: data.data?.type } | ||
| ); | ||
| } else if (response.status === 404) { | ||
| displayResult('result-valid', 'warning', | ||
| '⚠ Resource Not Found', | ||
| 'Accommodation ID not found. Try a different ID.', | ||
| data | ||
| ); | ||
| } else { | ||
| displayResult('result-valid', 'error', | ||
| '✗ Unexpected Error', | ||
| 'Valid operation failed unexpectedly', | ||
| data | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-valid', 'error', 'Test Error', error.message); | ||
| } | ||
| } | ||
| </script> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
The test file should not be committed to the repository or should be placed in a separate test directory. Exposing test interfaces in production can provide attackers with information about security mechanisms and potentially allow them to probe for vulnerabilities. Consider moving this file to a tests directory that is not deployed to production, or protect it with authentication checks.
Comment on lines
+1
to
+172
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>CSRF Protection Test</title> | ||
| <style> | ||
| body { font-family: Arial, sans-serif; max-width: 800px; margin: 50px auto; padding: 20px; } | ||
| .test-section { background: #f5f5f5; padding: 20px; margin: 20px 0; border-radius: 8px; } | ||
| .success { color: #28a745; } | ||
| .error { color: #dc3545; } | ||
| button { padding: 10px 20px; margin: 10px 5px; cursor: pointer; } | ||
| pre { background: #fff; padding: 15px; overflow-x: auto; border: 1px solid #ddd; } | ||
| .token-display { background: #fff; padding: 10px; margin: 10px 0; word-break: break-all; font-family: monospace; font-size: 12px; } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h1>🛡️ CSRF Protection Test Suite</h1> | ||
| <p>This page tests the CSRF protection implementation in the NearBy application.</p> | ||
|
|
||
| <?php | ||
| if (session_status() === PHP_SESSION_NONE) { | ||
| session_start(); | ||
| } | ||
| require_once __DIR__ . '/includes/helpers/csrf.php'; | ||
| $token = getCSRFToken(); | ||
| ?> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>1. Current CSRF Token</h2> | ||
| <p>Your current session CSRF token:</p> | ||
| <div class="token-display"><?= htmlspecialchars($token) ?></div> | ||
| <p><small>Token expires: <?= date('Y-m-d H:i:s', $_SESSION['csrf_token_time'] + 3600) ?></small></p> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>2. Test Valid Request</h2> | ||
| <p>This test will send a POST request WITH a valid CSRF token.</p> | ||
| <button onclick="testValidRequest()">Test Valid Request ✓</button> | ||
| <div id="result-valid"></div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>3. Test Missing Token</h2> | ||
| <p>This test will send a POST request WITHOUT a CSRF token.</p> | ||
| <button onclick="testMissingToken()">Test Missing Token ✗</button> | ||
| <div id="result-missing"></div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>4. Test Invalid Token</h2> | ||
| <p>This test will send a POST request with an INVALID CSRF token.</p> | ||
| <button onclick="testInvalidToken()">Test Invalid Token ✗</button> | ||
| <div id="result-invalid"></div> | ||
| </div> | ||
|
|
||
| <div class="test-section"> | ||
| <h2>5. Implementation Summary</h2> | ||
| <ul> | ||
| <li>✅ CSRF helper functions created in <code>includes/helpers/csrf.php</code></li> | ||
| <li>✅ All POST endpoints protected with CSRF validation</li> | ||
| <li>✅ All HTML forms include CSRF tokens</li> | ||
| <li>✅ JavaScript automatically includes CSRF tokens in AJAX requests</li> | ||
| <li>✅ Tokens expire after 1 hour</li> | ||
| <li>✅ Uses cryptographically secure random token generation</li> | ||
| <li>✅ Timing-safe token comparison to prevent timing attacks</li> | ||
| </ul> | ||
| </div> | ||
|
|
||
| <script> | ||
| const CSRF_TOKEN = '<?= $token ?>'; | ||
|
|
||
| async function displayResult(elementId, success, message, details = null) { | ||
| const element = document.getElementById(elementId); | ||
| const statusClass = success ? 'success' : 'error'; | ||
| const statusIcon = success ? '✓' : '✗'; | ||
| let html = `<p class="${statusClass}"><strong>${statusIcon} ${message}</strong></p>`; | ||
| if (details) { | ||
| html += `<pre>${JSON.stringify(details, null, 2)}</pre>`; | ||
| } | ||
| element.innerHTML = html; | ||
| } | ||
|
|
||
| async function testValidRequest() { | ||
| try { | ||
| const response = await fetch('api/posts/create.php', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-CSRF-Token': CSRF_TOKEN | ||
| }, | ||
| body: JSON.stringify({ | ||
| csrf_token: CSRF_TOKEN, | ||
| post_category: 'room', | ||
| location: 'Test Location', | ||
| description: 'CSRF test post', | ||
| contact_phone: '1234567890', | ||
| rent_or_price: '5000' | ||
| }) | ||
| }); | ||
| const data = await response.json(); | ||
|
|
||
| // Even if validation succeeds, the request may fail due to other validation | ||
| // What matters is that we DON'T get a 403 CSRF error | ||
| if (response.status === 403 && data.error?.includes('CSRF')) { | ||
| displayResult('result-valid', false, 'CSRF validation failed unexpectedly!', data); | ||
| } else { | ||
| displayResult('result-valid', true, 'CSRF token validated successfully! (Request passed CSRF check)', data); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-valid', false, 'Request failed', { error: error.message }); | ||
| } | ||
| } | ||
|
|
||
| async function testMissingToken() { | ||
| try { | ||
| const response = await fetch('api/posts/create.php', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ | ||
| post_category: 'room', | ||
| location: 'Test Location', | ||
| description: 'CSRF test post', | ||
| contact_phone: '1234567890', | ||
| rent_or_price: '5000' | ||
| }) | ||
| }); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 403) { | ||
| displayResult('result-missing', true, 'CSRF protection working! Request blocked as expected.', data); | ||
| } else { | ||
| displayResult('result-missing', false, 'CSRF protection FAILED! Request should have been blocked.', data); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-missing', false, 'Request failed', { error: error.message }); | ||
| } | ||
| } | ||
|
|
||
| async function testInvalidToken() { | ||
| try { | ||
| const response = await fetch('api/posts/create.php', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-CSRF-Token': 'invalid_token_12345' | ||
| }, | ||
| body: JSON.stringify({ | ||
| csrf_token: 'invalid_token_12345', | ||
| post_category: 'room', | ||
| location: 'Test Location', | ||
| description: 'CSRF test post', | ||
| contact_phone: '1234567890', | ||
| rent_or_price: '5000' | ||
| }) | ||
| }); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.status === 403) { | ||
| displayResult('result-invalid', true, 'CSRF protection working! Invalid token rejected as expected.', data); | ||
| } else { | ||
| displayResult('result-invalid', false, 'CSRF protection FAILED! Invalid token should have been rejected.', data); | ||
| } | ||
| } catch (error) { | ||
| displayResult('result-invalid', false, 'Request failed', { error: error.message }); | ||
| } | ||
| } | ||
| </script> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
The test file should not be committed to the repository or should be placed in a separate test directory. Exposing test interfaces in production can provide attackers with information about security mechanisms and potentially allow them to probe for vulnerabilities. Consider moving this file to a tests directory that is not deployed to production, or protect it with authentication checks.
Comment on lines
+213
to
+222
| function requireOwnership($ownsResource, $resourceType = 'resource') { | ||
| if (!$ownsResource) { | ||
| http_response_code(403); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => "Access denied. You don't have permission to modify this {$resourceType}." | ||
| ]); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For example, requireLogin() on line 183 uses 'error', requireRole() on line 202 uses 'error', but many other endpoints use 'message' for consistency. Consider standardizing on a single key ('message') throughout the application for consistent error handling on the client side.
Comment on lines
+248
to
+257
| function validateId($id, $fieldName = 'ID') { | ||
| if (!$id || !is_numeric($id)) { | ||
| http_response_code(400); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => "Invalid {$fieldName}. Please provide a valid numeric identifier." | ||
| ]); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For consistency with the rest of the application, consider using 'message' instead of 'error' as the key for error messages.
Comment on lines
+248
to
+259
| function validateId($id, $fieldName = 'ID') { | ||
| if (!$id || !is_numeric($id)) { | ||
| http_response_code(400); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => "Invalid {$fieldName}. Please provide a valid numeric identifier." | ||
| ]); | ||
| exit; | ||
| } | ||
| return (int) $id; | ||
| } |
There was a problem hiding this comment.
The validateId function accepts zero and negative numbers since it only checks is_numeric. Database IDs are typically positive integers starting from 1. Consider adding a check to ensure the ID is greater than 0: if (!$id || !is_numeric($id) || $id <= 0) to prevent potential issues with invalid ID values.
Comment on lines
+230
to
+239
| function requireResourceExists($exists, $resourceType = 'resource') { | ||
| if (!$exists) { | ||
| http_response_code(404); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => ucfirst($resourceType) . ' not found.' | ||
| ]); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For consistency with the rest of the application, consider using 'message' instead of 'error' as the key for error messages.
Comment on lines
+177
to
+186
| function requireLogin() { | ||
| if (!getCurrentUserId()) { | ||
| http_response_code(401); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => 'Authentication required. Please log in to continue.' | ||
| ]); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For consistency with the rest of the application, consider using 'message' instead of 'error' as the key for error messages.
Comment on lines
+193
to
+205
| function requireRole($requiredRole) { | ||
| requireLogin(); | ||
| $userRole = getCurrentUserRole(); | ||
|
|
||
| if ($userRole !== $requiredRole) { | ||
| http_response_code(403); | ||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => 'Access denied. Insufficient permissions.' | ||
| ]); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For consistency with the rest of the application, consider using 'message' instead of 'error' as the key for error messages.
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => false, | ||
| 'error' => 'Invalid or missing CSRF token. Please refresh the page and try again.' |
There was a problem hiding this comment.
The error response uses 'error' as the key for the error message, but other endpoints in the codebase use 'message'. For consistency with the rest of the application, consider using 'message' instead of 'error' as the key for error messages.
Suggested change
| 'error' => 'Invalid or missing CSRF token. Please refresh the page and try again.' | |
| 'message' => 'Invalid or missing CSRF token. Please refresh the page and try again.' |
Contributor
Author
|
@sumitrathor1 any update please merge |
Owner
|
@Ayaanshaikh12243 There is conflict in the file I request you to sync the new code then pls update your change |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#40