From 03c8cace8ab841a9244fff39469bd6f358be9965 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Tue, 27 Oct 2026 14:19:55 +1100 Subject: [PATCH] BE+FE: hardening --- CLAUDE.md | 3 ++ front/php/server/query_json.php | 34 +-------------- front/php/templates/auth.php | 1 + front/php/templates/security.php | 1 + server/workflows/manager.py | 9 +++- test/backend/test_query_json_write_surface.py | 41 +++++++++++++++++++ test/backend/test_workflows.py | 14 +++++++ 7 files changed, 70 insertions(+), 33 deletions(-) create mode 100644 test/backend/test_query_json_write_surface.py diff --git a/CLAUDE.md b/CLAUDE.md index dc2f8f776..ea3793c4c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,3 +98,6 @@ Procedural/how-to knowledge (running tests, resetting the DB, devcontainer manag - **No new PHP backend logic.** `front/` is being migrated away from PHP, so never add a new PHP endpoint or new server-side computation inside an existing PHP file. If a feature needs backend state or computation, add it to the Python server and expose it to the frontend through an existing read path (`app_state.json` via `query_json.php`, `table_settings.json`, a REST/GraphQL endpoint) - never re-derive logic in PHP that the Python side already knows or could easily expose. Editing existing PHP page logic (templating, bug fixes) is fine; this is about not growing the PHP surface area. - **A stateful UI feature (survives a reload, coordinates across tabs, reacts to a backend push) is a cross-cutting mechanism, not "just a UI feature."** Treat it like one before writing code: trace every existing consumer of the data it needs (e.g. everything that already reads `app_state.json`), not just the one file being edited. The visible surface looking small (a badge, an icon) says nothing about whether the state-propagation mechanism underneath already exists elsewhere. - **No JS/PHP test harness exists in `front/` — simulate before asking for a live test, every time, not after a live test fails.** Write a disposable Node/PHP script that runs the actual function(s) against realistic inputs (including inputs crossing from a different file than the one being edited) before calling a change ready to test. A diff review misses things a five-second script run catches - a renamed parameter still referenced by its old name, an ordering assumption that's wrong once two async steps are both in play. +- **Bug and security fixes are test-first: write the failing test, run it and see it fail on the unfixed code, then fix.** Don't edit source and add tests afterward. +- **Each user-editable config file has exactly one writer, and it must be CSRF-protected.** Read-only endpoints stay GET-only; never add a second write path. `requireCsrfToken()` reads `$_POST['csrf_token']`, so a raw-JSON-body endpoint cannot use it as-is. Loaders of user-editable JSON must tolerate a wrong top-level type and non-object entries rather than crash. +- **Every PHP `session_start()` must be preceded by `session_set_cookie_params(['samesite' => 'Lax', 'httponly' => true])`.** A static test checks the known session-starting files, so add any new one to its list. diff --git a/front/php/server/query_json.php b/front/php/server/query_json.php index 97be56c06..dcd722f3d 100755 --- a/front/php/server/query_json.php +++ b/front/php/server/query_json.php @@ -42,38 +42,8 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { http_response_code(400); echo json_encode(["error" => "Missing 'file' parameter"]); } -} elseif ($_SERVER['REQUEST_METHOD'] === 'POST') { - // Read the input JSON data - $inputData = file_get_contents("php://input"); - $decodedData = json_decode($inputData, true); - - if (json_last_error() !== JSON_ERROR_NONE) { - http_response_code(400); - echo json_encode(["error" => "Invalid JSON data"]); - exit; - } - - // Check if file parameter is provided and is workflows.json - if (!isset($_GET['file']) || $_GET['file'] !== "workflows.json") { - http_response_code(400); - echo json_encode(["error" => "Invalid or missing file parameter"]); - exit; - } - - $file = $_GET['file']; - $configRoot = getenv('NETALERTX_CONFIG') ?: '/data/config'; - $filePath = rtrim($configRoot, '/') . "/" . basename($file); - - // Save new workflows.json (replace existing content) - if (file_put_contents($filePath, json_encode($decodedData, JSON_PRETTY_PRINT))) { - http_response_code(200); - echo json_encode(["success" => "Workflows replaced successfully"]); - } else { - http_response_code(500); - echo json_encode(["error" => "Failed to update workflows.json"]); - } -} -else { +} else { + header('Allow: GET'); http_response_code(405); echo json_encode(["error" => "Method Not Allowed"]); } diff --git a/front/php/templates/auth.php b/front/php/templates/auth.php index a31b6412f..7ea317704 100755 --- a/front/php/templates/auth.php +++ b/front/php/templates/auth.php @@ -1,5 +1,6 @@ 'Lax', 'httponly' => true]); session_start(); } diff --git a/front/php/templates/security.php b/front/php/templates/security.php index af30651dc..9b926fa52 100755 --- a/front/php/templates/security.php +++ b/front/php/templates/security.php @@ -2,6 +2,7 @@ // Start session if not already started if (session_status() == PHP_SESSION_NONE) { + session_set_cookie_params(['samesite' => 'Lax', 'httponly' => true]); session_start(); } diff --git a/server/workflows/manager.py b/server/workflows/manager.py index 3c28994e3..c4f1b03a6 100755 --- a/server/workflows/manager.py +++ b/server/workflows/manager.py @@ -52,7 +52,7 @@ class WorkflowManager: # ------------------------------------------------------------------------- # Loading def load_workflows(self): - """Load workflows from workflows.json, rejecting any with invalid tokens.""" + """Load workflows from workflows.json, skipping non-object entries and any with invalid tokens.""" try: workflows_json_path = fullConfFolder + "/workflows.json" with open(workflows_json_path, "r") as f: @@ -61,8 +61,15 @@ class WorkflowManager: mylog("none", ["[WF] Failed to load workflows.json"]) return [] + if not isinstance(raw, list): + mylog("none", ["[WF] workflows.json must contain a list of workflows - ignoring file"]) + return [] + valid = [] for wf in raw: + if not isinstance(wf, dict): + mylog("none", ["[WF] Skipping workflows.json entry that is not an object"]) + continue if self._validate_workflow_tokens(wf): valid.append(wf) else: diff --git a/test/backend/test_query_json_write_surface.py b/test/backend/test_query_json_write_surface.py new file mode 100644 index 000000000..90828a16b --- /dev/null +++ b/test/backend/test_query_json_write_surface.py @@ -0,0 +1,41 @@ +"""Regression guards: workflows.json must only be writable through the CSRF-protected endpoint.""" + +import os +import re + +REPO_ROOT = os.path.join(os.path.dirname(__file__), "..", "..") +QUERY_JSON = os.path.join(REPO_ROOT, "front", "php", "server", "query_json.php") +SESSION_STARTERS = [ + os.path.join(REPO_ROOT, "front", "php", "templates", "security.php"), + os.path.join(REPO_ROOT, "front", "php", "templates", "auth.php"), +] + + +def _read(path): + """Return the text content of a file.""" + with open(path, "r") as f: + return f.read() + + +def test_query_json_has_no_write_path(): + """query_json.php is read-only: no POST handler and no file writes.""" + src = _read(QUERY_JSON) + assert not re.search(r"REQUEST_METHOD'\]\s*===\s*'POST'", src), "POST branch must not exist" + assert "file_put_contents" not in src, "query_json.php must not write files" + assert "php://input" not in src, "query_json.php must not read a request body" + + +def test_query_json_rejects_non_get_methods(): + """Any non-GET method falls through to a 405 response.""" + src = _read(QUERY_JSON) + assert "http_response_code(405)" in src + + +def test_session_cookie_sets_samesite(): + """Every file that starts the PHP session must set a SameSite attribute first.""" + for path in SESSION_STARTERS: + src = _read(path) + start = src.index("session_start()") + params = src.find("session_set_cookie_params") + assert params != -1 and params < start, f"{path}: SameSite must be set before session_start()" + assert re.search(r"'samesite'\s*=>\s*'(Lax|Strict)'", src), f"{path}: missing SameSite Lax/Strict" diff --git a/test/backend/test_workflows.py b/test/backend/test_workflows.py index fba81deb4..69a66a2b3 100644 --- a/test/backend/test_workflows.py +++ b/test/backend/test_workflows.py @@ -171,6 +171,20 @@ class TestLoadWorkflowsRejectsInvalidTokens(unittest.TestCase): mgr = self._make_manager_loading([wf]) self.assertEqual(len(mgr.workflows), 0) + def test_non_list_content_ignored(self): + for raw in (123, "text", {"name": "x"}, None): + mgr = self._make_manager_loading(raw) + self.assertEqual(mgr.workflows, []) + + def test_non_object_entries_skipped(self): + wf = { + "name": "Valid WF", "enabled": "Yes", + "trigger": {"object_type": "Devices", "event_type": "update"}, + "conditions": [], "actions": [] + } + mgr = self._make_manager_loading([1, "x", None, wf]) + self.assertEqual(len(mgr.workflows), 1) + # --------------------------------------------------------------------------- # DeviceInstance.queryByConditions