fix(security): verifier les fichiers televerses et retirer un intent Discord
SEC-021 -- rien ne validait le contrat signe upload_signed_contract se contentait d un nom de fichier non vide. ALLOWED_SIGNED_EXTENSIONS etait declaree juste a cote et jamais lue. Le fichier atterrissait sur le disque sous un nom que download_signed_contract sert ensuite : ce qu un joueur televerse est ce qu un gerant ouvre. Le nom seul ne suffisait pas non plus cote upload_contract, qui verifiait `.pdf` en fin de chaine -- payload.pdf ne dit rien des octets. pdf_upload_error() couvre les deux routes : extension dans la liste, puis signature %PDF- en tete de flux. Le flux est rembobine, l appelant enregistre toujours le fichier entier. OPS-014 -- intent Discord privilegie inutile Le bot demandait GUILD_MEMBERS et ne s en servait pas : rien n enumere ni ne recherche de membre de serveur, les personnes sont jointes par le discord_user_id enregistre sur leur compte. Retire. message_content reste : on_raw_reaction_add lit le texte de la reponse d un coach pour consigner un motif de refus. CI-003 et CI-005 sont deja appliques (permissions: contents: read, checkout@v4, exclusions de deploiement). L epinglage par SHA des actions n est pas fait : ce sont des actions GitHub de premiere partie, et l epingler sans Dependabot echange une exposition contre une autre. 11 tests, dont deux verifient que signer un contrat marche toujours et qu un autre joueur ne peut pas le faire. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
@@ -0,0 +1,146 @@
|
||||
"""What may be written to the contracts directory — SEC-021.
|
||||
|
||||
upload_signed_contract accepted any file with a non-empty name.
|
||||
ALLOWED_SIGNED_EXTENSIONS was declared next to it and never read. The file
|
||||
landed on disk under a name the application later hands back through
|
||||
download_signed_contract, so whatever a player uploaded is what a manager
|
||||
opens.
|
||||
|
||||
The name alone was never enough either: `payload.pdf` says nothing about
|
||||
the bytes. Both routes now check the signature as well.
|
||||
"""
|
||||
|
||||
import io
|
||||
import os
|
||||
|
||||
import pytest
|
||||
|
||||
from app.extensions import db
|
||||
from app.models import Contract
|
||||
|
||||
PDF_BYTES = b'%PDF-1.7\n1 0 obj\n<<>>\nendobj\ntrailer\n%%EOF\n'
|
||||
NOT_PDF_BYTES = b'MZ\x90\x00\x03\x00\x00\x00' # a Windows executable header
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def contract_for(app, tmp_path):
|
||||
"""A contract row whose file lives in a throwaway directory."""
|
||||
|
||||
def _make(player_id, uploader_id):
|
||||
stored = 'deadbeef.pdf'
|
||||
path = tmp_path / stored
|
||||
path.write_bytes(PDF_BYTES)
|
||||
with app.app_context():
|
||||
contract = Contract(
|
||||
player_id=player_id, uploaded_by_id=uploader_id,
|
||||
original_filename='contract.pdf', stored_filename=stored,
|
||||
file_path=str(path),
|
||||
)
|
||||
db.session.add(contract)
|
||||
db.session.commit()
|
||||
return contract.id, tmp_path
|
||||
|
||||
return _make
|
||||
|
||||
|
||||
class TestSignedUpload:
|
||||
def test_an_executable_named_pdf_is_refused(
|
||||
self, app, client, as_role, make_user, contract_for
|
||||
):
|
||||
player_id = as_role('player')
|
||||
admin_id = make_user('admin')
|
||||
contract_id, directory = contract_for(player_id, admin_id)
|
||||
|
||||
client.post(
|
||||
f'/users/contracts/{contract_id}/upload_signed',
|
||||
data={'signed_file': (io.BytesIO(NOT_PDF_BYTES), 'signed.pdf')},
|
||||
content_type='multipart/form-data', follow_redirects=True,
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
contract = db.session.get(Contract, contract_id)
|
||||
assert contract.status != 'signed'
|
||||
assert contract.signed_file_path is None
|
||||
assert not os.path.exists(directory / 'signed_deadbeef.pdf')
|
||||
|
||||
def test_a_foreign_extension_is_refused(
|
||||
self, app, client, as_role, make_user, contract_for
|
||||
):
|
||||
player_id = as_role('player')
|
||||
admin_id = make_user('admin')
|
||||
contract_id, _directory = contract_for(player_id, admin_id)
|
||||
|
||||
client.post(
|
||||
f'/users/contracts/{contract_id}/upload_signed',
|
||||
data={'signed_file': (io.BytesIO(b'<?php system($_GET[0]); ?>'), 'shell.php')},
|
||||
content_type='multipart/form-data', follow_redirects=True,
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(Contract, contract_id).status != 'signed'
|
||||
|
||||
def test_a_real_pdf_still_goes_through(
|
||||
self, app, client, as_role, make_user, contract_for
|
||||
):
|
||||
"""Guard against over-correcting: signing a contract is the point."""
|
||||
player_id = as_role('player')
|
||||
admin_id = make_user('admin')
|
||||
contract_id, directory = contract_for(player_id, admin_id)
|
||||
|
||||
client.post(
|
||||
f'/users/contracts/{contract_id}/upload_signed',
|
||||
data={'signed_file': (io.BytesIO(PDF_BYTES), 'signed.pdf')},
|
||||
content_type='multipart/form-data', follow_redirects=True,
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
contract = db.session.get(Contract, contract_id)
|
||||
assert contract.status == 'signed'
|
||||
assert contract.signed_at is not None
|
||||
assert os.path.exists(directory / 'signed_deadbeef.pdf')
|
||||
|
||||
def test_another_player_still_cannot_sign_it(
|
||||
self, app, client, as_role, make_user, contract_for
|
||||
):
|
||||
owner_id = make_user('player')
|
||||
admin_id = make_user('admin')
|
||||
contract_id, _directory = contract_for(owner_id, admin_id)
|
||||
as_role('player')
|
||||
|
||||
client.post(
|
||||
f'/users/contracts/{contract_id}/upload_signed',
|
||||
data={'signed_file': (io.BytesIO(PDF_BYTES), 'signed.pdf')},
|
||||
content_type='multipart/form-data', follow_redirects=True,
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(Contract, contract_id).status != 'signed'
|
||||
|
||||
|
||||
class TestTheHelper:
|
||||
def test_it_reports_a_missing_file(self, app):
|
||||
from app.routes.users import ALLOWED_CONTRACT_EXTENSIONS, pdf_upload_error
|
||||
|
||||
with app.test_request_context('/'):
|
||||
assert pdf_upload_error(None, ALLOWED_CONTRACT_EXTENSIONS)
|
||||
|
||||
def test_it_leaves_the_stream_readable(self, app):
|
||||
"""The signature check consumes bytes; the caller still has to save
|
||||
the whole file afterwards."""
|
||||
from werkzeug.datastructures import FileStorage
|
||||
|
||||
from app.routes.users import ALLOWED_CONTRACT_EXTENSIONS, pdf_upload_error
|
||||
|
||||
upload = FileStorage(stream=io.BytesIO(PDF_BYTES), filename='c.pdf')
|
||||
with app.test_request_context('/'):
|
||||
assert pdf_upload_error(upload, ALLOWED_CONTRACT_EXTENSIONS) is None
|
||||
assert upload.stream.read() == PDF_BYTES
|
||||
|
||||
def test_a_name_without_a_dot_is_refused(self, app):
|
||||
from werkzeug.datastructures import FileStorage
|
||||
|
||||
from app.routes.users import ALLOWED_CONTRACT_EXTENSIONS, pdf_upload_error
|
||||
|
||||
upload = FileStorage(stream=io.BytesIO(PDF_BYTES), filename='pdf')
|
||||
with app.test_request_context('/'):
|
||||
assert pdf_upload_error(upload, ALLOWED_CONTRACT_EXTENSIONS)
|
||||
Reference in New Issue
Block a user