Promote the 2026-08-30 review security fixes and supplies ordering to main

This commit is contained in:
2026-08-31 09:24:13 -04:00
parent 6680c3c5de
commit 0e3d5a6226
30 changed files with 991 additions and 98 deletions

View File

@@ -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},
)

View File

@@ -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'

View File

@@ -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

View File

@@ -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:

View File

@@ -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()

View File

@@ -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.

View File

@@ -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

View File

@@ -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):

View File

@@ -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']