From e593498de5e7ab272ae5dec69f2b7ac68b00352f Mon Sep 17 00:00:00 2001 From: Lukas Date: Mon, 20 Apr 2026 19:23:12 +0200 Subject: [PATCH] Strengthen setup password validation - Add backend Pydantic password complexity validation for setup - Update frontend setup page with password rule feedback and strength indicator - Add/adjust setup API tests for password validation - Document setup password requirements - Fix frontend test type annotation issue --- Docs/Instructions.md | 1 + backend/app/models/setup.py | 18 ++- backend/tests/test_main.py | 2 +- backend/tests/test_routers/test_setup.py | 56 +++++-- frontend/src/pages/SetupPage.tsx | 138 +++++++++++++++++- .../src/pages/__tests__/SetupPage.test.tsx | 43 +++++- .../src/utils/__tests__/queryUtils.test.ts | 5 +- 7 files changed, 241 insertions(+), 22 deletions(-) diff --git a/Docs/Instructions.md b/Docs/Instructions.md index 17670ca..0ebf211 100644 --- a/Docs/Instructions.md +++ b/Docs/Instructions.md @@ -244,6 +244,7 @@ Backend: `http://127.0.0.1:8000` ยท Frontend (Vite proxy): `http://127.0.0.1:517 ### API login (dev) The frontend SHA256-hashes the password before sending it to the API. +The initial setup password must be at least 8 characters long and include one uppercase letter, one number, and one special character from `!@#$%^&*()`. The session cookie is named `bangui_session`. ```bash diff --git a/backend/app/models/setup.py b/backend/app/models/setup.py index 4e6a55e..4b65c8d 100644 --- a/backend/app/models/setup.py +++ b/backend/app/models/setup.py @@ -3,7 +3,7 @@ Request, response, and domain models for the first-run configuration wizard. """ -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator class SetupRequest(BaseModel): @@ -16,6 +16,22 @@ class SetupRequest(BaseModel): min_length=8, description="Master password that protects the BanGUI interface.", ) + + @field_validator("master_password") + @classmethod + def validate_master_password(cls, value: str) -> str: + if len(value) < 8: + raise ValueError("Password must be at least 8 characters long.") + if not any(char.isupper() for char in value): + raise ValueError("Password must include at least one uppercase letter.") + if not any(char.isdigit() for char in value): + raise ValueError("Password must include at least one number.") + if not any(char in "!@#$%^&*()" for char in value): + raise ValueError( + "Password must include at least one special character (!@#$%^&*())." + ) + return value + database_path: str = Field( default="bangui.db", description="Filesystem path to the BanGUI SQLite application database.", diff --git a/backend/tests/test_main.py b/backend/tests/test_main.py index 184b855..1a5699c 100644 --- a/backend/tests/test_main.py +++ b/backend/tests/test_main.py @@ -298,7 +298,7 @@ async def test_startup_overrides_settings_from_persisted_setup(tmp_path: Path) - await init_db(db) await setup_service.run_setup( db, - master_password="supersecret123", + master_password="Supersecret1!", database_path=runtime_db_path, fail2ban_socket="/tmp/persisted.sock", timezone="Europe/Berlin", diff --git a/backend/tests/test_routers/test_setup.py b/backend/tests/test_routers/test_setup.py index 358edd8..d336d71 100644 --- a/backend/tests/test_routers/test_setup.py +++ b/backend/tests/test_routers/test_setup.py @@ -19,7 +19,7 @@ from app.services import setup_service # --------------------------------------------------------------------------- _SETUP_PAYLOAD: dict[str, object] = { - "master_password": "supersecret123", + "master_password": "Supersecret1!", "database_path": "bangui.db", "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", @@ -78,7 +78,7 @@ class TestGetSetupStatus: await client.post( "/api/setup", json={ - "master_password": "supersecret123", + "master_password": "Supersecret1!", "database_path": "bangui.db", "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", @@ -98,7 +98,7 @@ class TestPostSetup: response = await client.post( "/api/setup", json={ - "master_password": "supersecret123", + "master_password": "Supersecret1!", "database_path": "bangui.db", "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", @@ -117,10 +117,48 @@ class TestPostSetup: ) assert response.status_code == 422 + async def test_rejects_missing_uppercase_password(self, client: AsyncClient) -> None: + """Setup endpoint rejects passwords missing an uppercase character.""" + response = await client.post( + "/api/setup", + json={"master_password": "lowercase1!"}, + ) + assert response.status_code == 422 + assert any( + "uppercase" in error["msg"].lower() + for error in response.json()["detail"] + ) + + async def test_rejects_missing_number_password(self, client: AsyncClient) -> None: + """Setup endpoint rejects passwords missing a numeric character.""" + response = await client.post( + "/api/setup", + json={"master_password": "NoNumbers!"}, + ) + assert response.status_code == 422 + assert any( + "number" in error["msg"].lower() + for error in response.json()["detail"] + ) + + async def test_rejects_missing_special_character_password( + self, client: AsyncClient + ) -> None: + """Setup endpoint rejects passwords missing a required special character.""" + response = await client.post( + "/api/setup", + json={"master_password": "NoSpecial1"}, + ) + assert response.status_code == 422 + assert any( + "special character" in error["msg"].lower() + for error in response.json()["detail"] + ) + async def test_rejects_second_call(self, client: AsyncClient) -> None: """Setup endpoint returns 409 if setup has already been completed.""" payload = { - "master_password": "supersecret123", + "master_password": "Supersecret1!", "database_path": "bangui.db", "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", @@ -138,7 +176,7 @@ class TestPostSetup: """Setup endpoint uses defaults when optional fields are omitted.""" response = await client.post( "/api/setup", - json={"master_password": "supersecret123"}, + json={"master_password": "Supersecret1!"}, ) assert response.status_code == 201 @@ -150,7 +188,7 @@ class TestPostSetupRuntimeState: """App state should reflect setup settings immediately after setup.""" app, client = app_and_client payload = { - "master_password": "supersecret123", + "master_password": "Supersecret1!", "database_path": "bangui.db", "fail2ban_socket": "/tmp/persisted.sock", "timezone": "Europe/Berlin", @@ -193,7 +231,7 @@ class TestSetupRedirectMiddleware: """Protected endpoints are reachable (no redirect) after setup.""" await client.post( "/api/setup", - json={"master_password": "supersecret123"}, + json={"master_password": "Supersecret1!"}, ) # /api/auth/login should now be reachable (returns 405 GET not allowed, # not a setup redirect) @@ -220,7 +258,7 @@ class TestGetTimezone: await client.post( "/api/setup", json={ - "master_password": "supersecret123", + "master_password": "Supersecret1!", "timezone": "Europe/Berlin", }, ) @@ -413,7 +451,7 @@ class TestLifespanSetupCache: await init_db(db) await setup_service.run_setup( db, - master_password="supersecret123", + master_password="Supersecret1!", database_path=settings.database_path, fail2ban_socket=settings.fail2ban_socket, timezone=settings.timezone, diff --git a/frontend/src/pages/SetupPage.tsx b/frontend/src/pages/SetupPage.tsx index 6a266ad..cd61404 100644 --- a/frontend/src/pages/SetupPage.tsx +++ b/frontend/src/pages/SetupPage.tsx @@ -6,7 +6,7 @@ * All fields use Fluent UI v9 components and inline validation. */ -import { useEffect, useState } from "react"; +import { useEffect, useState, type ChangeEvent, type FormEvent } from "react"; import { Button, Field, @@ -19,7 +19,6 @@ import { tokens, } from "@fluentui/react-components"; import { useNavigate } from "react-router-dom"; -import type { ChangeEvent, FormEvent } from "react"; import { useSetup } from "../hooks/useSetup"; // --------------------------------------------------------------------------- @@ -63,6 +62,40 @@ const useStyles = makeStyles({ error: { marginBottom: tokens.spacingVerticalM, }, + passwordStrength: { + marginTop: tokens.spacingVerticalS, + display: "grid", + gap: tokens.spacingVerticalS, + }, + passwordStrengthBar: { + display: "grid", + gridTemplateColumns: "repeat(4, 1fr)", + gap: tokens.spacingHorizontalS, + width: "100%", + }, + passwordStrengthSegment: { + height: "8px", + borderRadius: tokens.borderRadiusSmall, + backgroundColor: tokens.colorNeutralStroke2, + }, + passwordStrengthSegmentActive: { + backgroundColor: tokens.colorPaletteGreenBorder1, + }, + passwordRuleList: { + margin: 0, + paddingLeft: tokens.spacingHorizontalL, + color: tokens.colorNeutralForeground2, + fontSize: "0.875rem", + }, + passwordRuleItem: { + marginBottom: tokens.spacingVerticalXS, + }, + passwordRuleItemPassed: { + color: tokens.colorPaletteGreenForeground1, + }, + passwordRuleItemFailed: { + color: tokens.colorPaletteRedForeground1, + }, }); // --------------------------------------------------------------------------- @@ -87,6 +120,46 @@ const DEFAULT_VALUES: FormValues = { sessionDurationMinutes: "60", }; +type ValidationErrors = Partial>; + +type PasswordRuleId = "length" | "uppercase" | "number" | "special"; + +interface PasswordRule { + id: PasswordRuleId; + label: string; + test: (password: string) => boolean; +} + +const PASSWORD_RULES: PasswordRule[] = [ + { + id: "length", + label: "At least 8 characters", + test: (password: string) => password.length >= 8, + }, + { + id: "uppercase", + label: "At least one uppercase letter", + test: (password: string) => /[A-Z]/.test(password), + }, + { + id: "number", + label: "At least one number", + test: (password: string) => /\d/.test(password), + }, + { + id: "special", + label: "At least one special character (!@#$%^&*())", + test: (password: string) => /[!@#$%^&*()]/.test(password), + }, +]; + +function getPasswordRuleStatus(password: string) { + return PASSWORD_RULES.map((rule) => ({ + ...rule, + satisfied: rule.test(password), + })); +} + // --------------------------------------------------------------------------- // Component // --------------------------------------------------------------------------- @@ -101,7 +174,9 @@ export function SetupPage(): React.JSX.Element { const { status, loading, error, submit, submitting, submitError } = useSetup(); const [values, setValues] = useState(DEFAULT_VALUES); - const [errors, setErrors] = useState>>({}); + const [errors, setErrors] = useState({}); + const passwordRules = getPasswordRuleStatus(values.masterPassword); + const passwordStrength = passwordRules.filter((rule) => rule.satisfied).length; const apiError = error ?? submitError; // Redirect to /login if setup has already been completed. @@ -125,11 +200,15 @@ export function SetupPage(): React.JSX.Element { } function validate(): boolean { - const next: Partial> = {}; + const next: ValidationErrors = {}; + const unmetPasswordRules = passwordRules.filter((rule) => !rule.satisfied); - if (values.masterPassword.length < 8) { - next.masterPassword = "Password must be at least 8 characters."; + if (!values.masterPassword.trim()) { + next.masterPassword = "Password is required."; + } else if (unmetPasswordRules.length > 0) { + next.masterPassword = "Password must meet all complexity requirements."; } + if (values.masterPassword !== values.confirmPassword) { next.confirmPassword = "Passwords do not match."; } @@ -219,8 +298,34 @@ export function SetupPage(): React.JSX.Element { !rule.satisfied) + ? { + children: ( +
    + {passwordRules.map((rule) => ( +
  • + {rule.label} +
    • + ))} +
+ ), + } + : undefined) + } + validationState={ + errors.masterPassword || passwordRules.some((rule) => !rule.satisfied) + ? "error" + : "none" + } > +
+
+ {passwordRules.map((rule) => ( + + ))} +
+ + {passwordStrength} of {passwordRules.length} rules satisfied + +
({ submitSetup: vi.fn(), })); -import { getSetupStatus } from "../../api/setup"; +import { getSetupStatus, submitSetup } from "../../api/setup"; const mockedGetSetupStatus = vi.mocked(getSetupStatus); +const mockedSubmitSetup = vi.mocked(submitSetup); function renderPage() { return render( @@ -59,6 +61,45 @@ describe("SetupPage", () => { expect(screen.queryByRole("progressbar")).not.toBeInTheDocument(); }); + it("displays password complexity feedback while the user types", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: false }); + renderPage(); + + await waitFor(() => { + expect( + screen.getByRole("heading", { name: /bangui setup/i }), + ).toBeInTheDocument(); + }); + + const user = userEvent.setup(); + const passwordInput = screen.getByLabelText(/master password/i); + await user.type(passwordInput, "Short1"); + + expect(screen.getByText(/2 of 4 rules satisfied/i)).toBeInTheDocument(); + expect(screen.getByText(/at least 8 characters/i)).toBeInTheDocument(); + expect(screen.getByText(/at least one uppercase letter/i)).toBeInTheDocument(); + expect(screen.getByText(/at least one special character/i)).toBeInTheDocument(); + }); + + it("does not submit the form when the password is too weak", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: false }); + mockedSubmitSetup.mockResolvedValue({ message: "Setup completed successfully. Please log in." }); + renderPage(); + + await waitFor(() => { + expect( + screen.getByRole("heading", { name: /bangui setup/i }), + ).toBeInTheDocument(); + }); + + const user = userEvent.setup(); + await user.type(screen.getByLabelText(/master password/i), "Short1"); + await user.type(screen.getByLabelText(/confirm password/i), "Short1"); + await user.click(screen.getByRole("button", { name: /complete setup/i })); + + expect(mockedSubmitSetup).not.toHaveBeenCalled(); + }); + it("redirects to /login when setup is already complete", async () => { mockedGetSetupStatus.mockResolvedValue({ completed: true }); renderPage(); diff --git a/frontend/src/utils/__tests__/queryUtils.test.ts b/frontend/src/utils/__tests__/queryUtils.test.ts index c8549c8..cf8214d 100644 --- a/frontend/src/utils/__tests__/queryUtils.test.ts +++ b/frontend/src/utils/__tests__/queryUtils.test.ts @@ -1,9 +1,10 @@ import { describe, expect, it } from "vitest"; import { areHistoryQueriesEqual } from "../queryUtils"; +import type { HistoryQuery } from "../../types/history"; describe("areHistoryQueriesEqual", () => { it("returns true for identical history queries", () => { - const a = { + const a: HistoryQuery = { range: "7d", origin: "blocklist", jail: "ssh", @@ -19,7 +20,7 @@ describe("areHistoryQueriesEqual", () => { }); it("returns false when a single query field differs", () => { - const base = { + const base: HistoryQuery = { range: "7d", origin: "all", jail: "ssh",