fix(auth): retirer un CAPTCHA qui ne protegeait rien, filtrer autrement
SEC-AUTH-008. L addition a deux operandes entre 1 et 10 avait dix-neuf reponses possibles et se resolvait en lisant la question comme une chaine. Elle n arretait aucune inscription automatisee. Elle ajoutait en revanche une etape a chaque personne, lecteur d ecran compris, contre une apparence de protection — ce qui est pire que rien, puisque ca se compte comme une protection. L autre branche proposee par l audit etait un vrai service de CAPTCHA : un tiers, une cle d API, une requete a chaque affichage, et un script etranger remis dans script-src, defaisant le travail qui a ferme SEC-WEB-001. Disproportionne pour le site d un club. A la place, deux verifications invisibles pour un visiteur : un champ piege, cache par la feuille de style et hors du parcours clavier, qu un robot remplisseur complete et qu une personne ne voit jamais ; et un delai minimal entre la remise du formulaire et son retour, l horodatage etant dans la session signee et non dans un champ. Le plafond est dit dans le code plutot que sous-entendu : ceci arrete le pourriel de masse, pas quelqu un qui lit la page. Ce qui filtrerait vraiment les inscriptions serait l activation des comptes par le staff — is_active_account vaut True par defaut. C est une decision de produit. L angle « session forgeable » du constat tombe : avec SECRET_KEY compromise (SEC-001), on forge une session connectee sur n importe quel compte et on n a aucune raison de s inscrire. Au passage, ARCH-005 en partie : le bloc « regenerer, purger les mots de passe, re-rendre » etait recopie quatre fois. Un seul helper, et la purge des mots de passe ne peut plus etre oubliee dans la cinquieme copie. Les refus sont journalises avec leur motif — c est le seul endroit ou un abus du formulaire devient visible — mais restent indistinguables pour l expediteur : nommer la regle indique comment la contourner. 429 tests.
This commit is contained in:
GGThed
@@ -0,0 +1,217 @@
|
||||
"""What now stands between a robot and a new account (SEC-AUTH-008).
|
||||
|
||||
The arithmetic CAPTCHA it replaces had nineteen possible answers and could
|
||||
be solved by reading the question as a string. It stopped nothing, cost every
|
||||
human a step, and — being counted as a protection — was worse than nothing.
|
||||
|
||||
Two checks took its place: a honeypot input, and a floor on how fast the
|
||||
form can come back. Both are invisible to a person filling in the form. Both
|
||||
are honest about their ceiling: they stop commodity spam, not somebody who
|
||||
reads the page.
|
||||
|
||||
The tests that matter most here are the ones asserting a *legitimate*
|
||||
sign-up still works. A screening rule that quietly refuses real people is a
|
||||
worse outcome than the CAPTCHA was.
|
||||
"""
|
||||
|
||||
import time
|
||||
|
||||
from app.routes.auth import (
|
||||
MIN_REGISTRATION_SECONDS,
|
||||
REGISTRATION_HONEYPOT_FIELD,
|
||||
REGISTRATION_ISSUED_KEY,
|
||||
)
|
||||
|
||||
FORM = {
|
||||
'username': 'brandnew',
|
||||
'email': '[email protected]',
|
||||
'password': 'Password123',
|
||||
'confirm_password': 'Password123',
|
||||
'full_name': 'Brand New',
|
||||
}
|
||||
|
||||
|
||||
def _issued_long_ago(client, seconds_ago=None):
|
||||
"""Pretend the form was handed out a while back.
|
||||
|
||||
Backdated rather than slept through: waiting three real seconds per test
|
||||
would add a minute to the suite for nothing.
|
||||
"""
|
||||
if seconds_ago is None:
|
||||
seconds_ago = MIN_REGISTRATION_SECONDS + 1
|
||||
with client.session_transaction() as session:
|
||||
session[REGISTRATION_ISSUED_KEY] = time.time() - seconds_ago
|
||||
|
||||
|
||||
def _account_exists(app, username='brandnew'):
|
||||
from app.models import User
|
||||
|
||||
with app.app_context():
|
||||
return User.query.filter_by(username=username).first() is not None
|
||||
|
||||
|
||||
class TestLegitimateSignUp:
|
||||
def test_a_person_filling_the_form_gets_an_account(self, app, client):
|
||||
client.get('/auth/register')
|
||||
_issued_long_ago(client)
|
||||
|
||||
client.post('/auth/register', data=dict(FORM), follow_redirects=True)
|
||||
|
||||
assert _account_exists(app)
|
||||
|
||||
def test_an_empty_honeypot_is_not_a_refusal(self, app, client):
|
||||
"""A browser submits the hidden input as an empty string, not absent."""
|
||||
_issued_long_ago(client)
|
||||
|
||||
client.post(
|
||||
'/auth/register',
|
||||
data=dict(FORM, **{REGISTRATION_HONEYPOT_FIELD: ''}),
|
||||
follow_redirects=True,
|
||||
)
|
||||
|
||||
assert _account_exists(app)
|
||||
|
||||
def test_correcting_a_typo_does_not_restart_the_clock(self, app, client):
|
||||
"""The dwell timer must survive a failed attempt.
|
||||
|
||||
Reissuing it on every re-render would refuse the second submission of
|
||||
anyone who fixes a mistake quickly — a rule that fires on real people
|
||||
and not on robots, which is the wrong way round.
|
||||
"""
|
||||
client.get('/auth/register')
|
||||
_issued_long_ago(client)
|
||||
|
||||
# First attempt fails validation: passwords do not match.
|
||||
client.post(
|
||||
'/auth/register',
|
||||
data=dict(FORM, confirm_password='Different123'),
|
||||
follow_redirects=True,
|
||||
)
|
||||
# Corrected and sent straight back, well inside the dwell floor.
|
||||
client.post('/auth/register', data=dict(FORM), follow_redirects=True)
|
||||
|
||||
assert _account_exists(app)
|
||||
|
||||
|
||||
class TestScreening:
|
||||
def test_a_filled_honeypot_is_refused(self, app, client):
|
||||
_issued_long_ago(client)
|
||||
|
||||
client.post(
|
||||
'/auth/register',
|
||||
data=dict(FORM, **{REGISTRATION_HONEYPOT_FIELD: 'http://spam.example'}),
|
||||
follow_redirects=True,
|
||||
)
|
||||
|
||||
assert not _account_exists(app)
|
||||
|
||||
def test_a_form_returned_instantly_is_refused(self, app, client):
|
||||
client.get('/auth/register')
|
||||
|
||||
client.post('/auth/register', data=dict(FORM), follow_redirects=True)
|
||||
|
||||
assert not _account_exists(app)
|
||||
|
||||
def test_a_post_that_never_fetched_the_form_is_refused(self, app, client):
|
||||
"""The strongest signal available: no form was ever issued."""
|
||||
client.post('/auth/register', data=dict(FORM), follow_redirects=True)
|
||||
|
||||
assert not _account_exists(app)
|
||||
|
||||
def test_the_two_rules_are_indistinguishable_to_the_sender(self, app, client):
|
||||
"""Naming the rule tells whoever tripped it how to avoid it.
|
||||
|
||||
Both refusals must read identically from outside. The log, which the
|
||||
sender cannot see, is where they are told apart.
|
||||
|
||||
One client per scenario, on purpose: the dwell stamp lives in the
|
||||
session and is deliberately kept across a refusal, so reusing a single
|
||||
client would carry the first scenario's backdated stamp into the
|
||||
second and the "too fast" case would never fire.
|
||||
"""
|
||||
_issued_long_ago(client)
|
||||
tripped_honeypot = client.post(
|
||||
'/auth/register',
|
||||
data=dict(FORM, **{REGISTRATION_HONEYPOT_FIELD: 'spam'}),
|
||||
follow_redirects=True,
|
||||
).get_data(as_text=True)
|
||||
|
||||
fresh = app.test_client()
|
||||
fresh.get('/auth/register')
|
||||
too_fast = fresh.post('/auth/register', data=dict(FORM), follow_redirects=True).get_data(
|
||||
as_text=True
|
||||
)
|
||||
|
||||
def flashes(page):
|
||||
return [line for line in page.splitlines() if 'alert-danger' in line]
|
||||
|
||||
assert flashes(tripped_honeypot) == flashes(too_fast)
|
||||
assert flashes(tripped_honeypot), 'no message at all is not the same as a uniform one'
|
||||
|
||||
|
||||
class TestTheFormItself:
|
||||
def test_no_arithmetic_question_is_asked(self, client):
|
||||
page = client.get('/auth/register').get_data(as_text=True)
|
||||
|
||||
assert 'captcha' not in page.lower()
|
||||
|
||||
def test_the_honeypot_is_hidden_from_assistive_technology(self, client):
|
||||
"""An input a screen reader announces is a trap for a person.
|
||||
|
||||
aria-hidden, tabindex=-1 and display:none all have to hold. The CSS
|
||||
rule lives in the stylesheet rather than in a style attribute so that
|
||||
it survives a future tightening of style-src.
|
||||
"""
|
||||
page = client.get('/auth/register').get_data(as_text=True)
|
||||
|
||||
assert 'class="honeypot" aria-hidden="true"' in page
|
||||
assert f'name="{REGISTRATION_HONEYPOT_FIELD}" tabindex="-1"' in page
|
||||
|
||||
def test_the_stylesheet_actually_hides_it(self, app):
|
||||
import os
|
||||
|
||||
with open(os.path.join(app.static_folder, 'css', 'style.css'), encoding='utf-8') as handle:
|
||||
css = handle.read()
|
||||
|
||||
block = css.split('.honeypot')[-1]
|
||||
assert 'display: none' in block.split('}')[0]
|
||||
|
||||
|
||||
class TestRefusalIsVisible:
|
||||
def test_a_refusal_is_written_to_the_audit_log(self, app, client, monkeypatch):
|
||||
"""Sign-up abuse leaves a trace or it is not happening, as far as
|
||||
anyone can tell."""
|
||||
recorded = []
|
||||
from app.routes import auth as auth_module
|
||||
|
||||
monkeypatch.setattr(
|
||||
auth_module,
|
||||
'log_auth_event',
|
||||
lambda event, **fields: recorded.append((event, fields)),
|
||||
)
|
||||
_issued_long_ago(client)
|
||||
|
||||
client.post(
|
||||
'/auth/register',
|
||||
data=dict(FORM, **{REGISTRATION_HONEYPOT_FIELD: 'spam'}),
|
||||
follow_redirects=True,
|
||||
)
|
||||
|
||||
assert recorded, 'the double was never called — the test would pass on nothing'
|
||||
assert recorded[0][0] == 'account.registration_refused'
|
||||
assert recorded[0][1]['reason'] == 'honeypot'
|
||||
|
||||
def test_the_reason_distinguishes_the_two_rules(self, app, client, monkeypatch):
|
||||
recorded = []
|
||||
from app.routes import auth as auth_module
|
||||
|
||||
monkeypatch.setattr(
|
||||
auth_module,
|
||||
'log_auth_event',
|
||||
lambda event, **fields: recorded.append((event, fields)),
|
||||
)
|
||||
client.get('/auth/register')
|
||||
|
||||
client.post('/auth/register', data=dict(FORM), follow_redirects=True)
|
||||
|
||||
assert recorded and recorded[0][1]['reason'] == 'too-fast'
|
||||
@@ -19,6 +19,8 @@ These tests state the guarantee, so that removing the rollback or
|
||||
reintroducing a mid-operation commit fails loudly.
|
||||
"""
|
||||
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
from app.models import User, UserGamertag
|
||||
@@ -78,9 +80,14 @@ class TestRegistrationIsOneOperation:
|
||||
}
|
||||
|
||||
def _submit(self, client, app, **overrides):
|
||||
# Registration is screened for robots (SEC-AUTH-008): the form has to
|
||||
# have been issued, and long enough ago. Backdated here rather than
|
||||
# slept through, so the suite does not pay three seconds per call.
|
||||
from app.routes.auth import MIN_REGISTRATION_SECONDS, REGISTRATION_ISSUED_KEY
|
||||
|
||||
with client.session_transaction() as session:
|
||||
session['captcha_answer'] = 4
|
||||
payload = dict(self.FORM, captcha_answer='4')
|
||||
session[REGISTRATION_ISSUED_KEY] = time.time() - MIN_REGISTRATION_SECONDS - 1
|
||||
payload = dict(self.FORM)
|
||||
payload.update(overrides)
|
||||
return client.post('/auth/register', data=payload, follow_redirects=True)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user