fix: Security upgrade passport from 0.5.3 to 0.6.0 #3000
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughReplaced cookie-session with express-session in Parse-Dashboard/Authentication.js, moved connect-flash initialization after session middleware, made logout asynchronous using Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant E as Express App
participant S as express-session
participant F as connect-flash
participant P as Passport
rect rgba(200,230,255,0.20)
note over E,S: New middleware order: session -> flash -> passport
C->>E: Request (login / protected)
E->>S: session middleware (attach req.session)
E->>F: flash middleware (attach req.flash)
E->>P: Passport authenticate (reads/writes req.session)
P-->>E: Auth result (user on req)
E-->>C: Response (Set-Cookie if session created)
end
rect rgba(220,255,220,0.20)
note over C,E: Logout (async)
C->>E: POST /logout
E->>P: req.logout(callback)
alt success
P-->>E: callback(null)
E-->>C: 302 Redirect to login
else error
P-->>E: callback(error)
E->>E: next(error)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
|
@mtrezza ready to review, i checked that everything works locally. So we remove the vulnerability warning of parse-dashboard |
Uffizzi Ephemeral Environment
|
![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
🧹 Nitpick comments (3)
package.json (1)
45-45: Session middleware added — plan for production store and cookie hardeningGood switch. For production, avoid the default MemoryStore; wire a store (Redis/Mongo/etc.) and ensure cookie options (secure/sameSite) are set in the middleware config in Authentication.js. Based on learnings
Parse-Dashboard/Authentication.js (2)
60-68: Optional: allow external session store via optionsTo avoid MemoryStore in production, accept options.sessionStore and pass it to express-session. Based on learnings
Apply:
- app.use(require('express-session')({ + app.use(require('express-session')({ name: 'parse_dash', secret: cookieSessionSecret, resave: false, saveUninitialized: false, + store: options.sessionStore || undefined, cookie: { maxAge: cookieSessionMaxAge != null ? Number(cookieSessionMaxAge) : undefined, httpOnly: true, sameSite: 'lax', secure: 'auto' } }));
87-92: Logout should also destroy the session and clear cookiereq.logout removes req.user but leaves the session. Destroy it to prevent reuse, then redirect.
Apply:
- app.get('/logout', function (req, res, next) { - req.logout(function (err) { - if (err) { return next(err); } - res.redirect(`${self.mountPath}login`); - }); - }); + app.get('/logout', function (req, res, next) { + req.logout(function (err) { + if (err) { return next(err); } + // Destroy session and clear cookie + if (req.session) { + req.session.destroy(() => { + res.clearCookie('parse_dash'); + res.redirect(`${self.mountPath}login`); + }); + } else { + res.clearCookie('parse_dash'); + res.redirect(`${self.mountPath}login`); + } + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
Parse-Dashboard/Authentication.js(2 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Parse-Dashboard/Authentication.js (1)
Parse-Dashboard/server.js (2)
cookieSessionSecret(20-20)cookieSessionMaxAge(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (2)
Parse-Dashboard/Authentication.js (1)
60-68: Harden session cookie and coerce maxAge
- In Parse-Dashboard/Authentication.js (lines 60–68), wrap
cookieSessionMaxAgeinNumber()if it may be a string and add:httpOnly: true, sameSite: 'lax', secure: 'auto'- Verify
app.set('trust proxy', 1)is enabled when running behind a proxy.package.json (1)
60-60: Passport 0.7.0 upgrade — no legacy API usage found Verified that all req.logout calls use the callback syntax; no remaining req.login/logIn or cookie-session references.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Parse-Dashboard/Authentication.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Parse-Dashboard/Authentication.js (1)
Parse-Dashboard/server.js (3)
app(160-160)cookieSessionSecret(20-20)cookieSessionMaxAge(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (1)
Parse-Dashboard/Authentication.js (1)
88-93: LGTM! Async logout correctly implemented.The logout handler correctly migrates to passport 0.7.0's asynchronous API, properly handling errors and redirecting only after successful logout.
|
@Moumouls is this a feature (i.e. any notable change for the dev / usr), or a fix due to the vulnerability? |
|
@coderabbitai Suggest a PR title that describes the change in this PR and is according to angular commit convention. |
|
Tip For best results, initiate chat on the files or code changes. @mtrezza, based on the changes in this PR (migrating from Primary recommendation: Alternative options: If emphasizing the authentication system change: If emphasizing the dependency updates: If this is considered more of a refactoring than a fix: Reasoning:
Would you like me to adjust any of these suggestions? |
mtrezza
changed the title
# [7.6.0-alpha.9](7.6.0-alpha.8...7.6.0-alpha.9) (2025-10-14) ### Bug Fixes * Security upgrade passport from 0.5.3 to 0.6.0 ([#3000](#3000)) ([fbb5e6d](fbb5e6d))
|
🎉 This change has been released in version 7.6.0-alpha.9 |
# [8.0.0](7.5.0...8.0.0) (2025-11-01) ### Bug Fixes * Add missing major version increase of dashboard release ([#3005](#3005)) ([5debb4d](5debb4d)) * Cannot connect to server with error invalid header name ([#3006](#3006)) ([ea4ec07](ea4ec07)) * Currently displayed view reloads when editing and saving a different view ([#3002](#3002)) ([794a35a](794a35a)) * Dashboard config objects stored on server with public read / write access ([#2997](#2997)) ([31a4639](31a4639)) * ESC key does not cancel editing in data browser cell ([#3001](#3001)) ([d1d7241](d1d7241)) * Filter text field in data browser partly looses focus when hitting enter key to apply filter ([#2992](#2992)) ([e3085b9](e3085b9)) * Filter text field in data browser partly looses focus when selecting in drop-down element by hitting enter key to apply filter ([#2993](#2993)) ([f4c17c7](f4c17c7)) * Info panel briefly shows cached media content from previously selected cell when using pre-fetch ([#3008](#3008)) ([dd6a85e](dd6a85e)) * Missing alert when changing data browser browser data while rows are selected ([#2994](#2994)) ([6cabaa3](6cabaa3)) * Security upgrade parse from 3.5.1 to 7.0.1 ([#3003](#3003)) ([5123fbf](5123fbf)) * Security upgrade passport from 0.5.3 to 0.6.0 ([#3000](#3000)) ([fbb5e6d](fbb5e6d)) * Session management issue that causes malformed redirect URLs ([#3011](#3011)) ([1649dd3](1649dd3)) * Storing view on server creates view key with hashed view name instead of UUID ([#2995](#2995)) ([7cb65f3](7cb65f3)) * Switching between browser tabs can cause illegible text color for config parameter value field ([#3010](#3010)) ([77c5c67](77c5c67)) * View table data may be retained when switching between views ([#2996](#2996)) ([ddc91c9](ddc91c9)) ### Features * Add `matches regex` filter to data browser replacing limited `string contains string` filter ([#2991](#2991)) ([64a9f71](64a9f71)) * Add info panel options `prefetchImage`, `prefetchVideo`, `prefetchAudio` to pre-fetch media content in the info panel ([#3009](#3009)) ([6796c9e](6796c9e)) * Add Parse Server version compatibility detection ([#3004](#3004)) ([9a7a60f](9a7a60f)) ### Performance Improvements * Storing, deleting, modifying view in server storage now only affects the specific view instead of updating all views ([#2998](#2998)) ([48cea3c](48cea3c)) ### BREAKING CHANGES * This increases the required minimum version to Parse Server 7. ([5debb4d](5debb4d))
|
🎉 This change has been released in version 8.0.0 |
3 participants
New Pull Request Checklist
Issue Description
Use express session system
Closes: #2759
Approach
Use express session
TODOs before merging
Note
I checked that everything works in locla, login without and with Parse Dashboard users is working, session is correctly set in Cookies with HTTP only
Summary by CodeRabbit
Bug Fixes
Chores