Revert password changes - #309
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe change replaces shared master-password validation with separate password configuration and checks for driver updates, finish-line updates, and snapshot creation. ChangesPassword validation updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/server/src/utils/validatePassword.ts (1)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent env-var reading and error-handling patterns across the two validators.
validateDriverUpdatePasswordreads its env var at module load (fail-fast at startup), whilevalidateSnapshotPasswordreads at call time (fails on first request). TheLapControllerdoes a rawprocess.envcomparison with no existence check at all (fail-open if unset). Consider unifying all three into a single pattern — ideally read-at-call-time with an explicit missing-env-var check — to avoid surprising behavior differences.♻️ Suggested unified pattern
export function validateDriverUpdatePassword(password: string): boolean { + const driverPassword = process.env.DRIVER_NAME_UPDATE_PASSWORD; + if (!driverPassword) { + throw new Error("DRIVER_NAME_UPDATE_PASSWORD environment variable is not configured"); + } - return password === validPassword; + return password === driverPassword; }This aligns
validateDriverUpdatePasswordwithvalidateSnapshotPassword's read-at-call-time approach and makes the missing-env-var behavior explicit.🤖 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 `@packages/server/src/utils/validatePassword.ts` around lines 30 - 43, Unify password validation to read environment variables at call time and reject missing configuration explicitly. Update validateDriverUpdatePassword to read and validate its driver-password env var inside the function, matching validateSnapshotPassword, and update LapController’s password comparison to use the same validator or equivalent missing-env-var check; preserve the existing valid-password comparisons and error behavior.
🤖 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 `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 140-141: Update the password validation in the LapController check
to reject requests when FINISH_LINE_UPDATE_PASSWORD is unset, preventing missing
credentials from passing validation. Replace the logger.error message with a
generic invalid-password message that never includes the supplied password,
matching the behavior of the other controllers.
In `@packages/server/src/controllers/routeControllers/snapshot.controller.ts`:
- Line 85: Update the snapshot controller flow around validateSnapshotPassword
so an unset SNAPSHOT_PASSWORD is validated during application startup, following
the existing validateDriverUpdatePassword pattern, rather than allowing its
configuration error to reach clients. If startup validation is not applicable,
catch that failure and route a generic 500 response without exposing the
missing-secret message.
In `@packages/server/src/utils/validatePassword.ts`:
- Around line 1-4: Update validateDriverUpdatePassword to read the
DRIVER_NAME_UPDATE_PASSWORD environment variable instead of MQTT_PASSWORD, and
change the missing-configuration error message to reference
DRIVER_NAME_UPDATE_PASSWORD so driver update validation uses its dedicated
secret.
---
Nitpick comments:
In `@packages/server/src/utils/validatePassword.ts`:
- Around line 30-43: Unify password validation to read environment variables at
call time and reject missing configuration explicitly. Update
validateDriverUpdatePassword to read and validate its driver-password env var
inside the function, matching validateSnapshotPassword, and update
LapController’s password comparison to use the same validator or equivalent
missing-env-var check; preserve the existing valid-password comparisons and
error behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c9b62c2-35c8-4099-ae28-3466aae0592c
📒 Files selected for processing (6)
packages/amplify/amplify/backend.tspackages/server/.env.examplepackages/server/src/controllers/LapController/LapController.tspackages/server/src/controllers/routeControllers/driver.controller.tspackages/server/src/controllers/routeControllers/snapshot.controller.tspackages/server/src/utils/validatePassword.ts
| if (password !== process.env.FINISH_LINE_UPDATE_PASSWORD) { | ||
| logger.error("Invalid Password: " + password); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Password is logged in the error message and the env-var check is fail-open.
Two issues on this password check:
-
CWE-532 — Sensitive data in logs:
logger.error("Invalid Password: " + password)logs the raw password value. The other controllers (driver.controller.ts,snapshot.controller.ts) log a generic "Invalid password attempt" message without the value. This is inconsistent and leaks credentials to log files. -
Fail-open when
FINISH_LINE_UPDATE_PASSWORDis unset: The raw comparisonpassword !== process.env.FINISH_LINE_UPDATE_PASSWORDpasses if both sides areundefined(env var not configured + missing password in payload). UnlikevalidateSnapshotPasswordwhich throws when the env var is missing, this check silently allows access.
🔒 Proposed fix: remove password from log and guard against unset env var
const { lat, long, password } = newCoordInfo;
- if (password !== process.env.FINISH_LINE_UPDATE_PASSWORD) {
- logger.error("Invalid Password: " + password);
+ const finishLinePassword = process.env.FINISH_LINE_UPDATE_PASSWORD;
+ if (!finishLinePassword || password !== finishLinePassword) {
+ logger.error("Invalid password attempt for finish line update");
return { error: "Invalid Password", invalidFields: ["password"] };
}📝 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.
| if (password !== process.env.FINISH_LINE_UPDATE_PASSWORD) { | |
| logger.error("Invalid Password: " + password); | |
| if (password !== process.env.FINISH_LINE_UPDATE_PASSWORD) { | |
| logger.error("Invalid Password: " + password); |
| if (password !== process.env.FINISH_LINE_UPDATE_PASSWORD) { | |
| logger.error("Invalid Password: " + password); | |
| const finishLinePassword = process.env.FINISH_LINE_UPDATE_PASSWORD; | |
| if (!finishLinePassword || password !== finishLinePassword) { | |
| logger.error("Invalid password attempt for finish line update"); | |
| return { error: "Invalid Password", invalidFields: ["password"] }; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 140-140: Avoid logging sensitive data
Context: logger.error("Invalid Password: " + password)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 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 `@packages/server/src/controllers/LapController/LapController.ts` around lines
140 - 141, Update the password validation in the LapController check to reject
requests when FINISH_LINE_UPDATE_PASSWORD is unset, preventing missing
credentials from passing validation. Replace the logger.error message with a
generic invalid-password message that never includes the supplied password,
matching the behavior of the other controllers.
Source: Linters/SAST tools
| } | ||
|
|
||
| if (!validateMasterPassword(password)) { | ||
| if (!validateSnapshotPassword(password)) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the asyncHandler implementation and error-handling middleware to verify
# whether raw error messages are sanitized before reaching the client.
rg -nC5 "asyncHandler" --type=ts packages/server/src/
rg -nC5 "error.*handler\|ErrorHandler\|err.*req.*res" --type=ts packages/server/src/ -g '!**/node_modules/**'Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 5895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== errorHandler implementation =="
cat -n packages/server/src/middleware/errorHandler.ts
echo
echo "== snapshot controller around validateSnapshotPassword =="
rg -nC6 "validateSnapshotPassword|createSnapshot|SNAPSHOT_PASSWORD" packages/server/src/controllers/routeControllers/snapshot.controller.ts
echo
echo "== server/app wiring for error middleware =="
rg -nC4 "app.use\\(|errorHandler|asyncHandler|express\\(" packages/server/src -g '!**/node_modules/**'
echo
echo "== startup validation for driver update password =="
rg -nC6 "validateDriverUpdatePassword|DRIVER_UPDATE_PASSWORD|throw new Error" packages/server/srcRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 23647
Avoid returning the missing-secret error to clients. validateSnapshotPassword() throws when SNAPSHOT_PASSWORD is unset, and errorHandler sends err.message in the JSON response, so the client can see SNAPSHOT_PASSWORD environment variable is not configured. Move this check to startup like validateDriverUpdatePassword() or map it to a generic 500.
🤖 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 `@packages/server/src/controllers/routeControllers/snapshot.controller.ts` at
line 85, Update the snapshot controller flow around validateSnapshotPassword so
an unset SNAPSHOT_PASSWORD is validated during application startup, following
the existing validateDriverUpdatePassword pattern, rather than allowing its
configuration error to reach clients. If startup validation is not applicable,
catch that failure and route a generic 500 response without exposing the
missing-secret message.
| const validPassword = process.env.MQTT_PASSWORD; | ||
|
|
||
| if (!validPassword) { | ||
| throw new Error("UI_MASTER_PASSWORD environment variable is not configured"); | ||
| throw new Error("MQTT_PASSWORD environment variable is not configured"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that DRIVER_NAME_UPDATE_PASSWORD is not referenced anywhere in the application code.
rg -n "DRIVER_NAME_UPDATE_PASSWORD" --type=ts -g '!**/*.env*' -g '!**/backend.ts'
# Also check if MQTT_PASSWORD is referenced beyond MQTT connection setup.
rg -nC3 "MQTT_PASSWORD" --type=ts -g '!**/*.env*' -g '!**/backend.ts'Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== validatePassword.ts ==\n'
git ls-files 'packages/server/src/utils/validatePassword.ts' 'packages/server/src/**' | cat
echo
wc -l packages/server/src/utils/validatePassword.ts 2>/dev/null || true
echo
cat -n packages/server/src/utils/validatePassword.ts 2>/dev/null || true
printf '\n== References to DRIVER_NAME_UPDATE_PASSWORD ==\n'
rg -n -S "DRIVER_NAME_UPDATE_PASSWORD" .
printf '\n== References to MQTT_PASSWORD ==\n'
rg -n -C 3 -S "MQTT_PASSWORD" packages/server/src .Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 9672
validateDriverUpdatePassword should read DRIVER_NAME_UPDATE_PASSWORD, not MQTT_PASSWORD. The new secret is wired into deployment, but this validator still gates driver updates on the MQTT broker password, so the two credentials remain coupled. Update the env var and error message here so driver updates use their own secret.
🤖 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 `@packages/server/src/utils/validatePassword.ts` around lines 1 - 4, Update
validateDriverUpdatePassword to read the DRIVER_NAME_UPDATE_PASSWORD environment
variable instead of MQTT_PASSWORD, and change the missing-configuration error
message to reference DRIVER_NAME_UPDATE_PASSWORD so driver update validation
uses its dedicated secret.
Source: Linters/SAST tools
2 participants
Summary by CodeRabbit
New Features
Bug Fixes