fix(files): ending every session ends every API token, and the share links hold their folder
- A password reset or change, "log out everywhere" and a revoked untrusted password revoke all of the account's API tokens too (audited): one minted from a stolen session no longer outlives the answer. - A bearer token or link grant with a non-ASCII character is refused (401/403), not a 500. - Making a token's grants or a folder link holds the folder, so a racing delete cannot fail the foreign key; a link's view count is one atomic UPDATE. - A folder link's card counts exactly the files its page lists (one `links.folder_files`). - A file with no extension is served without a trailing dot. - 0106's downgrade names a Hub folder so it never collides with one of the user's folders. - The speed baseline reads share tokens through the app (the database keeps them hashed/encrypted). - Tests: the token scope on the real session stack, the folder poster, the Hub refused for a link, 0106's downgrade data step, 0107 without a key; docs/comments follow (three folder kinds, folder links, key change, the cleanup job's token warnings).
This commit is contained in:
@@ -32,6 +32,7 @@ from app.models import (
|
||||
)
|
||||
from app.ratelimit import RateLimiter
|
||||
from app.routes import tokens as tokens_routes
|
||||
from app.security import hash_token
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
@@ -114,6 +115,17 @@ def test_a_token_has_its_shape_and_a_checksum_that_refuses_a_typo():
|
||||
assert not apitokens.well_formed(flipped)
|
||||
assert not apitokens.well_formed(raw[:-1])
|
||||
assert not apitokens.well_formed("xxx_" + raw[4:])
|
||||
# A header byte past ASCII arrives as one latin-1 character: refused, never a 500.
|
||||
assert not apitokens.well_formed(raw[:-1] + "é")
|
||||
|
||||
|
||||
def test_a_non_ascii_token_is_refused_as_any_bad_one(db, client):
|
||||
# The test client re-sends a high byte as two UTF-8 bytes, which arrive as two latin-1
|
||||
# characters: one "A" fewer keeps the header a token's length, so it is the checksum that meets
|
||||
# them, not the length check.
|
||||
raw = ("Bearer sfl_" + "A" * 47 + "é").encode("latin-1")
|
||||
r = TestClient(app, headers={"authorization": raw}).get("/api/files/folders")
|
||||
assert r.status_code == 401 and r.json()["detail"]["reason"] == "not_authenticated"
|
||||
|
||||
|
||||
def test_the_hub_preset_makes_one_hub_folder_with_every_right_and_shows_the_token_once(
|
||||
@@ -248,12 +260,23 @@ def test_a_token_is_judged_by_its_grants_even_beside_a_signed_in_cookie(db, make
|
||||
assert both.json() == [] # the token may not list: its grants decide
|
||||
|
||||
|
||||
def test_a_token_works_on_the_files_routes_only(db, make_user, as_user):
|
||||
alice = make_user()
|
||||
raw = _make(as_user(alice), preset="hub")["token"]
|
||||
def _signed_in(client, email="alice@example.com") -> TestClient:
|
||||
r = client.post(
|
||||
"/auth/password-login", json={"email": email, "password": "correct horse battery"}
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
return client
|
||||
|
||||
|
||||
def test_a_token_works_on_the_files_routes_only(db, make_user, client):
|
||||
"""Through the real session stack: under `as_user` every route without its header answers 401
|
||||
whatever `current_user` does with a token, so these refusals would prove nothing there."""
|
||||
make_user(email="alice@example.com")
|
||||
raw = _make(_signed_in(client), preset="hub")["token"]
|
||||
m = _bearer(raw)
|
||||
assert m.get("/api/files/folders").status_code == 200
|
||||
# Making a wider token, or reading the account, takes the user's session.
|
||||
assert m.get("/api/me").json() == {"authenticated": False} # the token is no one here
|
||||
assert m.get("/api/tokens").status_code == 401
|
||||
assert m.post("/api/tokens", json={"name": "wider", "preset": "hub"}).status_code == 401
|
||||
assert m.get("/api/files/usage").status_code == 401
|
||||
@@ -296,6 +319,55 @@ def test_only_the_files_routes_take_a_token():
|
||||
}
|
||||
|
||||
|
||||
def _reset_password(db, client, alice):
|
||||
from app.routes.auth import _issue_token
|
||||
|
||||
link = _issue_token(db, alice, "reset", timedelta(hours=1))
|
||||
r = client.post(
|
||||
"/auth/password-reset/confirm", json={"token": link, "password": "a brand new password"}
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
|
||||
|
||||
def _change_password(db, client, alice):
|
||||
r = client.post(
|
||||
"/auth/set-password",
|
||||
json={"password": "a brand new password", "current_password": "correct horse battery"},
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
|
||||
|
||||
def _log_out_others(db, client, alice):
|
||||
assert client.post("/auth/logout-others").status_code == 200
|
||||
|
||||
|
||||
def _revoke_untrusted_password(db, client, alice):
|
||||
from app.routes.auth import _revoke_password
|
||||
|
||||
_revoke_password(db, alice)
|
||||
db.commit()
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"end_sessions",
|
||||
[_reset_password, _change_password, _log_out_others, _revoke_untrusted_password],
|
||||
)
|
||||
def test_whatever_ends_every_session_ends_every_token_too(db, make_user, client, end_sessions):
|
||||
"""A token minted from a stolen session must not outlive the owner's answer to the theft."""
|
||||
alice = make_user(email="alice@example.com")
|
||||
bob = make_user(email="bob@example.com")
|
||||
raw = _make(_signed_in(client), preset="hub")["token"]
|
||||
bobs = apitokens.mint()
|
||||
db.add(ApiToken(user_id=bob.id, name="b", prefix=bobs[:12], token_hash=hash_token(bobs)))
|
||||
db.commit()
|
||||
assert _bearer(raw).get("/api/files/folders").status_code == 200
|
||||
end_sessions(db, client, alice)
|
||||
assert _bearer(raw).get("/api/files/folders").status_code == 401
|
||||
assert db.execute(select(ApiToken.user_id)).scalars().all() == [bob.id] # only hers went
|
||||
[revoked] = _audits(db, "api_token_revoke")
|
||||
assert revoked.actor_id == alice.id and "every session" in (revoked.summary or "")
|
||||
|
||||
|
||||
# --- a token that does not hold ------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
@@ -493,7 +493,7 @@ def _revoke_as_google_adoption(db, user) -> None:
|
||||
Google callback adopting an account whose email was never verified."""
|
||||
from app.routes.auth import revoke_untrusted_preset_password
|
||||
|
||||
revoke_untrusted_preset_password(user)
|
||||
revoke_untrusted_preset_password(db, user)
|
||||
db.commit()
|
||||
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ a file link; a document is never served to open as a page.
|
||||
"""
|
||||
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
from alembic import command
|
||||
@@ -15,9 +16,9 @@ from sqlalchemy import select, text
|
||||
|
||||
from app.config import settings
|
||||
from app.db import engine
|
||||
from app.downloads import og
|
||||
from app.downloads import folders, og
|
||||
from app.downloads import probe as probemod
|
||||
from app.models import AuditLog, DownloadJob, DownloadLink, FileFolder
|
||||
from app.models import AuditLog, DownloadJob, DownloadLink, FileFolder, MediaAsset
|
||||
|
||||
_VIDEO = {
|
||||
"streams": [
|
||||
@@ -125,6 +126,12 @@ def test_only_a_folder_of_ones_own_can_be_shared(shelf, db, as_user, make_user):
|
||||
r = owner.post(f"/api/downloads/folders/{source}/links", json={})
|
||||
assert r.status_code == 409
|
||||
assert r.json()["detail"]["reason"] == "share_folder_kind"
|
||||
# Nor the Hub: whatever a machine drops there would be public the moment it lands.
|
||||
hub = folders.hub_folder(db, shelf["user"].id).id
|
||||
db.commit()
|
||||
r = owner.post(f"/api/downloads/folders/{hub}/links", json={})
|
||||
assert r.status_code == 409
|
||||
assert r.json()["detail"]["reason"] == "share_folder_kind"
|
||||
|
||||
stranger = as_user(make_user())
|
||||
assert (
|
||||
@@ -154,6 +161,43 @@ def test_a_password_folder_link_shows_nothing_until_unlocked(shelf, client):
|
||||
assert client.get(files[0]["file_url"]).status_code == 200
|
||||
|
||||
|
||||
def test_a_files_poster_opens_by_the_links_own_rules(shelf, db, client):
|
||||
"""The poster route on its own: a password link's poster needs the unlock grant, and a file
|
||||
outside the folder has none through it, grant or not."""
|
||||
for job_id in (shelf["inside"][0], shelf["outside"]):
|
||||
asset = db.get(MediaAsset, db.get(DownloadJob, job_id).asset_id)
|
||||
asset.poster_path = f"poster-{job_id}.jpg"
|
||||
(Path(settings.files_root) / asset.poster_path).write_bytes(b"a poster frame")
|
||||
db.commit()
|
||||
token = _link(shelf, password="open sesame")["token"]
|
||||
inside = f"/api/public/watch/{token}/files/{shelf['inside'][0]}/poster.jpg"
|
||||
assert client.get(inside).status_code == 403
|
||||
|
||||
unlocked = client.post(f"/api/public/watch/{token}/unlock", json={"password": "open sesame"})
|
||||
[entry] = [f for f in unlocked.json()["files"] if f["id"] == shelf["inside"][0]]
|
||||
assert entry["poster_url"].startswith(inside + "?g=")
|
||||
got = client.get(entry["poster_url"])
|
||||
assert got.status_code == 200 and got.content == b"a poster frame"
|
||||
grant = entry["poster_url"].split("?g=", 1)[1]
|
||||
outside = f"/api/public/watch/{token}/files/{shelf['outside']}/poster.jpg?g={grant}"
|
||||
assert client.get(outside).status_code == 404
|
||||
|
||||
|
||||
def test_every_open_counts_as_a_view(shelf, client):
|
||||
made = _link(shelf)
|
||||
for _ in range(3):
|
||||
assert client.get(f"/api/public/watch/{made['token']}").status_code == 200
|
||||
[listed] = shelf["owner"].get(f"/api/downloads/folders/{shelf['folder']}/links").json()
|
||||
assert listed["view_count"] == 3
|
||||
|
||||
|
||||
def test_the_folder_list_is_capped_like_the_owners_files_list():
|
||||
from app.downloads import links as linksmod
|
||||
from app.routes.files import MAX_LIST
|
||||
|
||||
assert linksmod.FOLDER_MAX == MAX_LIST
|
||||
|
||||
|
||||
def test_a_document_in_a_shared_folder_only_downloads(shelf, client, probe):
|
||||
probe["data"] = None
|
||||
doc = shelf["upload"]("notes.html", shelf["folder"], b"<script>1</script>")
|
||||
@@ -234,6 +278,11 @@ def test_the_unfurl_names_the_folder_and_counts_its_files_only(shelf, db):
|
||||
for name in names:
|
||||
assert not name or name not in tags
|
||||
|
||||
# A file whose bytes are gone is off the page, and so off the card's count.
|
||||
gone = db.get(MediaAsset, db.get(DownloadJob, shelf["inside"][0]).asset_id)
|
||||
(Path(settings.files_root) / gone.rel_path).unlink()
|
||||
assert "1 file " in og._watch_tags(token, db)
|
||||
|
||||
locked = _link(shelf, password="pw")["token"]
|
||||
hidden = og._watch_tags(locked, db)
|
||||
assert "Friends" not in hidden and "Password-protected folder" in hidden
|
||||
|
||||
@@ -185,3 +185,43 @@ def test_the_downgrade_writes_the_tokens_back(db, key, owner_job):
|
||||
assert db.execute(select(DownloadLink.token_hash)).scalars().all() == [
|
||||
security.hash_token(token)
|
||||
]
|
||||
|
||||
|
||||
def test_without_a_key_the_migration_keeps_the_address_opening_but_not_shown(db, owner_job, client):
|
||||
"""No key to seal with: the hash alone is kept, the link still opens, its address is gone."""
|
||||
assert not security.can_encrypt()
|
||||
owner, job_id = owner_job
|
||||
db.commit() # an open transaction's locks would hold the downgrade's ALTER TABLE forever
|
||||
cfg = Config("alembic.ini")
|
||||
command.downgrade(cfg, "0106_api_tokens")
|
||||
try:
|
||||
with engine.begin() as conn:
|
||||
conn.execute(
|
||||
text("INSERT INTO download_links (job_id, token) VALUES (:j, 'an-old-token-sent')"),
|
||||
{"j": job_id},
|
||||
)
|
||||
finally:
|
||||
command.upgrade(cfg, "head")
|
||||
|
||||
(row,) = db.execute(text("SELECT token_hash, token_enc FROM download_links")).mappings().all()
|
||||
assert row["token_hash"] == security.hash_token("an-old-token-sent")
|
||||
assert row["token_enc"] is None
|
||||
assert client.get("/api/public/watch/an-old-token-sent").status_code == 200
|
||||
assert [e["url"] for e in owner.get(f"/api/downloads/{job_id}/links").json()] == [None]
|
||||
|
||||
|
||||
def test_a_downgrade_with_no_readable_copy_writes_a_fresh_token(db, key, owner_job, monkeypatch):
|
||||
"""A key since changed reads no copy: the old column gets a random token, so the link is
|
||||
revoked rather than left with no token (the column is NOT NULL there)."""
|
||||
owner, job_id = owner_job
|
||||
token = owner.post(f"/api/downloads/{job_id}/links", json={}).json()["token"]
|
||||
db.commit() # an open transaction's locks would hold the downgrade's ALTER TABLE forever
|
||||
monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key()))
|
||||
cfg = Config("alembic.ini")
|
||||
command.downgrade(cfg, "0106_api_tokens")
|
||||
try:
|
||||
with engine.connect() as conn:
|
||||
back = conn.execute(text("SELECT token FROM download_links")).scalars().all()
|
||||
finally:
|
||||
command.upgrade(cfg, "head")
|
||||
assert len(back) == 1 and back[0] and back[0] != token
|
||||
|
||||
@@ -53,6 +53,9 @@ class TestGrant:
|
||||
def test_malformed_grants_are_rejected_not_crashed(self):
|
||||
for bad in [None, "", "no-dot", "notanumber.sig"]:
|
||||
assert check_grant("tok", bad, 0) is False
|
||||
# The query string decodes as UTF-8: a signature past ASCII is just a wrong one.
|
||||
future = int(datetime.now(timezone.utc).timestamp()) + _GRANT_TTL
|
||||
assert check_grant("tok", f"{future}.é", 0) is False
|
||||
|
||||
|
||||
def _asset(**over):
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
"""Migration 0106's downgrade data step, DB-lane: a Hub folder holding a file becomes an own folder
|
||||
that keeps the file — under a name its user's own folders do not already have (their names are
|
||||
unique per user, case-blind), and with its token gone with the tokens table."""
|
||||
|
||||
from alembic import command
|
||||
from alembic.config import Config
|
||||
from sqlalchemy import text
|
||||
|
||||
from app.db import engine
|
||||
|
||||
|
||||
def test_a_hub_folder_with_a_file_comes_back_as_an_own_folder_beside_a_namesake(db, make_user):
|
||||
uid = make_user().id
|
||||
with engine.begin() as conn:
|
||||
hub = conn.execute(
|
||||
text("INSERT INTO file_folders (user_id, kind) VALUES (:u, 'hub') RETURNING id"),
|
||||
{"u": uid},
|
||||
).scalar_one()
|
||||
conn.execute(
|
||||
text("INSERT INTO file_folders (user_id, kind, name) VALUES (:u, 'own', :n)"),
|
||||
{"u": uid, "n": f"hub {hub}"}, # the name the downgrade would pick, in another case
|
||||
)
|
||||
held = conn.execute(
|
||||
text(
|
||||
"INSERT INTO download_jobs (user_id, source_kind, source_ref, profile_snapshot,"
|
||||
" status, folder_id) VALUES (:u, 'upload', 'x', '{}', 'queued', :f) RETURNING id"
|
||||
),
|
||||
{"u": uid, "f": hub},
|
||||
).scalar_one()
|
||||
conn.execute(
|
||||
text(
|
||||
"INSERT INTO api_tokens (user_id, name, prefix, token_hash) VALUES"
|
||||
" (:u, 'hub', 'sfl_abcd1234', 'h') "
|
||||
),
|
||||
{"u": uid},
|
||||
)
|
||||
cfg = Config("alembic.ini")
|
||||
command.downgrade(cfg, "0105_job_source_key")
|
||||
try:
|
||||
with engine.connect() as conn:
|
||||
kind, name = conn.execute(
|
||||
text("SELECT kind, name FROM file_folders WHERE id = :i"), {"i": hub}
|
||||
).one()
|
||||
folder_of_job = conn.execute(
|
||||
text("SELECT folder_id FROM download_jobs WHERE id = :j"), {"j": held}
|
||||
).scalar_one()
|
||||
finally:
|
||||
command.upgrade(cfg, "head")
|
||||
assert (kind, name) == ("own", f"Hub {hub} (2)")
|
||||
assert folder_of_job == hub
|
||||
@@ -96,6 +96,9 @@ class TestDownloadFilename:
|
||||
def test_does_not_double_an_extension_already_present(self):
|
||||
assert download_filename("clip.mp4", "mp4", Path("/x/f.mp4")) == "clip.mp4"
|
||||
|
||||
def test_no_extension_is_served_without_a_trailing_dot(self):
|
||||
assert download_filename("Makefile", None, Path("/x/upload-7")) == "Makefile"
|
||||
|
||||
def test_falls_back_to_the_file_suffix_when_no_container(self):
|
||||
assert download_filename("song", None, Path("/x/f.m4a")) == "song.m4a"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user