add multiple user and multiple language and fix cve - #137
Open
burghy86 wants to merge 2 commits into
Open
Conversation
added 2 commits
May 25, 2026 11:58
Replit-Commit-Author: Agent Replit-Commit-Session-Id: 1d5a5e7a-1e52-4679-b44e-03e5db1b76d1 Replit-Commit-Checkpoint-Type: full_checkpoint Replit-Commit-Event-Id: 03fa99f7-c526-4a2e-ba94-35482042662f Replit-Helium-Checkpoint-Created: true
Integrates internationalization with English and Italian translations, enhances the login process for multi-user environments, and includes security fixes. Replit-Commit-Author: Agent Replit-Commit-Session-Id: 1d5a5e7a-1e52-4679-b44e-03e5db1b76d1 Replit-Commit-Checkpoint-Type: full_checkpoint Replit-Commit-Event-Id: 488d1972-701b-43b3-8ba1-c20a2ddc316f Replit-Commit-Screenshot-Url: https://storage.googleapis.com/screenshot-production-us-central1/0cfb949a-9516-4ec4-ac9a-15fbc70c2b2a/1d5a5e7a-1e52-4679-b44e-03e5db1b76d1/kfWRAC8 Replit-Helium-Checkpoint-Created: true
WalkthroughThis PR implements internationalization (English/Italian), multi-user PIN authentication, dashboard language switching, server-side user verification with IP lockout, path traversal protection for file deletion, and includes CSS styling, CSV export improvements, search persistence, and Replit configuration. ChangesMulti-user authentication and internationalization infrastructure
Dashboard UI integration, styling, and hardening
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.js (1)
351-355:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMulti-user login is bypassed when
DUMBASSETS_PINis unset.This branch redirects from
/loginwheneverPINis empty, which breaks multi-user auth flow (and can cause redirect loops) because multi-user mode doesn’t requireDUMBASSETS_PIN.Suggested fix
- if (!PIN || PIN.trim() === '') { + if (!MULTI_USER_MODE && (!PIN || PIN.trim() === '')) { const returnTo = req.query.returnTo || (BASE_PATH + '/'); debugLog('No PIN set, redirecting to:', returnTo); return res.redirect(returnTo); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` around lines 351 - 355, The redirect that runs when PIN is empty improperly triggers in multi-user deployments; update the condition around PIN so it only redirects for single-user mode: replace the current if (!PIN || PIN.trim() === '') check with a guard that also verifies the app is not running in multi-user mode (e.g., check your existing multi-user flag/function instead of PIN alone), and only then compute returnTo and call res.redirect(returnTo); keep references to PIN, req.query.returnTo, BASE_PATH and res.redirect to locate the code to change.
🧹 Nitpick comments (2)
public/managers/settings.js (1)
648-649: ⚡ Quick winDon’t bother swapping
quantity || 1toquantity ?? 1inpublic/managers/settings.js—0is already turned into1upstream (stupidly)
- The quantity inputs enforce
min="1"(public/index.html).- Client form parsing also forces
1viaparseInt(...) || 1(public/managers/modalManager.js).- Server import + GET/back-compat normalization coerce
0to1(parseInt(get('quantity')) || 1andquantity: asset.quantity || 1/subAsset.quantity || 1inserver.js).- So the CSV export rows won’t contain real
0anyway; doing it only in the export is pointless—if you care about0, fix the upstream coercion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/managers/settings.js` around lines 648 - 649, Leave the export expression asset.quantity || 1 as-is (do not change it to asset.quantity ?? 1); the current upstream inputs and parsing coerce 0 to 1, so changing this line alone is pointless—if you need true-0 support, update the upstream coercion points instead (e.g., the client parsing in modalManager.js, server-side normalization in server.js, and the quantity input constraint).public/i18n/translations.js (1)
464-513: ⚡ Quick winAdd JSDoc to the public i18n API methods.
t,setLanguage,getLanguage, andapplyTranslationsare exposed publicly viawindow.i18n, but they currently lack API-level JSDoc.As per coding guidelines, "JSDoc comments must be present for public functions and APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/i18n/translations.js` around lines 464 - 513, Add JSDoc comments for the public i18n API functions exported on window: t, setLanguage, getLanguage, and applyTranslations (also mention window.t). For each function add a short description, `@param` tags (e.g., t(key: string, fallback?: string), setLanguage(lang: string)), and `@returns` tags where applicable (t returns string, getLanguage returns string) and include `@public/`@exports or similar tag per project style; place the comments immediately above the function declarations for detectability by documentation tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.js`:
- Around line 380-383: The /api/whoami route is registered at the root instead
of being mounted under BASE_PATH causing inconsistent routing; change the
registration from app.get('/api/whoami', ...) to use the BASE_PATH prefix (e.g.
app.get(`${BASE_PATH}/api/whoami`, ...)) so it matches other auth/login routes,
keeping the same auth check (req.session.authenticated) and response logic
(username and MULTI_USER_MODE); after changing the route, update any
client/dashboard code that fetches the endpoint to use window.appConfig.basePath
or BASE_PATH (e.g. `${window.appConfig.basePath}/api/whoami`) so calls align
with the new mount point.
---
Outside diff comments:
In `@server.js`:
- Around line 351-355: The redirect that runs when PIN is empty improperly
triggers in multi-user deployments; update the condition around PIN so it only
redirects for single-user mode: replace the current if (!PIN || PIN.trim() ===
'') check with a guard that also verifies the app is not running in multi-user
mode (e.g., check your existing multi-user flag/function instead of PIN alone),
and only then compute returnTo and call res.redirect(returnTo); keep references
to PIN, req.query.returnTo, BASE_PATH and res.redirect to locate the code to
change.
---
Nitpick comments:
In `@public/i18n/translations.js`:
- Around line 464-513: Add JSDoc comments for the public i18n API functions
exported on window: t, setLanguage, getLanguage, and applyTranslations (also
mention window.t). For each function add a short description, `@param` tags (e.g.,
t(key: string, fallback?: string), setLanguage(lang: string)), and `@returns` tags
where applicable (t returns string, getLanguage returns string) and include
`@public/`@exports or similar tag per project style; place the comments
immediately above the function declarations for detectability by documentation
tooling.
In `@public/managers/settings.js`:
- Around line 648-649: Leave the export expression asset.quantity || 1 as-is (do
not change it to asset.quantity ?? 1); the current upstream inputs and parsing
coerce 0 to 1, so changing this line alone is pointless—if you need true-0
support, update the upstream coercion points instead (e.g., the client parsing
in modalManager.js, server-side normalization in server.js, and the quantity
input constraint).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fc699a1-10e0-43ac-a299-be30e509372e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.replitpublic/config.jspublic/i18n/translations.jspublic/index.htmlpublic/login.htmlpublic/managers/settings.jspublic/script.jspublic/styles.cssserver.js
Comment on lines
+380
to
+383
| app.get('/api/whoami', (req, res) => { | ||
| if (!req.session.authenticated) return res.status(401).json({ error: 'Not authenticated' }); | ||
| res.json({ username: req.session.username || null, multiUser: MULTI_USER_MODE }); | ||
| }); |
There was a problem hiding this comment.
/api/whoami is mounted outside BASE_PATH.
All auth/login routes are BASE_PATH-aware, but this endpoint is root-mounted. That causes inconsistent routing in subpath deployments and complicates client calls.
Suggested fix
-app.get('/api/whoami', (req, res) => {
+app.get(BASE_PATH + '/api/whoami', (req, res) => {
if (!req.session.authenticated) return res.status(401).json({ error: 'Not authenticated' });
res.json({ username: req.session.username || null, multiUser: MULTI_USER_MODE });
});Downstream impact: update the dashboard fetch call to use ${window.appConfig.basePath}/api/whoami.
📝 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
| app.get('/api/whoami', (req, res) => { | |
| if (!req.session.authenticated) return res.status(401).json({ error: 'Not authenticated' }); | |
| res.json({ username: req.session.username || null, multiUser: MULTI_USER_MODE }); | |
| }); | |
| app.get(BASE_PATH + '/api/whoami', (req, res) => { | |
| if (!req.session.authenticated) return res.status(401).json({ error: 'Not authenticated' }); | |
| res.json({ username: req.session.username || null, multiUser: MULTI_USER_MODE }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 380 - 383, The /api/whoami route is registered at the
root instead of being mounted under BASE_PATH causing inconsistent routing;
change the registration from app.get('/api/whoami', ...) to use the BASE_PATH
prefix (e.g. app.get(`${BASE_PATH}/api/whoami`, ...)) so it matches other
auth/login routes, keeping the same auth check (req.session.authenticated) and
response logic (username and MULTI_USER_MODE); after changing the route, update
any client/dashboard code that fetches the endpoint to use
window.appConfig.basePath or BASE_PATH (e.g.
`${window.appConfig.basePath}/api/whoami`) so calls align with the new mount
point.
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.
Security (CVE / Path Traversal)
deleteAssetFileAsync: Added apath.resolve()check againstDATA_DIRto prevent path traversal vulnerabilities./api/delete-file: Implemented the same protection; now returns a403 Forbiddenstatus code if the resolved path falls outside ofDATA_DIR.Internationalization (i18n)
public/i18n/translations.js: Created a complete EN/IT translation module exposingwindow.i18nandwindow.t().index.htmlandlogin.htmlensuring it loads beforeconfig.js.data-i18nanddata-i18n-placeholderattributes to key elements..lang-btn,.language-options).Multi-user Support
/api/users: Endpoint added to fetch the user list (public, unauthenticated)./api/whoami: Endpoint added to return the current session user (authenticated)./verify-pin: Refactored to handle both single-user and multi-user modes (requiring username + PIN)./pin-length: Updated to returnmultiUser: truewhen active.login.htmlto support a new multi-step flow: User Selection → PIN Input → Back Button..user-btn,.user-list, and.back-btn.publicPathsto allow access to/api/usersand/i18n/.Bug Fixes
loadAssets()now caches the search term before fetching and reapplies it afterward.Quantitycolumn to both CSV generators (full and simple exports).High-level PR Summary
This PR introduces multi-user authentication support with username selection and individual PIN verification, adds internationalization (i18n) with English and Italian translations, and fixes a path traversal security vulnerability (CVE) in file deletion endpoints. The changes include a new translation module with
data-i18nattributes throughout the UI, a completely rewritten multi-step login flow that handles both single-user and multi-user modes, security enhancements usingpath.resolve()checks to prevent directory traversal attacks, and several bug fixes including missingQuantitycolumn in CSV exports and search query persistence issues.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
server.jspublic/i18n/translations.jspublic/login.htmlpublic/index.htmlpublic/config.jspublic/script.jspublic/managers/settings.jspublic/styles.csspackage-lock.json.replit.replitSummary by CodeRabbit
New Features
Bug Fixes