From 0e3d5a62263b419e773d278bd87edcc27db70657 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 31 Aug 2026 09:24:13 -0400 Subject: [PATCH] Promote the 2026-08-30 review security fixes and supplies ordering to main --- CHANGELOG.md | 85 +++++++++++++++ .../ShopDbEndpointAgent.Core/CommandClient.cs | 11 +- .../ShopDbEndpointAgent.Core/CommandWorker.cs | 3 +- plugins/computers/plugin.py | 17 +++ plugins/endpoints/api/routes.py | 81 +++++++++++++- .../0005_endpoint_command_pinnedsha256.py | 51 +++++++++ plugins/endpoints/models/endpoint.py | 13 +++ plugins/endpoints/plugin.py | 24 ++++- plugins/geenforce/api/routes.py | 16 +-- plugins/printers/api/asset_routes.py | 16 ++- plugins/tools/api/routes.py | 7 +- plugins/warranty/plugin.py | 8 ++ shopdb/__init__.py | 17 ++- shopdb/api/__init__.py | 8 ++ shopdb/core/api/applications.py | 6 +- shopdb/core/api/scripts.py | 6 +- shopdb/core/api/search.py | 52 ++++++--- shopdb/core/api/users.py | 15 +-- shopdb/core/models/user.py | 65 ++++++++--- shopdb/plugins/base.py | 21 ++-- shopdb/utils/clientip.py | 32 ++++-- tests/test_core/test_auth_ratelimit.py | 11 +- tests/test_core/test_clientip.py | 98 ++++++++++++++++- tests/test_core/test_deactivated_user.py | 61 +++++++++++ tests/test_core/test_guest_role.py | 58 ++++++++-- .../test_search_domain_permissions.py | 94 ++++++++++++++++ tests/test_plugin_migrations.py | 2 +- .../test_endpoint_delivery_descriptor.py | 70 ++++++++++++ .../test_geenforce_payload_hardening.py | 40 +++++-- .../test_supplies_card_ordering.py | 101 ++++++++++++++++++ 30 files changed, 991 insertions(+), 98 deletions(-) create mode 100644 plugins/endpoints/migrations/versions/0005_endpoint_command_pinnedsha256.py create mode 100644 tests/test_core/test_deactivated_user.py create mode 100644 tests/test_core/test_search_domain_permissions.py create mode 100644 tests/test_plugins/test_supplies_card_ordering.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 061d8a2e..2b6b5691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,91 @@ ADR-007 and ADR-002. ### Fixed +- **Global search answered twelve domains behind one permission.** The route is + gated on `assets.view`, and that was the entire check: looking up an asset + number also read out of the staff directory, the USB checkout history, the + notification log and the network inventory, none of which consult their own + permission. The `SEARCH_DOMAINS` settings are site feature toggles, not + authorization, and were never doing that job. Each domain now answers only to + a caller holding its permission - `employees.view` for the directory, + `usb.view` for checkouts, `network.view` for address and subnet matches, + `computers.view` for hostnames. Asset, custom-field and vendor/model results + stay on `assets.view`, since all three emit asset rows. The check is + `caller_holds` rather than `require_permission`: a search that refused the + whole request because one domain was off-limits would be useless, so a domain + the caller cannot see simply contributes nothing. This is also what makes + `assets.view` safe to hand to the guest role again, which is what lets a + signed-out visitor look up an asset number without also getting the staff + directory. + +- **The printer supplies card was ordered by printer name, so the emptiest + cartridge was not first and sometimes was not shown at all.** Rows sorted on + "is critical" and then alphabetically, and critical is everything at or below + 5% - so a cartridge at 5% and one at 0% tied on the first key and the alphabet + broke the tie. That is the wrong order on its own, and worse than it looks: + the dashboard queue keeps only the first few rows per source, so a printer + that was genuinely empty got truncated off the card while a fuller one held + its place. The card exists to answer "what do I walk out and change today" and + was answering it with the alphabet. Rows now sort by the emptiest cartridge on + the printer, ascending. Waste cartridges were already normalised to percent + remaining, so one ascending sort is worst-first for every supply type and the + critical flag falls out of it rather than needing its own key; the name only + breaks ties. + +- **A permission became grantable to anonymous visitors because of how it was + spelled.** `guest_grantable_permissions()` opened any permission whose name + ended `.view` or `.read` to the guest role, on the reasoning that reading is + the safe default. That is true of a permission gating a list of asset numbers + and false of one gating bytes, and a suffix cannot tell them apart. Twenty-four + of eighty-five qualified, and nobody had decided any of them: + `applications.view` reached the SYSTEM-context PowerShell bodies and the + installer binaries a site pushes to its bays; `backups.view` returned the full + payload that the separate `backups.download` permission exists to protect; + `assets.view` gated a global search that queries the staff directory, the USB + history and the network inventory with no per-domain check of its own; + `audit.view` and `settings.view` are admin-category and were opened by naming + alone. Every entry is now DECLARED, by core or by the plugin that owns it, + having read what its routes actually return. Adding one is a code change, + which is the point - it puts the decision in a diff rather than in a suffix. + The two routes serving script bodies and installer packages moved to a new + `applications.download` permission, which is not grantable, so the catalogue + metadata can still be opened to a site's floor without the bytes going with it. + +- **Deactivating an account did not end its sessions.** There is no token + denylist in this product, so the JWT user loader is the only place a + deactivated account can be stopped, and it did a bare lookup by id. Somebody + deactivated at 09:00 kept a working access token until it expired and a + refresh token that minted new ones for thirty days. `/api/auth/refresh` and + the API-token path already checked `isactive`; the loader now does too. + +- **The login rate limit could be reset by a header the caller writes.** + `client_ip()` read the first `X-Forwarded-For` hop unconditionally, so + rotating it per request bought an unbounded budget, and every audit row + recorded the forged address. This walked around `TRUSTED_PROXY_COUNT`, which + exists to decide whether a proxy is really in front: the app deliberately + leaves ProxyFix off by default for exactly this reason, and then read the + header anyway. Resolution is now from `remote_addr`, which ProxyFix has + already filled from the header where a proxy is configured. Two plugins had + hand-rolled the same mistake for their own limiters and now share the helper, + which is on `shopdb.api` so the third one does not. **A site behind IIS must + set `TRUSTED_PROXY_COUNT`** or its whole floor shares one rate-limit bucket; + INSTALL-WINDOWS-IIS.md now says so. + +- **What an operator approved and what an endpoint ran were two different + lookups.** Delivery descriptors for `run_script` and `update_app` are resolved + when the agent claims a command, not when the command is queued, and the + command row recorded no digest - the enqueue path resolved the descriptor + purely to validate it and threw the result away. Anyone able to replace a + script body or an installer package could do so in the gap, and the + re-resolved descriptor would then faithfully describe the new bytes: the agent + verifies correctly and runs something nobody approved, with the audit trail + naming the operator who queued the original. The approved digest is now pinned + on the command at enqueue and the claim refuses a descriptor that no longer + matches, failing the push instead of shipping it. Separately, an agent may + only finalize a command that is actually running, so a result cannot be posted + for a command that was never claimed nor overwrite a recorded failure with a + success. + - **The published site's API documentation page had no spec to show.** `shopdb/core/api/docs.py` reads `docs/openapi.json` at runtime to serve `/api/docs`, and publication excluded the whole of `docs/` because the prose diff --git a/agent/ShopDbEndpointAgent.Core/CommandClient.cs b/agent/ShopDbEndpointAgent.Core/CommandClient.cs index 296dc280..0dcc395c 100644 --- a/agent/ShopDbEndpointAgent.Core/CommandClient.cs +++ b/agent/ShopDbEndpointAgent.Core/CommandClient.cs @@ -64,10 +64,17 @@ public sealed class CommandClient : IDisposable // result is logged and swallowed so a transient network blip does not crash // the worker; the command simply stays 'running' server-side until a later // operator retry supersedes it. + // hostname rides along so the server can check this agent owns the command + // it is finalizing. The shared collector key does not say who is calling, + // so without it any agent can post a result for any command id. The server + // treats the parameter as optional for older agents and enforces it when + // present, so shipping this ahead of a fleet update is safe. public async Task ReportResultAsync( - int commandId, CommandResult result, CancellationToken token) + int commandId, string hostname, CommandResult result, + CancellationToken token) { - var url = $"api/endpoints/commands/{commandId}/result"; + var url = $"api/endpoints/commands/{commandId}/result?hostname=" + + Uri.EscapeDataString(hostname); using var response = await http.PostAsJsonAsync( url, result, JsonOptions, token); if (!response.IsSuccessStatusCode) diff --git a/agent/ShopDbEndpointAgent.Core/CommandWorker.cs b/agent/ShopDbEndpointAgent.Core/CommandWorker.cs index ec6dea0e..2d71232d 100644 --- a/agent/ShopDbEndpointAgent.Core/CommandWorker.cs +++ b/agent/ShopDbEndpointAgent.Core/CommandWorker.cs @@ -130,7 +130,8 @@ public sealed class CommandWorker : BackgroundService }; } - await client.ReportResultAsync(claim.EndpointCommandId, result, token); + await client.ReportResultAsync( + claim.EndpointCommandId, hostname, result, token); logger.LogInformation("Command {Id} finished succeeded={Succeeded}", claim.EndpointCommandId, result.Succeeded); diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index ba77db0f..3e6ed2ab 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -2013,3 +2013,20 @@ class ComputersPlugin(BasePlugin): ('computers.edit', 'Edit computers', 'computers'), ('computers.delete', 'Delete computers', 'computers'), ] + + def get_guest_grantable_permissions(self) -> List[str]: + """Declared because a site chose it: the floor looks PCs up by number. + + KNOWN GAP, tracked separately. The computer serializer is + BaseModel.to_dict(), so it publishes every column - serialnumber, the + Azure device and object ids - and the detail route resolves + accessmethods into working rdp:// and vnc:// links plus the full + Communication row. A guest granted this gets all of it, on the LIST + route as well, so it is one request rather than one per asset. + + The fix is a field allowlist on Computer.to_dict() plus suppressing the + resolved links unless the caller holds a remote-access permission. Do + not widen this hook until that lands; a site enabling guest access + today should weigh it. + """ + return ['computers.view'] diff --git a/plugins/endpoints/api/routes.py b/plugins/endpoints/api/routes.py index ed8fb5ae..087181c2 100644 --- a/plugins/endpoints/api/routes.py +++ b/plugins/endpoints/api/routes.py @@ -7,6 +7,7 @@ These routes are the human-facing read surface, gated by user JWT + RBAC. """ import hashlib +import hmac import json import os import time @@ -941,6 +942,43 @@ _CLAIM_HOLD_SECONDS = 25 _CLAIM_POLL_INTERVAL = 1.0 +def _pin_holds(command, descriptor): + """Does the descriptor still carry the digest this command was queued for? + + True when nothing was pinned (actions that deliver no content), or when the + descriptor is missing entirely - a vanished script or version already sends + the command through with no descriptor so the agent fails closed, and that + path is not a content swap. + """ + if not command.pinnedsha256: + return True + if descriptor is None: + return True + return hmac.compare_digest( + str(descriptor.get('sha256') or ''), command.pinnedsha256) + + +def _fail_on_pin_drift(command, action): + """Refuse a claim whose content changed after the operator approved it.""" + command.status = EndpointCommand.STATUS_FAILED + command.resultmessage = ( + 'Refused: the {} content changed after this command was queued' + .format(action)) + command.completedat = _naive_utcnow() + db.session.commit() + current_app.logger.warning( + 'Endpoint command %s (%s, asset %s) refused: pinned sha256 %s no ' + 'longer matches the resolved descriptor', + command.endpointcommandid, action, command.assetid, + command.pinnedsha256) + AuditLog.log('refused', 'EndpointCommand', + entityid=command.endpointcommandid, + entityname=action, + details='content digest changed after enqueue') + db.session.commit() + return ('', 204) + + def _reap_stale_commands(asset_id): """Expire in-flight commands an agent will never finish for this endpoint. @@ -1234,18 +1272,25 @@ def enqueue_endpoint_command(asset_id: int): # update_app pins a specific application version; verify it resolves to a # real installer now so a broken push is rejected at enqueue rather than # failing opaquely on the endpoint minutes later. + # Digest of the content this push was APPROVED against. Kept on the row so + # the claim path can refuse a descriptor that has changed since; see + # EndpointCommand.pinnedsha256. + pinnedsha256 = None + if action == 'update_app': - _descriptor, resolve_error = _resolve_update_app(target) + descriptor, resolve_error = _resolve_update_app(target) if resolve_error is not None: return error_response(ErrorCodes.VALIDATION_ERROR, resolve_error) + pinnedsha256 = (descriptor or {}).get('sha256') # run_script pins a ShopDB-hosted script; verify it resolves to a real # uploaded body now so a broken push is rejected at enqueue rather than # failing opaquely on the endpoint later. if action == 'run_script': - _script, script_error = _resolve_run_script(target) + script, script_error = _resolve_run_script(target) if script_error is not None: return error_response(ErrorCodes.VALIDATION_ERROR, script_error) + pinnedsha256 = (script or {}).get('sha256') state = EndpointState.query.filter_by(assetid=asset_id).first() if state is None: @@ -1277,6 +1322,7 @@ def enqueue_endpoint_command(asset_id: int): targetkind=targetkind if target else None, status=EndpointCommand.STATUS_PENDING, requestedby=getattr(current_user, 'userid', None), + pinnedsha256=pinnedsha256, ) db.session.add(command) @@ -1337,6 +1383,8 @@ def claim_next_command(): # fail closed rather than 500 the poll. if command.action == 'update_app': descriptor, _resolve_error = _resolve_update_app(command.target) + if not _pin_holds(command, descriptor): + return _fail_on_pin_drift(command, 'update_app') if descriptor is not None: claim['app'] = descriptor # run_script rides a resolved script descriptor: the agent gets a @@ -1347,6 +1395,8 @@ def claim_next_command(): # agent fail closed rather than 500 the poll. if command.action == 'run_script': script_descriptor, _script_error = _resolve_run_script(command.target) + if not _pin_holds(command, script_descriptor): + return _fail_on_pin_drift(command, 'run_script') if script_descriptor is not None: claim['script'] = script_descriptor return success_response(claim) @@ -1376,6 +1426,33 @@ def report_command_result(command_id: int): return error_response(ErrorCodes.NOT_FOUND, f'No command {command_id}', http_code=404) + # An agent may only finalize ITS OWN command. The collector key is shared + # fleet-wide, so without this any agent could post a result for any command + # id - marking an install succeeded on a machine that never ran it, which + # writes a false compliance record. Binding to the claimed hostname is a + # floor, not a fix: the credential still does not prove which endpoint is + # calling, and only a per-endpoint credential closes that. + # Optional because the shipped agent does not send it yet; enforced when it + # does, so an updated fleet gets the check without a flag day. + hostname = (request.args.get('hostname') or '').strip() + if hostname: + caller_asset_id = _resolve_asset_for_hostname(hostname) + if caller_asset_id != command.assetid: + return error_response( + ErrorCodes.FORBIDDEN, + 'This command belongs to another endpoint', http_code=403) + + # Only a claimed command can be finalized. Needs no agent change, and it + # closes two things the hostname check cannot: reporting on a command that + # was never claimed, and overwriting a terminal result - which would let a + # recorded failure be replaced by a success. + if command.status != EndpointCommand.STATUS_RUNNING: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Command {} is {}, not running'.format( + command_id, command.status), + http_code=409) + payload = request.get_json(silent=True) or {} succeeded = bool(payload.get('succeeded')) command.status = (EndpointCommand.STATUS_SUCCEEDED if succeeded diff --git a/plugins/endpoints/migrations/versions/0005_endpoint_command_pinnedsha256.py b/plugins/endpoints/migrations/versions/0005_endpoint_command_pinnedsha256.py new file mode 100644 index 00000000..ef62527d --- /dev/null +++ b/plugins/endpoints/migrations/versions/0005_endpoint_command_pinnedsha256.py @@ -0,0 +1,51 @@ +"""Pin the approved content digest on an endpoint command. + +Delivery descriptors (run_script, update_app) are re-resolved when the agent +claims a command, not when an operator queues it. Between those two moments the +underlying script body or installer package can be replaced by anyone who can +edit the applications catalog, and the descriptor's SHA-256 would then describe +the NEW bytes: the agent verifies correctly and runs something the operator +never approved. + +pinnedsha256 records the digest as it stood at enqueue. The claim path compares +and refuses the command if it has moved, so a swap fails the push instead of +shipping it. + +Null for actions that deliver no content, and null on rows queued before this +migration - those are treated as unpinned and claim as before. + +Idempotent: checks the live schema first so a db.create_all() site re-runs clean +on strict MySQL 8. +""" +from alembic import op + +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'endpoints0005cmdpin' +down_revision = 'endpoints0004apps' +branch_labels = None +depends_on = None + + +def _has_column(table_name, column_name): + from sqlalchemy import inspect + + inspector = inspect(op.get_bind()) + if not inspector.has_table(table_name): + return False + return any(col['name'] == column_name + for col in inspector.get_columns(table_name)) + + +def upgrade(): + if not _has_column('endpointcommands', 'pinnedsha256'): + op.add_column('endpointcommands', + sa.Column('pinnedsha256', sa.String(length=64), + nullable=True)) + + +def downgrade(): + if _has_column('endpointcommands', 'pinnedsha256'): + op.drop_column('endpointcommands', 'pinnedsha256') diff --git a/plugins/endpoints/models/endpoint.py b/plugins/endpoints/models/endpoint.py index 47a04e31..e859f125 100644 --- a/plugins/endpoints/models/endpoint.py +++ b/plugins/endpoints/models/endpoint.py @@ -172,6 +172,19 @@ class EndpointCommand(db.Model): # Who queued it (userid) for audit; the agent never sees this. requestedby = db.Column(db.Integer, nullable=True) + # The content digest as it stood WHEN AN OPERATOR APPROVED THIS PUSH. + # + # Delivery descriptors are re-resolved at claim time, minutes or hours + # later, so without this the bytes an operator approved and the bytes the + # agent runs are two different lookups against mutable content. Whoever can + # replace a script body or an installer package could swap it in the gap + # and the digest would faithfully describe the new bytes. + # + # Set at enqueue for run_script and update_app; null for actions that + # deliver no content. The claim path refuses a command whose descriptor no + # longer matches, so a swap fails the command instead of shipping. + pinnedsha256 = db.Column(db.String(64), nullable=True) + # Agent-reported outcome. resultjson holds structured detail (for example # the observed MTConnect running state); resultmessage is a short summary. resultmessage = db.Column(db.Text, nullable=True) diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 245eef0e..71526442 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -145,10 +145,11 @@ class EndpointsPlugin(BasePlugin): # including every logged-in username on the floor, because the # panels and the list were gated on the same string. # - # It is deliberately NOT named '.view'. guest_grantable_permissions - # opens any '.view' or '.read' permission to the guest role, so this - # name is what keeps the fleet roster off the public surface while - # per-asset telemetry can still be granted. + # The name avoids '.view' for readability, but the NAME no longer + # decides anything: guest_grantable_permissions is a declared list + # now, and this plugin blesses endpoints.view alone below. Adding + # endpoints.fleet there would be what opens the roster, and the + # docstring on that hook says why not to. ('endpoints.fleet', 'View the fleet-wide endpoint list and dashboards', 'endpoints'), # Issue an allowlisted command to an endpoint agent (for example the @@ -187,6 +188,21 @@ class EndpointsPlugin(BasePlugin): 'Submit endpoint agent telemetry (fleet reporting)', 'collector'), ] + def get_guest_grantable_permissions(self) -> List[str]: + """Declared because a site chose it: live status on a PC page. + + endpoints.view only. NOT endpoints.fleet - that is the roster, and the + whole reason the two were split. + + KNOWN GAP, tracked separately: the per-asset panels this opens are + keyed on a small integer asset id and the status panel emits the + logged-in username, so walking ids reconstructs most of the roster the + split exists to protect. Bounding or rate-limiting the per-asset panels + is the fix. Until then a site enabling guest access should know the + split is a speed bump here, not a wall. + """ + return ['endpoints.view'] + def get_settings_defaults(self) -> List[Dict]: """Settings owned by the endpoints agent-key kill switch. diff --git a/plugins/geenforce/api/routes.py b/plugins/geenforce/api/routes.py index f3d6217a..c0ee5df4 100644 --- a/plugins/geenforce/api/routes.py +++ b/plugins/geenforce/api/routes.py @@ -21,7 +21,7 @@ from sqlalchemy.exc import IntegrityError from shopdb.api import ( db, cache, success_response, error_response, ErrorCodes, require_permission, require_any_permission, - authorized_service_token, Setting, Application, + authorized_service_token, Setting, Application, client_ip, ) SHAREROOT_SETTING = 'geenforce_share_root' @@ -207,12 +207,14 @@ PAYLOAD_DOWNLOAD_RATELIMIT_WINDOW_SECONDS = 60 def _client_ip(): - """Caller IP for rate limiting, honoring the first X-Forwarded-For hop - (mirrors shopdb.core.api.auth._login_ip).""" - forwarded = request.headers.get('X-Forwarded-For') - if forwarded: - return forwarded.split(',')[0].strip() - return request.remote_addr or 'unknown' + """Caller IP for rate limiting. + + Uses the shared helper, which resolves from remote_addr. The raw + X-Forwarded-For hop is client-supplied, so keying a rate limit on it lets a + caller rotate the header and get an unbounded budget - the same reasoning + _trusted_client_ip() above already applies to the allowlist. + """ + return client_ip(request) or 'unknown' def _payload_max_bytes(): diff --git a/plugins/printers/api/asset_routes.py b/plugins/printers/api/asset_routes.py index da463726..3b3e7d39 100644 --- a/plugins/printers/api/asset_routes.py +++ b/plugins/printers/api/asset_routes.py @@ -2659,6 +2659,9 @@ def dashboard_supplies(): # dashboard card and the toner report were both doing. 'levelid': printer.get('levelid'), 'iscritical': any(s['status'] == 'critical' for s in depleted), + # The emptiest cartridge on this printer, which is what decides the + # row's place in the queue. depleted is sorted and non-empty here. + 'worstremaining': depleted[0]['remaining'], 'supplies': [{ 'text': '{} {}%'.format(_shortsupplyname(supply.get('name')), supply.get('remaining')), @@ -2667,7 +2670,18 @@ def dashboard_supplies(): } for supply in depleted], }) - rows.sort(key=lambda r: (not r['iscritical'], r['printername'])) + # Emptiest first. This sorted on iscritical then NAME, and critical is + # everything at or below 5%, so a cartridge at 5% and one at 0% tied on the + # first key and the alphabet decided which came first. That is bad ordering + # on its own and worse than it looks: the dashboard queue takes only the + # first few rows per source, so the printer that is actually empty was + # being truncated away while a fuller one held its place. + # + # remaining is already normalised for waste cartridges (a full waste bin + # reads as low remaining), so one ascending sort is worst-first for every + # supply type, and iscritical falls out of it rather than needing its own + # key. Name only breaks ties. + rows.sort(key=lambda r: (r['worstremaining'], r['printername'])) # Envelope rather than a bare list, so the card can say WHY it has no rows. # An empty card and a blind card must not look the same. return success_response({'rows': rows, diff --git a/plugins/tools/api/routes.py b/plugins/tools/api/routes.py index 9dd4231d..8c31acbe 100644 --- a/plugins/tools/api/routes.py +++ b/plugins/tools/api/routes.py @@ -37,7 +37,7 @@ from flask import current_app from flask_jwt_extended import jwt_required, get_jwt_identity, current_user from shopdb.api import ( - db, cache, Setting, + db, cache, Setting, client_ip, success_response, error_response, ErrorCodes, ) @@ -117,8 +117,9 @@ def _guest_ratelimited(): person has more addresses - but it stops an open endpoint being an easy place to park data or fill a disk. """ - address = (request.headers.get('X-Forwarded-For', '').split(',')[0].strip() - or request.remote_addr or 'unknown') + # Not the raw X-Forwarded-For hop: the caller sends that, so rotating it + # gives an unbounded budget and the limit stops being a limit. + address = client_ip(request) or 'unknown' bucket = int(_utcnow().timestamp()) // GUEST_CREATE_WINDOW_SECONDS key = f'securesendguest:{address}:{bucket}' used = cache.get(key) or 0 diff --git a/plugins/warranty/plugin.py b/plugins/warranty/plugin.py index 520c5e36..bb2f9e4c 100644 --- a/plugins/warranty/plugin.py +++ b/plugins/warranty/plugin.py @@ -174,5 +174,13 @@ class WarrantyPlugin(BasePlugin): ('warranty.delete', 'Delete warranties', 'warranty'), ] + def get_guest_grantable_permissions(self) -> List[str]: + """Coverage dates and vendor for an asset somebody is standing at. + + Nothing here is a credential and nothing reaches a supplier portal; the + provider lookups run server-side with keys the caller never sees. + """ + return ['warranty.view'] + def init_app(self, app: Flask, db_instance) -> None: logger.info(f"Warranty plugin initialized (v{self.meta.version})") diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 01a3bac5..55ada8fc 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -98,7 +98,13 @@ from .plugins import plugin_manager # model internals. The endpoints plugin resolves a delivered script by name and # serves its body by digest to the agent. Additive name on the surface, minor # bump. -__contract_version__ = '0.27.0' +# 0.28.0: added client_ip and strip_port to shopdb.api so a plugin does not +# hand-roll caller-IP extraction. Two plugins had, and both read the first +# X-Forwarded-For hop, which the caller sends - so a rate limit keyed on it was +# defeated by rotating the header. The helper resolves from remote_addr, which +# ProxyFix has already filled from the header when TRUSTED_PROXY_COUNT says a +# proxy is really in front. Additive names on the surface, minor bump. +__contract_version__ = '0.28.0' # Product release version (see ADR-007). The product version and the # plugin-contract version above are distinct series with independent @@ -211,7 +217,14 @@ def create_app(config_name: str = None) -> Flask: def user_lookup_callback(_jwt_header, jwt_data): from .core.models import User identity = jwt_data["sub"] - return db.session.get(User, int(identity)) + user = db.session.get(User, int(identity)) + # Deactivating an account has to end its sessions. There is no token + # denylist, so without this check the access token stays good until it + # expires and the refresh token keeps minting new ones for 30 days. + # /auth/refresh and the API-token path already check isactive. + if user is None or not user.isactive: + return None + return user @app.after_request def apply_security_headers(response): diff --git a/shopdb/api/__init__.py b/shopdb/api/__init__.py index 6393ab64..19d7626e 100644 --- a/shopdb/api/__init__.py +++ b/shopdb/api/__init__.py @@ -104,6 +104,12 @@ def collector_authorized(pluginname: str): # as script in this origin - see shopdb/utils/uploads.py. from shopdb.utils.uploads import send_upload, harden_upload_response +# Caller IP for rate limiting and audit. On the surface so a plugin does not +# reimplement it: the obvious hand-rolled version reads X-Forwarded-For, which +# the caller supplies, so a rate limit keyed on it can be rotated away to +# nothing. See shopdb/utils/clientip.py. +from shopdb.utils.clientip import client_ip, strip_port + # Import-mode helpers: preserve legacy timestamps during a bulk data import from shopdb.utils.import_mode import ( apply_import_timestamps, @@ -346,6 +352,8 @@ __all__ = [ # Serving uploads 'send_upload', 'harden_upload_response', + 'client_ip', + 'strip_port', # Import-mode helpers 'apply_import_timestamps', 'import_mode_active', diff --git a/shopdb/core/api/applications.py b/shopdb/core/api/applications.py index 6712a2d3..023a12dd 100644 --- a/shopdb/core/api/applications.py +++ b/shopdb/core/api/applications.py @@ -656,13 +656,17 @@ def upload_application_package(app_id: int): @applications_bp.route('/package/', methods=['GET']) -@require_permission('applications.view') +@require_permission('applications.download') def serve_application_package(filename): """Download an uploaded installer. Authenticated, unlike the image: this is licensed vendor software, and an open URL would publish it to anything that can reach the site. Always sent as an attachment so a browser saves it rather than trying to render it. + + Gated on applications.download, not applications.view: a guest may be + granted the metadata permission, and "an open URL would publish it" is just + as true of a URL any anonymous visitor can reach. """ return send_upload(_apppackage_dir(), filename, as_attachment=True) diff --git a/shopdb/core/api/scripts.py b/shopdb/core/api/scripts.py index 3206835c..8b606d41 100644 --- a/shopdb/core/api/scripts.py +++ b/shopdb/core/api/scripts.py @@ -196,11 +196,15 @@ def upload_script_body(script_id: int): @scripts_bp.route('/body/', methods=['GET']) -@require_permission('applications.view') +@require_permission('applications.download') def download_script_body(filename: str): """Operator-facing script body download (JWT/RBAC). The endpoint agent uses the collector-authenticated plugin route instead; this one is for humans reviewing what a script does. + + Gated on applications.download, not applications.view: this serves the + SYSTEM-context PowerShell a site pushes to its bays, and a guest may be + granted the metadata permission. """ return send_upload(_script_dir(), filename, as_attachment=True) diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index 11218a9b..3f9d7c3d 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -18,7 +18,7 @@ from shopdb.core.models import ( from shopdb.core.api.settings import get_cached_settings from shopdb.core.services.spellfix import Vocabulary from shopdb.utils.responses import success_response -from shopdb.utils.authz import require_permission +from shopdb.utils.authz import require_permission, caller_holds logger = logging.getLogger(__name__) @@ -1233,22 +1233,42 @@ def global_search(): results = [] search_term = f'%{query}%' - # Run all search domains - results.extend(_search_applications(query, search_term)) - results.extend(_search_knowledgebase(query, search_term)) - results.extend(_search_guides(query, search_term)) - results.extend(_search_employees(query, search_term)) - results.extend(_search_assets(query, search_term)) - results.extend(_search_measuringtools(query, search_term)) - results.extend(_search_usbdevices(query, search_term)) - results.extend(_search_printeditems(query, search_term)) - results.extend(_search_customfields(query, search_term)) - results.extend(_search_notifications(query, search_term)) - results.extend(_search_hostnames(query, search_term)) - results.extend(_search_vendor_model_type(query, search_term)) + # Every domain answers only to a caller who holds ITS permission. + # + # The route is gated on assets.view alone, and that used to be the whole + # check: one permission then queried twelve domains, so it read out of the + # staff directory, the USB checkout history, the notification log and the + # network inventory to anybody who could look up an asset number. The + # SEARCH_DOMAINS settings are feature toggles a site sets, not authorization, + # and were never doing this job. + # + # caller_holds rather than require_permission: a search that refuses the + # whole request because one domain is off-limits is useless. Each domain + # simply contributes nothing. + domains = [ + ('assets.view', _search_assets), + # Emit asset results routed to an asset page, so they belong to the + # same grant as the assets themselves. + ('assets.view', _search_customfields), + ('assets.view', _search_vendor_model_type), + ('applications.view', _search_applications), + ('kb.view', _search_knowledgebase), + ('guides.view', _search_guides), + ('employees.view', _search_employees), + ('measuringtools.view', _search_measuringtools), + ('usb.view', _search_usbdevices), + ('printedparts.view', _search_printeditems), + ('notifications.view', _search_notifications), + ('computers.view', _search_hostnames), + ] + for permission, search in domains: + if caller_holds(permission): + results.extend(search(query, search_term)) - # IP-specific searches - if classification['is_ip']: + # IP-specific searches. An address and the subnet it sits in are network + # inventory, and knowing what answers on 10.20.x is a different question + # from looking up an asset number. + if classification['is_ip'] and caller_holds('network.view'): results.extend(_search_by_ip(query, search_term)) results.extend(_search_subnets(query)) diff --git a/shopdb/core/api/users.py b/shopdb/core/api/users.py index 54492075..64cc06e0 100644 --- a/shopdb/core/api/users.py +++ b/shopdb/core/api/users.py @@ -351,19 +351,20 @@ def update_role(roleid: int): # Update permissions if 'permissions' in data and role.rolename != 'admin': # The guest role is anyone with no account, so what it may hold is - # bounded: reads, plus the handful of writes a plugin has explicitly - # declared safe to do anonymously. Refused at the door rather than - # warned about - a checkbox that quietly makes deleting an asset - # anonymous is not a warning anybody reads twice. + # bounded to what core or a plugin has explicitly declared grantable. + # Refused at the door rather than warned about - a checkbox that + # quietly makes deleting an asset anonymous is not a warning anybody + # reads twice. if role.rolename == Role.GUEST: allowed = guest_grantable_permissions() refused = sorted(set(data['permissions']) - allowed) if refused: return error_response( ErrorCodes.VALIDATION_ERROR, - 'The guest role cannot hold {}. A guest may be granted any ' - 'view permission, and only writes a plugin has declared ' - 'safe to do without an account.'.format(', '.join(refused))) + 'The guest role cannot hold {}. A guest may only be ' + 'granted permissions core or the owning plugin has ' + 'declared grantable, which is a code change rather than a ' + 'setting.'.format(', '.join(refused))) old_perms = [p.name for p in role.permissions] perms = Permission.query.filter(Permission.name.in_(data['permissions'])).all() new_perms = [p.name for p in perms] diff --git a/shopdb/core/models/user.py b/shopdb/core/models/user.py index 6f7561e2..ba10489a 100644 --- a/shopdb/core/models/user.py +++ b/shopdb/core/models/user.py @@ -46,6 +46,12 @@ class Permission(db.Model): ('assets.delete', 'Delete assets', 'assets'), # Applications ('applications.view', 'View applications', 'applications'), + # Deliberately NOT '.view': reading the catalogue and pulling the bytes + # are different acts. A script body is SYSTEM-context PowerShell and an + # installer is a vendor binary, so the routes that serve them are gated + # here rather than on the metadata permission a site may hand to guests. + ('applications.download', + 'Download application installers and script bodies', 'applications'), ('applications.create', 'Create applications', 'applications'), ('applications.edit', 'Edit applications', 'applications'), ('applications.delete', 'Delete applications', 'applications'), @@ -152,28 +158,60 @@ def full_permission_catalog(include_disabled=False): return catalog +# Core permissions a site MAY grant the guest role. Declared, not derived. +# Each one has been read against what its routes actually return. +# +# Deliberately absent, with the reason, so nobody re-adds them by reflex: +# settings.view - every setting row, and secret masking is a substring test +# audit.view - usernames, addresses, and the before/after of every change +# users.view - gates nothing; list_users is require_role('admin') +CORE_GUEST_GRANTABLE = { + # Aggregate counts only; no row-level data reaches the caller. + 'reports.view', + # Catalogue metadata. Safe only because the bytes moved to + # applications.download, which is not grantable. + 'applications.view', + # Assets, and the global search over them. Safe only because search now + # gates each domain on its own permission: holding this reaches assets and + # nothing else, where it used to reach the staff directory and the USB, + # notification and network domains too. + 'assets.view', +} + + def guest_grantable_permissions(): """Which permissions the guest role may be granted. - Reading is the safe default and covers almost everything a site wants to - open up: any `.view` or `.read` permission may be handed to guests. + Every entry is DECLARED, by core below or by a plugin through + get_guest_grantable_permissions(). Nothing qualifies by being spelled a + particular way. - A write is different - it lets somebody with no account change this - database, and the audit trail can then say when and from where but never - who. So a write has to be BLESSED by whoever owns it, through the plugin's - get_guest_grantable_permissions(). Secure Send is the case that exists: - creating a one-view link for somebody is the whole point of the page, and - the page has always been usable by people with no account here. + It used to: any name ending `.view` or `.read` was grantable, on the + reasoning that reading is the safe default. That is true of a permission + that gates a list of asset numbers and false of one that gates bytes, and + the suffix cannot tell them apart. `applications.view` reached the + SYSTEM-context PowerShell bodies and the installer binaries; + `backups.view` returned the full payload the separate backups.download + permission exists to protect; `assets.view` gated a global search that + queries the staff directory. None of those were decided. They were named. - Everything else is refused at the door rather than warned about, because a - checkbox that quietly makes deleting an asset anonymous is not a warning - anybody reads twice. + So the rule is inverted: a permission is grantable when somebody wrote it + down, having looked at what it actually serves. Adding one is a code + change, which is the point - it puts the decision in a diff rather than in + a suffix. + + Naming one here does not open it. It makes the checkbox available in the + role grid; a site still has to tick it. + + The bar, for a read: could an anonymous visitor hold this, forever, and + would the site be content with everything it returns? For a write, add + that the act can never be attributed to anybody. """ from flask import current_app, has_app_context catalog = full_permission_catalog() - grantable = {name for name, _, _ in catalog - if name.endswith('.view') or name.endswith('.read')} + known = {name for name, _, _ in catalog} + grantable = {name for name in CORE_GUEST_GRANTABLE if name in known} if not has_app_context(): return grantable @@ -181,7 +219,6 @@ def guest_grantable_permissions(): if not pm: return grantable - known = {name for name, _, _ in catalog} for name, plugin in pm.get_all_plugins().items(): if not pm.registry.is_enabled(name): continue diff --git a/shopdb/plugins/base.py b/shopdb/plugins/base.py index 6114b80a..850f412a 100644 --- a/shopdb/plugins/base.py +++ b/shopdb/plugins/base.py @@ -107,17 +107,24 @@ class BasePlugin(ABC): def get_guest_grantable_permissions(self) -> List[str]: """Permissions this plugin allows a site to grant to the GUEST role. - A guest is anyone with no account. Any `.view` or `.read` permission is - grantable without asking a plugin - reading is the safe default. This - hook is for the exception: a WRITE that is genuinely meant to be usable - by somebody who never signs in. + A guest is anyone with no account. EVERY permission of this plugin's + that a site may grant one has to be named here, reads included. + + Reads used to be automatic: anything ending `.view` or `.read` was + grantable. That treats a name as a decision, and it went wrong in the + obvious way - permissions that gated installer bytes, backup payloads + and a search across the staff directory were all anonymous-grantable + because of how they were spelled. So the plugin that owns a permission + now says, having looked at what its routes return. Naming one here does not open it. It makes the checkbox available in the role grid; a site still has to tick it. Return [] (the default) and - none of this plugin's writes can ever be made anonymous. + nothing of this plugin's can be made anonymous. - The bar: could this action be attributed to nobody, forever, and still - be defensible in the audit log? If not, leave it out. + The bar for a read: could an anonymous visitor hold this, forever, and + would the site be content with everything the routes return - every + column of every serializer, not just the ones the UI happens to show? + For a write, add that the act can never be attributed to anybody. """ return [] diff --git a/shopdb/utils/clientip.py b/shopdb/utils/clientip.py index 4b22e826..c61eb7fd 100644 --- a/shopdb/utils/clientip.py +++ b/shopdb/utils/clientip.py @@ -6,6 +6,23 @@ every connection - so it must be stripped before the value is stored (audit), matched (dashboard IP fallback), or used as a rate-limit key (login). Left in, it makes every request look like a different client. +X-Forwarded-For is NOT read here. A client can send that header itself, so +trusting it unconditionally hands any caller a free identity: the login rate +limit and the account lockout are keyed on this value, and the audit log +records it. Reading it directly also walked around TRUSTED_PROXY_COUNT, which +exists precisely to decide whether a proxy is really in front (see the ProxyFix +block in shopdb/__init__.py). + +The trusted case is already handled upstream. When TRUSTED_PROXY_COUNT is set, +ProxyFix resolves X-Forwarded-For into remote_addr before the request reaches a +view, so remote_addr is the real client. When it is unset, remote_addr is the +peer that actually connected. Either way remote_addr is the trustworthy source, +which is why this module only strips the port from it. + +A site behind IIS or nginx MUST set TRUSTED_PROXY_COUNT (1 for a single proxy), +or every request will key on the proxy's own address and the per-client rate +limit becomes one shared bucket. + Port stripping is IPv6-aware: a bracketed [2001:db8::1]:443 loses its port; a bare 2001:db8::1 (many colons, no brackets) is returned untouched. """ @@ -26,14 +43,11 @@ def strip_port(ipaddress): def client_ip(request_obj): - """First-hop client IP behind one proxy, port stripped. + """Client IP, port stripped, from the connecting peer. - Honors the first X-Forwarded-For hop, else remote_addr. Returns None only - when neither is present (callers that need a non-null key coerce it). + Resolved from remote_addr, which ProxyFix has already rewritten from + X-Forwarded-For when TRUSTED_PROXY_COUNT says a proxy is really in front. + Returns None only when remote_addr is absent (callers that need a non-null + key coerce it). """ - forwarded = request_obj.headers.get('X-Forwarded-For') - if forwarded: - raw = forwarded.split(',')[0].strip() - else: - raw = request_obj.remote_addr - return strip_port(raw) + return strip_port(request_obj.remote_addr) diff --git a/tests/test_core/test_auth_ratelimit.py b/tests/test_core/test_auth_ratelimit.py index 84ab1e71..1e70b1f2 100644 --- a/tests/test_core/test_auth_ratelimit.py +++ b/tests/test_core/test_auth_ratelimit.py @@ -32,11 +32,18 @@ def ratelimit_on(app): def _bad_login(client, ip): - """Attempt a login for a non-existent user from a given source IP.""" + """Attempt a login for a non-existent user from a given source IP. + + The caller is set through REMOTE_ADDR, the peer that actually connected, + because that is what the limiter keys on. It used to be sent as + X-Forwarded-For, which meant these tests changed identity using the very + header a real attacker would rotate to escape the limit - so the suite + could not have told the difference. + """ return client.post( '/api/auth/login', json={'username': 'ghost', 'password': 'wrong'}, - headers={'X-Forwarded-For': ip}, + environ_base={'REMOTE_ADDR': ip}, ) diff --git a/tests/test_core/test_clientip.py b/tests/test_core/test_clientip.py index b7d7dedd..a77da169 100644 --- a/tests/test_core/test_clientip.py +++ b/tests/test_core/test_clientip.py @@ -1,4 +1,12 @@ -"""strip_port / client_ip normalization (ARR forwards clientip:port).""" +"""client_ip: which address the app treats as the caller, and port normalization. + +The answer is remote_addr. X-Forwarded-For is written by the caller, and the +login rate limit, the account lockout and the audit log all key on this value, +so honoring the header let a caller rotate it for an unbounded budget and an +untraceable trail. Where a proxy really is in front, TRUSTED_PROXY_COUNT turns +on ProxyFix and the header arrives already resolved into remote_addr - ports +included, because ARR forwards clientip:port. +""" from shopdb.utils.clientip import strip_port, client_ip @@ -34,12 +42,94 @@ class _FakeRequest: self.remote_addr = remote_addr -def test_client_ip_prefers_forwarded_first_hop_stripped(): - request_obj = _FakeRequest(headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'}, - remote_addr='127.0.0.1') +def test_client_ip_ignores_the_forwarded_header(): + """The caller sends X-Forwarded-For, so it cannot decide who the caller is. + + This test used to assert the opposite. The login rate limit, the account + lockout and the audit log all key on client_ip, so honoring the header let + a caller rotate it for an unbounded budget and an untraceable trail. Where + a proxy really is in front, TRUSTED_PROXY_COUNT turns on ProxyFix and the + header reaches us already resolved into remote_addr. + """ + request_obj = _FakeRequest( + headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'}, + remote_addr='127.0.0.1') + assert client_ip(request_obj) == '127.0.0.1' + + +def test_client_ip_strips_a_port_proxyfix_passed_through(): + """ARR writes clientip:port into the header and ProxyFix copies it verbatim + into remote_addr, so the port still has to come off.""" + request_obj = _FakeRequest(headers={}, remote_addr='10.0.0.5:51000') assert client_ip(request_obj) == '10.0.0.5' def test_client_ip_falls_back_to_remote_addr_stripped(): request_obj = _FakeRequest(headers={}, remote_addr='192.168.1.9:60000') assert client_ip(request_obj) == '192.168.1.9' + + +# --------------------------------------------------------------------------- +# Through a real request, and through the login limiter that keys on this. +# --------------------------------------------------------------------------- +def test_the_forwarded_header_does_not_decide_the_client_ip(app): + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', headers={'X-Forwarded-For': '10.0.0.99'}, + environ_base={'REMOTE_ADDR': '203.0.113.7'}): + from flask import request + assert client_ip(request) == '203.0.113.7' + + +def test_the_peer_address_decides_the_client_ip(app): + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', environ_base={'REMOTE_ADDR': '203.0.113.8'}): + from flask import request + assert client_ip(request) == '203.0.113.8' + + +def test_a_forwarded_port_is_still_stripped(app): + """IIS ARR writes "clientip:port" and ProxyFix passes it through verbatim, + so the port has to come off remote_addr or every request looks like a new + client.""" + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', environ_base={'REMOTE_ADDR': '203.0.113.9:54321'}): + from flask import request + assert client_ip(request) == '203.0.113.9' + + +def test_rotating_the_forwarded_header_does_not_reset_the_login_limiter(app, + client, + db): + """The budget follows the connection, not a header the caller writes. + + Sprayed across many usernames on purpose: the per-account lockout bounds + guessing at ONE account, so the IP limiter is the only thing standing + between a caller and unlimited attempts spread across the directory. That + is what rotating the header used to defeat. + """ + caller = {'REMOTE_ADDR': '203.0.113.50'} + previous = dict(app.config) + app.config['AUTH_RATELIMIT_ENABLED'] = True + app.config['AUTH_RATELIMIT_MAX'] = 3 + try: + seen = set() + for hop in range(10): + response = client.post( + '/api/auth/login', + headers={'X-Forwarded-For': '198.51.100.{}'.format(hop)}, + environ_base=caller, + json={'username': 'nobody{}'.format(hop), 'password': 'wrong'}) + seen.add(response.status_code) + if response.status_code == 429: + break + finally: + app.config['AUTH_RATELIMIT_ENABLED'] = previous.get( + 'AUTH_RATELIMIT_ENABLED', False) + app.config['AUTH_RATELIMIT_MAX'] = previous.get('AUTH_RATELIMIT_MAX', 30) + assert 429 in seen, 'rotating X-Forwarded-For bought an unbounded budget' diff --git a/tests/test_core/test_deactivated_user.py b/tests/test_core/test_deactivated_user.py new file mode 100644 index 00000000..01b10432 --- /dev/null +++ b/tests/test_core/test_deactivated_user.py @@ -0,0 +1,61 @@ +"""Deactivating an account ends its sessions. + +There is no token denylist in this product, so the JWT user loader is the only +place a deactivated account can be stopped. It did a bare lookup by id, which +meant deactivating somebody left their access token working until it expired and +their refresh token minting new ones for thirty days. /auth/refresh and the API +token path already checked isactive; the loader did not. +""" +from werkzeug.security import generate_password_hash + + +def _login(client, username='deactivateme', password='testpass'): + response = client.post('/api/auth/login', + json={'username': username, 'password': password}) + assert response.status_code == 200, response.get_json() + return response.get_json()['data'] + + +def _make_user(db, username='deactivateme'): + from shopdb.core.models import User + + user = User( + username=username, + email=f'{username}@test.local', + passwordhash=generate_password_hash('testpass'), + ) + db.session.add(user) + db.session.commit() + return user + + +def test_a_live_token_stops_working_once_the_account_is_deactivated(client, db): + user = _make_user(db) + tokens = _login(client) + headers = {'Authorization': 'Bearer {}'.format(tokens['access_token'])} + + assert client.get('/api/auth/me', headers=headers).status_code == 200 + + user.isactive = False + db.session.commit() + + assert client.get('/api/auth/me', headers=headers).status_code == 401 + + +def test_a_deactivated_account_cannot_refresh(client, db): + user = _make_user(db, 'refreshme') + tokens = _login(client, 'refreshme') + headers = {'Authorization': 'Bearer {}'.format(tokens['refresh_token'])} + + user.isactive = False + db.session.commit() + + assert client.post('/api/auth/refresh', headers=headers).status_code == 401 + + +def test_an_active_account_is_unaffected(client, db): + """The loader must not reject everybody; isactive defaults true.""" + _make_user(db, 'stillhere') + tokens = _login(client, 'stillhere') + headers = {'Authorization': 'Bearer {}'.format(tokens['access_token'])} + assert client.get('/api/auth/me', headers=headers).status_code == 200 diff --git a/tests/test_core/test_guest_role.py b/tests/test_core/test_guest_role.py index a59c8b7f..3a37d511 100644 --- a/tests/test_core/test_guest_role.py +++ b/tests/test_core/test_guest_role.py @@ -98,12 +98,39 @@ def test_a_site_with_no_guest_role_behaves_as_it_always_did(client, db): # The guardrail # --------------------------------------------------------------------------- -def test_a_view_permission_is_grantable(app): +def test_a_declared_permission_is_grantable(app): from shopdb.core.models import guest_grantable_permissions with app.app_context(): grantable = guest_grantable_permissions() - assert 'assets.view' in grantable assert 'reports.view' in grantable + assert 'applications.view' in grantable + + +def test_a_view_permission_is_not_grantable_merely_for_being_a_view(app): + """The suffix stopped deciding. These are all '.view' and none is declared. + + settings.view gates every setting row; audit.view the before/after of every + change; users.view nothing at all. Each was anonymous-grantable purely + because of how it was spelled. + + assets.view is deliberately NOT in this list. It was, while global search + answered twelve domains behind it; now that search gates each domain on its + own permission the grant reaches assets alone, and the floor needs it to + look an asset number up without signing in. + """ + from shopdb.core.models import guest_grantable_permissions + with app.app_context(): + grantable = guest_grantable_permissions() + for name in ('settings.view', 'audit.view', 'users.view'): + assert name not in grantable, name + + +def test_the_bytes_behind_applications_are_not_grantable(app): + """applications.view is grantable only because the bytes moved off it.""" + from shopdb.core.models import guest_grantable_permissions + with app.app_context(): + grantable = guest_grantable_permissions() + assert 'applications.download' not in grantable def test_a_write_permission_is_not_grantable(app): @@ -127,13 +154,29 @@ def test_the_api_refuses_to_grant_a_guest_a_write(client, db, auth_headers, assert guestrole.permissions == [] -def test_the_api_allows_granting_a_guest_a_view(client, db, auth_headers, - guestrole): +def test_the_api_allows_granting_a_guest_a_declared_read(client, db, + auth_headers, + guestrole): + response = client.put('/api/users/roles/{}'.format(guestrole.roleid), + headers=auth_headers, + json={'permissions': ['reports.view']}) + assert response.status_code == 200 + assert [p.name for p in guestrole.permissions] == ['reports.view'] + + +def test_the_api_refuses_an_undeclared_read(client, db, auth_headers, + guestrole): + """This test used to grant guests audit.view and pass, which is the bug. + + audit.view ends in '.view' and gates usernames, addresses and the + before/after of every change in the database. + """ response = client.put('/api/users/roles/{}'.format(guestrole.roleid), headers=auth_headers, json={'permissions': ['audit.view']}) - assert response.status_code == 200 - assert [p.name for p in guestrole.permissions] == ['audit.view'] + assert response.status_code == 400 + assert 'audit.view' in response.get_json()['data']['error']['message'] + assert guestrole.permissions == [] def test_the_guest_role_cannot_be_deleted(client, auth_headers, guestrole): @@ -155,8 +198,9 @@ def test_the_role_list_says_what_a_guest_may_hold(client, auth_headers, rows = client.get('/api/users/roles', headers=auth_headers).get_json()['data'] guest = next(r for r in rows if r['rolename'] == 'guest') assert guest['isguest'] is True - assert 'assets.view' in guest['grantable'] + assert 'reports.view' in guest['grantable'] assert 'assets.delete' not in guest['grantable'] + assert 'audit.view' not in guest['grantable'] other = next((r for r in rows if r['rolename'] != 'guest'), None) if other is not None: diff --git a/tests/test_core/test_search_domain_permissions.py b/tests/test_core/test_search_domain_permissions.py new file mode 100644 index 00000000..9e249a0e --- /dev/null +++ b/tests/test_core/test_search_domain_permissions.py @@ -0,0 +1,94 @@ +"""Global search answers each domain only to a caller who holds ITS permission. + +The route is gated on assets.view, and that used to be the entire check. One +permission then queried twelve domains, so anyone who could look up an asset +number also read out of the staff directory, the USB checkout history, the +notification log and the network inventory. + +That is why assets.view could not be handed to guests, which cost the floor its +search. Gating each domain separately is what makes the grant safe again: it +reaches assets and nothing else. +""" +from unittest.mock import patch + +import pytest + +from shopdb.core.models import Permission, Role + + +def _grant_guest(db, *names): + guest = Role.guest() + guest.permissions = [p for p in Permission.query.filter( + Permission.name.in_(names)).all()] + db.session.commit() + + +@pytest.fixture +def guestrole(db): + """The seeded guest role, holding nothing.""" + Permission.seed() + Role.seedguest() + db.session.commit() + return Role.guest() + + +def _search(client, term='a'): + response = client.get('/api/search?q={}'.format(term)) + assert response.status_code == 200, response.get_json() + return response.get_json()['data'] + + +FAKE_EMPLOYEE = [{ + 'First_Name': 'Ada', 'Last_Name': 'Smith', 'SSO': 508123, + 'Team': 'Machining', 'Role': 'Operator', +}] + + +def _with_employee_directory(): + """The HR directory is an external database and is unreachable in tests, so + the domain silently returned nothing and a leak test over it proved nothing. + Patched here so there is something real to leak.""" + return patch('shopdb.core.api.search._employee_rows', + return_value=FAKE_EMPLOYEE) + + +def test_assets_view_alone_does_not_reach_the_staff_directory(client, db, + guestrole): + """The finding this exists for: ?q=smith returned employee records to a + signed-out visitor holding only assets.view.""" + _grant_guest(db, 'assets.view') + with _with_employee_directory(): + data = _search(client, 'smith') + assert 'employee' not in {row['type'] for row in data['results']} + + +def test_employees_view_does_reach_it(client, db, guestrole): + """The other direction, so the gate is not passing by refusing everything.""" + _grant_guest(db, 'assets.view', 'employees.view') + with _with_employee_directory(): + data = _search(client, 'smith') + employees = [row for row in data['results'] if row['type'] == 'employee'] + assert len(employees) == 1 + assert employees[0]['title'] == 'Ada Smith' + + +def test_search_still_answers_a_guest_with_assets_view(client, db, guestrole): + """The regression that prompted this: guests lost search entirely. + + assets.view left the grantable set because of the leak above, which took + the floor's asset lookup with it. + """ + _grant_guest(db, 'assets.view') + response = client.get('/api/search?q=press') + assert response.status_code == 200 + + +def test_a_guest_with_nothing_granted_is_refused(client, db, guestrole): + _grant_guest(db) + assert client.get('/api/search?q=press').status_code in (401, 403) + + +def test_assets_view_is_grantable_again(app): + from shopdb.core.models import guest_grantable_permissions + with app.app_context(): + assert 'assets.view' in guest_grantable_permissions() diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index d2dd6f9b..25159e49 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -94,7 +94,7 @@ EXPECTED_HEAD_REVISION['warranty'] = 'warranty0002proof' # adds the endpointcommands queue plus the MTConnect state columns, 0003 # adds the generic command target/targetkind columns, and 0004 adds the # endpointapplications SBOM snapshot table. -EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0004apps' +EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0005cmdpin' # Plugins built after the cutover: their 0001 baseline really creates tables the # core chain never owned. diff --git a/tests/test_plugins/test_endpoint_delivery_descriptor.py b/tests/test_plugins/test_endpoint_delivery_descriptor.py index a099a80a..9d038997 100644 --- a/tests/test_plugins/test_endpoint_delivery_descriptor.py +++ b/tests/test_plugins/test_endpoint_delivery_descriptor.py @@ -79,3 +79,73 @@ def test_a_package_with_no_recorded_digest_is_refused_not_delivered_unverified(a assert descriptor is None assert 'SHA-256' in error + + +# --------------------------------------------------------------------------- +# The digest is pinned when an operator approves the push, not when the agent +# claims it. Between those two moments the bytes behind a descriptor can be +# replaced by anyone who can edit the catalog, and the re-resolved descriptor +# would faithfully describe the NEW bytes. +# --------------------------------------------------------------------------- + +def test_an_unpinned_command_still_claims(app): + """Actions that deliver no content, and rows queued before the pin existed.""" + from plugins.endpoints.api.routes import _pin_holds + from plugins.endpoints.models import EndpointCommand + + with app.app_context(): + command = EndpointCommand(assetid=1, action='mtconnect_start', + pinnedsha256=None) + assert _pin_holds(command, None) is True + assert _pin_holds(command, {'sha256': 'b' * 64}) is True + + +def test_a_matching_digest_claims(app): + from plugins.endpoints.api.routes import _pin_holds + from plugins.endpoints.models import EndpointCommand + + with app.app_context(): + command = EndpointCommand(assetid=1, action='run_script', + pinnedsha256='c' * 64) + assert _pin_holds(command, {'sha256': 'c' * 64}) is True + + +def test_a_swapped_digest_does_not_claim(app): + """The finding this exists for: approve one script, ship another. + + An operator queues run_script against a reviewed body. Before the agent's + long poll returns, somebody with applications.edit replaces the body and + the digest is restamped. Without the pin the claim re-resolves, hands the + agent the attacker's digest, and the agent verifies it correctly. + """ + from plugins.endpoints.api.routes import _pin_holds + from plugins.endpoints.models import EndpointCommand + + with app.app_context(): + command = EndpointCommand(assetid=1, action='run_script', + pinnedsha256='c' * 64) + assert _pin_holds(command, {'sha256': 'd' * 64}) is False + + +def test_a_vanished_descriptor_is_not_treated_as_a_swap(app): + """A deleted script already sends the command through with no descriptor so + the agent fails closed; that path must not be reported as tampering.""" + from plugins.endpoints.api.routes import _pin_holds + from plugins.endpoints.models import EndpointCommand + + with app.app_context(): + command = EndpointCommand(assetid=1, action='run_script', + pinnedsha256='c' * 64) + assert _pin_holds(command, None) is True + + +def test_enqueue_records_the_digest_it_approved(app, hosted_app): + """The resolved descriptor used to be discarded at enqueue (`_descriptor`).""" + from plugins.endpoints.api.routes import _resolve_update_app + + appid, appversionid = hosted_app + with app.app_context(): + descriptor, error = _resolve_update_app(str(appversionid)) + assert error is None + # This is the value the enqueue path now writes to pinnedsha256. + assert descriptor['sha256'] == 'a' * 64 diff --git a/tests/test_plugins/test_geenforce_payload_hardening.py b/tests/test_plugins/test_geenforce_payload_hardening.py index 1d7bc508..c25f0a3c 100644 --- a/tests/test_plugins/test_geenforce_payload_hardening.py +++ b/tests/test_plugins/test_geenforce_payload_hardening.py @@ -107,21 +107,47 @@ def test_payload_download_is_rate_limited(client, app, auth_headers): def test_rate_limit_is_per_ip(client, app, auth_headers): - """A second caller (different X-Forwarded-For) has its own budget: one IP - being throttled must not throttle everyone.""" + """A second caller has its own budget: one IP being throttled must not + throttle everyone. + + Callers are distinguished by REMOTE_ADDR, the peer that actually connected, + which is what ProxyFix fills from X-Forwarded-For when a proxy is really in + front of this instance. + """ sha = _store(app, b'per-ip blob') key = _fetch_key(client, auth_headers) - hot = {**key, 'X-Forwarded-For': '203.0.113.202'} - cool = {**key, 'X-Forwarded-For': '203.0.113.203'} + hot = {'REMOTE_ADDR': '203.0.113.202'} + cool = {'REMOTE_ADDR': '203.0.113.203'} with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1, GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300): assert client.get(f'/api/geenforce/payload/{sha}', - headers=hot).status_code == 200 + headers=key, environ_base=hot).status_code == 200 assert client.get(f'/api/geenforce/payload/{sha}', - headers=hot).status_code == 429 + headers=key, environ_base=hot).status_code == 429 # Fresh IP still gets its first request through. assert client.get(f'/api/geenforce/payload/{sha}', - headers=cool).status_code == 200 + headers=key, environ_base=cool).status_code == 200 + + +def test_rate_limit_cannot_be_reset_by_forging_a_forwarded_header( + client, app, auth_headers): + """The budget follows the connection, not a header the caller writes. + + The limiter used to key on the first X-Forwarded-For hop, so rotating that + header per request bought an unbounded budget and the limit stopped being + a limit. + """ + sha = _store(app, b'forged header blob') + key = _fetch_key(client, auth_headers) + caller = {'REMOTE_ADDR': '203.0.113.210'} + with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1, + GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300): + assert client.get(f'/api/geenforce/payload/{sha}', headers=key, + environ_base=caller).status_code == 200 + for hop in ('198.51.100.1', '198.51.100.2', '198.51.100.3'): + spoofed = {**key, 'X-Forwarded-For': hop} + assert client.get(f'/api/geenforce/payload/{sha}', headers=spoofed, + environ_base=caller).status_code == 429 def test_rate_limit_can_be_disabled(client, app, auth_headers): diff --git a/tests/test_plugins/test_supplies_card_ordering.py b/tests/test_plugins/test_supplies_card_ordering.py new file mode 100644 index 00000000..1eb40fef --- /dev/null +++ b/tests/test_plugins/test_supplies_card_ordering.py @@ -0,0 +1,101 @@ +"""The supplies card must put the emptiest cartridge first. + +It sorted on iscritical, then on PRINTER NAME. Critical is everything at or +below 5%, so a cartridge at 5% and one at 0% tied on the first key and the +alphabet decided the order. + +That is bad ordering on its own, and worse than it looks: the dashboard queue +takes only the first few rows per source, so a printer that is genuinely empty +was truncated out of the card while a fuller one held its place. The card +existed to answer "what do I walk out and change today" and was answering it +with the alphabet. +""" +from unittest.mock import patch + +import pytest + + +def _call(client, auth_headers): + response = client.get('/api/printers/dashboard/supplies', + headers=auth_headers) + assert response.status_code == 200 + return response.get_json()['data'] + + +@pytest.fixture(autouse=True) +def _clear_cache(app): + from shopdb.extensions import cache + cache.delete('printers_low_supplies') + yield + cache.delete('printers_low_supplies') + + +def _printer(printerid, name, remaining, status='critical'): + return { + 'printerid': printerid, + 'printername': name, + 'assetnumber': 'A{}'.format(printerid), + 'location': 'Shop floor', + 'mapx': None, 'mapy': None, 'levelid': None, + 'supplies': [{ + 'name': 'Black Toner', 'level': remaining, 'remaining': remaining, + 'status': status, 'iswaste': False, 'isdrum': False, + 'partnumbers': [], 'lastseen': None, 'color': 'black', + 'supplytype': 'toner', + }], + } + + +def _patched(rows): + return patch('plugins.printers.api.asset_routes._get_low_supplies_data', + return_value={'printers': rows, + 'summary': {'total_checked': len(rows), + 'low': 0, 'critical': len(rows)}}) + + +def test_the_empty_cartridge_comes_before_the_nearly_empty_one(client, + auth_headers): + """'Alpha' at 5% used to precede 'Zulu' at 0% purely on the name.""" + with _patched([_printer(1, 'Alpha', 5), _printer(2, 'Zulu', 0)]): + data = _call(client, auth_headers) + + order = [row['printername'] for row in data['rows']] + assert order == ['Zulu', 'Alpha'] + + +def test_rows_are_ordered_by_how_empty_they_are(client, auth_headers): + with _patched([_printer(1, 'Alpha', 4), + _printer(2, 'Bravo', 0), + _printer(3, 'Charlie', 2), + _printer(4, 'Delta', 5)]): + data = _call(client, auth_headers) + + remaining = [row['worstremaining'] for row in data['rows']] + assert remaining == sorted(remaining) + assert remaining == [0, 2, 4, 5] + + +def test_a_zero_percent_printer_survives_the_queue_truncation(client, + auth_headers): + """The dashboard queue keeps only the first few rows per source. + + With five fuller printers whose names sort earlier, the empty one used to + fall off the end of the card entirely. + """ + rows = [_printer(index, name, 5) for index, name in enumerate( + ['Alpha', 'Bravo', 'Charlie', 'Delta', 'Echo'], start=1)] + rows.append(_printer(6, 'Zulu', 0)) + + with _patched(rows): + data = _call(client, auth_headers) + + # QUEUEPERSOURCE is 5 on the frontend; first place is what matters here. + assert data['rows'][0]['printername'] == 'Zulu' + assert [r['printername'] for r in data['rows'][:5]].count('Zulu') == 1 + + +def test_the_name_still_breaks_a_tie(client, auth_headers): + with _patched([_printer(1, 'Bravo', 3), _printer(2, 'Alpha', 3)]): + data = _call(client, auth_headers) + + assert [row['printername'] for row in data['rows']] == ['Alpha', 'Bravo']