🛡️ Sentinel: [CRITICAL] Fix hardcoded staff password#42
🛡️ Sentinel: [CRITICAL] Fix hardcoded staff password#42
Conversation
- Removed hardcoded 'SAC_MUSEUM_2026' password from client-side `js/main.js`. - Implemented secure backend verification endpoint `/api/verify-staff` in `backend/main.py`. - Added `StaffLogin` model to `backend/models.py`. - Password comparison now uses `hmac.compare_digest` to prevent timing attacks. - Staff password is now configurable via the `STAFF_PASSWORD` environment variable. - Added comprehensive integration tests in `backend/tests/test_staff.py`. - Verified UI flow with automated Playwright verification. Co-authored-by: LVT-ENG <214667862+LVT-ENG@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates staff password verification from the frontend to a new backend API endpoint /api/verify-staff. The implementation includes a StaffLogin model, secure password comparison using hmac.compare_digest, and a Vite proxy configuration. Feedback identifies a critical security risk regarding the hardcoded default password in the backend and suggests using monkeypatch in tests to avoid hardcoding credentials.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Yes |
There was a problem hiding this comment.
Pull request overview
This PR moves the staff “private pass” verification from a hardcoded frontend string to a backend-validated endpoint, so the password is no longer exposed in client-side JavaScript.
Changes:
- Added a new FastAPI endpoint (
POST /api/verify-staff) to validate staff credentials server-side. - Refactored the frontend staff check to call the backend endpoint via
fetch. - Added a Vite dev proxy for
/apito forward requests to the backend during local development, plus new backend tests for the staff verification endpoint.
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
vite.config.js |
Adds dev-time /api proxy to the FastAPI server. |
js/main.js |
Replaces hardcoded staff password check with an async call to /api/verify-staff. |
backend/main.py |
Introduces /api/verify-staff and reads STAFF_PASSWORD from env. |
backend/models.py |
Adds StaffLogin request model for the staff verification endpoint. |
backend/tests/test_staff.py |
Adds tests intended to cover staff verification success/failure cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @app.post("/api/verify-staff") | ||
| def verify_staff(login: StaffLogin): | ||
| """🛡️ Secure staff password verification via backend.""" |
There was a problem hiding this comment.
STAFF_PASSWORD is read from the environment with no default and can be None/empty. hmac.compare_digest(login.password, STAFF_PASSWORD) will raise a TypeError when STAFF_PASSWORD is None, causing a 500 instead of a clean auth failure. Please explicitly handle the missing/empty configuration (e.g., fail closed with a clear 5xx error, or provide an explicit dev-only default as described in the PR).
| """🛡️ Secure staff password verification via backend.""" | |
| """🛡️ Secure staff password verification via backend.""" | |
| # Ensure staff authentication is properly configured before comparing. | |
| if not STAFF_PASSWORD: | |
| # Fail closed with a clear server-side configuration error. | |
| raise HTTPException( | |
| status_code=500, | |
| detail="STAFF_PASSWORD is not configured on the server.", | |
| ) |
| @@ -0,0 +1,23 @@ | |||
| import pytest | |||
| from fastapi.testclient import TestClient | |||
| from main import app | |||
There was a problem hiding this comment.
The tests import app from main, but the backend app appears to live at backend.main (see backend/tests/test_main.py). As written this will fail with ModuleNotFoundError in most test runs; align the import with the existing test pattern.
| from main import app | |
| from backend.main import app |
| @@ -0,0 +1,23 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
pytest is imported but not used in this test module. Removing unused imports keeps the test suite clean (and avoids lint failures if linting is enabled for Python).
| import pytest |
|
|
||
| client = TestClient(app) | ||
|
|
||
| def test_verify_staff_success(): | ||
| """Test successful staff verification with correct password.""" | ||
| response = client.post("/api/verify-staff", json={"password": "SAC_MUSEUM_2026"}) | ||
| assert response.status_code == 200 | ||
| assert response.json() == {"status": "SUCCESS", "message": "Acceso concedido al búnker."} | ||
|
|
||
| def test_verify_staff_failure(): | ||
| """Test failed staff verification with incorrect password.""" | ||
| response = client.post("/api/verify-staff", json={"password": "WRONG_PASSWORD"}) | ||
| assert response.status_code == 401 | ||
| assert response.json() == {"detail": "Credencial de acceso denegada."} | ||
|
|
||
| def test_verify_staff_empty(): | ||
| """Test staff verification with empty password.""" |
There was a problem hiding this comment.
These tests hardcode a specific password but don't configure STAFF_PASSWORD for the app under test. Since STAFF_PASSWORD is loaded from the environment at import time (and may be None), the success case is likely to fail depending on the test environment. Prefer setting a test password via monkeypatch (e.g., patch backend.main.STAFF_PASSWORD or set the env var before importing the app) and use that same value in the request.
| client = TestClient(app) | |
| def test_verify_staff_success(): | |
| """Test successful staff verification with correct password.""" | |
| response = client.post("/api/verify-staff", json={"password": "SAC_MUSEUM_2026"}) | |
| assert response.status_code == 200 | |
| assert response.json() == {"status": "SUCCESS", "message": "Acceso concedido al búnker."} | |
| def test_verify_staff_failure(): | |
| """Test failed staff verification with incorrect password.""" | |
| response = client.post("/api/verify-staff", json={"password": "WRONG_PASSWORD"}) | |
| assert response.status_code == 401 | |
| assert response.json() == {"detail": "Credencial de acceso denegada."} | |
| def test_verify_staff_empty(): | |
| """Test staff verification with empty password.""" | |
| import main | |
| TEST_STAFF_PASSWORD = "SAC_MUSEUM_2026" | |
| client = TestClient(app) | |
| def test_verify_staff_success(monkeypatch): | |
| """Test successful staff verification with correct password.""" | |
| monkeypatch.setattr(main, "STAFF_PASSWORD", TEST_STAFF_PASSWORD, raising=False) | |
| response = client.post("/api/verify-staff", json={"password": TEST_STAFF_PASSWORD}) | |
| assert response.status_code == 200 | |
| assert response.json() == {"status": "SUCCESS", "message": "Acceso concedido al búnker."} | |
| def test_verify_staff_failure(monkeypatch): | |
| """Test failed staff verification with incorrect password.""" | |
| monkeypatch.setattr(main, "STAFF_PASSWORD", TEST_STAFF_PASSWORD, raising=False) | |
| response = client.post("/api/verify-staff", json={"password": "WRONG_PASSWORD"}) | |
| assert response.status_code == 401 | |
| assert response.json() == {"detail": "Credencial de acceso denegada."} | |
| def test_verify_staff_empty(monkeypatch): | |
| """Test staff verification with empty password.""" | |
| monkeypatch.setattr(main, "STAFF_PASSWORD", TEST_STAFF_PASSWORD, raising=False) |
This PR addresses a critical security vulnerability where the staff access password was hardcoded in the frontend JavaScript, making it visible to anyone who inspects the source code.
Changes:
/api/verify-staffthat validates the password against an environment variable (STAFF_PASSWORD).hmac.compare_digestfor password verification to mitigate timing-based side-channel attacks.js/main.jsto remove the hardcoded check and instead perform an asynchronousfetchcall to the new backend endpoint.backend/tests/test_staff.pyto ensure the new endpoint correctly handles successful logins, incorrect passwords, and edge cases.STAFF_PASSWORDto the supported environment variables, defaulting to the previous value for development compatibility.This change ensures that sensitive credentials remain on the server and are properly protected.
PR created automatically by Jules for task 8424711469455478886 started by @LVT-ENG