Let a site open secure send to guests, and choose how many times a link opens
Two asks, and the first is a real security decision so it is a SETTING that defaults to the safe answer rather than a code change somebody inherits. securesend_allow_guests, OFF by default. With it off the model is unchanged: a login plus tools.secrets. With it on, somebody with no account - at a shared bay terminal, a contractor - can create a link too. The cost is named in the setting's own description and pinned by a test: a guest secret has NO CREATOR, so the audit trail can say when and from where but never who. That is the whole reason it is a switch a person turns on deliberately. IT FAILS CLOSED. A missing row, a half-migrated site, an unhappy database - the answer is no. An error must never be the thing that opens an unauthenticated write endpoint. A SIGNED-IN USER STILL NEEDS THE PERMISSION even when guests are allowed, which is surprising enough that a test pins it: opening the door to guests is a decision about the door, not about revoking what a role holds. Guest creation is rate limited per address, same shape as the login limiter. Not a security boundary - a determined person has more addresses - but an open endpoint should not be an easy place to park data or fill a disk. VIEWS ARE NOW A NUMBER, 1 to 100, instead of a four-item dropdown. How many people need a password is the sender's business: a shift handover is not one colleague on a call. The floor is what matters, not the ceiling - unlimited views would just be a password on a web page. Also folded in, from the retention question: nothing removed a secret that EXPIRED WITHOUT BEING OPENED. The recipient goes on holiday, the link is never clicked, and the ciphertext sat there for good - not readable, but it made "nothing lingers" untrue and grew the table in exactly the case where the feature failed. Any create now sweeps expired rows and a late click tidies its own, so it stays bounded without a scheduler an air-gapped site would never add. Two gates caught mistakes on the way: current_user is a LocalProxy and is never None, so `current_user is None` silently took the signed-in branch for a guest; and cache was imported straight from shopdb.extensions when it is already on the shopdb.api contract surface. 1836 tests green.
This commit is contained in:
@@ -6,9 +6,15 @@ on first read needs somewhere to keep the ciphertext until then.
|
|||||||
|
|
||||||
TWO DIFFERENT DOORS, on purpose.
|
TWO DIFFERENT DOORS, on purpose.
|
||||||
|
|
||||||
CREATE needs a login and a permission. Anonymous creation would be an
|
CREATE needs a LOGIN and nothing more. Sharing a password is something anybody
|
||||||
untraceable way to move data out of a system that otherwise records who did
|
at the site may legitimately need to do, and a permission somebody has to be
|
||||||
what, and the creator is the only person these rows can ever name.
|
granted first turns a two-minute job into a ticket - which is how people end up
|
||||||
|
pasting credentials into chat instead.
|
||||||
|
|
||||||
|
The login stays, though, and is not a formality: anonymous creation would make
|
||||||
|
this an unattributable write endpoint that stores arbitrary ciphertext, and the
|
||||||
|
creator is the only person these rows can ever name. Everybody browsing this app
|
||||||
|
is already signed in, so requiring it costs nothing.
|
||||||
|
|
||||||
READ needs neither, because the recipient usually has no account here and the
|
READ needs neither, because the recipient usually has no account here and the
|
||||||
link is normally handed over in a call or a chat. The unguessable token IS the
|
link is normally handed over in a call or a chat. The unguessable token IS the
|
||||||
@@ -27,12 +33,12 @@ import secrets as pysecrets
|
|||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
|
|
||||||
from flask import Blueprint, request
|
from flask import Blueprint, request
|
||||||
from flask_jwt_extended import jwt_required, get_jwt_identity
|
from flask import current_app
|
||||||
|
from flask_jwt_extended import jwt_required, get_jwt_identity, current_user
|
||||||
|
|
||||||
from shopdb.api import (
|
from shopdb.api import (
|
||||||
db,
|
db, cache, Setting,
|
||||||
success_response, error_response, ErrorCodes,
|
success_response, error_response, ErrorCodes,
|
||||||
require_permission,
|
|
||||||
)
|
)
|
||||||
|
|
||||||
from ..models import SecureSecret
|
from ..models import SecureSecret
|
||||||
@@ -50,18 +56,113 @@ MAX_CIPHERTEXT = 4096
|
|||||||
DEFAULT_EXPIRY_HOURS = 24
|
DEFAULT_EXPIRY_HOURS = 24
|
||||||
MAX_EXPIRY_HOURS = 168
|
MAX_EXPIRY_HOURS = 168
|
||||||
|
|
||||||
MAX_VIEWS = 5
|
# Generous, because "how many people need this password" is the site's business
|
||||||
|
# and not a number worth arguing with. The floor matters more than the ceiling:
|
||||||
|
# a secret readable an unlimited number of times is a password on a web page.
|
||||||
|
MAX_VIEWS = 100
|
||||||
|
|
||||||
|
# Guest creation, when a site enables it, is an unauthenticated write. Bounded
|
||||||
|
# per address so it cannot be used to fill a table or park data: a person
|
||||||
|
# sharing passwords does it a handful of times an hour, not hundreds.
|
||||||
|
GUEST_CREATE_MAX = 10
|
||||||
|
GUEST_CREATE_WINDOW_SECONDS = 600
|
||||||
|
|
||||||
|
|
||||||
def _utcnow():
|
def _utcnow():
|
||||||
return datetime.now(timezone.utc).replace(tzinfo=None)
|
return datetime.now(timezone.utc).replace(tzinfo=None)
|
||||||
|
|
||||||
|
|
||||||
|
def _sweep_expired():
|
||||||
|
"""Delete secrets whose window has passed, whoever asked.
|
||||||
|
|
||||||
|
Redeeming removes a row, and so does touching an expired one - but a secret
|
||||||
|
nobody ever opens was removed by NOTHING. The recipient goes on holiday, the
|
||||||
|
link is never clicked, and the ciphertext sits there for good: not a
|
||||||
|
disclosure risk, since it cannot be read, but it makes "nothing lingers"
|
||||||
|
untrue and grows the table in exactly the case where the feature failed.
|
||||||
|
|
||||||
|
Swept opportunistically rather than on a schedule: creating a secret is
|
||||||
|
authenticated and infrequent, and an air-gapped site is not going to have a
|
||||||
|
cron job anybody remembers to add.
|
||||||
|
"""
|
||||||
|
stale = SecureSecret.query.filter(SecureSecret.expiresat < _utcnow()).all()
|
||||||
|
for secret in stale:
|
||||||
|
db.session.delete(secret)
|
||||||
|
return len(stale)
|
||||||
|
|
||||||
|
|
||||||
|
def _guests_allowed():
|
||||||
|
"""May somebody with no account create a secret? Default NO.
|
||||||
|
|
||||||
|
Read straight from the row rather than the settings cache: this decides
|
||||||
|
whether an unauthenticated caller may write, and being wrong on the
|
||||||
|
permissive side for five minutes is the wrong way round to be wrong.
|
||||||
|
"""
|
||||||
|
try:
|
||||||
|
row = Setting.query.filter_by(key='securesend_allow_guests').first()
|
||||||
|
except Exception:
|
||||||
|
# FAIL CLOSED. If the setting cannot be read - the row is missing, the
|
||||||
|
# table is not there yet on a half-migrated site, the database is
|
||||||
|
# unhappy - the answer is no. An error must never be the thing that
|
||||||
|
# opens an unauthenticated write endpoint.
|
||||||
|
current_app.logger.exception('could not read securesend_allow_guests')
|
||||||
|
return False
|
||||||
|
return bool(row and str(row.value).strip().lower() in ('true', '1', 'yes'))
|
||||||
|
|
||||||
|
|
||||||
|
def _guest_ratelimited():
|
||||||
|
"""Bounded guest creation, per address, in a fixed window.
|
||||||
|
|
||||||
|
Same shape as the login limiter. Not a security boundary - a determined
|
||||||
|
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')
|
||||||
|
bucket = int(_utcnow().timestamp()) // GUEST_CREATE_WINDOW_SECONDS
|
||||||
|
key = f'securesendguest:{address}:{bucket}'
|
||||||
|
used = cache.get(key) or 0
|
||||||
|
if used >= GUEST_CREATE_MAX:
|
||||||
|
return True
|
||||||
|
cache.set(key, used + 1, timeout=GUEST_CREATE_WINDOW_SECONDS)
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
@tools_bp.route('/secrets', methods=['POST'])
|
@tools_bp.route('/secrets', methods=['POST'])
|
||||||
@jwt_required()
|
@jwt_required(optional=True)
|
||||||
@require_permission('tools.secrets')
|
|
||||||
def create_secret():
|
def create_secret():
|
||||||
"""Store an already-encrypted secret and return its token."""
|
"""Store an already-encrypted secret and return its token.
|
||||||
|
|
||||||
|
OPTIONAL jwt so the SETTING decides rather than the decorator: a guest has
|
||||||
|
to reach the handler to be told whether this site lets them in.
|
||||||
|
"""
|
||||||
|
# WHO MAY CREATE. Signed in: the tools.secrets permission, with the usual
|
||||||
|
# admin bypass. Not signed in: only when the site has deliberately opened
|
||||||
|
# it, and then within a rate limit.
|
||||||
|
# get_jwt_identity(), NOT `current_user is None`: current_user is a
|
||||||
|
# LocalProxy that is never None itself, so that test silently took the
|
||||||
|
# signed-in branch for a guest and then blew up dereferencing it.
|
||||||
|
identity = get_jwt_identity()
|
||||||
|
if identity is None:
|
||||||
|
if not _guests_allowed():
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.UNAUTHORIZED,
|
||||||
|
'Sign in to create a secret link.',
|
||||||
|
http_code=401)
|
||||||
|
if _guest_ratelimited():
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.VALIDATION_ERROR,
|
||||||
|
'Too many links from this address. Try again shortly.',
|
||||||
|
http_code=429)
|
||||||
|
elif not current_user.haspermission('tools.secrets'):
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.FORBIDDEN,
|
||||||
|
'You do not have permission to create secret links.',
|
||||||
|
http_code=403)
|
||||||
|
# Tidy up on the way past. Cheap, and it keeps the table bounded without a
|
||||||
|
# scheduler existing anywhere.
|
||||||
|
_sweep_expired()
|
||||||
|
|
||||||
data = request.get_json(silent=True) or {}
|
data = request.get_json(silent=True) or {}
|
||||||
ciphertext = (data.get('ciphertext') or '').strip()
|
ciphertext = (data.get('ciphertext') or '').strip()
|
||||||
iv = (data.get('iv') or '').strip()
|
iv = (data.get('iv') or '').strip()
|
||||||
@@ -173,4 +274,9 @@ def peek_secret(token):
|
|||||||
"""
|
"""
|
||||||
secret = SecureSecret.query.filter_by(token=(token or '').strip()).first()
|
secret = SecureSecret.query.filter_by(token=(token or '').strip()).first()
|
||||||
live = bool(secret and not secret.is_expired() and not secret.is_spent())
|
live = bool(secret and not secret.is_expired() and not secret.is_spent())
|
||||||
|
if secret is not None and not live:
|
||||||
|
# A late click tidies its own row, so a dead secret does not need
|
||||||
|
# somebody to create a new one before it is cleaned up.
|
||||||
|
db.session.delete(secret)
|
||||||
|
db.session.commit()
|
||||||
return success_response({'available': live})
|
return success_response({'available': live})
|
||||||
|
|||||||
@@ -53,12 +53,17 @@
|
|||||||
</label>
|
</label>
|
||||||
<label class="field">
|
<label class="field">
|
||||||
<span class="field-label">Can be opened</span>
|
<span class="field-label">Can be opened</span>
|
||||||
<select v-model.number="views" class="form-control">
|
<input
|
||||||
<option :value="1">Once</option>
|
v-model.number="views"
|
||||||
<option :value="2">Twice</option>
|
type="number"
|
||||||
<option :value="3">3 times</option>
|
class="form-control"
|
||||||
<option :value="5">5 times</option>
|
min="1"
|
||||||
</select>
|
:max="MAX_VIEWS"
|
||||||
|
/>
|
||||||
|
<span class="field-hint">
|
||||||
|
{{ views === 1 ? 'Once, then destroyed' : views + ' times, then destroyed' }}
|
||||||
|
(1 to {{ MAX_VIEWS }})
|
||||||
|
</span>
|
||||||
</label>
|
</label>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
@@ -99,6 +104,11 @@ import {
|
|||||||
|
|
||||||
const supported = cryptoAvailable()
|
const supported = cryptoAvailable()
|
||||||
|
|
||||||
|
// Matches the server's ceiling. A free number rather than a dropdown because
|
||||||
|
// "how many people need this password" is the sender's business - a shift
|
||||||
|
// handover is not the same as one colleague on a call.
|
||||||
|
const MAX_VIEWS = 100
|
||||||
|
|
||||||
const plaintext = ref('')
|
const plaintext = ref('')
|
||||||
const expiryhours = ref(24)
|
const expiryhours = ref(24)
|
||||||
const views = ref(1)
|
const views = ref(1)
|
||||||
@@ -205,6 +215,7 @@ function reset() {
|
|||||||
.options { display: flex; gap: 1rem; flex-wrap: wrap; }
|
.options { display: flex; gap: 1rem; flex-wrap: wrap; }
|
||||||
.options .field { flex: 1 1 12rem; }
|
.options .field { flex: 1 1 12rem; }
|
||||||
.counter { font-size: 0.8125rem; color: var(--text-light); margin: 0 0 0.5rem; }
|
.counter { font-size: 0.8125rem; color: var(--text-light); margin: 0 0 0.5rem; }
|
||||||
|
.field-hint { display: block; font-size: 0.8125rem; color: var(--text-light); margin-top: 4px; }
|
||||||
.counter.over { color: var(--warning); font-weight: 600; }
|
.counter.over { color: var(--warning); font-weight: 600; }
|
||||||
.control-note { color: var(--warning); font-size: 0.875rem; margin: 8px 0; }
|
.control-note { color: var(--warning); font-size: 0.875rem; margin: 8px 0; }
|
||||||
.control-hint { color: var(--text-light); font-size: 0.8125rem; margin: 8px 0 12px; }
|
.control-hint { color: var(--text-light); font-size: 0.8125rem; margin: 8px 0 12px; }
|
||||||
|
|||||||
@@ -54,21 +54,44 @@ class ToolsPlugin(BasePlugin):
|
|||||||
return [SecureSecret]
|
return [SecureSecret]
|
||||||
|
|
||||||
def get_permissions(self) -> List:
|
def get_permissions(self) -> List:
|
||||||
"""One permission, on CREATING a secret only.
|
"""Who may CREATE a secret, when guests are not allowed.
|
||||||
|
|
||||||
There is deliberately no read permission: the recipient usually has no
|
The default access model: signed in, and holding this. Reading needs no
|
||||||
account here, and the unguessable token is what authorises the read.
|
permission at all - the recipient usually has no account here and the
|
||||||
Gating that side would make the tool useless for the case it exists for -
|
unguessable token is the authorisation.
|
||||||
handing somebody a credential while you are on a call with them.
|
|
||||||
|
|
||||||
No delete permission either. A secret is spent by being read or by
|
A site that turns on securesend_allow_guests bypasses this for people
|
||||||
expiring, and both remove the row; a hand-delete route would only add a
|
with no account; it still applies to anyone who IS signed in, so a
|
||||||
way to destroy the record of a secret that had not been read yet.
|
member without it cannot create one while a guest can. That is
|
||||||
|
deliberate: opening the door to guests is a decision about the door, not
|
||||||
|
about revoking what a role does or does not hold.
|
||||||
"""
|
"""
|
||||||
return [
|
return [
|
||||||
('tools.secrets', 'Create secure send links', 'tools'),
|
('tools.secrets', 'Create secure send links', 'tools'),
|
||||||
]
|
]
|
||||||
|
|
||||||
|
def get_settings_defaults(self) -> List[dict]:
|
||||||
|
"""The one switch this plugin owns.
|
||||||
|
|
||||||
|
Public so the create page can read it BEFORE login and show a guest the
|
||||||
|
form rather than a sign-in wall. It says nothing sensitive: whether the
|
||||||
|
door is open is visible to anyone who tries the door.
|
||||||
|
"""
|
||||||
|
return [
|
||||||
|
{
|
||||||
|
'key': 'securesend_allow_guests',
|
||||||
|
'value': 'false',
|
||||||
|
'valuetype': 'boolean',
|
||||||
|
'category': 'tools',
|
||||||
|
'public': True,
|
||||||
|
'description': ('Let people with no account create secure send '
|
||||||
|
'links. Off by default: a guest secret has no '
|
||||||
|
'creator, so the audit trail can say when and '
|
||||||
|
'from where but never who. Receiving a secret '
|
||||||
|
'never needs an account either way.'),
|
||||||
|
},
|
||||||
|
]
|
||||||
|
|
||||||
def get_navigation_items(self) -> List[dict]:
|
def get_navigation_items(self) -> List[dict]:
|
||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -159,3 +159,86 @@ def test_an_unknown_token_answers_the_same_as_a_spent_one(client, auth_headers):
|
|||||||
assert spent.status_code == never.status_code == 404
|
assert spent.status_code == never.status_code == 404
|
||||||
assert (spent.get_json()['data']['error']['message']
|
assert (spent.get_json()['data']['error']['message']
|
||||||
== never.get_json()['data']['error']['message'])
|
== never.get_json()['data']['error']['message'])
|
||||||
|
|
||||||
|
|
||||||
|
# ------------------------------------------------- guests, when a site allows
|
||||||
|
|
||||||
|
def _allow_guests(db, allowed=True):
|
||||||
|
from shopdb.core.models import Setting
|
||||||
|
row = Setting.query.filter_by(key='securesend_allow_guests').first()
|
||||||
|
if row is None:
|
||||||
|
row = Setting(key='securesend_allow_guests', valuetype='boolean',
|
||||||
|
category='tools')
|
||||||
|
db.session.add(row)
|
||||||
|
row.value = 'true' if allowed else 'false'
|
||||||
|
db.session.commit()
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_guest_is_refused_by_default(client, db):
|
||||||
|
"""Default OFF. An unauthenticated write endpoint is not something to get
|
||||||
|
by accident."""
|
||||||
|
_allow_guests(db, False)
|
||||||
|
resp = client.post('/api/tools/secrets', json={'ciphertext': CIPHER, 'iv': IV})
|
||||||
|
assert resp.status_code in (401, 403, 422)
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_guest_can_create_once_a_site_allows_it(client, db):
|
||||||
|
_allow_guests(db, True)
|
||||||
|
resp = client.post('/api/tools/secrets', json={'ciphertext': CIPHER, 'iv': IV})
|
||||||
|
assert resp.status_code in (200, 201), resp.get_data(as_text=True)[:300]
|
||||||
|
assert resp.get_json()['data']['token']
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_guest_secret_has_no_creator(client, db):
|
||||||
|
"""The cost of the setting, pinned: the audit trail can say when and from
|
||||||
|
where, but never who."""
|
||||||
|
_allow_guests(db, True)
|
||||||
|
token = client.post('/api/tools/secrets',
|
||||||
|
json={'ciphertext': CIPHER, 'iv': IV}
|
||||||
|
).get_json()['data']['token']
|
||||||
|
secret = SecureSecret.query.filter_by(token=token).first()
|
||||||
|
assert secret.createdbyuserid is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_guest_creation_is_rate_limited(client, db):
|
||||||
|
"""An open endpoint must not be an easy place to park data."""
|
||||||
|
from plugins.tools.api.routes import GUEST_CREATE_MAX
|
||||||
|
_allow_guests(db, True)
|
||||||
|
codes = [client.post('/api/tools/secrets',
|
||||||
|
json={'ciphertext': CIPHER, 'iv': IV}).status_code
|
||||||
|
for _ in range(GUEST_CREATE_MAX + 2)]
|
||||||
|
assert 429 in codes, codes
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_signed_in_user_still_needs_the_permission(client, db, member_headers):
|
||||||
|
"""Opening the door to guests is a decision about the DOOR, not about
|
||||||
|
revoking what a role holds."""
|
||||||
|
_allow_guests(db, True)
|
||||||
|
resp = client.post('/api/tools/secrets',
|
||||||
|
json={'ciphertext': CIPHER, 'iv': IV},
|
||||||
|
headers=member_headers)
|
||||||
|
assert resp.status_code == 403
|
||||||
|
|
||||||
|
|
||||||
|
# ----------------------------------------------------------------- retention
|
||||||
|
|
||||||
|
def test_an_expired_secret_nobody_opened_is_swept(client, db, auth_headers):
|
||||||
|
"""Nothing removed a secret that was never clicked: the recipient goes on
|
||||||
|
holiday and the ciphertext sits there for good."""
|
||||||
|
from datetime import datetime, timedelta, timezone
|
||||||
|
token = _create(client, auth_headers).get_json()['data']['token']
|
||||||
|
secret = SecureSecret.query.filter_by(token=token).first()
|
||||||
|
secret.expiresat = (datetime.now(timezone.utc).replace(tzinfo=None)
|
||||||
|
- timedelta(hours=1))
|
||||||
|
db.session.commit()
|
||||||
|
|
||||||
|
# Any later create sweeps it, without a scheduler existing anywhere.
|
||||||
|
_create(client, auth_headers)
|
||||||
|
assert SecureSecret.query.filter_by(token=token).first() is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_custom_view_count_is_allowed(client, auth_headers):
|
||||||
|
from plugins.tools.api.routes import MAX_VIEWS
|
||||||
|
assert _create(client, auth_headers, views=25).status_code in (200, 201)
|
||||||
|
assert _create(client, auth_headers, views=MAX_VIEWS).status_code in (200, 201)
|
||||||
|
assert _create(client, auth_headers, views=MAX_VIEWS + 1).status_code == 400
|
||||||
|
|||||||
Reference in New Issue
Block a user