Guard /change-password against anonymous visitors - #8414
Conversation
A signed-out visitor was served the complete change-password form, fully rendered and interactive. Submitting could only ever fail: with no token userId resolves to '' and the backend rejects updatePassword. Render Unauthorized when there is no token. It already redirects to the login route with a return_url, which is the behaviour the other manage surfaces have. Closes plone#8367
There was a problem hiding this comment.
🟡 Changes recommended
The new test uses a Route path="*" catch-all that can be unreliable in react-router v5 and should be adjusted to a pathless fallback route to avoid flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Volto’s /change-password route so anonymous visitors no longer see an interactive change-password form that cannot succeed; instead, the ChangePassword component renders the existing Unauthorized behavior which redirects to login with a return_url.
Changes:
- Added a token guard in
ChangePasswordto render<Unauthorized />when signed out. - Added a test case asserting the form is not rendered for anonymous visitors and that navigation occurs away from the route.
- Added a news fragment documenting the bugfix.
File summaries
| File | Description |
|---|---|
| packages/volto/src/components/manage/Preferences/ChangePassword.jsx | Adds an auth/token guard to prevent rendering the change-password form for anonymous visitors. |
| packages/volto/src/components/manage/Preferences/ChangePassword.test.jsx | Adds an anonymous-visitor test using router navigation to validate redirect behavior. |
| packages/volto/news/8367.bugfix | Documents the fix in the Volto changelog/news fragments. |
Review details
Suppressed comments (1)
packages/volto/src/components/manage/Preferences/ChangePassword.jsx:77
- This adds a second
useSelectorsubscription (token) even thoughuserIdis already derived from the same state; it introduces redundant store subscriptions and duplicate reads. Consider selecting{token, userId}together (or derivinguserIdfromtoken) to keep the render triggers consistent and slightly reduce overhead.
const token = useSelector((state) => state.userSession.token);
const userId = useSelector(
(state) =>
state.userSession.token ? jwtDecode(state.userSession.token).sub : '',
shallowEqual,
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <Switch> | ||
| <Route exact path="/change-password" component={ChangePassword} /> | ||
| <Route path="*" render={() => <div id="redirected" />} /> | ||
| </Switch> |
There was a problem hiding this comment.
Agreed — changed in bc29796. A <Route> with no path always matches, so as the last child of the <Switch> it is the catch-all; path="*" was leaning on path-to-regexp to do the same job.
Re-checked that the test still isolates what it is meant to: removing the !token early return from ChangePassword fails it, restoring it passes.
A <Route> with no `path` always matches, so as the last child of a <Switch> it is the catch-all. `path="*"` leans on path-to-regexp instead, which is not guaranteed to match every redirected location in react-router v5. Verified the test still isolates the guard: removing the `!token` early return fails it, restoring it passes.
Closes #8367.
A signed-out visitor loading
/change-passwordwas served the complete form — Current password / New password / Confirm — fully rendered and interactive. As @mpalomaki noted on the issue, submitting could only ever fail: with no tokenuserIdresolves to''and the backend rejectsupdatePassword. So it is a form that cannot succeed rather than a data leak, but it is still the wrong thing to show.Took the component-level guard suggested in the issue, as the smallest change that covers every registration:
ChangePasswordnow rendersUnauthorizedwhen there is no token.Unauthorizedalready does exactly what the issue asks for in that state — it redirects to the login route with?return_url=, so a signed-out visitor lands on login and is returned afterwards. No new redirect logic was needed.I left
nonContentRoutesPublicalone. The guard covers the behaviour, and removing/change-passwordfrom that list looked like it could affect route classification more broadly than this issue calls for. Happy to do that too if you'd prefer both.Test
ChangePassword.test.jsxgains an anonymous case asserting the form is not rendered and that the router has navigated away.Worth flagging: it renders through a
Switch/Routerather than mounting the component directly. Mounted directly, the redirect re-mountsChangePasswordat the new path and it redirects again — "Maximum update depth exceeded". That is an artefact of rendering a redirecting component outside a route, not a loop in the product, but it is why the test is shaped this way.2/2 passing in that file. Prettier clean.
Note for anyone reproducing locally:
packages/registryhas to be built (npx tsupin that package) before volto's tests can resolve@plone/registry, otherwise every test file fails to transform on a clean checkout too.