Add pw change page /password/ - #91
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The nginx config as added is invalid (nested location) and the password-change endpoint currently has critical server-side security issues (shell injection + missing server-side whitelist validation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new /password/ page and backend endpoint intended to let users change the MIRTE password via the web interface, and wires it into nginx + install script so it’s deployed on the robot image.
Changes:
- Add a static HTML password-change page that POSTs JSON to a backend endpoint.
- Add a PHP endpoint that updates the
mirteuser password viasu/chpasswd. - Expose
/password/via nginx and copy the new site into/var/www/html/during install.
File summaries
| File | Description |
|---|---|
sites/password/index.html |
New UI for changing the password and toggling password visibility. |
sites/password/change_password.php |
New PHP endpoint to validate inputs and run the password change command. |
sites/401.html |
Adds a link to the new password page from the 401 page. |
nginx.conf |
Adds routing intended to serve /password/ and execute PHP. |
install_web.sh |
Installs php-fpm and copies the new /password/ site to the web root. |
Review details
Suppressed comments (1)
sites/password/index.html:83
- Same issue for the new password field:
type="input"makes the password visible by default and misaligns the toggle button state. Use apasswordinput and mark it as a new password for browser password managers.
<input type="input" id="new-password" name="new-password" required>
<button type="button" class="toggle-password" data-target="new-password">Hide</button>
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // su will return after a few seconds if incorrect, otherwise it's 'instantaneous' and the old password was correct. | ||
| // return code 0 means success, other means failure (incorrect old password) | ||
| exec("echo $old_password | su -c \"echo mirte:$new_password | sudo chpasswd \" mirte", $out, $return_var); |
There was a problem hiding this comment.
because it's already 'safe' as the regex checks it. But I'll add it to be 100% sure!
There was a problem hiding this comment.
I was afraight that the escapeshellarg would add " or ' to the string, messing up mirte:$new_password for chpasswd, but it works!
mklomp
left a comment
mklomp
left a comment
There was a problem hiding this comment.
I think this is good for now, as it solves our problem when we run the workshops. Two things that we might need to change for future implementations:
- Maybe change implementation language?
But more imoirtantly:
- In the current setup we give the ability to change the default password when it is connected to a shared network. So basically anyone can also change the password (since they know the default one). I think the actual check should not be the ability to change the password. I think we should not allow connecting to other networks as long as the password was not changed. This of course is easy for the web interface, but probably harder when connecting to a network through the commandline.
There was a problem hiding this comment.
As discussed, PHP is fine for now. But in the future we might want to refactor this to a language we are already running on the MIRTE (eg pyhon or maybe nodejs).
There was a problem hiding this comment.
When connecting with the commandline, you already had to change it (ssh), except with vscode, then that requirement isn't there.
No description provided.