🛡️ Sentinel: [HIGH] Secure staff verification and remove hardcoded password#36
🛡️ Sentinel: [HIGH] Secure staff verification and remove hardcoded password#36
Conversation
…ssword - Move staff password verification to the backend. - Implement `/api/verify-staff` endpoint in `backend/main.py`. - Use `hmac.compare_digest` to prevent timing attacks. - Retrieve staff password from `STAFF_PASSWORD` environment variable. - Update `js/main.js` to call the backend verification endpoint. - Add `StaffLogin` Pydantic model to `backend/models.py`. - Add `vite.config.js` with API proxy for local development. - Add integration tests for staff verification in `backend/tests/test_staff.py`. - Remove hardcoded password 'SAC_MUSEUM_2026' from the client-side code. 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.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the application by addressing a critical vulnerability where a staff password was hardcoded on the client-side. The changes refactor the staff verification process to a secure backend endpoint, utilizing environment variables for sensitive data and timing-safe comparisons, thereby preventing unauthorized access and improving overall system integrity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 implements a new staff login verification system. It moves the staff password from a hardcoded frontend value to a backend environment variable, introduces a secure /api/verify-staff endpoint using hmac.compare_digest, and updates the frontend to use this new API. Corresponding backend tests have been added, and a Vite proxy configuration was included. The review suggests refactoring the test setup logic into a pytest fixture to reduce duplication and adding a missing assertion to the test_verify_staff_empty_password test for robustness.
| import pytest | ||
| from fastapi.testclient import TestClient | ||
| from backend.main import app | ||
| import os | ||
|
|
||
| client = TestClient(app) | ||
|
|
||
| def test_verify_staff_success(monkeypatch): | ||
| # Set a known staff password for testing | ||
| monkeypatch.setenv("STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
| # Reload the app's STAFF_PASSWORD from env | ||
| import backend.main | ||
| monkeypatch.setattr(backend.main, "STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
|
|
||
| response = client.post("/api/verify-staff", json={"password": "TEST_STAFF_PASS"}) | ||
| assert response.status_code == 200 | ||
| assert response.json()["status"] == "SUCCESS" | ||
|
|
||
| def test_verify_staff_failure(monkeypatch): | ||
| monkeypatch.setenv("STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
| import backend.main | ||
| monkeypatch.setattr(backend.main, "STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
|
|
||
| response = client.post("/api/verify-staff", json={"password": "WRONG_PASSWORD"}) | ||
| assert response.status_code == 401 | ||
| assert response.json()["detail"] == "Acceso denegado" | ||
|
|
||
| def test_verify_staff_empty_password(monkeypatch): | ||
| monkeypatch.setenv("STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
| import backend.main | ||
| monkeypatch.setattr(backend.main, "STAFF_PASSWORD", "TEST_STAFF_PASS") | ||
|
|
||
| response = client.post("/api/verify-staff", json={"password": ""}) | ||
| assert response.status_code == 401 |
There was a problem hiding this comment.
The test setup logic is repeated across all test functions. This can be refactored into a single pytest fixture with autouse=True to reduce code duplication and improve maintainability.
Additionally, test_verify_staff_empty_password is missing an assertion to verify the response body. I've added it to make the test more robust and consistent with the other failure case.
import pytest
from fastapi.testclient import TestClient
from backend.main import app
import os
client = TestClient(app)
@pytest.fixture(autouse=True)
def mock_staff_password(monkeypatch):
"""Mocks the staff password for the duration of the tests."""
test_password = "TEST_STAFF_PASS"
monkeypatch.setenv("STAFF_PASSWORD", test_password)
# Since the env var is read at module load, we also need to patch the variable in the imported module.
import backend.main
monkeypatch.setattr(backend.main, "STAFF_PASSWORD", test_password)
def test_verify_staff_success():
response = client.post("/api/verify-staff", json={"password": "TEST_STAFF_PASS"})
assert response.status_code == 200
assert response.json()["status"] == "SUCCESS"
def test_verify_staff_failure():
response = client.post("/api/verify-staff", json={"password": "WRONG_PASSWORD"})
assert response.status_code == 401
assert response.json()["detail"] == "Acceso denegado"
def test_verify_staff_empty_password():
response = client.post("/api/verify-staff", json={"password": ""})
assert response.status_code == 401
assert response.json()["detail"] == "Acceso denegado"1 participant
🚨 Severity: HIGH
💡 Vulnerability: A staff password was hardcoded in the client-side JavaScript (
js/main.js), making it trivial for anyone to discover.🎯 Impact: Unauthorized access to staff-only functionality or dashboards.
🔧 Fix: Refactored the verification flow to use a secure backend endpoint. The password is now stored in an environment variable and compared using a timing-safe
hmac.compare_digestcall.✅ Verification:
backend/tests/test_staff.py(all passed).PR created automatically by Jules for task 4052392736350921056 started by @LVT-ENG