Add full AVSTube PHP/MySQL video sharing site with admin, popup ads, and SQL schema#45
Open
Add full AVSTube PHP/MySQL video sharing site with admin, popup ads, and SQL schema#45
Conversation
📝 WalkthroughWalkthroughThis pull request introduces AVSTube, a complete PHP/MySQL video sharing platform. It includes a MySQL database schema with tables for videos, comments, users, ads, announcements, settings, and analytics; a full admin panel with authentication, dashboards, and CRUD management for videos, ads, announcements, and settings; public-facing frontend pages for video listing, searching, viewing with comments, and analytics tracking; supporting configuration, utility helpers, CSS styling, and JavaScript functionality; and updated documentation reflecting the new project structure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebb6c70c0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+12
to
+13
| $stats = $pdo->prepare('INSERT INTO video_stats (video_id, view_date, daily_views) VALUES (:video_id, CURDATE(), 1) ON DUPLICATE KEY UPDATE daily_views = daily_views + 1'); | ||
| $stats->execute(['video_id' => $id]); |
There was a problem hiding this comment.
Move the video_stats insert until after confirming the video exists. Right now video.php writes to video_stats before checking $video, so requests like video.php?id=999999 (nonexistent ID) hit a foreign-key violation (video_stats.video_id -> videos.id) and throw a PDO exception instead of redirecting cleanly to the homepage.
Useful? React with 👍 / 👎.
| <?php endif; ?> | ||
| </div> | ||
| <h2><?= e($video['title']) ?></h2> | ||
| <p class="views mb-0"><i class="fa fa-eye"></i> <?= number_format((int) $video['views'] + 1) ?> lượt xem</p> |
There was a problem hiding this comment.
The displayed view total is inflated by one on every page load because views are already incremented in SQL before fetching the record, but the template still renders number_format((int) $video['views'] + 1). This causes user-facing analytics to be consistently inaccurate even though the stored value is correct.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
comment.php-20-21 (1)
20-21:⚠️ Potential issue | 🟡 MinorRedirect occurs even when validation fails or video_id is invalid.
When
$videoIdis 0 or validation fails, the user is redirected tovideo.php?id=0without feedback. This could cause confusion or errors.🐛 Proposed fix: Validate before redirect
+if ($videoId <= 0) { + header('Location: /'); + exit; +} + if ($videoId > 0 && $username !== '' && $comment !== '') { // ... insert logic } -header('Location: video.php?id=' . $videoId); +header('Location: video.php?id=' . $videoId . ($username === '' || $comment === '' ? '&error=validation' : '')); exit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comment.php` around lines 20 - 21, The redirect is happening unconditionally with header('Location: video.php?id=' . $videoId); exit;, causing navigation to video.php?id=0 when $videoId is invalid; update the logic to validate $videoId first (ensure it is a positive integer and corresponds to an existing video) and only perform the header redirect when validation succeeds, otherwise return an error response or redirect to a safe fallback (e.g., an error page or list view) and provide user-friendly feedback instead of redirecting to id=0.admin/login.php-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorAvoid trimming password input.
Trimming the password may cause authentication failures for users who intentionally included leading/trailing whitespace in their password. The password should be used exactly as entered.
🔧 Proposed fix
$username = trim($_POST['username'] ?? ''); - $password = trim($_POST['password'] ?? ''); + $password = $_POST['password'] ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/login.php` around lines 11 - 12, The password is being altered by trim() which can reject valid passwords containing leading/trailing whitespace; stop trimming the password and use the raw POST value for authentication instead. Specifically, change how $password is assigned in admin/login.php (replace trim($_POST['password'] ?? '') with the unmodified $_POST['password'] ?? '') while keeping $username = trim($_POST['username'] ?? '') as-is; ensure downstream code (e.g., authentication checks) expects the exact submitted string.video.php-9-22 (1)
9-22:⚠️ Potential issue | 🟡 MinorView count incremented before verifying video exists.
The view count and stats are updated (Lines 9-13) before checking if the video actually exists (Lines 15-22). This inflates metrics for non-existent video IDs and wastes database writes.
🔧 Proposed fix: verify video exists first
$id = (int) ($_GET['id'] ?? 0); if ($id <= 0) { header('Location: index.php'); exit; } -$update = $pdo->prepare('UPDATE videos SET views = views + 1 WHERE id = :id'); -$update->execute(['id' => $id]); - -$stats = $pdo->prepare('INSERT INTO video_stats (video_id, view_date, daily_views) VALUES (:video_id, CURDATE(), 1) ON DUPLICATE KEY UPDATE daily_views = daily_views + 1'); -$stats->execute(['video_id' => $id]); - $stmt = $pdo->prepare('SELECT * FROM videos WHERE id = :id LIMIT 1'); $stmt->execute(['id' => $id]); $video = $stmt->fetch(); if (!$video) { header('Location: index.php'); exit; } + +$update = $pdo->prepare('UPDATE videos SET views = views + 1 WHERE id = :id'); +$update->execute(['id' => $id]); + +$stats = $pdo->prepare('INSERT INTO video_stats (video_id, view_date, daily_views) VALUES (:video_id, CURDATE(), 1) ON DUPLICATE KEY UPDATE daily_views = daily_views + 1'); +$stats->execute(['video_id' => $id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@video.php` around lines 9 - 22, Move the SELECT that fetches the video (the $stmt prepare/execute and $video fetch) to run before any updates so you verify existence first; specifically, run the SELECT ('SELECT * FROM videos WHERE id = :id LIMIT 1') using $stmt and check !$video before executing the $update (UPDATE videos ... via $update) and $stats (INSERT INTO video_stats ... via $stats) statements—only run $update->execute(...) and $stats->execute(...) after the existence check passes.assets/js/main.js-22-28 (1)
22-28:⚠️ Potential issue | 🟡 MinorClose button does not allow graceful dismissal.
The popup's close button (
<button class="popup-close">) exists in HTML (index.php Line 138) but has no dedicated handler. Clicking anywhere on the popup, including the close button, triggers a redirect. Users expect the × button to close the popup without navigation.🔧 Proposed fix: allow close button to dismiss without redirect
popup.style.display = 'flex'; + const closeBtn = popup.querySelector('.popup-close'); + if (closeBtn) { + closeBtn.addEventListener('click', (e) => { + e.stopPropagation(); + localStorage.setItem('popupViewed', 'true'); + popup.style.display = 'none'; + }); + } + const redirect = () => { localStorage.setItem('popupViewed', 'true'); const randomLink = links[Math.floor(Math.random() * links.length)]; window.location.href = randomLink; }; - popup.addEventListener('click', redirect); + popup.querySelector('.popup-content img')?.addEventListener('click', redirect);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/main.js` around lines 22 - 28, The popup's current click handler (popup.addEventListener('click', redirect)) causes the redirect() function to run even when the close button (class "popup-close") is clicked; update the logic so the close button dismisses the popup without navigation: remove or change the global click listener to inspect the click event (or add a dedicated handler on the ".popup-close" element) and if the event target matches ".popup-close" then mark popupViewed and close/hide the popup (but do not call redirect()), otherwise run redirect(); ensure you reference the existing redirect() function and the ".popup-close" selector when implementing this conditional handling.index.php-9-14 (1)
9-14:⚠️ Potential issue | 🟡 MinorAdd LIMIT to search query to prevent unbounded results.
The default query (line 13) limits results to 24, but the search query (lines 10-11) has no limit. A broad search term could return thousands of rows, causing memory and performance issues.
Proposed fix
if ($search !== '') { - $stmt = $pdo->prepare('SELECT * FROM videos WHERE title LIKE :q ORDER BY created_at DESC'); + $stmt = $pdo->prepare('SELECT * FROM videos WHERE title LIKE :q ORDER BY created_at DESC LIMIT 100'); $stmt->execute(['q' => '%' . $search . '%']); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.php` around lines 9 - 14, The search branch currently prepares and executes a SELECT without a row limit, which can return unbounded results; update the SQL in the block using $pdo->prepare(...) where $stmt is created to include a LIMIT (e.g., LIMIT 24) and keep the existing ORDER BY created_at DESC and bound parameter $search, or make the limit a configurable variable (e.g., $limit) and bind/validate it before use so the search path matches the default query's result cap.admin/popup.php-5-16 (1)
5-16:⚠️ Potential issue | 🟡 MinorNo URL validation for image and link fields.
The form accepts any string for
image,link1, andlink2without validating they are valid URLs. This could lead to:
- Broken popup displays with invalid image paths
- Potential for
javascript:URLs in links (XSS when clicked)Consider adding URL validation
if ($_SERVER['REQUEST_METHOD'] === 'POST') { $image = trim($_POST['image'] ?? ''); $link1 = trim($_POST['link1'] ?? ''); $link2 = trim($_POST['link2'] ?? ''); + + // Basic URL validation + if ($image !== '' && !filter_var($image, FILTER_VALIDATE_URL)) { + // Handle error or continue with relative path + } + foreach ([$link1, $link2] as $link) { + if ($link !== '' && !preg_match('#^https?://#i', $link)) { + // Reject non-http(s) links to prevent javascript: URLs + } + } $links = implode(',', array_filter([$link1, $link2]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/popup.php` around lines 5 - 16, Validate and sanitize the submitted $image, $link1 and $link2 before inserting: use filter_var($value, FILTER_VALIDATE_URL) and/or parse_url to ensure the scheme is http or https (reject or clear values with non-http schemes like javascript:), ensure $image points to a valid image extension/content-type if required, and only then build $links (array_filter) and execute the prepared statement; update the POST handling in popup.php (variables $image, $link1, $link2, $links and the insert block) to perform these checks and return an error or skip invalid entries rather than inserting raw strings.
🧹 Nitpick comments (9)
admin/logout.php (1)
1-5: Session cleanup is incomplete.
session_destroy()destroys the session data on the server but doesn't clear the$_SESSIONsuperglobal or the session cookie. For a more secure logout, consider a complete cleanup.🔒 Proposed fix for complete session cleanup
<?php require_once __DIR__ . '/../config.php'; +$_SESSION = []; +if (ini_get('session.use_cookies')) { + $params = session_get_cookie_params(); + setcookie(session_name(), '', time() - 42000, + $params['path'], $params['domain'], + $params['secure'], $params['httponly'] + ); +} session_destroy(); header('Location: login.php'); exit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/logout.php` around lines 1 - 5, The logout flow currently only calls session_destroy(); enhance it to fully clear the session by unsetting the $_SESSION array, regenerating or clearing the session cookie (use session_name() and setcookie(...) with a past expiry to remove it), and then calling session_destroy() to remove server data; ensure this logic is placed in the logout script that currently uses session_destroy(), and keep the final header('Location: login.php') and exit to redirect after cleanup.sql/avstube.sql (1)
4-14: Consider adding an index ontitlefor search performance.The application performs
LIKE '%query%'searches on the title column. While a B-tree index won't help with leading wildcards, a full-text index could improve search performance as the video count grows.📈 Optional: Add full-text index for search
CREATE TABLE IF NOT EXISTS videos ( id INT AUTO_INCREMENT PRIMARY KEY, title VARCHAR(255) NOT NULL, description TEXT, thumbnail VARCHAR(255) NOT NULL, video_url VARCHAR(255) NOT NULL, type ENUM('embed', 'mp4') NOT NULL DEFAULT 'embed', duration VARCHAR(20) DEFAULT '00:00', views INT NOT NULL DEFAULT 0, - created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + FULLTEXT INDEX ft_title (title) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/avstube.sql` around lines 4 - 14, Add a FULLTEXT index on the videos.title column to improve search performance for text queries: modify the CREATE TABLE statement for videos (or run an ALTER TABLE videos ...) to add a FULLTEXT index on the title column; reference the table name "videos" and column "title" and ensure the index type is FULLTEXT so full-text searches can be used instead of/alongside LIKE '%query%'.search.php (1)
1-3: Functional but unconventional approach to query forwarding.Modifying the
$_GETsuperglobal directly works, but is unusual. Sinceindex.php(line 4) already performstrim($_GET['q'] ?? ''), the trim here is redundant. Consider passing the search query via a variable instead.♻️ Alternative: Use a shared variable
<?php -$_GET['q'] = trim($_GET['q'] ?? ''); +$search = trim($_GET['q'] ?? ''); require __DIR__ . '/index.php';Then modify
index.phpto check for$searchvariable first:$search = $search ?? trim($_GET['q'] ?? '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@search.php` around lines 1 - 3, Replace the direct mutation of $_GET['q'] in search.php with passing the trimmed query via a local variable: compute the trimmed value (trim($_GET['q'] ?? '')) and assign it to a variable named $search, then require index.php; update index.php to prefer an existing $search variable by using $search = $search ?? trim($_GET['q'] ?? ''); so index.php uses the forwarded $search when present and falls back to the original $_GET handling otherwise.admin/settings.php (1)
9-11:VALUES()function is deprecated in MySQL 8.0.20+.The
VALUES(setting_value)syntax inON DUPLICATE KEY UPDATEis deprecated. Consider updating to either an alias-based approach (MySQL recommended) or alternative parameter binding for forward compatibility.♻️ Proposed fix for MySQL 8.0.20+ compatibility
- $stmt = $pdo->prepare('INSERT INTO settings (setting_key, setting_value) VALUES (:k, :v) ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value)'); + $stmt = $pdo->prepare('INSERT INTO settings (setting_key, setting_value) VALUES (:k, :v) AS new ON DUPLICATE KEY UPDATE setting_value = new.setting_value'); $stmt->execute(['k' => $key, 'v' => $value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/settings.php` around lines 9 - 11, The query uses the deprecated VALUES() in the ON DUPLICATE KEY UPDATE clause; update the prepared statement in settings.php (the $stmt = $pdo->prepare(...) call) to use a row alias and reference that alias instead: change the SQL to "INSERT INTO settings (setting_key, setting_value) VALUES (:k, :v) AS new ON DUPLICATE KEY UPDATE setting_value = new.setting_value" and keep the same $stmt->execute(['k'=>$key,'v'=>$value]) usage so the duplicate-key branch uses the aliased value instead of VALUES().README.md (1)
47-51: Default credentials documented - ensure they're changed in production.The default admin credentials (
admin/admin2006) are documented here. This is fine for development, but ensure documentation or setup process reminds users to change these credentials before deploying to production.Consider adding a note like:
## 3) Tài khoản admin mặc định - Username: `admin` - Password: `admin2006` - URL: `/admin/login.php` + +⚠️ **Lưu ý:** Đổi mật khẩu admin ngay sau khi cài đặt trên môi trường production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 47 - 51, Add a clear production-warning next to the documented default admin credentials (the lines showing "Username: `admin`", "Password: `admin2006`", and "URL: `/admin/login.php`") instructing users to change the admin username/password before deploying to production and recommending creating a secure password or disabling the default account; update the README entry to include this brief note and, if applicable, a pointer to setup steps or a script to rotate the default credentials.admin/login.php (1)
10-25: Consider adding brute force protection.The login endpoint lacks rate limiting, allowing unlimited authentication attempts. This makes the application vulnerable to credential stuffing and brute force attacks.
Consider implementing:
- Account lockout after N failed attempts
- Progressive delays between attempts
- CAPTCHA after several failures
- IP-based rate limiting
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/login.php` around lines 10 - 25, The login flow in admin/login.php currently allows unlimited attempts; add brute-force protection by tracking failed attempts and enforcing lockouts/delays: before verifying credentials check a lockout field (e.g., users.failed_attempts, users.lockout_until or a separate login_attempts table keyed by username or IP from $_SERVER['REMOTE_ADDR']), reject attempts while lockout_until > now with a clear error; on failed auth (when $user is null or password_verify fails) increment failed_attempts and set lockout_until after N failures (and optionally record attempt timestamp for progressive backoff), on successful auth reset failed_attempts/lockout_until and proceed to set $_SESSION['admin_id'] and redirect; additionally add optional progressive sleep/delay or CAPTCHA trigger after M failures and consider IP-based rate limiting for anonymous usernames.admin/partials.php (1)
6-7: Consider adding Subresource Integrity (SRI) for CDN assets.External CDN resources load without integrity checks. Adding SRI hashes helps protect against compromised CDNs serving malicious code.
- echo '<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css" rel="stylesheet">'; - echo '<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.5.0/css/all.min.css">'; + echo '<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-QWTKZyjpPEjISv5WaRU9OFeRpok6YctnYmDr5pNlyT2bRjXh0JMhjY6hW+ALEwIH" crossorigin="anonymous">'; + echo '<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.5.0/css/all.min.css" integrity="sha512-Avb2QiuDEEvB4bZJYdft2mNjVShBftLdPG8FJ0V7irTLQ8Uo16qPpBQtBiM1XKTK5JA2j5sU5HKR3aUEsWPG3g==" crossorigin="anonymous">';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/partials.php` around lines 6 - 7, The two echo statements output Bootstrap and Font Awesome CDN links without Subresource Integrity; update the echo lines that output 'https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css' and 'https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.5.0/css/all.min.css' to include the correct integrity="sha384-..." SRI hashes for those exact versions and add crossorigin="anonymous" attributes (ensure the integrity values match the files served by those exact CDN URLs). Preserve the existing URLs and quoting style, and verify the hashes against the CDN or by computing the SHA384 base64 hashes before committing.assets/css/style.css (1)
162-174: Missing focus styles for popup close button.The
.popup-closebutton lacks:focusstyles, which is important for keyboard accessibility. Users navigating with keyboard need visible focus indication.Add focus styles
.popup-close { position: absolute; top: -12px; right: -12px; width: 36px; height: 36px; border-radius: 50%; border: none; background: `#c00`; color: `#fff`; font-size: 20px; line-height: 1; + cursor: pointer; +} + +.popup-close:focus { + outline: 2px solid `#fff`; + outline-offset: 2px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/css/style.css` around lines 162 - 174, The .popup-close button lacks keyboard focus styles; add a visible, high-contrast focus state (use :focus and preferably :focus-visible) for the .popup-close selector that preserves its circular shape (border-radius) and contrast—e.g., apply a clear outline or box-shadow, set outline-offset to avoid clipping, and ensure color/contrast remains accessible when focused; update the CSS rules for .popup-close:focus and .popup-close:focus-visible accordingly so keyboard users can see when the close button is focused.admin/popup.php (1)
11-13: Non-atomic DELETE + INSERT could leave ads in inconsistent state.If the INSERT fails after DELETE succeeds, the popup ad is lost. Consider wrapping in a transaction.
Proposed transaction wrapper
+ $pdo->beginTransaction(); + try { $pdo->exec("DELETE FROM ads WHERE type='popup'"); $stmt = $pdo->prepare("INSERT INTO ads (type,image,link,status) VALUES ('popup',:image,:link,1)"); $stmt->execute(['image' => $image, 'link' => $links]); + $pdo->commit(); + } catch (Exception $e) { + $pdo->rollBack(); + throw $e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/popup.php` around lines 11 - 13, The DELETE followed by INSERT is not atomic and can leave ads blank if the INSERT fails; wrap the operations using $pdo->beginTransaction(), perform $pdo->exec("DELETE FROM ads WHERE type='popup'") and the prepared INSERT ($pdo->prepare(...) / $stmt->execute(...)) inside a try block, then call $pdo->commit() on success and $pdo->rollBack() in the catch to restore state; ensure exceptions from prepare/execute are propagated or logged so failures are visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/ads.php`:
- Around line 5-15: Change the state-changing GET handlers for 'toggle' and
'delete' to accept POST and validate a CSRF token: stop reading $_GET['toggle']
and $_GET['delete'], read $_POST['toggle'] and $_POST['delete'] instead, and
before executing the prepared statements (the UPDATE in prepare('UPDATE ads SET
status = IF(status=1,0,1) WHERE id=:id') and the DELETE in prepare('DELETE FROM
ads WHERE id=:id')), call your CSRF validation helper (e.g.,
validate_csrf_token($_POST['csrf_token']) or the same token check used in
admin/announcements.php), abort with a redirect/error if the token is invalid,
and update the UI forms that trigger these actions to use method="POST" and
include the csrf_token hidden field.
- Around line 16-23: The POST handler currently allows creating ads without CSRF
validation and accepts arbitrary image URLs; add a CSRF token check at the start
of the POST branch (verify a submitted token against the session token) and
abort if missing/invalid, then validate/sanitize the input values before calling
$pdo->prepare(...)->execute(...): use the same in_array(...) logic for $type,
trim inputs, validate the $link and $image with filter_var(...,
FILTER_VALIDATE_URL) and enforce allowed schemes (only http and https) for
banner ads (reject or normalize javascript: and other schemes), and ensure you
escape or canonicalize values as needed before inserting with the existing
prepared statement to prevent storing malicious URLs.
In `@admin/announcements.php`:
- Around line 10-18: Add CSRF token generation/validation: ensure a CSRF token
is stored in $_SESSION['csrf_token'] and add a hidden form field named
"csrf_token" (use the existing e() helper when rendering) in the announcement
creation form; then, in the POST handling branch (where
$_SERVER['REQUEST_METHOD'] === 'POST' and before inserting via
$pdo->prepare(...)->execute(...)), validate that $_POST['csrf_token'] exists and
exactly matches $_SESSION['csrf_token'], and if it fails, abort (e.g., redirect
or show error) instead of executing the DB insert to prevent CSRF.
- Around line 5-9: The delete flow currently uses $_GET['delete'] and executes a
state-changing DELETE via GET; change this to accept only POST by checking
$_SERVER['REQUEST_METHOD'] === 'POST' and isset($_POST['delete']), validate a
CSRF token from $_POST['csrf_token'] against $_SESSION['csrf_token'], then
perform the $pdo->prepare('DELETE FROM announcements WHERE
id=:id')->execute(['id' => (int) $_POST['delete']]) and redirect as before; also
update the UI so the delete button submits a POST form including the hidden
csrf_token and delete id.
In `@admin/settings.php`:
- Around line 5-15: Add CSRF token generation and validation around the POST
handling in admin/settings.php: when rendering the settings form generate/store
a token in session (e.g. $_SESSION['csrf_token']), include it as a hidden field
in the form, and in the POST branch (the block starting with if
($_SERVER['REQUEST_METHOD'] === 'POST') that reads $logo/$banner and updates the
DB) validate the submitted token exists and matches the session token using a
timing-safe comparison (e.g. hash_equals); if the token is missing/invalid, stop
processing (return a 400/403 or redirect) before executing the DB updates and
header redirect. Ensure the session is started when generating/validating the
token and optionally regenerate the token after successful submission.
In `@admin/videos.php`:
- Around line 5-11: The delete flow in videos.php currently reads the id from
$_GET['delete'] and executes a DELETE (vulnerable to CSRF); change it to accept
only POST and validate a CSRF token: stop using $_GET['delete'], read and cast
$_POST['delete'] inside the same handler that currently prepares and executes
the DELETE (the prepare/execute block), verify a server-side CSRF token (e.g.,
compare $_POST['csrf_token'] to the session token) before calling
$stmt->execute(['id' => $id]), and keep the redirect (header('Location:
videos.php')) after successful deletion; update the UI delete control to submit
a POST form with the hidden delete id and csrf_token and an onsubmit confirm
dialog.
- Around line 22-40: The upload handlers for MP4 and thumbnail rely only on file
extension and call move_uploaded_file without checks; update the logic around
the $_FILES['mp4_file'] and $_FILES['thumbnail_file'] handling to: validate MIME
types using finfo_file (e.g., accept video/mp4 for mp4 and
image/jpeg,image/png,image/webp for thumbnails), enforce a maximum file size
check against $_FILES['...']['size'], verify/create target directories (check
is_dir and mkdir with proper permissions) before moving, and check the return
value of move_uploaded_file to handle/log failures and avoid setting $videoUrl
or $thumbnail on failure; ensure you still generate unique names
(uniqid('video_', true) / uniqid('thumb_', true)) and only set those variables
after successful validation and move.
In `@comment.php`:
- Around line 4-18: Add server-side CSRF validation before processing the POST
in comment submission: implement and use a verifyCsrf() helper (and csrfToken()
to generate tokens) and call verifyCsrf() at the top of the request handling
block that reads $videoId/$username/$comment; if verifyCsrf() returns false,
abort the insert (return error or 403) and do not execute the
$stmt->execute(...). Ensure the comment form includes the csrf_token generated
by csrfToken() so the posted csrf_token is validated by verifyCsrf().
In `@config.php`:
- Around line 4-8: The constants DB_HOST, DB_NAME, DB_USER, DB_PASS (and
BASE_URL) are hardcoded which leaks credentials and hinders deployment; update
config.php to read these values from environment variables (e.g., via getenv or
a dotenv loader) with sensible fallbacks and remove plaintext secrets from
source; ensure the app gracefully errors if required vars are missing, document
required env names, and add a .env.example (but keep .env out of version
control) so deployments supply real credentials securely.
- Around line 21-22: In the PDOException catch block replace the current
die('Database connection failed: ' . htmlspecialchars($e->getMessage())) usage
with logic that logs the full exception server-side (e.g., call
error_log($e->getMessage()) or use your app logger with $e) and returns a
generic, non-sensitive error to the user (e.g., die('Database connection failed.
Please contact support.')); locate this change in the catch (PDOException $e)
block and update the die call to avoid echoing $e->getMessage() to users while
ensuring the exception is recorded in server logs.
- Around line 35-41: The redirect in adminOnly() uses a relative path which
breaks when config.php is included from other directories; change the redirect
to an absolute URL using the application's base URL constant (e.g., BASE_URL or
similar) so the header call always points to the correct login page (e.g.,
header('Location: ' . BASE_URL . '/login.php')), ensure BASE_URL is
defined/available before use, and keep the exit after the header; update the
adminOnly() function to use that absolute path.
In `@index.php`:
- Around line 95-97: The Google ad HTML stored in $googleCodes and emitted via
$code['image'] is being rendered raw and creates an XSS vector; change the
rendering in the foreach that outputs $code['image'] to either escape it (e.g.,
HTML-encode with htmlspecialchars/ENT_QUOTES/UTF-8) when you only expect plain
text, or pass it through a safe HTML sanitizer/allowlist (e.g., HTMLPurifier or
strip_tags with a strict allowed-tags list) if legitimate embed HTML must be
preserved; also add validation/sanitization when admins save this field and
document that only trusted admins should manage ad/embed content.
In `@sql/avstube.sql`:
- Around line 25-30: The users table currently sets role VARCHAR(20) NOT NULL
DEFAULT 'admin', which makes new accounts admins by default; change the default
to a non-privileged role (e.g., DEFAULT 'user') by updating the CREATE TABLE
statement (users, role column) and add a migration/ALTER TABLE to change
existing schema/defaults and any seed data or tests that assume 'admin' as the
default so they use 'user' or explicitly set role when creating admin accounts.
In `@video.php`:
- Around line 62-67: The form in video.php is missing CSRF protection; generate
a cryptographically secure token (e.g., store a random token in
$_SESSION['csrf_token'] when rendering the page), add a hidden input named
csrf_token to the existing form (alongside video_id, username, comment), and
then in comment.php validate that the submitted csrf_token matches the session
token (and reject the request if missing/invalid), rotating or expiring the
token after successful use to prevent reuse.
- Around line 49-53: The iframe src is using unvalidated $video['video_url'],
risking javascript: XSS; add a helper safeVideoUrl(string $url) (e.g. in
config.php) that parse_url()s the value, ensures scheme is 'http' or 'https'
(and optionally checks a domain allowlist) and returns an empty string on
failure, then update the video.php template to call
safeVideoUrl($video['video_url']) before outputting (still wrap with e()) and
only render the iframe when the helper returns a non-empty string (otherwise
render a safe fallback or omit the iframe); also apply the same safeVideoUrl
check for the <video> src to be consistent.
---
Minor comments:
In `@admin/login.php`:
- Around line 11-12: The password is being altered by trim() which can reject
valid passwords containing leading/trailing whitespace; stop trimming the
password and use the raw POST value for authentication instead. Specifically,
change how $password is assigned in admin/login.php (replace
trim($_POST['password'] ?? '') with the unmodified $_POST['password'] ?? '')
while keeping $username = trim($_POST['username'] ?? '') as-is; ensure
downstream code (e.g., authentication checks) expects the exact submitted
string.
In `@admin/popup.php`:
- Around line 5-16: Validate and sanitize the submitted $image, $link1 and
$link2 before inserting: use filter_var($value, FILTER_VALIDATE_URL) and/or
parse_url to ensure the scheme is http or https (reject or clear values with
non-http schemes like javascript:), ensure $image points to a valid image
extension/content-type if required, and only then build $links (array_filter)
and execute the prepared statement; update the POST handling in popup.php
(variables $image, $link1, $link2, $links and the insert block) to perform these
checks and return an error or skip invalid entries rather than inserting raw
strings.
In `@assets/js/main.js`:
- Around line 22-28: The popup's current click handler
(popup.addEventListener('click', redirect)) causes the redirect() function to
run even when the close button (class "popup-close") is clicked; update the
logic so the close button dismisses the popup without navigation: remove or
change the global click listener to inspect the click event (or add a dedicated
handler on the ".popup-close" element) and if the event target matches
".popup-close" then mark popupViewed and close/hide the popup (but do not call
redirect()), otherwise run redirect(); ensure you reference the existing
redirect() function and the ".popup-close" selector when implementing this
conditional handling.
In `@comment.php`:
- Around line 20-21: The redirect is happening unconditionally with
header('Location: video.php?id=' . $videoId); exit;, causing navigation to
video.php?id=0 when $videoId is invalid; update the logic to validate $videoId
first (ensure it is a positive integer and corresponds to an existing video) and
only perform the header redirect when validation succeeds, otherwise return an
error response or redirect to a safe fallback (e.g., an error page or list view)
and provide user-friendly feedback instead of redirecting to id=0.
In `@index.php`:
- Around line 9-14: The search branch currently prepares and executes a SELECT
without a row limit, which can return unbounded results; update the SQL in the
block using $pdo->prepare(...) where $stmt is created to include a LIMIT (e.g.,
LIMIT 24) and keep the existing ORDER BY created_at DESC and bound parameter
$search, or make the limit a configurable variable (e.g., $limit) and
bind/validate it before use so the search path matches the default query's
result cap.
In `@video.php`:
- Around line 9-22: Move the SELECT that fetches the video (the $stmt
prepare/execute and $video fetch) to run before any updates so you verify
existence first; specifically, run the SELECT ('SELECT * FROM videos WHERE id =
:id LIMIT 1') using $stmt and check !$video before executing the $update (UPDATE
videos ... via $update) and $stats (INSERT INTO video_stats ... via $stats)
statements—only run $update->execute(...) and $stats->execute(...) after the
existence check passes.
---
Nitpick comments:
In `@admin/login.php`:
- Around line 10-25: The login flow in admin/login.php currently allows
unlimited attempts; add brute-force protection by tracking failed attempts and
enforcing lockouts/delays: before verifying credentials check a lockout field
(e.g., users.failed_attempts, users.lockout_until or a separate login_attempts
table keyed by username or IP from $_SERVER['REMOTE_ADDR']), reject attempts
while lockout_until > now with a clear error; on failed auth (when $user is null
or password_verify fails) increment failed_attempts and set lockout_until after
N failures (and optionally record attempt timestamp for progressive backoff), on
successful auth reset failed_attempts/lockout_until and proceed to set
$_SESSION['admin_id'] and redirect; additionally add optional progressive
sleep/delay or CAPTCHA trigger after M failures and consider IP-based rate
limiting for anonymous usernames.
In `@admin/logout.php`:
- Around line 1-5: The logout flow currently only calls session_destroy();
enhance it to fully clear the session by unsetting the $_SESSION array,
regenerating or clearing the session cookie (use session_name() and
setcookie(...) with a past expiry to remove it), and then calling
session_destroy() to remove server data; ensure this logic is placed in the
logout script that currently uses session_destroy(), and keep the final
header('Location: login.php') and exit to redirect after cleanup.
In `@admin/partials.php`:
- Around line 6-7: The two echo statements output Bootstrap and Font Awesome CDN
links without Subresource Integrity; update the echo lines that output
'https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css' and
'https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.5.0/css/all.min.css' to
include the correct integrity="sha384-..." SRI hashes for those exact versions
and add crossorigin="anonymous" attributes (ensure the integrity values match
the files served by those exact CDN URLs). Preserve the existing URLs and
quoting style, and verify the hashes against the CDN or by computing the SHA384
base64 hashes before committing.
In `@admin/popup.php`:
- Around line 11-13: The DELETE followed by INSERT is not atomic and can leave
ads blank if the INSERT fails; wrap the operations using
$pdo->beginTransaction(), perform $pdo->exec("DELETE FROM ads WHERE
type='popup'") and the prepared INSERT ($pdo->prepare(...) /
$stmt->execute(...)) inside a try block, then call $pdo->commit() on success and
$pdo->rollBack() in the catch to restore state; ensure exceptions from
prepare/execute are propagated or logged so failures are visible.
In `@admin/settings.php`:
- Around line 9-11: The query uses the deprecated VALUES() in the ON DUPLICATE
KEY UPDATE clause; update the prepared statement in settings.php (the $stmt =
$pdo->prepare(...) call) to use a row alias and reference that alias instead:
change the SQL to "INSERT INTO settings (setting_key, setting_value) VALUES (:k,
:v) AS new ON DUPLICATE KEY UPDATE setting_value = new.setting_value" and keep
the same $stmt->execute(['k'=>$key,'v'=>$value]) usage so the duplicate-key
branch uses the aliased value instead of VALUES().
In `@assets/css/style.css`:
- Around line 162-174: The .popup-close button lacks keyboard focus styles; add
a visible, high-contrast focus state (use :focus and preferably :focus-visible)
for the .popup-close selector that preserves its circular shape (border-radius)
and contrast—e.g., apply a clear outline or box-shadow, set outline-offset to
avoid clipping, and ensure color/contrast remains accessible when focused;
update the CSS rules for .popup-close:focus and .popup-close:focus-visible
accordingly so keyboard users can see when the close button is focused.
In `@README.md`:
- Around line 47-51: Add a clear production-warning next to the documented
default admin credentials (the lines showing "Username: `admin`", "Password:
`admin2006`", and "URL: `/admin/login.php`") instructing users to change the
admin username/password before deploying to production and recommending creating
a secure password or disabling the default account; update the README entry to
include this brief note and, if applicable, a pointer to setup steps or a script
to rotate the default credentials.
In `@search.php`:
- Around line 1-3: Replace the direct mutation of $_GET['q'] in search.php with
passing the trimmed query via a local variable: compute the trimmed value
(trim($_GET['q'] ?? '')) and assign it to a variable named $search, then require
index.php; update index.php to prefer an existing $search variable by using
$search = $search ?? trim($_GET['q'] ?? ''); so index.php uses the forwarded
$search when present and falls back to the original $_GET handling otherwise.
In `@sql/avstube.sql`:
- Around line 4-14: Add a FULLTEXT index on the videos.title column to improve
search performance for text queries: modify the CREATE TABLE statement for
videos (or run an ALTER TABLE videos ...) to add a FULLTEXT index on the title
column; reference the table name "videos" and column "title" and ensure the
index type is FULLTEXT so full-text searches can be used instead of/alongside
LIKE '%query%'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f9782b-da0b-40b8-8693-09e1f0a57560
📒 Files selected for processing (20)
README.mdadmin/ads.phpadmin/announcements.phpadmin/auth.phpadmin/dashboard.phpadmin/index.phpadmin/login.phpadmin/logout.phpadmin/partials.phpadmin/popup.phpadmin/settings.phpadmin/videos.phpassets/css/style.cssassets/js/main.jscomment.phpconfig.phpindex.phpsearch.phpsql/avstube.sqlvideo.php
Comment on lines
+5
to
+15
| if (isset($_GET['toggle'])) { | ||
| $id = (int) $_GET['toggle']; | ||
| $pdo->prepare('UPDATE ads SET status = IF(status=1,0,1) WHERE id=:id')->execute(['id' => $id]); | ||
| header('Location: ads.php'); | ||
| exit; | ||
| } | ||
| if (isset($_GET['delete'])) { | ||
| $pdo->prepare('DELETE FROM ads WHERE id=:id')->execute(['id' => (int) $_GET['delete']]); | ||
| header('Location: ads.php'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
CSRF vulnerability: state-changing operations via GET requests.
Both toggle and delete actions use GET requests without CSRF protection. These can be exploited via image tags, links, or other cross-site vectors to modify ad state without authorization.
Convert these to POST requests with CSRF token validation, similar to the fix suggested for admin/announcements.php.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/ads.php` around lines 5 - 15, Change the state-changing GET handlers
for 'toggle' and 'delete' to accept POST and validate a CSRF token: stop reading
$_GET['toggle'] and $_GET['delete'], read $_POST['toggle'] and $_POST['delete']
instead, and before executing the prepared statements (the UPDATE in
prepare('UPDATE ads SET status = IF(status=1,0,1) WHERE id=:id') and the DELETE
in prepare('DELETE FROM ads WHERE id=:id')), call your CSRF validation helper
(e.g., validate_csrf_token($_POST['csrf_token']) or the same token check used in
admin/announcements.php), abort with a redirect/error if the token is invalid,
and update the UI forms that trigger these actions to use method="POST" and
include the csrf_token hidden field.
Comment on lines
+16
to
+23
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | ||
| $type = in_array($_POST['type'] ?? '', ['banner', 'google'], true) ? $_POST['type'] : 'banner'; | ||
| $image = trim($_POST['image'] ?? ''); | ||
| $link = trim($_POST['link'] ?? ''); | ||
| $pdo->prepare('INSERT INTO ads (type,image,link,status) VALUES (:type,:image,:link,1)')->execute(compact('type', 'image', 'link')); | ||
| header('Location: ads.php'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Missing CSRF protection and URL validation on ad creation.
- The POST handler lacks CSRF token validation.
- The
imagefield accepts any input without validation. For banner ads, this could allow storing malicious URLs (e.g.,javascript:schemes) that would be rendered elsewhere.
🔒 Proposed fix
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
+ if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== ($_SESSION['csrf_token'] ?? '')) {
+ http_response_code(403);
+ exit('Invalid CSRF token');
+ }
$type = in_array($_POST['type'] ?? '', ['banner', 'google'], true) ? $_POST['type'] : 'banner';
$image = trim($_POST['image'] ?? '');
$link = trim($_POST['link'] ?? '');
+
+ // Validate URLs for banner type
+ if ($type === 'banner') {
+ if (!filter_var($image, FILTER_VALIDATE_URL) || !preg_match('/^https?:\/\//', $image)) {
+ $image = '';
+ }
+ if ($link && (!filter_var($link, FILTER_VALIDATE_URL) || !preg_match('/^https?:\/\//', $link))) {
+ $link = '';
+ }
+ }
+
$pdo->prepare('INSERT INTO ads (type,image,link,status) VALUES (:type,:image,:link,1)')->execute(compact('type', 'image', 'link'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | |
| $type = in_array($_POST['type'] ?? '', ['banner', 'google'], true) ? $_POST['type'] : 'banner'; | |
| $image = trim($_POST['image'] ?? ''); | |
| $link = trim($_POST['link'] ?? ''); | |
| $pdo->prepare('INSERT INTO ads (type,image,link,status) VALUES (:type,:image,:link,1)')->execute(compact('type', 'image', 'link')); | |
| header('Location: ads.php'); | |
| exit; | |
| } | |
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | |
| if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== ($_SESSION['csrf_token'] ?? '')) { | |
| http_response_code(403); | |
| exit('Invalid CSRF token'); | |
| } | |
| $type = in_array($_POST['type'] ?? '', ['banner', 'google'], true) ? $_POST['type'] : 'banner'; | |
| $image = trim($_POST['image'] ?? ''); | |
| $link = trim($_POST['link'] ?? ''); | |
| // Validate URLs for banner type | |
| if ($type === 'banner') { | |
| if (!filter_var($image, FILTER_VALIDATE_URL) || !preg_match('/^https?:\/\//', $image)) { | |
| $image = ''; | |
| } | |
| if ($link && (!filter_var($link, FILTER_VALIDATE_URL) || !preg_match('/^https?:\/\//', $link))) { | |
| $link = ''; | |
| } | |
| } | |
| $pdo->prepare('INSERT INTO ads (type,image,link,status) VALUES (:type,:image,:link,1)')->execute(compact('type', 'image', 'link')); | |
| header('Location: ads.php'); | |
| exit; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/ads.php` around lines 16 - 23, The POST handler currently allows
creating ads without CSRF validation and accepts arbitrary image URLs; add a
CSRF token check at the start of the POST branch (verify a submitted token
against the session token) and abort if missing/invalid, then validate/sanitize
the input values before calling $pdo->prepare(...)->execute(...): use the same
in_array(...) logic for $type, trim inputs, validate the $link and $image with
filter_var(..., FILTER_VALIDATE_URL) and enforce allowed schemes (only http and
https) for banner ads (reject or normalize javascript: and other schemes), and
ensure you escape or canonicalize values as needed before inserting with the
existing prepared statement to prevent storing malicious URLs.
Comment on lines
+5
to
+9
| if (isset($_GET['delete'])) { | ||
| $pdo->prepare('DELETE FROM announcements WHERE id=:id')->execute(['id' => (int) $_GET['delete']]); | ||
| header('Location: announcements.php'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
CSRF vulnerability: state-changing DELETE via GET request.
The delete action is triggered via a GET request, which is vulnerable to CSRF attacks (e.g., an attacker can embed <img src="announcements.php?delete=1"> to delete announcements). State-changing operations should use POST requests with CSRF token validation.
🔒 Proposed fix: use POST with CSRF token
-if (isset($_GET['delete'])) {
- $pdo->prepare('DELETE FROM announcements WHERE id=:id')->execute(['id' => (int) $_GET['delete']]);
- header('Location: announcements.php');
- exit;
-}
+if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['delete'])) {
+ if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== ($_SESSION['csrf_token'] ?? '')) {
+ http_response_code(403);
+ exit('Invalid CSRF token');
+ }
+ $pdo->prepare('DELETE FROM announcements WHERE id=:id')->execute(['id' => (int) $_POST['delete']]);
+ header('Location: announcements.php');
+ exit;
+}Then update the delete button in the table to use a form:
<form method="post" style="display:inline">
<input type="hidden" name="csrf_token" value="<?= e($_SESSION['csrf_token'] ?? '') ?>">
<input type="hidden" name="delete" value="<?= (int)$r['id'] ?>">
<button class="btn btn-sm btn-danger">Delete</button>
</form>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isset($_GET['delete'])) { | |
| $pdo->prepare('DELETE FROM announcements WHERE id=:id')->execute(['id' => (int) $_GET['delete']]); | |
| header('Location: announcements.php'); | |
| exit; | |
| } | |
| if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['delete'])) { | |
| if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== ($_SESSION['csrf_token'] ?? '')) { | |
| http_response_code(403); | |
| exit('Invalid CSRF token'); | |
| } | |
| $pdo->prepare('DELETE FROM announcements WHERE id=:id')->execute(['id' => (int) $_POST['delete']]); | |
| header('Location: announcements.php'); | |
| exit; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/announcements.php` around lines 5 - 9, The delete flow currently uses
$_GET['delete'] and executes a state-changing DELETE via GET; change this to
accept only POST by checking $_SERVER['REQUEST_METHOD'] === 'POST' and
isset($_POST['delete']), validate a CSRF token from $_POST['csrf_token'] against
$_SESSION['csrf_token'], then perform the $pdo->prepare('DELETE FROM
announcements WHERE id=:id')->execute(['id' => (int) $_POST['delete']]) and
redirect as before; also update the UI so the delete button submits a POST form
including the hidden csrf_token and delete id.
Comment on lines
+10
to
+18
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | ||
| $title = trim($_POST['title'] ?? ''); | ||
| $content = trim($_POST['content'] ?? ''); | ||
| if ($title !== '' && $content !== '') { | ||
| $pdo->prepare('INSERT INTO announcements (title, content) VALUES (:title, :content)')->execute(compact('title', 'content')); | ||
| } | ||
| header('Location: announcements.php'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Missing CSRF protection on POST form submission.
The announcement creation form lacks CSRF token validation, making it vulnerable to cross-site request forgery attacks.
🔒 Proposed fix: add CSRF token validation
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
+ if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== ($_SESSION['csrf_token'] ?? '')) {
+ http_response_code(403);
+ exit('Invalid CSRF token');
+ }
$title = trim($_POST['title'] ?? '');
$content = trim($_POST['content'] ?? '');And add the token field to the form in Line 23:
<input type="hidden" name="csrf_token" value="<?= e($_SESSION['csrf_token'] ?? '') ?>">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/announcements.php` around lines 10 - 18, Add CSRF token
generation/validation: ensure a CSRF token is stored in $_SESSION['csrf_token']
and add a hidden form field named "csrf_token" (use the existing e() helper when
rendering) in the announcement creation form; then, in the POST handling branch
(where $_SERVER['REQUEST_METHOD'] === 'POST' and before inserting via
$pdo->prepare(...)->execute(...)), validate that $_POST['csrf_token'] exists and
exactly matches $_SESSION['csrf_token'], and if it fails, abort (e.g., redirect
or show error) instead of executing the DB insert to prevent CSRF.
Comment on lines
+5
to
+15
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | ||
| $logo = trim($_POST['site_logo'] ?? ''); | ||
| $banner = trim($_POST['site_banner'] ?? ''); | ||
|
|
||
| foreach (['site_logo' => $logo, 'site_banner' => $banner] as $key => $value) { | ||
| $stmt = $pdo->prepare('INSERT INTO settings (setting_key, setting_value) VALUES (:k, :v) ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value)'); | ||
| $stmt->execute(['k' => $key, 'v' => $value]); | ||
| } | ||
| header('Location: settings.php?saved=1'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Missing CSRF protection on settings form.
Admin forms should include CSRF token validation to prevent cross-site request forgery attacks, especially for state-changing operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/settings.php` around lines 5 - 15, Add CSRF token generation and
validation around the POST handling in admin/settings.php: when rendering the
settings form generate/store a token in session (e.g. $_SESSION['csrf_token']),
include it as a hidden field in the form, and in the POST branch (the block
starting with if ($_SERVER['REQUEST_METHOD'] === 'POST') that reads
$logo/$banner and updates the DB) validate the submitted token exists and
matches the session token using a timing-safe comparison (e.g. hash_equals); if
the token is missing/invalid, stop processing (return a 400/403 or redirect)
before executing the DB updates and header redirect. Ensure the session is
started when generating/validating the token and optionally regenerate the token
after successful submission.
Comment on lines
+35
to
+41
| function adminOnly(): void | ||
| { | ||
| if (!isAdmin()) { | ||
| header('Location: login.php'); | ||
| exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Relative redirect path in adminOnly() may fail when included from non-admin directories.
The adminOnly() function redirects to login.php using a relative path. When config.php is included from files outside the admin/ directory (e.g., index.php, video.php), this redirect will point to the wrong location.
🐛 Proposed fix using BASE_URL
function adminOnly(): void
{
if (!isAdmin()) {
- header('Location: login.php');
+ header('Location: ' . BASE_URL . '/admin/login.php');
exit;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.php` around lines 35 - 41, The redirect in adminOnly() uses a relative
path which breaks when config.php is included from other directories; change the
redirect to an absolute URL using the application's base URL constant (e.g.,
BASE_URL or similar) so the header call always points to the correct login page
(e.g., header('Location: ' . BASE_URL . '/login.php')), ensure BASE_URL is
defined/available before use, and keep the exit after the header; update the
adminOnly() function to use that absolute path.
Comment on lines
+95
to
+97
| <?php foreach ($googleCodes as $code): ?> | ||
| <div class="panel-dark p-3 mb-4"><?= $code['image'] ?></div> | ||
| <?php endforeach; ?> |
There was a problem hiding this comment.
XSS vulnerability: Google ad code rendered without escaping.
Line 96 outputs $code['image'] directly without escaping. While this field is named "image", for Google ad type it stores HTML/JS embed code from the database. If an attacker gains admin access or the database is compromised, this allows arbitrary script injection.
Consider sanitizing or validating this content, or at minimum documenting that only trusted admins should manage this field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.php` around lines 95 - 97, The Google ad HTML stored in $googleCodes
and emitted via $code['image'] is being rendered raw and creates an XSS vector;
change the rendering in the foreach that outputs $code['image'] to either escape
it (e.g., HTML-encode with htmlspecialchars/ENT_QUOTES/UTF-8) when you only
expect plain text, or pass it through a safe HTML sanitizer/allowlist (e.g.,
HTMLPurifier or strip_tags with a strict allowed-tags list) if legitimate embed
HTML must be preserved; also add validation/sanitization when admins save this
field and document that only trusted admins should manage ad/embed content.
Comment on lines
+25
to
+30
| CREATE TABLE IF NOT EXISTS users ( | ||
| id INT AUTO_INCREMENT PRIMARY KEY, | ||
| username VARCHAR(100) UNIQUE NOT NULL, | ||
| password VARCHAR(255) NOT NULL, | ||
| role VARCHAR(20) NOT NULL DEFAULT 'admin' | ||
| ); |
There was a problem hiding this comment.
Default role of 'admin' is dangerous for user registration.
If user registration is added in the future, new users would automatically become admins. The default should be a non-privileged role like 'user'.
🔒 Proposed fix
CREATE TABLE IF NOT EXISTS users (
id INT AUTO_INCREMENT PRIMARY KEY,
username VARCHAR(100) UNIQUE NOT NULL,
password VARCHAR(255) NOT NULL,
- role VARCHAR(20) NOT NULL DEFAULT 'admin'
+ role VARCHAR(20) NOT NULL DEFAULT 'user'
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE IF NOT EXISTS users ( | |
| id INT AUTO_INCREMENT PRIMARY KEY, | |
| username VARCHAR(100) UNIQUE NOT NULL, | |
| password VARCHAR(255) NOT NULL, | |
| role VARCHAR(20) NOT NULL DEFAULT 'admin' | |
| ); | |
| CREATE TABLE IF NOT EXISTS users ( | |
| id INT AUTO_INCREMENT PRIMARY KEY, | |
| username VARCHAR(100) UNIQUE NOT NULL, | |
| password VARCHAR(255) NOT NULL, | |
| role VARCHAR(20) NOT NULL DEFAULT 'user' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/avstube.sql` around lines 25 - 30, The users table currently sets role
VARCHAR(20) NOT NULL DEFAULT 'admin', which makes new accounts admins by
default; change the default to a non-privileged role (e.g., DEFAULT 'user') by
updating the CREATE TABLE statement (users, role column) and add a
migration/ALTER TABLE to change existing schema/defaults and any seed data or
tests that assume 'admin' as the default so they use 'user' or explicitly set
role when creating admin accounts.
Comment on lines
+49
to
+53
| <?php if ($video['type'] === 'embed'): ?> | ||
| <iframe src="<?= e($video['video_url']) ?>" frameborder="0" allowfullscreen></iframe> | ||
| <?php else: ?> | ||
| <video controls src="<?= e($video['video_url']) ?>"></video> | ||
| <?php endif; ?> |
There was a problem hiding this comment.
Validate video_url before embedding in iframe.
The video_url is output directly into an iframe src attribute. If an admin inadvertently stores a javascript: URL or malicious embed, this could lead to XSS. Consider validating that URLs use allowed protocols (http/https) and optionally allowed domains.
🔒 Proposed validation approach
Add a helper function in config.php:
function safeVideoUrl(string $url): string
{
$parsed = parse_url($url);
if (!$parsed || !isset($parsed['scheme']) || !in_array($parsed['scheme'], ['http', 'https'], true)) {
return '';
}
return $url;
}Then in video.php:
<?php if ($video['type'] === 'embed'): ?>
- <iframe src="<?= e($video['video_url']) ?>" frameborder="0" allowfullscreen></iframe>
+ <?php $safeUrl = safeVideoUrl($video['video_url']); ?>
+ <?php if ($safeUrl): ?>
+ <iframe src="<?= e($safeUrl) ?>" frameborder="0" allowfullscreen></iframe>
+ <?php endif; ?>
<?php else: ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@video.php` around lines 49 - 53, The iframe src is using unvalidated
$video['video_url'], risking javascript: XSS; add a helper safeVideoUrl(string
$url) (e.g. in config.php) that parse_url()s the value, ensures scheme is 'http'
or 'https' (and optionally checks a domain allowlist) and returns an empty
string on failure, then update the video.php template to call
safeVideoUrl($video['video_url']) before outputting (still wrap with e()) and
only render the iframe when the helper returns a non-empty string (otherwise
render a safe fallback or omit the iframe); also apply the same safeVideoUrl
check for the <video> src to be consistent.
Comment on lines
+62
to
+67
| <form method="post" action="comment.php" class="mb-3"> | ||
| <input type="hidden" name="video_id" value="<?= (int) $video['id'] ?>"> | ||
| <input class="form-control bg-dark text-light border-secondary mb-2" name="username" placeholder="Tên của bạn" required maxlength="50"> | ||
| <textarea class="form-control bg-dark text-light border-secondary mb-2" name="comment" rows="3" placeholder="Nội dung bình luận" required maxlength="500"></textarea> | ||
| <button class="btn btn-red w-100" type="submit">Gửi bình luận</button> | ||
| </form> |
There was a problem hiding this comment.
Comment form lacks CSRF protection.
The comment submission form doesn't include a CSRF token, making it vulnerable to cross-site request forgery attacks where an attacker could post comments on behalf of users.
Add a CSRF token field to the form and validate it in comment.php.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@video.php` around lines 62 - 67, The form in video.php is missing CSRF
protection; generate a cryptographically secure token (e.g., store a random
token in $_SESSION['csrf_token'] when rendering the page), add a hidden input
named csrf_token to the existing form (alongside video_id, username, comment),
and then in comment.php validate that the submitted csrf_token matches the
session token (and reject the request if missing/invalid), rotating or expiring
the token after successful use to prevent reuse.
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
1 participant
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.
Motivation
#111header,#dddtext, red brand#c00, thumbnail/duration, hover scale, responsive grid).localStorage.Description
index.php,video.php,search.php,comment.php,config.php,assets/css/style.css,assets/js/main.js,uploads/,admin/*, andsql/avstube.sqlwith initial seed data and admin account.embedandmp4, comments (stored incommentstable), search by title viasearch.php, and banner/Google ad zones.adsrecord, displays full-screen image, redirects on click or close, and setslocalStorage.popupViewed = trueso the popup does not reappear; also added daily view stats (video_stats) updated on each video view.Testing
rg --files -g '*.php' | xargs -n1 php -land all PHP files reportedNo syntax errors detected.php -S 0.0.0.0:8080to smoke-test the app startup successfully.SIGSEGVin this environment so a screenshot could not be captured.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Documentation