[WIP] fix: authentication when booting into GUI mode #1368
Conversation
WalkthroughThis update removes all two-factor authentication (2FA) logic from the PHP login flow, simplifying authentication to require only a username and password. The login UI and backend checks are adjusted accordingly, with related functions and conditional rendering eliminated. Additionally, session initialization logic is introduced in the default page layout for cases where no session exists, and notification display logic is updated to use a web component instead of JavaScript pop-ups. Minor formatting and timestamp updates are included, and a new npm "prepare" script is added to the UI package. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage (PHP)
participant System (Shadow/SSO)
participant Session
User->>LoginPage (PHP): Submit username & password
LoginPage (PHP)->>System (Shadow/SSO): verifyUsernamePasswordAndSSO()
alt Credentials valid
LoginPage (PHP)->>Session: Start session, set variables
LoginPage (PHP)-->>User: Redirect to dashboard
else Invalid credentials
LoginPage (PHP)-->>User: Show error message
end
sequenceDiagram
participant Browser
participant DefaultPageLayout (PHP)
Browser->>DefaultPageLayout (PHP): Load page
DefaultPageLayout (PHP)->>DefaultPageLayout (PHP): Check if session exists
alt No session
DefaultPageLayout (PHP)->>DefaultPageLayout (PHP): Start session, set variables
end
DefaultPageLayout (PHP)-->>Browser: Render page
Browser->>Browser: On visibilitychange event
alt $myPage['Load'] > 0
Browser->>Browser: Reload page
else
Browser->>Browser: nchanFocusStart()
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (5)
api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch (1)
9-57:⚠️ Potential issueSSO helper has several edge-case bugs
strlen($password) > 800– many JWTs are ~600 – 700 chars. Rejecting shorter valid tokens will confuse users.preg_match('/^[A-Za-z0-9-_]+.[A-Za-z0-9-_]+.[A-Za-z0-9-_]+$/', …)– the dots are unescaped, so any character is accepted as a separator. Use\..- Splitting response with
explode('{', $output[0], 2)depends on tool output format; preferjson_decode(implode("\n",$output), true).Suggested quick fixes:
- if (strlen($password) > 800) { + if (strlen($password) >= 500) { - if (!preg_match('/^[A-Za-z0-9\-_]+\.[A-Za-z0-9\-_]+\.[A-Za-z0-9\-_]+$/', $password)) { + if (!preg_match('/^[A-Za-z0-9\-_]+\.[A-Za-z0-9\-_]+\.[A-Za-z0-9\-_]+$/', $password)) { - $jsonParts = explode('{', $output[0], 2); - ... + $response = json_decode(implode("\n", $output), true);These tighten validation and reduce false negatives.
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (3)
30-45: 🛠️ Refactor suggestion
openNotifiernow calls nothing → dead code / async noise
openNotifier()still performs an AJAX request but all$.jGrowllines were stripped, leaving a loop that does nothing. This produces needless network calls every 15 s and silently discards notifications.Either:
- Emit notifications with the new
<uui-toaster>, or- Remove the polling entirely.
Example draft:
-$.each($.parseJSON(msg), function(i, notify){ ... }); +uuiToaster = document.querySelector('uui-toaster'); +JSON.parse(msg).forEach(n => { + uuiToaster.show({ message: n.subject + '<br>' + n.description, + color: n.importance }); +});
54-57:⚠️ Potential issueLeft-over bell icon references break JS
The
<div class='nav-user…element was removed, but later code (lines ~81-85) still manipulates$('#bell'). In browsers this now throws “Cannot read properties of null (reading ‘removeClass’)”.Purge those jQuery calls or feature-flag them with
if ($('#bell').length) ….
100-101: 🛠️ Refactor suggestion
<uui-toaster>inserted but never usedThe markup is present, yet no script pushes messages to it. Users will perceive notifications as lost. Hook
openNotifier()(or a new consumer) todocument.querySelector('uui-toaster').show(...).api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php (1)
15-18:⚠️ Potential issueRegex matches anything – escape the dots
preg_match('/^[A-Za-z0-9-_]+.[A-Za-z0-9-_]+.[A-Za-z0-9-_]+$/', $password)
The.meta-character means “any char”. As written, the check accepts any character between segments, defeating format validation. Escape the dots:-'/^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+$/' +'/^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+$/'(Note the backslashes).
🧹 Nitpick comments (6)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (1)
1291-1299: Reload-on-focus can create unnecessary refresh loopsEvery time the tab regains focus and
$myPage['Load'] > 0, you force a fullwindow.location.reload().If the page is already configured to auto-reload through
timers.reload, this extra reload can double-fire, consume bandwidth, and reset UI state for users who simply alt-tab quickly.- window.location.reload(); + // Avoid double reload when timers.reload is already active + if (!timers?.focusReload) { + timers.focusReload = setTimeout(() => location.reload(), 100); + }Consider debouncing or re-using the existing
timers.reloadlogic.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
1278-1292: Minor regex typo may stop focus-resume reload from workingThe block is fine functionally, but note that both branches repeat identical
document.hiddenchecks. You can simplify:if (document.hidden) { nchanFocusStop(); } else if (<?=(isset($myPage['Load']) && $myPage['Load'] > 0) ? 'true' : 'false'?>) { location.reload(); } else { nchanFocusStart(); }This removes one level of nested
ifs.api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts (2)
43-58: Missing opening PHP tag is OK but fragileBecause you insert before the final
?>, the added snippet inherits the already-open PHP context. A future maintainer might move the closing tag and break execution.Safer: wrap the injected block explicitly.
-const newPhpCode = ` -if (session_status() == PHP_SESSION_NONE) { +const newPhpCode = ` +<?php +if (session_status() == PHP_SESSION_NONE) { ... -my_logger("Page accessed without session; created session for root user."); -}`; +my_logger("Page accessed without session; created session for root user."); +?>`;This makes the patch self-contained.
65-66: Transformer order: move auth patch first
removeNotificationBell&replaceToastsoperate on HTML/JS, whilepatchGuiBootAuthchanges PHP before the doctype. Running it first avoids re-parsing the string multiple times.-const transformers = [removeBell, replaceToasts, addToaster, patchGuiBootAuth]; +const transformers = [patchGuiBootAuth, removeBell, replaceToasts, addToaster];Tiny perf gain and clearer intent.
api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch (1)
61-72: Side-effect of function rename: old verifier still present?You replaced calls to
verifyUsernamePasswordwithverifyUsernamePasswordAndSSO, but the original function is still defined elsewhere. For clarity and to avoid accidental reuse, consider:// @deprecated – superseded by verifyUsernamePasswordAndSSO() function verifyUsernamePassword(...) { /* call new fn */ }This provides backward compatibility without silent divergence.
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php (1)
427-438: Duplicate HTML & PHP blocks – factor into includeThe root login form markup is duplicated across snapshot and fixture. Extract to a shared
login-form.inc.phpto reduce maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php(14 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php(14 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php(2 hunks)api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch(1 hunks)justfile(0 hunks)unraid-ui/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- justfile
🧰 Additional context used
🧠 Learnings (1)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2)
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-01-31T22:01:41.842Z
Learning: The default-page-layout.patch is used to remove the old jGrowl notification system from Unraid pages, as notifications are handled by a new system implemented on a different page.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-01-31T22:01:41.842Z
Learning: The default-page-layout.patch removes the old jGrowl notification system and is complemented by the unraid-toaster component implementation. The new system is added through the DefaultPageLayout modification which inserts the toaster component with proper position configuration based on user preferences.
🧬 Code Graph Analysis (2)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php (2)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php (6)
readFromFile(20-32)appendToFile(34-43)writeToFile(45-54)cleanupFails(64-88)isValidTimeStamp(57-62)verifyUsernamePassword(90-102)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php (1)
_(108-108)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php (2)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php (6)
readFromFile(71-83)appendToFile(85-94)writeToFile(96-105)cleanupFails(115-139)isValidTimeStamp(108-113)verifyUsernamePassword(141-153)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php (1)
_(108-108)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1)
1-1: Skip: Timestamp fixture update
This update simply refreshes the fixture’s last-download timestamp. No functional behavior has changed.unraid-ui/package.json (1)
16-16: Approve the newpreparescript
Adding"prepare": "pnpm build"ensures the UI assets are automatically built after dependencies are installed, streamlining local development and deployment.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1)
1-1: Skip: Timestamp fixture update
This change only updates the test fixture timestamp; there are no code logic modifications.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1)
1-1: Skip: Timestamp fixture update
The timestamp was refreshed to keep test snapshots in sync. No functional changes to review.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1)
1-1: Skip: Timestamp fixture update
This fixture’s timestamp aligns with other snapshot updates. No code behavior impacted.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php (1)
20-24: Shell-argument length risk
escapeshellarg($password)guards against injection, but extremely long JWTs (>128 kB) will break the exec buffer. Consider piping via STDIN:proc_open('/etc/rc.d/rc.unraid-api sso validate-token', $descriptors, $pipes); fwrite($pipes[0], $password);
...d-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php
Outdated
Show resolved
Hide resolved
api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts
Show resolved
Hide resolved
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
Outdated
Show resolved
Hide resolved
...rc/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
35-49:⚠️ Potential issueSecurity concern: Automatic root session creation needs additional safeguards
Creating a root session automatically for localhost requests poses a security risk. While the comment indicates this should "only be possible when booting into GUI mode," there's no code-level verification to enforce this constraint. An attacker could potentially trigger this code path outside of the boot process.
Consider adding a verifiable server-side condition before granting root privileges, such as checking for a GUI-boot specific flag or environment variable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php(2 hunks)api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts (2)
35-41: Good regex pattern that accounts for whitespaceThe regex pattern now correctly includes
\s*before the?>which makes it resilient against any potential leading whitespace in the source file.
71-71: Verify the execution order of transformersThe
patchGuiBootAuthtransformer is added as the last in the chain. Make sure this is the intended order, as the other transformers might modify the content in ways that could affect the injection point.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
293-298: Improved page refresh logicThe updated code now intelligently handles window visibility changes:
- If
myPage['Load']is set and greater than 0, it reloads the entire page- Otherwise, it only resumes live updates with
nchanFocusStart()This approach optimizes for both cases - full refresh when needed and efficient incremental updates otherwise.
api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/src/core/utils/validation/is-gui-mode.ts (2)
9-19: Consider memoisingisGuiMode()to avoid spawningpgrepevery call
execa('pgrep', …)is inexpensive once, but this helper will likely be called repeatedly during a single API lifetime (e.g. once per request fromDefaultPageLayoutModification).
A tiny in-memory cache with a short TTL (even 1-2 seconds) eliminates dozens of extra forks without behaviour changes.-let isGuiMode = async (): Promise<boolean> => { +const GUI_TTL = 2_000; // ms +let cachedValue: boolean | null = null; +let cachedAt = 0; + +const isGuiMode = async (): Promise<boolean> => { + if (cachedValue !== null && Date.now() - cachedAt < GUI_TTL) { + return cachedValue; + } try { // Use pgrep to check if slim process is running const { exitCode } = await execa('pgrep', ['slim'], { reject: false }); - // exitCode 0 means process was found, 1 means not found - return exitCode === 0; + cachedValue = exitCode === 0; // 0 → GUI mode + cachedAt = Date.now(); + return cachedValue; } catch (error) { internalLogger.error('Error checking GUI mode: %s', error); return false; } };Nice-to-have but improves performance noticeably on busy installations.
16-17: Use structured logging for errors
internalLogger.error('Error checking GUI mode: %s', error)flattens the stack trace.
Passing the error as metadata keeps the stack searchable:internalLogger.error({ err: error }, 'Error checking GUI mode');api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts (1)
75-87: Minor: avoid doubleisGuiMode()calls
patchGuiBootAuthandgetPregeneratedPatcheach invokeisGuiMode().
With the memoisation suggested earlier this is less costly, but you could still compute once and pass the result down (this.isUnraidGuiModeor via a closure) to simplify flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(1 hunks)api/src/core/utils/validation/is-gui-mode.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts(2 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/dev/Unraid.net/myservers.cfg
- api/dev/states/myservers.cfg
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
- api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/DefaultPageLayout.php.modified.snapshot.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/core/utils/validation/is-gui-mode.ts (1)
api/src/core/log.ts (1)
internalLogger(39-39)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts
Show resolved
Hide resolved
api/src/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification.ts
Show resolved
Hide resolved
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit