diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index b7d82975..11c4c38a 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -15,7 +15,7 @@ and ShopDB, the same discipline the backups collector uses for machine numbers. import json import logging -from datetime import datetime +from datetime import datetime, timezone from pathlib import Path from typing import Dict, List, Optional, Type @@ -56,18 +56,35 @@ KNOWN_PAYLOAD_KEYS = frozenset({ def _parse_dt(value): - """Best-effort parse of an ISO-8601 string to naive UTC, or None.""" + """Best-effort parse of an ISO-8601 string to naive UTC, or None. + + astimezone(tz=None) converts to the SERVER's local zone, not to UTC, so an + agent reporting 17:26:16Z was stored as 13:26:16 on an EDT server and then + re-served labelled 'Z'. The same row's server-written lastreportedat stayed + true UTC, so one row carried two time bases four hours apart, and the + dashboard's 48-hour stability window - which compares a real UTC cutoff + against these values - dropped events four hours early. + + Invisible on a UTC server, which is why it survived: nothing in the + Dockerfile, the compose files or the Windows deploy pins TZ, so whether the + data is right depends on the host's clock settings. + + AGENT-CONTRACT.md mandates UTC on the wire. A NAIVE value is therefore + taken as UTC already, rather than guessed at. + """ if not value: return None if isinstance(value, datetime): - return value.replace(tzinfo=None) + if value.tzinfo is not None: + return value.astimezone(timezone.utc).replace(tzinfo=None) + return value text = str(value).strip().replace('Z', '+00:00') try: parsed = datetime.fromisoformat(text) except ValueError: return None if parsed.tzinfo is not None: - parsed = parsed.astimezone(tz=None).replace(tzinfo=None) + parsed = parsed.astimezone(timezone.utc).replace(tzinfo=None) return parsed diff --git a/plugins/geenforce/api/routes.py b/plugins/geenforce/api/routes.py index c0ee5df4..d66d2986 100644 --- a/plugins/geenforce/api/routes.py +++ b/plugins/geenforce/api/routes.py @@ -454,6 +454,15 @@ def create_scope(): if not scopename: return error_response(ErrorCodes.VALIDATION_ERROR, 'scopename is required', http_code=400) + # The name becomes a directory on the imaging share. Refused here as well + # as at the write, so a bad name never reaches the database in the first + # place - see valid_scopename for what a traversing name did. + if not service.valid_scopename(scopename): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'scopename may contain only letters, digits, dot, dash and ' + 'underscore, and must start with a letter or digit', + http_code=400) if phase not in PHASES: return error_response(ErrorCodes.VALIDATION_ERROR, f'phase must be one of {PHASES}', http_code=400) diff --git a/plugins/geenforce/service.py b/plugins/geenforce/service.py index 2b814993..8b2af4aa 100644 --- a/plugins/geenforce/service.py +++ b/plugins/geenforce/service.py @@ -10,6 +10,7 @@ Kept out of the CLI and routes so both share one implementation: import hashlib import json import os +import re import tempfile from datetime import datetime, timezone @@ -532,6 +533,30 @@ def store_inline_payload(entry, filename, contenttype, rawbytes): return payload +# A scope name becomes a DIRECTORY on the imaging share, so it has to be a +# single safe path segment. It was validated only as "not empty", which meant a +# name like "../escape" or an absolute path wrote the manifest outside the +# share root - proven by writing /tmp/manifest.json and files one and two +# levels above it. The real exposure is the permission split: geenforce.manage +# creates the scope, geenforce.publish writes the file, so a manage-only author +# could aim a publisher's write anywhere that process can reach. +# +# Allowed: letters, digits, dot, dash, underscore. Not '.' or '..', and no +# separator of either platform - the share is written from Linux and read by +# Windows, so a backslash is a separator too. +SCOPENAME_PATTERN = re.compile(r'^[A-Za-z0-9][A-Za-z0-9._-]*$') + + +def valid_scopename(scopename): + """True when this name is safe to use as a directory on the share.""" + name = (scopename or '').strip() + if not name or name in ('.', '..') or len(name) > 100: + return False + if '/' in name or '\\' in name or '\x00' in name: + return False + return bool(SCOPENAME_PATTERN.match(name)) + + def export_scope_to_share(scopename, phase, shareroot): """Write a scope's current published JSON to the share, backing up the old file to _meta/history first. Returns the written path.""" @@ -546,7 +571,21 @@ def export_scope_to_share(scopename, phase, shareroot): if phase == 'preinstall': target = os.path.join(shareroot, 'preinstall.json') else: + # Checked again here, not only at creation: rows predating the + # validation exist, and this is the line that actually writes. + if not valid_scopename(scopename): + raise ValueError( + f'Refusing to export scope {scopename!r}: the name is not a ' + f'safe directory name') target = os.path.join(shareroot, scopename, 'manifest.json') + + # Belt and braces: whatever the name, the resolved target must sit inside + # the share root. + root = os.path.realpath(shareroot) + if os.path.commonpath([root, os.path.realpath(os.path.dirname(target))]) != root: + raise ValueError( + f'Refusing to export scope {scopename!r}: it resolves outside the ' + f'share root') os.makedirs(os.path.dirname(target), exist_ok=True) if os.path.isfile(target): diff --git a/shopdb/core/api/maplevels.py b/shopdb/core/api/maplevels.py index 78bd7f06..a67e03d6 100644 --- a/shopdb/core/api/maplevels.py +++ b/shopdb/core/api/maplevels.py @@ -36,10 +36,27 @@ def _blueprint_dir(): # ============================================================================= # Read - public +# +# ADR-017 rule 6: the level list is readable without authentication, because the +# printer installer map runs optional-auth before anyone logs in and needs a +# blueprint. It follows the precedent of printer install-list and the slide +# feed, both still public. +# +# afce2437 ("Reading anything needs a permission") swept 165 open GET routes +# and caught these three in the net. Fail-closed, so nothing leaked - but the +# anonymous installer map has drawn no floor plan since, while this same file +# kept the docstring saying the reads are public, the API inventory kept +# declaring auth "none", and PrinterInstallerMap.vue kept its comment +# explaining why. Four places agreed and the code did not. +# +# Restored rather than amending the ADR: blueprint paths and level names are +# not secrets, the asset positions on them already render on public kiosk +# pages, and the feature the rule exists for is broken without it. These three +# are registered in tests/test_core/test_public_read_surface.py. # ============================================================================= @maplevels_bp.route('', methods=['GET']) -@require_permission('assets.view') +@jwt_required(optional=True) def list_levels(): """Every active level, grouped by building, in display order. @@ -74,7 +91,7 @@ def list_levels(): @maplevels_bp.route('/', methods=['GET']) -@require_permission('assets.view') +@jwt_required(optional=True) def get_level(levelid): level = db.session.get(MapLevel, levelid) if not level or not level.isactive: @@ -84,7 +101,7 @@ def get_level(levelid): @maplevels_bp.route('//blueprint/', methods=['GET']) -@require_permission('assets.view') +@jwt_required(optional=True) def serve_blueprint(filename, levelid=None): """Serve a level's blueprint image. Public, for the same reason as above.""" return send_upload(_blueprint_dir(), filename) diff --git a/tests/test_core/test_maplevels_public.py b/tests/test_core/test_maplevels_public.py new file mode 100644 index 00000000..46d18698 --- /dev/null +++ b/tests/test_core/test_maplevels_public.py @@ -0,0 +1,56 @@ +"""The floor-map level reads answer a caller with no account. + +ADR-017 rule 6 says so, docs/api-inventory.json declares auth "none" for all +three, PrinterInstallerMap.vue carries a comment explaining why, and the module +docstring in shopdb/core/api/maplevels.py says the reads are public. The code +disagreed with all four: afce2437 swept 165 genuinely-open GET routes behind +require_permission and caught these in the net, so the anonymous installer map +drew no floor plan. + +Nothing leaked - it failed closed - but a feature was off for a fortnight while +every document said it was on. test_public_read_surface.py checks that a public +route is REGISTERED as public; this checks that it actually answers, which is +the part that was broken. +""" + + +def _level(db): + from shopdb.core.models.maplevel import Building, MapLevel + + building = Building(buildingname='Public Works', sortorder=1) + db.session.add(building) + db.session.flush() + level = MapLevel(buildingid=building.buildingid, levelname='Ground', + sortorder=1, blueprintlight='floorplan.svg') + db.session.add(level) + db.session.commit() + return level.levelid + + +def test_the_level_list_answers_an_anonymous_caller(client, db): + levelid = _level(db) + response = client.get('/api/maplevels') + assert response.status_code == 200, response.get_json() + payload = response.get_json()['data'] + names = [level['levelname'] + for building in payload['buildings'] + for level in building['levels']] + assert 'Ground' in names + assert levelid + + +def test_one_level_answers_an_anonymous_caller(client, db): + levelid = _level(db) + response = client.get('/api/maplevels/%d' % levelid) + assert response.status_code == 200, response.get_json() + assert response.get_json()['data']['levelname'] == 'Ground' + + +def test_writes_are_still_gated(client, db): + """The reads going back to public must not take the writes with them.""" + levelid = _level(db) + assert client.post('/api/maplevels/buildings', + json={'buildingname': 'Sneaky'}).status_code in (401, 403) + assert client.put('/api/maplevels/%d' % levelid, + json={'levelname': 'Renamed'}).status_code in (401, 403) + assert client.delete('/api/maplevels/%d' % levelid).status_code in (401, 403) diff --git a/tests/test_core/test_public_read_surface.py b/tests/test_core/test_public_read_surface.py index 793e65c0..d77a8efd 100644 --- a/tests/test_core/test_public_read_surface.py +++ b/tests/test_core/test_public_read_surface.py @@ -59,6 +59,13 @@ PUBLIC_ENDPOINTS = { 'notifications.get_employee_recognitions', 'slides.feed', 'slides.serve_image', + # The floor-map levels and their blueprints. ADR-017 rule 6: the printer + # installer map runs optional-auth before anyone logs in and needs a + # blueprint to draw on. Names and image paths, not asset data - and the + # asset positions that go on top already render on public kiosk pages. + 'maplevels.list_levels', 'maplevels.get_level', + 'maplevels.serve_blueprint', + # Parts kiosk: one item by its code, and that item's picture. # kiosk_item takes no JWT but is NOT open: same kiosk flag + device token # as the write. Gating only the write would have left the enumeration diff --git a/tests/test_plugins/test_endpoints_plugin.py b/tests/test_plugins/test_endpoints_plugin.py index 32531bb9..a779cd03 100644 --- a/tests/test_plugins/test_endpoints_plugin.py +++ b/tests/test_plugins/test_endpoints_plugin.py @@ -272,3 +272,66 @@ def test_fleet_list_route_resolves_without_trailing_slash(): endpoint_slash, _ = adapter.match('/api/endpoints/', method='GET') assert endpoint_slash.endswith('list_endpoints') + + +# --- agent timestamps are UTC on the wire and UTC in the column -------------- +# +# _parse_dt used astimezone(tz=None), which converts to the SERVER's local +# zone. An agent reporting 17:26:16Z was stored as 13:26:16 on an EDT server +# and re-served labelled 'Z', while the same row's server-written +# lastreportedat stayed true UTC - one row, two time bases, four hours apart. +# The dashboard's 48-hour stability window compares a real UTC cutoff against +# these values, so events left it four hours early. +# +# It is invisible on a UTC server, which is how it survived, and nothing in the +# Dockerfile, the compose files or the Windows deploy pins TZ. + +def _in_zone(zonename, call): + """Run call() with the process in a named zone, then put it back.""" + import os + import time + + before = os.environ.get('TZ') + os.environ['TZ'] = zonename + time.tzset() + try: + return call() + finally: + if before is None: + os.environ.pop('TZ', None) + else: + os.environ['TZ'] = before + time.tzset() + + +def test_a_utc_timestamp_survives_a_non_utc_server(): + from datetime import datetime + + from plugins.endpoints.plugin import _parse_dt + + parsed = _in_zone('America/New_York', + lambda: _parse_dt('2026-09-03T17:26:16Z')) + assert parsed == datetime(2026, 9, 3, 17, 26, 16), \ + 'the wire value is UTC and the column is UTC; no local conversion' + + +def test_an_offset_timestamp_is_converted_to_utc_not_to_server_local(): + from datetime import datetime + + from plugins.endpoints.plugin import _parse_dt + + parsed = _in_zone('America/New_York', + lambda: _parse_dt('2026-09-03T17:26:16+05:00')) + assert parsed == datetime(2026, 9, 3, 12, 26, 16) + + +def test_a_naive_timestamp_is_taken_as_utc_already(): + """AGENT-CONTRACT.md mandates UTC on the wire, so a value with no offset is + UTC rather than a guess about the agent's zone.""" + from datetime import datetime + + from plugins.endpoints.plugin import _parse_dt + + parsed = _in_zone('Asia/Tokyo', + lambda: _parse_dt('2026-09-03T17:26:16')) + assert parsed == datetime(2026, 9, 3, 17, 26, 16) diff --git a/tests/test_plugins/test_geenforce_export.py b/tests/test_plugins/test_geenforce_export.py index d020bb14..082462ec 100644 --- a/tests/test_plugins/test_geenforce_export.py +++ b/tests/test_plugins/test_geenforce_export.py @@ -63,3 +63,61 @@ def test_export_backs_up_existing(client, db, app, auth_headers, tmp_path): historydir = tmp_path / '_meta' / 'history' assert historydir.is_dir() assert any(historydir.iterdir()) + + +# --- a scope name is a directory on the share -------------------------------- +# +# create_scope checked only that the name was non-empty, and the export did +# os.path.join(shareroot, scopename, 'manifest.json'). So "../escape" wrote one +# level above the share root and "/tmp/anywhere" wrote wherever it liked. The +# exposure is the permission split: geenforce.manage creates the scope, +# geenforce.publish writes the file, so a manage-only author could aim a +# publisher's write at any path that process can reach. + +import pytest + + +@pytest.mark.parametrize('name', [ + '../escape', + '../../escape', + '/tmp/absolute', + 'sub/dir', + 'back\\slash', + '..', + '.', +]) +def test_a_traversing_scope_name_is_refused_at_creation(client, db, + auth_headers, name): + created = client.post('/api/geenforce/scopes', + json={'scopename': name, 'phase': 'runtime'}, + headers=auth_headers) + assert created.status_code == 400, (name, created.get_json()) + assert 'scopename' in created.get_json()['data']['error']['message'] + + +def test_a_normal_scope_name_is_still_accepted(client, db, auth_headers): + """The names sites actually use must keep working, or the guard is worse + than the hole.""" + for name in ('common', 'display', 'gea-shopfloor-cmm', 'bay.3105_a'): + created = client.post('/api/geenforce/scopes', + json={'scopename': name, 'phase': 'runtime'}, + headers=auth_headers) + assert created.status_code in (200, 201), (name, created.get_json()) + + +def test_the_export_refuses_a_traversing_name_even_from_the_database( + app, db, tmp_path): + """Rows created before the validation existed are still out there, and the + export is the line that actually writes.""" + with app.app_context(): + service.replace_scope_draft('../legacy-escape', 'runtime', + {'Version': '2.6', 'Applications': []}) + service.publish_scope('../legacy-escape', 'runtime', notes='v1') + service.db.session.commit() + + with pytest.raises(ValueError) as refused: + service.export_scope_to_share('../legacy-escape', 'runtime', + str(tmp_path)) + assert 'safe directory name' in str(refused.value) + + assert not (tmp_path.parent / 'legacy-escape').exists()