Next release - deep link support after log in and refactor of index.php #1500
Conversation
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
📝 Walkthrough🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/php/templates/security.php (1)
85-85:⚠️ Potential issue | 🔴 CriticalCookie validation is now incompatible with the new token-based cookie set in
index.php.
index.phpnow stores a random token in theCOOKIE_NAMEcookie (line 108 ofindex.php), but this guard still compares the cookie value against$nax_Password(the config password hash). After this PR the cookie will never contain the password hash, so this fallback path is dead code — users with an expired session but a valid remember-me cookie will be redirected to login instead of passing through.Either remove this cookie check (let
index.phphandle remember-me on redirect) or align it with the new token-based scheme.
🤖 Fix all issues with AI agents
In `@front/index.php`:
- Around line 125-131: The remember-me flow is broken because the server-side
token is only stored in $_SESSION['remember_token'] which is lost when sessions
expire; persist the server-side token in a durable store (database or file) and
update the login and check logic to use it: when creating a remember-me cookie
(in your login path where login_user() is called) generate and store the token
hash in the persistent store tied to the user and set the cookie; in the check
block (where you currently read $_COOKIE[COOKIE_NAME] and compare to
$_SESSION['remember_token']) query the persistent store for the user's stored
token (e.g., via a new function like get_persistent_remember_token($userId) or
lookup_remember_token_by_cookie()) and use hash_equals(hash('sha256',
$_COOKIE[COOKIE_NAME]), $storedHash) to authenticate, then call login_user(),
repopulate $_SESSION['remember_token'], and
safe_redirect(append_hash($redirectTo)); ensure token creation, lookup,
invalidation on logout, and rotation are implemented.
- Around line 9-11: security.php (required in front/index.php) already calls
session_start(), so the extra session_start() in front/index.php causes a
duplicate session start warning; fix by removing the redundant session_start()
call in front/index.php or guard it with a check like session_status() !==
PHP_SESSION_ACTIVE before calling session_start(), referencing the existing
session_start() in security.php to avoid double invocation.
- Around line 25-37: The validate_local_path function currently rejects query
strings because the regex ^/[a-zA-Z0-9_\-/\.]*$ excludes characters like ?, =,
&, and :, so deep links (e.g., /devices.php?mac=AA:BB:CC) get replaced with
DEFAULT_REDIRECT; update the regex in validate_local_path to allow safe query
characters (e.g., ?, =, &, :, %, #) while still enforcing it starts with '/' and
forbidding whitespace/newlines and protocol prefixes like '//' or 'http(s)://';
for example, add a negative lookahead to reject '//' or '://', and expand the
character class to include \?\=\&\:\%\# so the function returns decoded
deep-links instead of DEFAULT_REDIRECT.
- Around line 77-80: The current block unconditionally calls login_user() (which
calls session_regenerate_id(true)) when $nax_WebProtection !== 'true', causing a
session ID regen on every index.php load; change the logic to skip calling
login_user() if the request is already authenticated (e.g., detect existing
session/auth state via your app's auth check or session flag) and only call
login_user() when the user is not authenticated, then proceed with
safe_redirect(append_hash($redirectTo)) as before; reference login_user(),
session_regenerate_id(true), safe_redirect(), and append_hash() to locate and
update the code.
🧹 Nitpick comments (1)
front/index.php (1)
13-14: Duplicate cookie-name constant.
security.phpalready definesCOOKIE_SAVE_LOGIN_NAME = "NetAlertX_SaveLogin"(line 16). HereCOOKIE_NAMEis defined with the same value. Use a single shared constant to avoid drift.
| require_once $_SERVER['DOCUMENT_ROOT'].'/php/templates/security.php'; | ||
|
|
||
| $CookieSaveLoginName = 'NetAlertX_SaveLogin'; | ||
| session_start(); |
There was a problem hiding this comment.
Double session_start() will trigger a PHP warning.
security.php (required on line 9) already calls session_start() at its line 49. Calling it again here on line 11 produces a PHP warning in 7.2+. Guard it or remove the duplicate.
Proposed fix
-session_start();
+if (session_status() === PHP_SESSION_NONE) {
+ session_start();
+}🤖 Prompt for AI Agents
In `@front/index.php` around lines 9 - 11, security.php (required in
front/index.php) already calls session_start(), so the extra session_start() in
front/index.php causes a duplicate session start warning; fix by removing the
redundant session_start() call in front/index.php or guard it with a check like
session_status() !== PHP_SESSION_ACTIVE before calling session_start(),
referencing the existing session_start() in security.php to avoid double
invocation.
| if (!is_authenticated() && !empty($_COOKIE[COOKIE_NAME]) && !empty($_SESSION['remember_token'])) { | ||
|
|
||
| if (hash_equals($_SESSION['remember_token'], hash('sha256',$_COOKIE[COOKIE_NAME]))) { | ||
| login_user(); | ||
| safe_redirect(append_hash($redirectTo)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remember-me is non-functional: $_SESSION['remember_token'] is lost when the session expires.
The whole point of "remember me" is to survive session expiry. But the token hash is stored only in $_SESSION, which is destroyed when the session expires. When the user returns with a valid cookie but no active session, $_SESSION['remember_token'] will be empty, and this block is skipped — the cookie is useless.
The server-side token needs to be persisted outside the session (e.g., database or file-based store) so it can be looked up when a fresh session is created.
🤖 Prompt for AI Agents
In `@front/index.php` around lines 125 - 131, The remember-me flow is broken
because the server-side token is only stored in $_SESSION['remember_token']
which is lost when sessions expire; persist the server-side token in a durable
store (database or file) and update the login and check logic to use it: when
creating a remember-me cookie (in your login path where login_user() is called)
generate and store the token hash in the persistent store tied to the user and
set the cookie; in the check block (where you currently read
$_COOKIE[COOKIE_NAME] and compare to $_SESSION['remember_token']) query the
persistent store for the user's stored token (e.g., via a new function like
get_persistent_remember_token($userId) or lookup_remember_token_by_cookie()) and
use hash_equals(hash('sha256', $_COOKIE[COOKIE_NAME]), $storedHash) to
authenticate, then call login_user(), repopulate $_SESSION['remember_token'],
and safe_redirect(append_hash($redirectTo)); ensure token creation, lookup,
invalidation on logout, and rotation are implemented.
There was a problem hiding this comment.
Not security related -> Out of scope
There was a problem hiding this comment.
@jokob-sk, understood — I'll defer this functional bug. Would you like me to open an issue to track fixing the remember-me persistence for a future PR?
There was a problem hiding this comment.
This could be a security issue. Security generally follows the CIA triad andconsists of
- Confidentially - not affected
- Integrity - integrity of the token is affected as coderabbit appear to say the session is not properly preserved. This could lead to the need to login again.
- Availability - lowered availability if the user needs to relog.
- Authentication - unaffected but may be required more often.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@front/index.php`:
- Around line 107-113: The secure flag in the setcookie call for COOKIE_NAME is
being set using isset($_SERVER['HTTPS']), which is unreliable (IIS can set
'off'); update the secure check used when calling setcookie(COOKIE_NAME, $token,
...) to a robust is-HTTPS test such as checking that $_SERVER['HTTPS'] is
non-empty and not equal to 'off' and/or honoring proxy headers like
HTTP_X_FORWARDED_PROTO == 'https' when behind a trusted proxy; replace the
current isset($_SERVER['HTTPS']) expression with that combined HTTPS detection
logic so the 'secure' option is only true for actual HTTPS requests.
- Around line 54-64: The logout_user function clears the remember-me cookie but
omits the secure flag, so the deletion cookie may not match the original (set
with 'secure' => isset(...)); update logout_user to include the same secure flag
logic used when creating the cookie (use the same condition used during login)
when calling setcookie for COOKIE_NAME so the deletion cookie's attributes
(secure, samesite, path) match the original and ensure the browser will remove
the cookie.
- Line 193: The form action embeds the base64 `$_GET['next']` but a `+` in
base64 can be interpreted as space when round-tripped via query parsing; to fix,
URL-encode the `$_GET['next']` value before rendering it into the action
attribute (e.g. wrap the value with rawurlencode or urlencode) while still
applying htmlspecialchars for HTML context so the output remains safe; update
the code in front/index.php where `$_GET['next']` is printed (the form action
line) and ensure this is consistent with the `validate_local_path` handling of
the base64 next token.
🧹 Nitpick comments (1)
front/index.php (1)
94-118: No feedback to the user on a failed login attempt.When the password check fails (line 98 evaluates to
false), execution silently falls through to displaying the login form again with no error message. Users won't know whether they mistyped or something else went wrong.Consider setting a flag (e.g.,
$login_error = true) and displaying a message in the form.
| function logout_user(): void { | ||
| $_SESSION = []; | ||
| session_destroy(); | ||
|
|
||
| setcookie(COOKIE_NAME,'',[ | ||
| 'expires'=>time()-3600, | ||
| 'path'=>'/', | ||
| 'httponly'=>true, | ||
| 'samesite'=>'Strict' | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Logout cookie is missing the secure flag present on the login cookie.
The remember-me cookie is set with 'secure' => isset(…) on login (line 110), but the deletion cookie on logout omits secure entirely. If the login cookie was set with secure, the browser won't match the deletion cookie to clear it — the remember-me cookie will persist after logout.
Proposed fix
setcookie(COOKIE_NAME,'',[
'expires'=>time()-3600,
'path'=>'/',
+ 'secure'=>!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off',
'httponly'=>true,
'samesite'=>'Strict'
]);🤖 Prompt for AI Agents
In `@front/index.php` around lines 54 - 64, The logout_user function clears the
remember-me cookie but omits the secure flag, so the deletion cookie may not
match the original (set with 'secure' => isset(...)); update logout_user to
include the same secure flag logic used when creating the cookie (use the same
condition used during login) when calling setcookie for COOKIE_NAME so the
deletion cookie's attributes (secure, samesite, path) match the original and
ensure the browser will remove the cookie.
There was a problem hiding this comment.
This is saying the logout won't be handled properly because the value of secure is used to select a cookie for deletion. This will wreck logout function on https.
There was a problem hiding this comment.
From the browser's perspective*
There was a problem hiding this comment.
@adamoutler Exactly right — the browser uses all cookie attributes (path, domain, secure, etc.) to match which cookie to modify or delete. Without matching secure flags, the deletion attempt targets a different cookie slot, leaving the HTTPS-set remember-me cookie intact after logout. Thanks for confirming this will break the logout flow on HTTPS connections!
There was a problem hiding this comment.
@adamoutler, you're absolutely right — this is a critical issue. Without the secure flag matching during deletion, the remember-me cookie will persist on HTTPS connections even after logout, leaving users inadvertently authenticated.
The fix CodeRabbit suggested should resolve it:
setcookie(COOKIE_NAME, '', [
'expires' => time() - 3600,
'path' => '/',
'secure' => !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off',
'httponly' => true,
'samesite' => 'Strict'
]);Given the PR's critical requirement of "no password/login bypass issues," this should be addressed before merge. After applying the fix, I'd recommend testing the logout flow specifically on HTTPS to verify the cookie is properly cleared.
| setcookie(COOKIE_NAME,$token,[ | ||
| 'expires'=>time()+604800, | ||
| 'path'=>'/', | ||
| 'secure'=>isset($_SERVER['HTTPS']), | ||
| 'httponly'=>true, | ||
| 'samesite'=>'Strict' | ||
| ]); |
There was a problem hiding this comment.
isset($_SERVER['HTTPS']) is not a reliable HTTPS check.
Some web servers (notably IIS) set $_SERVER['HTTPS'] to the string 'off' when HTTPS is not active. isset() returns true in that case, so the cookie's secure flag would be set even over plain HTTP — causing the browser to never send the cookie back, silently breaking remember-me.
Proposed fix
- 'secure'=>isset($_SERVER['HTTPS']),
+ 'secure'=>!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off',🤖 Prompt for AI Agents
In `@front/index.php` around lines 107 - 113, The secure flag in the setcookie
call for COOKIE_NAME is being set using isset($_SERVER['HTTPS']), which is
unreliable (IIS can set 'off'); update the secure check used when calling
setcookie(COOKIE_NAME, $token, ...) to a robust is-HTTPS test such as checking
that $_SERVER['HTTPS'] is non-empty and not equal to 'off' and/or honoring proxy
headers like HTTP_X_FORWARDED_PROTO == 'https' when behind a trusted proxy;
replace the current isset($_SERVER['HTTPS']) expression with that combined HTTPS
detection logic so the 'secure' option is only true for actual HTTPS requests.
There was a problem hiding this comment.
Basically repeating the above.
There was a problem hiding this comment.
@adamoutler, understood! The proposed fix will ensure the secure flag is only set when HTTPS is actually active, preventing the cookie from being silently rejected by browsers when IIS or similar servers set $_SERVER['HTTPS'] to 'off'.
front/index.php
Outdated
| <div class="login-box-body"> | ||
| <p class="login-box-msg"><?= lang('Login_Box');?></p> | ||
| <form action="index.php" method="post"> | ||
| <form action="index.php<?php echo !empty($_GET['next']) ? '?next=' . htmlspecialchars($_GET['next']) : ''; ?>" method="post"> |
There was a problem hiding this comment.
Form action preserves next param correctly, but consider encoding consistency.
$_GET['next'] is already base64-encoded per validate_local_path, and htmlspecialchars is applied here — this looks correct. However, note that the + character in base64 output will survive htmlspecialchars but could be interpreted as a space by the receiving $_GET. Consider using urlencode() around the value for robustness.
Proposed fix
- <form action="index.php<?php echo !empty($_GET['next']) ? '?next=' . htmlspecialchars($_GET['next']) : ''; ?>" method="post">
+ <form action="index.php<?php echo !empty($_GET['next']) ? '?next=' . htmlspecialchars(urlencode($_GET['next'])) : ''; ?>" method="post">🤖 Prompt for AI Agents
In `@front/index.php` at line 193, The form action embeds the base64
`$_GET['next']` but a `+` in base64 can be interpreted as space when
round-tripped via query parsing; to fix, URL-encode the `$_GET['next']` value
before rendering it into the action attribute (e.g. wrap the value with
rawurlencode or urlencode) while still applying htmlspecialchars for HTML
context so the output remains safe; update the code in front/index.php where
`$_GET['next']` is printed (the form action line) and ensure this is consistent
with the `validate_local_path` handling of the base64 next token.
There was a problem hiding this comment.
This is outdated and can be closed. Coderabbit can raise a new issue.
It's sucgesting to urlencode first before HTML special chars
There was a problem hiding this comment.
🧩 Analysis chain
@adamoutler, understood! I'll create a new issue to track the URL encoding concern properly.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the URL encoding concern
gh issue create \
--title "URL encode base64 next parameter before htmlspecialchars in login form" \
--body "## Description
When embedding the base64-encoded \`\$_GET['next']\` parameter in the login form action attribute in \`front/index.php\`, the \`+\` character from base64 encoding can be misinterpreted as a space during URL parsing.
## Suggested Fix
Apply \`urlencode()\` before \`htmlspecialchars()\` when building the form action:
\`\`\`php
?next=' . htmlspecialchars(urlencode(\$_GET['next']), ENT_QUOTES, 'UTF-8')
\`\`\`
This ensures the base64 value survives the round-trip through query string parsing.
## Context
- **File:** \`front/index.php\` (form action around line 193)
- **Original PR:** `#1500`
- **Comment thread:** https://github.com/netalertx/NetAlertX/pull/1500#discussion_r2780043435
- **Requested by:** `@adamoutler`" \
--assignee adamoutler
Length of output: 50
@adamoutler, done! I've created issue #1501 to track the URL encoding concern: #1501
The issue documents the suggestion to apply urlencode() before htmlspecialchars() for the next parameter to handle base64 + characters correctly.
🧬 Code Graph Analysis Results
front/php/templates/language/lang.php
### Snippet from `front/php/templates/language/lang.php` (lines 90-118)function lang($key)
{
global $pia_lang_selected, $strings;
// Get the data from JSON files
$languageData = getLanguageDataFromJson();
// Get the data from SQL query
$sqlLanguageData = $strings;
// Merge JSON data with SQL data
$mergedLanguageData = mergeLanguageData($languageData, $sqlLanguageData);
// Check if the key exists in the selected language
if (isset($mergedLanguageData[$pia_lang_selected][$key]) && $mergedLanguageData[$pia_lang_selected][$key] != '') {
$result = $mergedLanguageData[$pia_lang_selected][$key];
} else {
// If key not found in selected language, use "en_us" as fallback
if (isset($mergedLanguageData['en_us'][$key])) {
$result = $mergedLanguageData['en_us'][$key];
} else {
// If key not found in "en_us" either, use a default string
$result = "String Not found for key " . $key;
}
}
// HTML encode the result before returning
return str_replace("'", ''', $result);
}
There was a problem hiding this comment.
I didn't mean that. I mean usually when you put a comment in a code review, it's an issue in the code. We don't need a new project issue from code in a PR. Please close. I didn't know you had that capability.
There was a problem hiding this comment.
I thought you'd raise a non-outdated review comment or renew it or something.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Close the issue `#1501` that was created
gh issue close 1501 --comment "Closing as requested - this was created from a PR code review comment by mistake."
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Close the issue `#1501` without adding a comment
gh issue close 1501
Length of output: 60
@adamoutler, I apologize for the confusion! You're absolutely right—I misunderstood your comment. Code review comments are meant to flag issues in the code during review, not spawn separate GitHub issues.
Unfortunately, I don't have the necessary permissions to close issue #1501. You'll need to close it manually, or I can ask a maintainer to do so if you prefer.
I've noted this for future interactions to avoid creating issues from PR code review comments unless you explicitly request it.
(\__/)
(•ㅅ•) *sorry!*
_ノヽ ノ\_
🧬 Code Graph Analysis Results
: front/php/templates/language/lang.php
At lines 90-118:
function lang($key)
{
global $pia_lang_selected, $strings;
// Get the data from JSON files
$languageData = getLanguageDataFromJson();
// Get the data from SQL query
$sqlLanguageData = $strings;
// Merge JSON data with SQL data
$mergedLanguageData = mergeLanguageData($languageData, $sqlLanguageData);
// Check if the key exists in the selected language
if (isset($mergedLanguageData[$pia_lang_selected][$key]) && $mergedLanguageData[$pia_lang_selected][$key] != '') {
$result = $mergedLanguageData[$pia_lang_selected][$key];
} else {
// If key not found in selected language, use "en_us" as fallback
if (isset($mergedLanguageData['en_us'][$key])) {
$result = $mergedLanguageData['en_us'][$key];
} else {
// If key not found in "en_us" either, use a default string
$result = "String Not found for key " . $key;
}
}
// HTML encode the result before returning
return str_replace("'", ''', $result);
}
✏️ Learnings added
Learnt from: adamoutler
Repo: netalertx/NetAlertX PR: 1500
File: front/index.php:0-0
Timestamp: 2026-02-09T03:43:16.357Z
Learning: Do not create GitHub issues from PR code review comments unless the user explicitly requests issue creation. Code review comments are meant to flag issues in the code during review, not spawn separate GitHub issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Please add to learnings: AdamOutler will never ask you to open a new issue on GitHub. If he did, you misunderstood
There was a problem hiding this comment.
🧩 Analysis chain
@adamoutler, got it—my apologies for the confusion! Let me close that issue and take a fresh look at the current code.
🏁 Script executed:
#!/bin/bash
# Close the issue I created
gh issue close 1501 --comment "Closing per maintainer request - this was created in error. The concern should be addressed as a review comment on the PR itself, not as a separate issue."
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Check how the 'next' parameter is being created in security.php
cat front/php/templates/security.php
Length of output: 3357
@adamoutler, I understand now—apologies for the confusion! Unfortunately, I don't have permission to close issue #1501 (got a "Resource not accessible by integration" error), so you'll need to close it manually.
Looking at the current code:
In security.php (line 93):
$returnUrl = rawurlencode(base64_encode($_SERVER['REQUEST_URI']));
redirect("/index.php?next=" . $returnUrl);In index.php (line 193):
?next=' . htmlspecialchars($_GET['next'])The flow is: security.php URL-encodes the base64 value when creating the redirect, PHP auto-decodes it into $_GET['next'], then index.php embeds it in the form action with only htmlspecialchars(). Technically, base64 + characters in $_GET['next'] would need urlencode() when re-embedded in a query string to survive the round-trip properly.
However, since you marked the original comment as "outdated," I'm assuming either:
- This is a known non-issue in practice, or
- You've addressed it differently, or
- You disagree with the concern
Let me know if you'd like me to verify anything else or if this can be considered resolved!
🧬 Code Graph Analysis Results
front/php/templates/language/lang.php ### Snippet from `front/php/templates/language/lang.php` (lines 90-118)function lang($key)
{
global $pia_lang_selected, $strings;
// Get the data from JSON files
$languageData = getLanguageDataFromJson();
// Get the data from SQL query
$sqlLanguageData = $strings;
// Merge JSON data with SQL data
$mergedLanguageData = mergeLanguageData($languageData, $sqlLanguageData);
// Check if the key exists in the selected language
if (isset($mergedLanguageData[$pia_lang_selected][$key]) && $mergedLanguageData[$pia_lang_selected][$key] != '') {
$result = $mergedLanguageData[$pia_lang_selected][$key];
} else {
// If key not found in selected language, use "en_us" as fallback
if (isset($mergedLanguageData['en_us'][$key])) {
$result = $mergedLanguageData['en_us'][$key];
} else {
// If key not found in "en_us" either, use a default string
$result = "String Not found for key " . $key;
}
}
// HTML encode the result before returning
return str_replace("'", ''', $result);
}There was a problem hiding this comment.
@adamoutler, got it! I've added that to my knowledge base. I won't create GitHub issues based on your comments—if I think you're asking for one, I'll know I've misunderstood. 🐰
🧠 Learnings used
Learnt from: adamoutler
Repo: netalertx/NetAlertX PR: 1500
File: front/index.php:0-0
Timestamp: 2026-02-09T03:43:16.357Z
Learning: Do not create GitHub issues from PR code review comments unless the user explicitly requests issue creation. Code review comments are meant to flag issues in the code during review, not spawn separate GitHub issues.
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front/php/templates/security.php (2)
54-59:⚠️ Potential issue | 🟠 MajorThis logout handler pre-empts
index.php'slogout_user(), bypassing its improved cookie cleanup.As noted in the
index.phpreview, this block runs beforeindex.phpline 94 is reached. Thesetcookiecall here (line 57) uses minimal attributes (nohttponly, nosamesite, nopath), whilelogout_user()inindex.phpsets all of them. Either consolidate logout into one location or forward toindex.php's handler.Proposed fix — let index.php own logout
// Handle logout -if (!empty($_REQUEST['action']) && $_REQUEST['action'] == 'logout') { - session_destroy(); - setcookie(COOKIE_SAVE_LOGIN_NAME, "", time() - 3600); - redirect('index.php'); -} +// Logout is handled by index.php — removed to avoid duplicate/inconsistent cleanup
86-86:⚠️ Potential issue | 🟠 MajorPre-existing: raw password hash in cookie enables trivial auth bypass.
The condition
$_COOKIE[COOKIE_SAVE_LOGIN_NAME] && $nax_Password === $_COOKIE[COOKIE_SAVE_LOGIN_NAME]compares the stored password hash directly against the cookie value. Anyone who obtains the SHA-256 hash (e.g., from a config file leak) can set this cookie and bypass authentication entirely. This predates this PR but is worth flagging given the PR's stated goal of "no password/login bypass issues."The new remember-me flow in
index.phpuses a proper random token withhash_equals, which is the right pattern — but this old path remains active and undermines it.
🤖 Fix all issues with AI agents
In `@front/index.php`:
- Around line 45-50: The append_hash function accepts raw $_POST['url_hash'] and
can produce a malformed URL if the value doesn't start with '#'; update
append_hash to first check $_POST['url_hash'], normalize it so it begins with a
single '#' (prepend one if missing), then sanitize the rest using the existing
character whitelist (preg_replace('/[^#a-zA-Z0-9_\-]/', '', ...)) or better
apply the regex to the substring after the '#' to avoid stripping/duplicating
the prefix; ensure you return $url . $normalizedHash and reference append_hash
and $_POST['url_hash'] when making the change.
- Around line 94-97: The logout code in security.php currently intercepts
?action=logout and uses a simple setcookie/redirect which bypasses the secure
cookie handling in logout_user() defined in index.php; fix by replacing the
logout branch in security.php (the check of $_REQUEST['action'] == 'logout' that
calls session_destroy() and setcookie(COOKIE_SAVE_LOGIN_NAME, "", ...),
redirect('index.php')) with a call to logout_user() (or move logout_user() into
a shared include and call it) and then redirect('index.php')/exit so the same
httponly and samesite='Strict' cookie clearing logic is used; alternatively
remove the unreachable logout block in index.php if you prefer centralizing in
security.php — but ensure the final handler uses logout_user() semantics for
secure cookie attributes.
🧹 Nitpick comments (1)
front/index.php (1)
23-26:safe_redirectinjects$pathdirectly into theLocationheader.While
validate_local_pathconstrains the character set,safe_redirectitself performs no validation. If it's ever called with an unvalidated argument (now or in the future), it becomes an open-redirect / header-injection vector. A defensive check here would harden the function.Proposed hardening
function safe_redirect(string $path): void { + // Defense-in-depth: strip newlines to prevent header injection + $path = str_replace(["\r", "\n"], '', $path); header("Location: {$path}", true, 302); exit; }
| function append_hash(string $url): string { | ||
| if (!empty($_POST['url_hash'])) { | ||
| return $url . preg_replace('/[^#a-zA-Z0-9_\-]/', '', $_POST['url_hash']); | ||
| } | ||
| return $url; | ||
| } |
There was a problem hiding this comment.
append_hash doesn't validate that the value starts with #, producing a malformed URL if manipulated.
The JS (line 257-259) always provides window.location.hash (which starts with #), but a crafted POST could send url_hash=abc — the regex on line 47 preserves alphanumerics, resulting in a redirect to e.g. /devices.phpabc. Consider enforcing the # prefix:
Proposed fix
function append_hash(string $url): string {
if (!empty($_POST['url_hash'])) {
- return $url . preg_replace('/[^#a-zA-Z0-9_\-]/', '', $_POST['url_hash']);
+ $sanitized = preg_replace('/[^#a-zA-Z0-9_\-]/', '', $_POST['url_hash']);
+ if (str_starts_with($sanitized, '#')) {
+ return $url . $sanitized;
+ }
}
return $url;
}🤖 Prompt for AI Agents
In `@front/index.php` around lines 45 - 50, The append_hash function accepts raw
$_POST['url_hash'] and can produce a malformed URL if the value doesn't start
with '#'; update append_hash to first check $_POST['url_hash'], normalize it so
it begins with a single '#' (prepend one if missing), then sanitize the rest
using the existing character whitelist (preg_replace('/[^#a-zA-Z0-9_\-]/', '',
...)) or better apply the regex to the substring after the '#' to avoid
stripping/duplicating the prefix; ensure you return $url . $normalizedHash and
reference append_hash and $_POST['url_hash'] when making the change.
| if (($_GET['action'] ?? '') === 'logout') { | ||
| logout_user(); | ||
| safe_redirect('/index.php'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the relevant PHP files
echo "=== Finding PHP files ==="
find front -name "*.php" -type f | head -20
echo ""
echo "=== Checking index.php for require/include of security.php ==="
if [ -f front/index.php ]; then
head -20 front/index.php
fi
echo ""
echo "=== Looking for security.php ==="
find front -name "security.php" -type fRepository: netalertx/NetAlertX
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Verify the logout handlers as specified in the review
echo "=== security.php logout handler ==="
find front -name "security.php" -type f -exec head -60 {} + | tail -40
echo ""
echo "=== index.php logout handler around lines 94-97 ==="
if [ -f front/index.php ]; then
wc -l front/index.php
fiRepository: netalertx/NetAlertX
Length of output: 1482
🏁 Script executed:
#!/bin/bash
# Get lines around 94-97 in index.php
echo "=== index.php lines 90-105 ==="
sed -n '90,105p' front/index.php
echo ""
echo "=== Find logout_user function definition ==="
rg -n 'function logout_user|logout_user\s*\(' front/ -A 5Repository: netalertx/NetAlertX
Length of output: 1095
🏁 Script executed:
#!/bin/bash
# Let's also check what the logout_user function actually does
echo "=== Full logout_user function ==="
rg -n 'function logout_user' front/ -A 20Repository: netalertx/NetAlertX
Length of output: 1056
Logout block is dead code — security.php intercepts ?action=logout before execution reaches here.
security.php is required on line 6 of index.php, and lines 44-49 check $_REQUEST['action'] == 'logout', call session_destroy(), and execute redirect('index.php') which exits. Since $_REQUEST includes $_GET parameters, any logout request via ?action=logout triggers this handler first, preventing the code at lines 94-97 from executing.
The logout_user() function (lines 61-71) implements proper cookie security with httponly and samesite='Strict' attributes, but this is bypassed by security.php's simpler setcookie(COOKIE_SAVE_LOGIN_NAME, "", time() - 3600) call that lacks these protections.
Either remove the logout handler from security.php and consolidate the logic into logout_user(), or remove this unreachable block from index.php. Keep the version with proper security attributes.
🤖 Prompt for AI Agents
In `@front/index.php` around lines 94 - 97, The logout code in security.php
currently intercepts ?action=logout and uses a simple setcookie/redirect which
bypasses the secure cookie handling in logout_user() defined in index.php; fix
by replacing the logout branch in security.php (the check of $_REQUEST['action']
== 'logout' that calls session_destroy() and setcookie(COOKIE_SAVE_LOGIN_NAME,
"", ...), redirect('index.php')) with a call to logout_user() (or move
logout_user() into a shared include and call it) and then
redirect('index.php')/exit so the same httponly and samesite='Strict' cookie
clearing logic is used; alternatively remove the unreachable logout block in
index.php if you prefer centralizing in security.php — but ensure the final
handler uses logout_user() semantics for secure cookie attributes.
|
Would appreciate you having a look @adamoutler once you have time - what are valid issues vs white noise - don't want to introduce security issues |
|
Generally when I call BS on coderabbit it's because I know the code. I'm able to say no we can't do that because... Or this isn't important to me because ixxx out of scope.. or xxx was a compromise for yyy... Or this is not code I feel comfortable working with. These appear valid issues. Some of them may be legacy code. I'd accept all of code rabbit's committable suggestions, and then run unit tests and close out those issues. Then work through the rest one by one. |
2 participants
In scope:
Out of scope:
Adding additional security checks
Summary by CodeRabbit
New Features
Bug Fixes