From 1a8f426e93a774f568431fd2b1744d9e7942d050 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Thu, 3 Sep 2026 16:03:47 -0400 Subject: [PATCH] Make revoking final, keep password rules in one place, drop a grant nobody read Three credential and authz findings, all the same shape: a second path to a decision, with weaker rules than the path built for it. PUT /api/apitokens/ {"isactive": true} un-revoked a token, and the ORIGINAL secret worked again - so "revoked" meant "paused until somebody flips it back", for a credential whose secret is assumed to be out by the act of revoking it. Revocation is one way now. Deactivating through the same route still works; bringing one back does not, and the refusal says to mint a new token. PUT /api/users/ accepted a `password` field with no current-password proof and no length rule, while /api/auth/change-password requires both. When two routes reach the same state the weaker one is the real rule, so the stronger one was decoration. Changing your own password through the users route is refused and points at the right endpoint. An ADMIN may still set one - that is how a temporary password is issued - with the same 8-character floor, and mustchangepassword is now set, because a password somebody else chose is temporary by definition. tools.secrets was declared guest-grantable, so the role grid offered it and stored it, and the secure-send route never consulted the guest role at all: it decides on securesend_allow_guests, which fails closed and carries the rate limit. Ticking the box changed nothing in either direction, so the screen and the behaviour disagreed. The declaration is gone; tools.view stays, because it really does gate the nav entry. Removing it needed care: update_role refuses the WHOLE request when a submitted permission is not grantable, and the grid resubmits what the role already holds, so a site whose guest role held tools.secrets could no longer save that role at all - the deadlock fixed in 723a4ff, re-created from the other end. Migration 7d41 revokes the grant from the guest role, and nothing is lost because it granted nothing. Eight tests. The first two verified by planting: remove either guard and three fail. Scope: product --- .../versions/7d41_drop_guest_tools_secrets.py | 56 +++++++++++++ plugins/tools/plugin.py | 20 +++-- shopdb/core/api/apitokens.py | 13 ++- shopdb/core/api/users.py | 21 ++++- tests/test_core/test_credential_paths.py | 80 +++++++++++++++++++ tests/test_core/test_guest_role.py | 32 ++++++++ 6 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 migrations/versions/7d41_drop_guest_tools_secrets.py create mode 100644 tests/test_core/test_credential_paths.py diff --git a/migrations/versions/7d41_drop_guest_tools_secrets.py b/migrations/versions/7d41_drop_guest_tools_secrets.py new file mode 100644 index 00000000..50ac629d --- /dev/null +++ b/migrations/versions/7d41_drop_guest_tools_secrets.py @@ -0,0 +1,56 @@ +"""Revoke a guest grant that never did anything, before it locks the role. + +tools.secrets was declared guest-grantable, so the role grid offered it and +stored it - and the secure-send route never consulted the guest ROLE at all. It +decides on the securesend_allow_guests setting, which fails closed and carries +the rate limit. A site that ticked the box got no change in behaviour, in either +direction. + +The plugin no longer declares it grantable. That alone would repeat the deadlock +fixed in 723a4ff: update_role refuses the WHOLE request when a submitted +permission is not grantable, and the role grid resubmits what the role already +holds - so a site whose guest role holds tools.secrets could no longer save the +guest role at all, including to change something else entirely. + +So the grant is removed here, from the one role that could hold it. Nothing is +lost: it granted nothing. Guest secret-link creation stays exactly where it was, +under securesend_allow_guests. + +Only the GUEST role is touched. A signed-in role holding tools.secrets is +unaffected - that permission works normally for an account. + +Revision ID: 7d41_drop_guest_tools_secrets +Revises: 7d40_machine_maintenance_dates +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d41_drop_guest_tools_secrets' +down_revision = '7d40_machine_maintenance_dates' +branch_labels = None +depends_on = None + + +def upgrade(): + bind = op.get_bind() + inspector = sa.inspect(bind) + tables = set(inspector.get_table_names()) + if not {'roles', 'permissions', 'rolepermissions'} <= tables: + return + + bind.execute(sa.text( + 'DELETE FROM rolepermissions WHERE roleid IN ' + '(SELECT roleid FROM roles WHERE rolename = :guest) ' + 'AND permissionid IN ' + '(SELECT permissionid FROM permissions WHERE name = :permission)'), + {'guest': 'guest', 'permission': 'tools.secrets'}) + + +def downgrade(): + """Deliberately a no-op. + + Re-granting a permission that nothing reads would restore the confusion + rather than any capability. + """ diff --git a/plugins/tools/plugin.py b/plugins/tools/plugin.py index 0dd105af..3cb97642 100644 --- a/plugins/tools/plugin.py +++ b/plugins/tools/plugin.py @@ -86,13 +86,21 @@ class ToolsPlugin(BasePlugin): from where, and never who. That is the cost, and it is why this is the only write in the product offered to guests at all. - tools.view rides along because tools.secrets without it is a page a - guest holds permission to use and no way to reach: it gates the nav - entry, and the browser tools behind it (codes, conversions) run client - side and touch no data at all. Declared explicitly rather than inferred - from the .view suffix, which is what the grantable rule now asks for. + tools.secrets is NOT here, and that is the fix for a grant that did + nothing. Whether a guest may create a secret link is decided by the + securesend_allow_guests setting - which fails closed, carries the rate + limit, and is read straight from the row rather than the cache. The + route never consulted the guest ROLE, so ticking tools.secrets for + guests in the role grid was accepted, stored, and ignored: the site + looked open and was not, or looked closed and was not. Two mechanisms + for one decision, and the setting is the one with the safeguards. + + tools.view stays grantable: it gates the nav entry, and the browser + tools behind it (codes, conversions) run client side and touch no data + at all. Declared explicitly rather than inferred from the .view suffix, + which is what the grantable rule now asks for. """ - return ['tools.secrets', 'tools.view'] + return ['tools.view'] def get_settings_defaults(self) -> List[dict]: """The one switch this plugin owns. diff --git a/shopdb/core/api/apitokens.py b/shopdb/core/api/apitokens.py index 51a60148..da45d984 100644 --- a/shopdb/core/api/apitokens.py +++ b/shopdb/core/api/apitokens.py @@ -163,7 +163,18 @@ def update_apitoken(tokenid: int): return error_response(ErrorCodes.VALIDATION_ERROR, 'name cannot be empty') token.name = newname if 'isactive' in data: - token.isactive = bool(data['isactive']) + # Revocation is ONE WAY. A revoked token's secret is out there - that + # is why it was revoked - and re-activating the row made the same + # secret work again, so "revoked" meant "paused until somebody flips it + # back". Deactivating through this route is still allowed; bringing one + # back is not, and the answer says to mint a new one. + wanted = bool(data['isactive']) + if wanted and not token.isactive: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'A revoked token cannot be re-activated - its secret is ' + 'already out. Create a new token instead.') + token.isactive = wanted if 'scopes' in data: scopelist, scope_error = _validate_scopes(data.get('scopes'), token.user) if scope_error is not None: diff --git a/shopdb/core/api/users.py b/shopdb/core/api/users.py index 64cc06e0..b2688317 100644 --- a/shopdb/core/api/users.py +++ b/shopdb/core/api/users.py @@ -187,9 +187,28 @@ def update_user(userid: int): user.failedlogins = 0 changes['unlocked'] = {'old': True, 'new': False} - # Password change + # Password change. + # + # An ADMIN may set one here - that is how a temporary password is issued, + # and mustchangepassword forces the holder to replace it at next login. + # Somebody changing their OWN password may not: this route asks for no + # current password and enforces no length, so it was a way around + # /api/auth/change-password, which requires both. The two rules have to + # agree or the weaker one is the real one. if 'password' in data and data['password']: + if not current_user.hasrole('admin'): + return error_response( + ErrorCodes.FORBIDDEN, + 'Change your own password at /api/auth/change-password, which ' + 'requires your current one.', + http_code=403) + if len(data['password']) < 8: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Password must be at least 8 characters') user.passwordhash = generate_password_hash(data['password']) + # Issued by somebody else, so it is temporary by definition. + user.mustchangepassword = True changes['password'] = {'old': '***', 'new': '***'} if changes: diff --git a/tests/test_core/test_credential_paths.py b/tests/test_core/test_credential_paths.py new file mode 100644 index 00000000..61122614 --- /dev/null +++ b/tests/test_core/test_credential_paths.py @@ -0,0 +1,80 @@ +"""Two ways to change a credential must not disagree. + +Both of these were found by the 2026-09-03 run, and both are the same shape: a +second route that reached the same state with fewer rules than the route +designed for it. + + * PUT /api/users/ accepted a `password` field with no current-password + proof and no length rule, while /api/auth/change-password requires both. + The weaker route is the real rule, so the stronger one was decoration. + + * PUT /api/apitokens/ {"isactive": true} un-revoked a token, and the + ORIGINAL secret worked again. "Revoked" meant "paused until somebody flips + it back" - for a credential whose secret is, by the act of revoking it, + assumed to be out. +""" + + +def _token(client, headers, name='cred-test'): + created = client.post('/api/apitokens', headers=headers, + json={'name': name}) + assert created.status_code in (200, 201), created.get_json() + return created.get_json()['data'] + + +def test_a_user_cannot_change_their_own_password_through_the_users_route( + client, db, member_headers, member_user): + """The route that asks for no current password must refuse the change, not + perform it.""" + response = client.put('/api/users/%d' % member_user.userid, + headers=member_headers, json={'password': 'brandnew1'}) + assert response.status_code == 403, response.get_json() + assert 'change-password' in response.get_json()['data']['error']['message'] + + +def test_an_admin_may_still_issue_a_temporary_password(client, db, + auth_headers, + member_user): + """Issuing one is how a locked-out user gets back in - and it is temporary + by definition, so the holder must replace it at next login.""" + response = client.put('/api/users/%d' % member_user.userid, + headers=auth_headers, json={'password': 'issued12345'}) + assert response.status_code == 200, response.get_json() + db.session.refresh(member_user) + assert member_user.mustchangepassword is True + + +def test_an_admin_issued_password_still_has_to_be_long_enough(client, db, + auth_headers, + member_user): + response = client.put('/api/users/%d' % member_user.userid, + headers=auth_headers, json={'password': 'short'}) + assert response.status_code == 400, response.get_json() + + +def test_a_revoked_token_cannot_be_brought_back(client, db, auth_headers): + token = _token(client, auth_headers, name='cred-revoke') + tokenid = token['tokenid'] + secret = token['secret'] + + assert client.delete('/api/apitokens/%d' % tokenid, + headers=auth_headers).status_code == 200 + refused = client.put('/api/apitokens/%d' % tokenid, headers=auth_headers, + json={'isactive': True}) + assert refused.status_code == 400, refused.get_json() + assert 'new token' in refused.get_json()['data']['error']['message'] + + # And the original secret stays dead, which is the point. + denied = client.get('/api/assets', + headers={'Authorization': 'Bearer %s' % secret}) + assert denied.status_code == 401 + + +def test_a_live_token_can_still_be_deactivated_through_the_same_route( + client, db, auth_headers): + """One-way, not frozen: turning one OFF here still works.""" + token = _token(client, auth_headers, name='cred-deactivate') + response = client.put('/api/apitokens/%d' % token['tokenid'], + headers=auth_headers, json={'isactive': False}) + assert response.status_code == 200, response.get_json() + assert response.get_json()['data']['isactive'] is False diff --git a/tests/test_core/test_guest_role.py b/tests/test_core/test_guest_role.py index 5d21dad9..941254f6 100644 --- a/tests/test_core/test_guest_role.py +++ b/tests/test_core/test_guest_role.py @@ -290,3 +290,35 @@ def test_capabilities_does_not_report_what_the_guest_role_holds(client, db, grant(db, guestrole, 'assets.view') body = client.get('/api/dashboard/capabilities').get_json()['data'] assert set(body) == {'cansearch'} + + +# --- a grant nobody reads is worse than no grant ---------------------------- +# +# tools.secrets was declared guest-grantable, so the role grid offered it and +# stored it, and the secure-send route never consulted the guest ROLE - it +# decides on securesend_allow_guests, which fails closed and carries the rate +# limit. Ticking the box changed nothing in either direction, so the screen and +# the behaviour disagreed. One decision, one mechanism. + +def test_tools_secrets_is_not_offered_as_a_guest_grant(app): + from shopdb.core.models import guest_grantable_permissions + + with app.app_context(): + grantable = guest_grantable_permissions() + assert 'tools.secrets' not in grantable + assert 'tools.view' in grantable, \ + 'the nav entry gate is a real guest grant and must stay' + + +def test_the_guest_role_still_saves_when_it_holds_only_grantable_permissions( + client, db, auth_headers, guestrole): + """The deadlock guard: removing something from the grantable set must not + stop the guest role being saved (see 723a4ff).""" + from shopdb.core.models import Role + + guest = Role.query.filter_by(rolename=Role.GUEST).first() + assert guest is not None + + saved = client.put('/api/users/roles/%d' % guest.roleid, headers=auth_headers, + json={'permissions': ['tools.view']}) + assert saved.status_code == 200, saved.get_json()