Open
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughA comprehensive news website application was added with a public-facing news bulletin, article pages, admin authentication and management panel, tracking analytics, and advertisement overlay functionality. A standalone decryption script was removed. The system includes PHP backend utilities, database schema, CSRF/authentication middleware, and frontend JavaScript for analytics and ads. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (6)
public_html/lib/csrf.php (1)
18-22: CSRF token is never invalidated after successful verification.The token lives for the entire session. If an attacker observes it once (via logs, referrer header, or a brief XSS window), every subsequent form action is exploitable. Consider rotating the token after each successful
csrf_checkcall:♻️ Proposed token rotation
function csrf_check(?string $token): bool { ensure_session(); - return isset($_SESSION['csrf_token']) && hash_equals($_SESSION['csrf_token'], (string)$token); + if (!isset($_SESSION['csrf_token']) || !hash_equals($_SESSION['csrf_token'], (string)$token)) { + return false; + } + // Rotate the token after successful verification + $_SESSION['csrf_token'] = bin2hex(random_bytes(16)); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/lib/csrf.php` around lines 18 - 22, The csrf_check function currently leaves $_SESSION['csrf_token'] valid for the whole session; change it to consume/rotate the token after a successful verification: inside csrf_check (and using ensure_session()) perform the hash_equals check as now, but when it returns true immediately unset or replace $_SESSION['csrf_token'] with a freshly generated token (e.g., from random_bytes/bin2hex) so the old token cannot be reused; keep the timing-safe comparison and return true only after rotation/consumption, and return false without rotating on mismatch.public_html/sql/database.sql (2)
29-33: No foreign-key constraint onstats.post_id— orphaned rows accumulate on post deletion.
post_idreferencesposts(id)logically but there is noFOREIGN KEYdeclaration, so deleting a post leaves its stats orphaned indefinitely.♻️ Proposed FK constraint
post_id INT NULL, + CONSTRAINT fk_stats_post FOREIGN KEY (post_id) REFERENCES posts (id) ON DELETE SET NULL,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/sql/database.sql` around lines 29 - 33, The stats table lacks a FOREIGN KEY on column post_id referencing posts(id), causing orphaned rows when posts are deleted; update the schema for table stats (or ALTER TABLE stats) to add a FOREIGN KEY constraint on post_id -> posts(id) and include an appropriate referential action such as ON DELETE CASCADE (or ON DELETE SET NULL if you prefer preserving stats) so deleting a row in posts removes or handles related rows in stats; ensure the constraint name is clear (e.g., fk_stats_post_id) and that post_id has the matching type/index required for the FK.
36-42:VALUES()inON DUPLICATE KEY UPDATEis deprecated since MySQL 8.0.20.The use of
VALUES()to access new row values inINSERT ... ON DUPLICATE KEY UPDATEstatements is now deprecated, and is subject to removal in a future MySQL release. Instead, use row and column aliases.♻️ Modernised syntax using row alias
-INSERT INTO admins (username, password_hash) VALUES -('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G') -ON DUPLICATE KEY UPDATE password_hash = VALUES(password_hash); +INSERT INTO admins (username, password_hash) VALUES +('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G') AS new_row +ON DUPLICATE KEY UPDATE password_hash = new_row.password_hash; -INSERT INTO settings (id, ads_enabled, ad_link, ad_title, ad_body, contact_link) VALUES -(1, 0, '', '', '', '') -ON DUPLICATE KEY UPDATE ads_enabled = VALUES(ads_enabled); +INSERT INTO settings (id, ads_enabled, ad_link, ad_title, ad_body, contact_link) VALUES +(1, 0, '', '', '', '') AS new_row +ON DUPLICATE KEY UPDATE ads_enabled = new_row.ads_enabled;Note: the row-alias syntax requires MySQL ≥ 8.0.19 and is not supported on MariaDB as of recent versions; if MariaDB compatibility is needed the
INSERT IGNOREapproach is the safest common ground.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/sql/database.sql` around lines 36 - 42, The INSERT statements for admins and settings use the deprecated VALUES() in their ON DUPLICATE KEY UPDATE clauses; change them to use a row alias (e.g., alias the inserted row as new) and reference new.password_hash and new.ads_enabled in the UPDATE expressions for the INSERT INTO admins (...) and INSERT INTO settings (...) statements respectively, or—if MariaDB compatibility is required—use an equivalent INSERT IGNORE or separate UPDATE fallback approach instead.public_html/assets/css/admin.css (1)
10-10: Overly broadinputselector will break checkbox/radio inputs; missingbox-sizingreset.
width: 100%applied to everyinputdistortstype="checkbox"andtype="radio"controls. Combined with the missingbox-sizing: border-boxreset (present instyle.cssbut absent here), thepadding: 12pxon awidth: 100%element can overflow its container in some browsers.🛠️ Suggested fix
-input, textarea, button { width: 100%; padding: 12px; margin: 6px 0 12px; border-radius: 8px; border: 1px solid `#1f2937`; background: `#0d152b`; color: `#e2e8f0`; } +* { box-sizing: border-box; } +input:not([type="checkbox"]):not([type="radio"]), textarea, button { width: 100%; padding: 12px; margin: 6px 0 12px; border-radius: 8px; border: 1px solid `#1f2937`; background: `#0d152b`; color: `#e2e8f0`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/assets/css/admin.css` at line 10, The broad rule targeting input, textarea, button causes checkbox/radio controls to stretch and there's no box-sizing reset; change the selector to exclude checkboxes/radios (e.g. use input:not([type="checkbox"]):not([type="radio"]), textarea, button or list explicit input types) and add box-sizing: border-box to that rule so padding doesn't cause overflow; update the rule that currently mentions input, textarea, button to use the narrower selector and include box-sizing: border-box.public_html/lib/slugify.php (1)
5-5:iconvTRANSLIT reliability for Vietnamese is locale-dependent.On many Linux shared hosts,
iconv('UTF-8', 'ASCII//TRANSLIT//IGNORE', ...)falls back to dropping characters entirely when the system locale lacks a suitable transliteration table, so a Vietnamese title like "Tin tức hôm nay" may silently produce the random'bai-xxxxxxxx'fallback instead of a readable slug. Consider using PHP'sintlextension (Transliterator::create('Any-Latin; Latin-ASCII')) which carries its own transliteration data and is independent of the OS locale.♻️ Portable alternative using intl
- $text = iconv('UTF-8', 'ASCII//TRANSLIT//IGNORE', $text); + if (function_exists('transliterator_transliterate')) { + $text = transliterator_transliterate('Any-Latin; Latin-ASCII; [\u0080-\u7fff] remove', $text) ?: $text; + } else { + $text = iconv('UTF-8', 'ASCII//TRANSLIT//IGNORE', $text) ?: $text; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/lib/slugify.php` at line 5, The current iconv('UTF-8','ASCII//TRANSLIT//IGNORE',$text) call is locale-dependent and can drop Vietnamese characters; replace it by using the intl Transliterator when available: check for class_exists('Transliterator') and, if present, create Transliterator::create('Any-Latin; Latin-ASCII') and transliterate $text (falling back to the existing iconv call only if Transliterator is unavailable), then continue the rest of slug normalization; target the line that currently calls iconv and update the slugify flow to use Transliterator first for deterministic, locale-independent transliteration.public_html/assets/js/adflow.js (1)
39-41:escapeHtmlhelper is defined after its first call-site.It works because function declarations are hoisted, but defining helpers before use is the conventional JS pattern and easier to follow for readers.
♻️ Proposed refactor
(function(){ + function escapeHtml(str){ + return String(str).replace(/[&<>"']/g, ch => ({'&':'&','<':'<','>':'>','"':'"','\'':'''}[ch])); + } + const data = window.__SITE; // ... - function escapeHtml(str){ - return String(str).replace(/[&<>"']/g, ch => ({'&':'&','<':'<','>':'>','"':'"','\'':'''}[ch])); - } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/assets/js/adflow.js` around lines 39 - 41, The escapeHtml helper is defined after it's first used; move the escapeHtml function declaration so it appears before its first call-site (or convert it to a const/arrow assigned before use) to follow conventional top-down reading; ensure the function name escapeHtml remains unchanged and that callers expecting String coercion still work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public_html/.htaccess`:
- Around line 1-11: The /sql and /logs directories are still web-accessible
despite "Options -Indexes"; move the physical directories out of public_html so
they cannot be served, and update any paths that reference them; if moving is
not possible, add per-directory deny files (create public_html/sql/.htaccess and
public_html/logs/.htaccess containing "Require all denied") or add root-level
deny rules that block requests to ^/sql and ^/logs before the existing
RewriteCond/RewriteRule block (ensure the deny runs prior to the RewriteEngine
rules that route to post.php) so direct file access is forbidden.
In `@public_html/admin/login.php`:
- Around line 8-15: After successful authentication via admin_login($username,
$password) call, immediately call session_regenerate_id(true) before redirecting
(i.e., insert session_regenerate_id(true) between the admin_login success branch
and the header('Location: /admin/panel.php')/exit) to prevent session fixation;
additionally, add CSRF protection by generating a token into the session when
rendering the login form and validating that token on POST (compare
$_POST['csrf_token'] to the session token) before calling admin_login.
In `@public_html/admin/logout.php`:
- Around line 5-6: The logout endpoint currently calls admin_logout() on any
GET, so add a CSRF token check before invoking admin_logout(): require a non-GET
method (e.g., POST) and validate a submitted token (e.g., $_POST['csrf_token'])
against the stored session token (e.g., $_SESSION['csrf_token']); only call
admin_logout() and then header('Location: /admin/login.php') when the token
matches, otherwise return a 400/403 response and do not perform the logout;
ensure the token name matches how other forms generate it and clear/regenerate
the session token after successful logout.
In `@public_html/admin/panel.php`:
- Around line 63-65: The current construction of $baseUrl using
$_SERVER['HTTP_HOST'] is unsafe because HTTP_HOST is user-controlled; replace it
so $baseUrl is derived from a trusted configuration value (e.g. a BASE_URL
constant/setting) or validated against a strict allowlist of allowed hostnames
before use; update the code paths that set $baseUrl (look for $scheme and
$baseUrl in this file) to prefer the configured BASE_URL, fall back to building
from $_SERVER only after validating against the allowlist, and log or reject
requests with invalid hosts.
- Around line 123-124: The onclick embeds the PHP string into JavaScript but
currently uses htmlspecialchars which only escapes HTML; change the onclick
argument to use a JS-safe literal by emitting json_encode($full) (e.g.,
onclick="copyLink(<?php echo json_encode($full); ?>)") while keeping
htmlspecialchars($full, ENT_QUOTES, 'UTF-8') for the visible <p> content; update
the copyLink(...) call to use the json_encoded value so the string cannot break
out into JS.
- Around line 46-52: The ad_link and contact_link values ($adLink and
$contactLink) are not validated and may contain unsafe schemes; before executing
the UPDATE in the panel.php handler, validate each link's scheme (e.g., using
parse_url or filter_var) and only allow empty or schemes "http" and "https"; if
a link fails validation, either normalize it to an empty string or return a
validation error to the caller, then proceed to bind the sanitized $adLink and
$contactLink into the existing $stmt->execute call so only safe URLs are
persisted.
In `@public_html/api/track.php`:
- Around line 1-22: The endpoint lacks a Content-Type header and doesn't guard
the HTTP method; update the top of the script to only proceed when
$_SERVER['REQUEST_METHOD'] === 'POST' (otherwise return a 405 JSON response) and
ensure you set header('Content-Type: application/json') before sending the JSON
reply; keep the existing logic that computes $type/$slug, does the slug lookup,
and calls record_stat($pdo, $type === 'ad_click' ? 'ad_click' : 'view', $postId,
60) so that DB updates only occur on POST and the response is correctly labeled
as application/json.
In `@public_html/assets/js/adflow.js`:
- Around line 29-30: The current ad click logic uses window.open(...,
'noopener') and directly assigns data.adLink to window.location.href, which
leaves the Referer header exposed and allows unsafe schemes; update the
window.open call to include 'noreferrer' in the feature string (e.g.,
'noopener,noreferrer') and validate/sanitize data.adLink before navigating:
parse data.adLink with the URL constructor (or equivalent), ensure its protocol
is either 'http:' or 'https:', and only then navigate (e.g.,
window.location.href or location.assign) or otherwise abort; update the
references in this file around the window.open and window.location.href usages
to include these checks and safe navigation.
In `@public_html/assets/js/track.js`:
- Around line 10-12: The fetch call in the else branch that posts payload to
'/api/track.php' can reject and currently has no rejection handler; update the
call in public_html/assets/js/track.js (the else branch that does
fetch('/api/track.php', {method:'POST', body: payload,
credentials:'same-origin'})) to handle promise rejections—either await the fetch
inside an async function and wrap in try/catch or append a .catch(...) to the
fetch promise to log or silently handle network errors so no unhandled Promise
rejection occurs.
In `@public_html/config.php`:
- Around line 7-11: The file currently hardcodes sensitive constants (DB_PASS,
APP_KEY and DB_USER) in tracked config.php; change it to load these from
environment variables (e.g., getenv('DB_PASS')/getenv('APP_KEY')) or require an
untracked override file (e.g., config.local.php added to .gitignore) that
defines/overrides DB_USER, DB_PASS and APP_KEY, and ensure config.php provides
only safe defaults/documentation and falls back to env/override values without
committing real secrets.
In `@public_html/lib/auth.php`:
- Around line 12-16: After verifying credentials in the login branch (the if
($row && password_verify($password, $row['password_hash'])) block in
public_html/lib/auth.php), call session_regenerate_id(true) immediately after
successful verification and before setting session variables (currently setting
$_SESSION['admin_id'], $_SESSION['admin_username'], $_SESSION['csrf_token']) so
the session ID is replaced and the old session is deleted to prevent session
fixation.
- Around line 34-36: The current positional setcookie(session_name(), '', time()
- 3600, $params['path'], $params['domain'], $params['secure'],
$params['httponly']) call should be replaced with the PHP 7.3+ options-array
form to include SameSite; update the code around the session.use_cookies check
and session_get_cookie_params() so you call setcookie(session_name(), '',
['expires' => time() - 3600, 'path' => $params['path'], 'domain' =>
$params['domain'], 'secure' => $params['secure'], 'httponly' =>
$params['httponly'], 'samesite' => 'Lax']) (use 'Lax' or 'Strict' per policy) to
ensure the SameSite attribute is set on the session cookie being cleared.
In `@public_html/lib/db.php`:
- Around line 21-26: In the catch block in public_html/lib/db.php (the
PDOException handler that currently checks APP_DEBUG, calls
file_put_contents(.../../logs/app.log) and then render_error_page()), stop
writing to a potentially web-accessible ../logs/app.log; instead log errors via
PHP's error_log() or to a directory outside the document root (or syslog) and
preserve the same message format/level, then call render_error_page(); if
APP_DEBUG is true keep throwing the exception as before. Replace the
file_put_contents call with error_log(...) or a path guaranteed to be outside
the web root and ensure proper permissions.
In `@public_html/lib/error_handler.php`:
- Around line 28-30: The final "return true" after calling render_error_page()
is unreachable because render_error_page() always exits; remove the dead
statement to avoid misleading code. Locate the block where
file_put_contents(..., $error, FILE_APPEND) is followed by render_error_page()
and delete the trailing "return true" (or alternatively refactor
render_error_page() to return instead of exiting and update any callers),
ensuring the error handler's flow is consistent with render_error_page()'s
behavior.
- Around line 43-52: The shutdown handler registered via
register_shutdown_function currently only logs fatal errors in non-debug mode
and does not produce user-visible output; update the handler (the anonymous
function in register_shutdown_function) to also clear any output buffers
(ob_start/ob_get_level/ob_end_clean pattern) and then either call
render_error_page() if headers/output permit or emit a minimal
plain-text/fallback error message to stdout so the browser doesn't get a
blank/partial page; keep the APP_DEBUG branch unchanged, ensure the same error
string used for logging is reused for the fallback output, and preserve logging
to app.log when APP_DEBUG is false.
In `@public_html/lib/track.php`:
- Around line 6-14: The current IP extraction loop trusts forwarded headers like
HTTP_X_FORWARDED_FOR which are spoofable; change logic to prefer and return
REMOTE_ADDR by default and only parse X_FORWARDED_FOR/HTTP_X_REAL_IP when the
immediate peer (REMOTE_ADDR) is in a configured trusted proxy allowlist (e.g.,
$trustedProxies), otherwise ignore those headers; update the foreach or replace
it so it first checks REMOTE_ADDR, then if in_array($_SERVER['REMOTE_ADDR'],
$trustedProxies) parse HTTP_X_FORWARDED_FOR (taking the first IP from the comma
list) or HTTP_X_REAL_IP, and keep using the existing trimming/first-IP logic on
$val when allowed.
In `@public_html/post.php`:
- Line 27: This page currently duplicates view tracking: server-side
record_stat(...) in post.php and client-side track.js which posts to
/api/track.php; choose server-side only by removing the client-side tracker —
delete the <script> include/inline invocation that loads track.js (and any
inline calls to the track API) from this page so record_stat($pdo, 'view',
(int)$post['id'], 60) is the sole source of truth; ensure no other inline JS
still posts to /api/track.php for this page.
- Around line 60-62: The iframe that renders the video embed (the template block
that outputs the $vid variable) lacks a sandbox attribute; update the iframe
element to include a restrictive sandbox (for example: sandbox="allow-scripts
allow-forms allow-popups allow-presentation") and avoid unsafe tokens like
allow-same-origin and allow-top-navigation unless explicitly required and
validated, so that third-party embeds cannot escape the frame or access the
parent origin; locate the iframe that echoes htmlspecialchars($vid, ENT_QUOTES,
'UTF-8') and add the sandbox attribute (and adjust any allow/allowfullscreen
attributes accordingly) to harden the embed.
In `@public_html/README.md`:
- Line 3: Remove the hardcoded default credentials from public README and
instead instruct operators to set their own admin password after import; update
README to (a) remove "admin/admin567" and replace with a short instruction to
run a one-time setup or call password_hash() to create an admin password, and
(b) add a prominent ⚠️ warning that any default password must be changed
immediately after import; also update database.sql to replace the seeded
bcrypt/hash for the admin user with a clear placeholder comment (or no hash) so
no working credentials are stored in the repo, and mention in README to update
config.php DB settings as part of the secure setup.
In `@public_html/sql/database.sql`:
- Around line 36-38: The current INSERT into the admins table uses ON DUPLICATE
KEY UPDATE which will overwrite the existing admin password hash on every schema
re-run; change the statement for admins (username, password_hash) so it only
inserts when the admin does not already exist—for example replace the ON
DUPLICATE KEY UPDATE form with an INSERT ... SELECT ... WHERE NOT EXISTS (SELECT
1 FROM admins WHERE username = 'admin') (referencing the admins table and the
username/password_hash columns) so the seed row is skipped if an admin is
present.
---
Nitpick comments:
In `@public_html/assets/css/admin.css`:
- Line 10: The broad rule targeting input, textarea, button causes
checkbox/radio controls to stretch and there's no box-sizing reset; change the
selector to exclude checkboxes/radios (e.g. use
input:not([type="checkbox"]):not([type="radio"]), textarea, button or list
explicit input types) and add box-sizing: border-box to that rule so padding
doesn't cause overflow; update the rule that currently mentions input, textarea,
button to use the narrower selector and include box-sizing: border-box.
In `@public_html/assets/js/adflow.js`:
- Around line 39-41: The escapeHtml helper is defined after it's first used;
move the escapeHtml function declaration so it appears before its first
call-site (or convert it to a const/arrow assigned before use) to follow
conventional top-down reading; ensure the function name escapeHtml remains
unchanged and that callers expecting String coercion still work.
In `@public_html/lib/csrf.php`:
- Around line 18-22: The csrf_check function currently leaves
$_SESSION['csrf_token'] valid for the whole session; change it to consume/rotate
the token after a successful verification: inside csrf_check (and using
ensure_session()) perform the hash_equals check as now, but when it returns true
immediately unset or replace $_SESSION['csrf_token'] with a freshly generated
token (e.g., from random_bytes/bin2hex) so the old token cannot be reused; keep
the timing-safe comparison and return true only after rotation/consumption, and
return false without rotating on mismatch.
In `@public_html/lib/slugify.php`:
- Line 5: The current iconv('UTF-8','ASCII//TRANSLIT//IGNORE',$text) call is
locale-dependent and can drop Vietnamese characters; replace it by using the
intl Transliterator when available: check for class_exists('Transliterator')
and, if present, create Transliterator::create('Any-Latin; Latin-ASCII') and
transliterate $text (falling back to the existing iconv call only if
Transliterator is unavailable), then continue the rest of slug normalization;
target the line that currently calls iconv and update the slugify flow to use
Transliterator first for deterministic, locale-independent transliteration.
In `@public_html/sql/database.sql`:
- Around line 29-33: The stats table lacks a FOREIGN KEY on column post_id
referencing posts(id), causing orphaned rows when posts are deleted; update the
schema for table stats (or ALTER TABLE stats) to add a FOREIGN KEY constraint on
post_id -> posts(id) and include an appropriate referential action such as ON
DELETE CASCADE (or ON DELETE SET NULL if you prefer preserving stats) so
deleting a row in posts removes or handles related rows in stats; ensure the
constraint name is clear (e.g., fk_stats_post_id) and that post_id has the
matching type/index required for the FK.
- Around line 36-42: The INSERT statements for admins and settings use the
deprecated VALUES() in their ON DUPLICATE KEY UPDATE clauses; change them to use
a row alias (e.g., alias the inserted row as new) and reference
new.password_hash and new.ads_enabled in the UPDATE expressions for the INSERT
INTO admins (...) and INSERT INTO settings (...) statements respectively, or—if
MariaDB compatibility is required—use an equivalent INSERT IGNORE or separate
UPDATE fallback approach instead.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public_html/logs/app.logis excluded by!**/*.log
📒 Files selected for processing (22)
dec.pypublic_html/.htaccesspublic_html/README.mdpublic_html/admin/login.phppublic_html/admin/logout.phppublic_html/admin/panel.phppublic_html/api/track.phppublic_html/assets/css/admin.csspublic_html/assets/css/style.csspublic_html/assets/js/adflow.jspublic_html/assets/js/track.jspublic_html/config.phppublic_html/error.phppublic_html/index.phppublic_html/lib/auth.phppublic_html/lib/csrf.phppublic_html/lib/db.phppublic_html/lib/error_handler.phppublic_html/lib/slugify.phppublic_html/lib/track.phppublic_html/post.phppublic_html/sql/database.sql
💤 Files with no reviewable changes (1)
- dec.py
Comment on lines
+1
to
+11
| Options -Indexes | ||
| RewriteEngine On | ||
|
|
||
| RewriteCond %{REQUEST_URI} !^/admin | ||
| RewriteCond %{REQUEST_URI} !^/assets | ||
| RewriteCond %{REQUEST_URI} !^/api | ||
| RewriteCond %{REQUEST_URI} !^/sql | ||
| RewriteCond %{REQUEST_URI} !^/logs | ||
| RewriteCond %{REQUEST_FILENAME} !-f | ||
| RewriteCond %{REQUEST_FILENAME} !-d | ||
| RewriteRule ^([A-Za-z0-9\-]+)/?$ post.php?slug=$1 [L,QSA] |
There was a problem hiding this comment.
Critical: /sql and /logs directories are publicly accessible.
Options -Indexes only suppresses directory listing. It does not block direct file access — http://domain/sql/database.sql is fully readable by anyone, exposing the entire schema and the seeded admin bcrypt hash. If log files land in /logs, they are similarly exposed (stack traces, DB errors, IP addresses).
The safest fix is to move both directories outside public_html. If that isn't feasible, add explicit deny rules:
🔒 Proposed .htaccess fix (deny access to sensitive directories)
Options -Indexes
RewriteEngine On
+# Block direct access to sensitive directories
+<DirectoryMatch "^(.*/)(sql|logs)/">
+ Require all denied
+</DirectoryMatch>
+
RewriteCond %{REQUEST_URI} !^/adminAlternatively, place a deny.htaccess inside each directory:
# public_html/sql/.htaccess and public_html/logs/.htaccess
Require all denied🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/.htaccess` around lines 1 - 11, The /sql and /logs directories
are still web-accessible despite "Options -Indexes"; move the physical
directories out of public_html so they cannot be served, and update any paths
that reference them; if moving is not possible, add per-directory deny files
(create public_html/sql/.htaccess and public_html/logs/.htaccess containing
"Require all denied") or add root-level deny rules that block requests to ^/sql
and ^/logs before the existing RewriteCond/RewriteRule block (ensure the deny
runs prior to the RewriteEngine rules that route to post.php) so direct file
access is forbidden.
Comment on lines
+8
to
+15
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | ||
| $username = trim($_POST['username'] ?? ''); | ||
| $password = $_POST['password'] ?? ''; | ||
| if (admin_login($username, $password)) { | ||
| header('Location: /admin/panel.php'); | ||
| exit; | ||
| } | ||
| $error = 'Sai tài khoản hoặc mật khẩu'; |
There was a problem hiding this comment.
Regenerate session ID after login; add CSRF to the login form.
Successful authentication should call session_regenerate_id(true) to mitigate session fixation. Also consider adding a CSRF token to the login form to prevent login CSRF.
🔒 Minimal fix for session fixation
if (admin_login($username, $password)) {
+ session_regenerate_id(true);
header('Location: /admin/panel.php');
exit;
}📝 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') { | |
| $username = trim($_POST['username'] ?? ''); | |
| $password = $_POST['password'] ?? ''; | |
| if (admin_login($username, $password)) { | |
| header('Location: /admin/panel.php'); | |
| exit; | |
| } | |
| $error = 'Sai tài khoản hoặc mật khẩu'; | |
| if ($_SERVER['REQUEST_METHOD'] === 'POST') { | |
| $username = trim($_POST['username'] ?? ''); | |
| $password = $_POST['password'] ?? ''; | |
| if (admin_login($username, $password)) { | |
| session_regenerate_id(true); | |
| header('Location: /admin/panel.php'); | |
| exit; | |
| } | |
| $error = 'Sai tài khoản hoặc mật khẩu'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/admin/login.php` around lines 8 - 15, After successful
authentication via admin_login($username, $password) call, immediately call
session_regenerate_id(true) before redirecting (i.e., insert
session_regenerate_id(true) between the admin_login success branch and the
header('Location: /admin/panel.php')/exit) to prevent session fixation;
additionally, add CSRF protection by generating a token into the session when
rendering the login form and validating that token on POST (compare
$_POST['csrf_token'] to the session token) before calling admin_login.
Comment on lines
+5
to
+6
| admin_logout(); | ||
| header('Location: /admin/login.php'); |
There was a problem hiding this comment.
Logout endpoint is vulnerable to CSRF.
Any page an admin visits can silently trigger a GET /admin/logout.php (e.g., via <img src="...">), forcing the admin out of their session. Add a CSRF token check before calling admin_logout().
🛡️ Proposed fix
<?php
require_once __DIR__ . '/../lib/auth.php';
+require_once __DIR__ . '/../lib/csrf.php';
require_once __DIR__ . '/../lib/error_handler.php';
+admin_require();
+csrf_verify(); // verify POST token, or gate on POST method
admin_logout();
header('Location: /admin/login.php');
exit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/admin/logout.php` around lines 5 - 6, The logout endpoint
currently calls admin_logout() on any GET, so add a CSRF token check before
invoking admin_logout(): require a non-GET method (e.g., POST) and validate a
submitted token (e.g., $_POST['csrf_token']) against the stored session token
(e.g., $_SESSION['csrf_token']); only call admin_logout() and then
header('Location: /admin/login.php') when the token matches, otherwise return a
400/403 response and do not perform the logout; ensure the token name matches
how other forms generate it and clear/regenerate the session token after
successful logout.
Comment on lines
+46
to
+52
| $adsEnabled = isset($_POST['ads_enabled']) ? 1 : 0; | ||
| $adLink = trim($_POST['ad_link'] ?? ''); | ||
| $adTitle = trim($_POST['ad_title'] ?? ''); | ||
| $adBody = trim($_POST['ad_body'] ?? ''); | ||
| $contactLink = trim($_POST['contact_link'] ?? ''); | ||
| $stmt = $pdo->prepare('UPDATE settings SET ads_enabled=?, ad_link=?, ad_title=?, ad_body=?, contact_link=? WHERE id = 1'); | ||
| $stmt->execute([$adsEnabled, $adLink, $adTitle, $adBody, $contactLink]); |
There was a problem hiding this comment.
Validate ad_link / contact_link schemes before saving.
Unvalidated URLs allow javascript: or other unsafe schemes that later get rendered on public pages. Restrict to http/https (or empty) before persisting.
✅ Example validation guard
} elseif ($action === 'save_settings') {
$adsEnabled = isset($_POST['ads_enabled']) ? 1 : 0;
$adLink = trim($_POST['ad_link'] ?? '');
$adTitle = trim($_POST['ad_title'] ?? '');
$adBody = trim($_POST['ad_body'] ?? '');
$contactLink = trim($_POST['contact_link'] ?? '');
+ foreach (['adLink' => $adLink, 'contactLink' => $contactLink] as $label => $link) {
+ if ($link !== '' && !preg_match('~^https?://~i', $link)) {
+ $message = 'Link không hợp lệ';
+ goto render_after_post;
+ }
+ }
$stmt = $pdo->prepare('UPDATE settings SET ads_enabled=?, ad_link=?, ad_title=?, ad_body=?, contact_link=? WHERE id = 1');
$stmt->execute([$adsEnabled, $adLink, $adTitle, $adBody, $contactLink]);
$message = 'Đã lưu cài đặt';
}
+render_after_post:📝 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
| $adsEnabled = isset($_POST['ads_enabled']) ? 1 : 0; | |
| $adLink = trim($_POST['ad_link'] ?? ''); | |
| $adTitle = trim($_POST['ad_title'] ?? ''); | |
| $adBody = trim($_POST['ad_body'] ?? ''); | |
| $contactLink = trim($_POST['contact_link'] ?? ''); | |
| $stmt = $pdo->prepare('UPDATE settings SET ads_enabled=?, ad_link=?, ad_title=?, ad_body=?, contact_link=? WHERE id = 1'); | |
| $stmt->execute([$adsEnabled, $adLink, $adTitle, $adBody, $contactLink]); | |
| $adsEnabled = isset($_POST['ads_enabled']) ? 1 : 0; | |
| $adLink = trim($_POST['ad_link'] ?? ''); | |
| $adTitle = trim($_POST['ad_title'] ?? ''); | |
| $adBody = trim($_POST['ad_body'] ?? ''); | |
| $contactLink = trim($_POST['contact_link'] ?? ''); | |
| foreach (['adLink' => $adLink, 'contactLink' => $contactLink] as $label => $link) { | |
| if ($link !== '' && !preg_match('~^https?://~i', $link)) { | |
| $message = 'Link không hợp lệ'; | |
| goto render_after_post; | |
| } | |
| } | |
| $stmt = $pdo->prepare('UPDATE settings SET ads_enabled=?, ad_link=?, ad_title=?, ad_body=?, contact_link=? WHERE id = 1'); | |
| $stmt->execute([$adsEnabled, $adLink, $adTitle, $adBody, $contactLink]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/admin/panel.php` around lines 46 - 52, The ad_link and
contact_link values ($adLink and $contactLink) are not validated and may contain
unsafe schemes; before executing the UPDATE in the panel.php handler, validate
each link's scheme (e.g., using parse_url or filter_var) and only allow empty or
schemes "http" and "https"; if a link fails validation, either normalize it to
an empty string or return a validation error to the caller, then proceed to bind
the sanitized $adLink and $contactLink into the existing $stmt->execute call so
only safe URLs are persisted.
Comment on lines
+63
to
+65
| $scheme = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off') ? 'https://' : 'http://'; | ||
| $baseUrl = $scheme . ($_SERVER['HTTP_HOST'] ?? 'domain'); | ||
| ?><!DOCTYPE html> |
There was a problem hiding this comment.
Don’t trust HTTP_HOST for base URL construction.
HTTP_HOST is user-controlled and can inject malicious hostnames into the admin UI. Use a configured BASE_URL (from config) or a strict allowlist.
🔧 Example (config-based)
-$scheme = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off') ? 'https://' : 'http://';
-$baseUrl = $scheme . ($_SERVER['HTTP_HOST'] ?? 'domain');
+$baseUrl = rtrim(BASE_URL, '/');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/admin/panel.php` around lines 63 - 65, The current construction
of $baseUrl using $_SERVER['HTTP_HOST'] is unsafe because HTTP_HOST is
user-controlled; replace it so $baseUrl is derived from a trusted configuration
value (e.g. a BASE_URL constant/setting) or validated against a strict allowlist
of allowed hostnames before use; update the code paths that set $baseUrl (look
for $scheme and $baseUrl in this file) to prefer the configured BASE_URL, fall
back to building from $_SERVER only after validating against the allowlist, and
log or reject requests with invalid hosts.
Comment on lines
+6
to
+14
| foreach (['HTTP_CLIENT_IP','HTTP_X_FORWARDED_FOR','HTTP_X_REAL_IP','REMOTE_ADDR'] as $key) { | ||
| if (!empty($_SERVER[$key])) { | ||
| $val = $_SERVER[$key]; | ||
| if (strpos($val, ',') !== false) { | ||
| $val = trim(explode(',', $val)[0]); | ||
| } | ||
| return $val; | ||
| } | ||
| } |
There was a problem hiding this comment.
Forwarded IP headers are spoofable.
Using HTTP_X_FORWARDED_FOR without a trusted proxy allowlist lets clients fake IPs, which undermines de-duplication and stats accuracy. Prefer REMOTE_ADDR unless you explicitly trust proxy headers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/lib/track.php` around lines 6 - 14, The current IP extraction
loop trusts forwarded headers like HTTP_X_FORWARDED_FOR which are spoofable;
change logic to prefer and return REMOTE_ADDR by default and only parse
X_FORWARDED_FOR/HTTP_X_REAL_IP when the immediate peer (REMOTE_ADDR) is in a
configured trusted proxy allowlist (e.g., $trustedProxies), otherwise ignore
those headers; update the foreach or replace it so it first checks REMOTE_ADDR,
then if in_array($_SERVER['REMOTE_ADDR'], $trustedProxies) parse
HTTP_X_FORWARDED_FOR (taking the first IP from the comma list) or
HTTP_X_REAL_IP, and keep using the existing trimming/first-IP logic on $val when
allowed.
| $bypassAd = (isset($_GET['ad']) && $_GET['ad'] === '0'); | ||
| $adEnabled = ((int)($settings['ads_enabled'] ?? 0) === 1) && !$bypassAd && !empty($settings['ad_link']); | ||
|
|
||
| record_stat($pdo, 'view', (int)$post['id'], 60); |
There was a problem hiding this comment.
Double view tracking: server-side record_stat and client-side track.js both fire for every page load.
Line 27 records a view synchronously via record_stat(). track.js (loaded at Line 80) then POSTs the same event to /api/track.php, which also calls record_stat(). The 60-second deduplication window in record_stat prevents a double DB insert, but the second DB round-trip is wasteful, and the dual-path architecture is confusing about which one is authoritative.
Pick one strategy and remove the other:
- Server-side only (simpler, works without JS, already implemented): remove
track.jsfrom this page. - Client-side only (lower request latency, bot traffic not counted): remove the
record_statcall on Line 27.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/post.php` at line 27, This page currently duplicates view
tracking: server-side record_stat(...) in post.php and client-side track.js
which posts to /api/track.php; choose server-side only by removing the
client-side tracker — delete the <script> include/inline invocation that loads
track.js (and any inline calls to the track API) from this page so
record_stat($pdo, 'view', (int)$post['id'], 60) is the sole source of truth;
ensure no other inline JS still posts to /api/track.php for this page.
Comment on lines
+60
to
+62
| <?php else: ?> | ||
| <iframe src="<?php echo htmlspecialchars($vid, ENT_QUOTES, 'UTF-8'); ?>" allowfullscreen></iframe> | ||
| <?php endif; ?> |
There was a problem hiding this comment.
<iframe> for video embeds has no sandbox attribute.
Without sandbox, embedded third-party content inherits the full origin context: it can execute scripts, navigate the top frame, and access cookies. This is a significant attack surface when $vid URLs are admin-controlled but potentially sourced externally.
🛡️ Proposed fix
- <iframe src="<?php echo htmlspecialchars($vid, ENT_QUOTES, 'UTF-8'); ?>" allowfullscreen></iframe>
+ <iframe src="<?php echo htmlspecialchars($vid, ENT_QUOTES, 'UTF-8'); ?>"
+ allowfullscreen
+ sandbox="allow-scripts allow-same-origin allow-presentation allow-popups"></iframe>📝 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
| <?php else: ?> | |
| <iframe src="<?php echo htmlspecialchars($vid, ENT_QUOTES, 'UTF-8'); ?>" allowfullscreen></iframe> | |
| <?php endif; ?> | |
| <?php else: ?> | |
| <iframe src="<?php echo htmlspecialchars($vid, ENT_QUOTES, 'UTF-8'); ?>" | |
| allowfullscreen | |
| sandbox="allow-scripts allow-same-origin allow-presentation allow-popups"></iframe> | |
| <?php endif; ?> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/post.php` around lines 60 - 62, The iframe that renders the video
embed (the template block that outputs the $vid variable) lacks a sandbox
attribute; update the iframe element to include a restrictive sandbox (for
example: sandbox="allow-scripts allow-forms allow-popups allow-presentation")
and avoid unsafe tokens like allow-same-origin and allow-top-navigation unless
explicitly required and validated, so that third-party embeds cannot escape the
frame or access the parent origin; locate the iframe that echoes
htmlspecialchars($vid, ENT_QUOTES, 'UTF-8') and add the sandbox attribute (and
adjust any allow/allowfullscreen attributes accordingly) to harden the embed.
| @@ -0,0 +1,13 @@ | |||
| # Website tin tức đơn giản | |||
|
|
|||
| Upload toàn bộ thư mục `public_html` lên hosting (DataOnline.vn), import `sql/database.sql`, cập nhật thông tin DB trong `config.php`. Sau khi import có sẵn tài khoản admin/admin567. | |||
There was a problem hiding this comment.
Default credentials (admin/admin567) must not be documented in a public repository.
The README + database.sql together tell anyone the exact admin username and password before the site owner has a chance to change it. Any deployment where the password is not immediately rotated is trivially compromisable.
Recommendations:
- Remove the password from the README entirely. Instruct the operator to set their own password with
password_hash()after import, or add a first-run setup wizard. - At minimum, add a prominent
⚠️ warning that the default password must be changed immediately after the SQL import. - Consider replacing the seeded bcrypt hash in
database.sqlwith a placeholder comment rather than a real working hash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/README.md` at line 3, Remove the hardcoded default credentials
from public README and instead instruct operators to set their own admin
password after import; update README to (a) remove "admin/admin567" and replace
with a short instruction to run a one-time setup or call password_hash() to
create an admin password, and (b) add a prominent ⚠️ warning that any default
password must be changed immediately after import; also update database.sql to
replace the seeded bcrypt/hash for the admin user with a clear placeholder
comment (or no hash) so no working credentials are stored in the repo, and
mention in README to update config.php DB settings as part of the secure setup.
Comment on lines
+36
to
+38
| INSERT INTO admins (username, password_hash) VALUES | ||
| ('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G') | ||
| ON DUPLICATE KEY UPDATE password_hash = VALUES(password_hash); |
There was a problem hiding this comment.
ON DUPLICATE KEY UPDATE will silently reset the admin password on every schema re-run.
If a developer or automated migration pipeline re-applies database.sql to a production database, line 38 overwrites the live password hash with the default admin567 hash. The operator may not notice until an attacker uses the published default credentials.
The seed row should be skipped if the admin already exists:
🔒 Proposed fix — skip insert if admin exists
-INSERT INTO admins (username, password_hash) VALUES
-('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G')
-ON DUPLICATE KEY UPDATE password_hash = VALUES(password_hash);
+INSERT IGNORE INTO admins (username, password_hash) VALUES
+('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G');📝 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
| INSERT INTO admins (username, password_hash) VALUES | |
| ('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G') | |
| ON DUPLICATE KEY UPDATE password_hash = VALUES(password_hash); | |
| INSERT IGNORE INTO admins (username, password_hash) VALUES | |
| ('admin', '$2y$12$V.97JyiCRI3EUNfz4uojbupFsTqpoZObHXGGy/tFFiV.kFCGAEj7G'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/sql/database.sql` around lines 36 - 38, The current INSERT into
the admins table uses ON DUPLICATE KEY UPDATE which will overwrite the existing
admin password hash on every schema re-run; change the statement for admins
(username, password_hash) so it only inserts when the admin does not already
exist—for example replace the ON DUPLICATE KEY UPDATE form with an INSERT ...
SELECT ... WHERE NOT EXISTS (SELECT 1 FROM admins WHERE username = 'admin')
(referencing the admins table and the username/password_hash columns) so the
seed row is skipped if an admin is present.
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.
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Documentation
Chores