From 2bdf1ec76f32eccb59768dad424c797d642a7dcb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 4 Nov 2026 19:01:49 +0100 Subject: [PATCH 01/13] fix(editor): a crop of a video with wide pixels keeps what its frame showed The browser reports such a video by its shown size and ffmpeg's crop counts stored pixels, so a frame over the middle half of a 320 stored, 640 shown video kept the whole width. A crop now carries the size it was measured on and is scaled to the file's own pixels. A spec from before it did is cut as it always was. Also drops a stale note in downloads/quota.py: edit jobs are held to the cap. --- backend/app/downloads/edit.py | 32 ++++- backend/app/downloads/quota.py | 3 +- backend/tests/test_edit_crop_frame.py | 112 ++++++++++++++++++ frontend/src/lib/api/downloads.ts | 4 +- .../files/VideoEditor.crop.dom.test.tsx | 56 +++++++++ frontend/src/modules/files/VideoEditor.tsx | 6 +- 6 files changed, 206 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_edit_crop_frame.py create mode 100644 frontend/src/modules/files/VideoEditor.crop.dom.test.tsx diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 2ce485b1..c0e51e90 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -60,6 +60,31 @@ class EditAborted(Exception): _MAX_SEGMENTS = 100 +def _crop_frame(crop: dict) -> dict | None: + """The picture size a crop was measured on, or None when it has none it could be scaled by (a + spec from before it was sent, which `crop_filter` cuts in the file's own pixels).""" + try: + w, h = int(crop["frame_w"]), int(crop["frame_h"]) + # silent-ok: no frame is a crop's older form, and it is still the crop that was asked for. + except (KeyError, TypeError, ValueError): + return None + return {"frame_w": w, "frame_h": h} if w > 0 and h > 0 else None + + +def crop_filter(crop: dict) -> str: + """The ffmpeg `crop` of an edit. Its numbers count the picture as the browser reports it, which + is its SHOWN size: a video with wide pixels (320 stored, 640 shown) would be cut off to one side + in ffmpeg's stored ones, so they are scaled to `iw` / `ih` by the size they were measured on. A + rotated file needs nothing more: both the browser and ffmpeg's filters see it upright.""" + frame_w, frame_h = crop.get("frame_w"), crop.get("frame_h") + if not frame_w or not frame_h: + return f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}" + return ( + f"crop=iw*{crop['w']}/{frame_w}:ih*{crop['h']}/{frame_h}" + f":iw*{crop['x']}/{frame_w}:ih*{crop['y']}/{frame_h}" + ) + + def normalize_edit_spec(spec: dict | None) -> dict: """Canonicalize a raw edit spec: clamp/round trim, coerce crop to ints, drop empties. @@ -81,7 +106,8 @@ def normalize_edit_spec(spec: dict | None) -> dict: ) c = None if c and c["w"] > 0 and c["h"] > 0 and c["x"] >= 0 and c["y"] >= 0: - out["crop"] = c + frame = _crop_frame(crop) + out["crop"] = {**c, **frame} if frame else c # Multi-segment JOIN (cut-list → one concatenated file). Takes precedence over `trim`. segs = spec.get("segments") @@ -268,7 +294,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] if reencode: # Input -ss + re-encode is frame-accurate (decodes from the prior keyframe, discards up to # the exact cut). A crop adds the filter; an accurate trim only evens the frame. - cropf = f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}," if crop else "" + cropf = f"{crop_filter(crop)}," if crop else "" cmd += ["-vf", cropf + EVEN_FRAME, *X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -313,7 +339,7 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: ] if needs_reencode(spec): - cropf = f",crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}" if crop else "" + cropf = f",{crop_filter(crop)}" if crop else "" parts, labels = [], [] for i, s in enumerate(segs): st, en = s["start_s"], s["end_s"] diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index 179b3b92..3843613d 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -16,7 +16,8 @@ That is a per-job ceiling, not an aggregate ledger: concurrent jobs of one user remainder (in-flight bytes are not `ready`, so `footprint` cannot see them), so a wave of max_concurrent downloads can still land over the cap — the deliberate trade, since a download's size is unknown until it finishes. CLIP jobs consult it before their fetch or encode and again with -their output's real size before it is placed. Edit jobs do not consult it at all (tracked as SEC-19). +their output's real size before it is placed. Edit jobs consult it with their output's real size +before it is placed (`test_edit_cap.py`). """ from dataclasses import dataclass diff --git a/backend/tests/test_edit_crop_frame.py b/backend/tests/test_edit_crop_frame.py new file mode 100644 index 00000000..98b151b4 --- /dev/null +++ b/backend/tests/test_edit_crop_frame.py @@ -0,0 +1,112 @@ +"""The editor's crop lands on the part of the picture the frame was drawn on, also when a video's +pixels are not square. The browser reports such a video by its shown size (320 stored, 640 shown), +and ffmpeg's `crop` counts stored pixels: measured on a real clip, a frame over the middle half kept +half the frame's red surroundings. So a crop carries the size it was measured on. + +Real ffmpeg: the pixels of the output are the oracle. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import edit, pipeline, service, storage +from app.downloads import upload as uploadmod +from app.downloads.models import DownloadJob, MediaAsset + +pytestmark = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _source(path: Path, size: str, box: str, sar: int) -> None: + """Red, with the picture's middle half green: a crop of that middle holds no red.""" + subprocess.run( + [ + "ffmpeg", "-y", "-loglevel", "error", + "-f", "lavfi", "-i", f"color=c=red:s={size}:d=2:r=10", + "-f", "lavfi", "-i", "sine=duration=2", + "-vf", f"drawbox={box}:c=lime:t=fill,setsar={sar}", + "-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", str(path), + ], + check=True, + ) # fmt: skip + + +def _red_and_green(path: Path) -> tuple[int, int]: + raw = subprocess.run( + ["ffmpeg", "-loglevel", "error", "-i", str(path), "-frames:v", "1", + "-f", "rawvideo", "-pix_fmt", "rgb24", "-"], + check=True, + capture_output=True, + ).stdout # fmt: skip + red = green = 0 + for i in range(0, len(raw) - 2, 3): + if raw[i] > 180 and raw[i + 1] < 80: + red += 1 + elif raw[i + 1] > 180 and raw[i] < 80: + green += 1 + return red, green + + +def _cut(db, make_user, tmp_path, src: Path, crop: dict) -> Path: + user = make_user() + source = uploadmod.ingest_upload( + db, user.id, filename="clip.mp4", src_path=src, size=src.stat().st_size + ) + assert source.status == "done" + job = service.enqueue_edit(db, user.id, source, {"crop": crop}) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" # the worker's claim + db.commit() + + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + job, asset = db.get(DownloadJob, job.id), db.get(MediaAsset, job.asset_id) + assert (job.status, job.error, asset.status) == ("done", None, "ready") + return storage.abs_path(str(tmp_path), asset.rel_path) + + +def test_a_crop_of_a_video_with_wide_pixels_keeps_what_the_frame_showed( + db, make_user, tmp_path, monkeypatch +): + monkeypatch.setattr(settings, "files_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _source(src, "320x240", "x=80:y=60:w=160:h=120", sar=2) + + # What the editor sends for the middle half of this video: the browser shows it at 640 x 240. + final = _cut( + db, make_user, tmp_path, src, + {"x": 160, "y": 60, "w": 320, "h": 120, "frame_w": 640, "frame_h": 240}, + ) # fmt: skip + + red, green = _red_and_green(final) + assert (red, green) == (0, 160 * 120) + + +def test_a_crop_of_square_pixels_is_cut_where_it_was_drawn(db, make_user, tmp_path, monkeypatch): + monkeypatch.setattr(settings, "files_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _source(src, "480x640", "x=120:y=160:w=240:h=320", sar=1) + + final = _cut( + db, make_user, tmp_path, src, + {"x": 120, "y": 160, "w": 240, "h": 320, "frame_w": 480, "frame_h": 640}, + ) # fmt: skip + + assert _red_and_green(final) == (0, 240 * 320) + + +def test_a_crop_stored_before_it_carried_its_frame_is_cut_as_it_always_was(): + spec = edit.normalize_edit_spec({"crop": {"x": 4, "y": 6, "w": 100, "h": 50}}) + assert spec["crop"] == {"x": 4, "y": 6, "w": 100, "h": 50} + cmd = edit.build_edit_cmd(Path("in.mp4"), Path("out.mp4"), spec, "mp4") + assert cmd[cmd.index("-vf") + 1].startswith("crop=100:50:4:6,") + + +@pytest.mark.parametrize("frame", [{"frame_w": 0, "frame_h": 240}, {"frame_w": "x", "frame_h": 2}]) +def test_a_frame_that_cannot_be_read_is_not_divided_by(frame): + spec = edit.normalize_edit_spec({"crop": {"x": 4, "y": 6, "w": 100, "h": 50, **frame}}) + assert spec["crop"] == {"x": 4, "y": 6, "w": 100, "h": 50} diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index c959c63d..acc06f26 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -70,7 +70,9 @@ export interface ClipRequest { export interface EditSpec { trim?: { start_s: number; end_s?: number }; segments?: { start_s: number; end_s: number }[]; - crop?: { x: number; y: number; w: number; h: number }; + // In the picture's size as the browser reports it, which `frame_w` x `frame_h` says: the server + // scales them to the file's own pixels (a video's stored and shown sizes can differ). + crop?: { x: number; y: number; w: number; h: number; frame_w?: number; frame_h?: number }; accurate?: boolean; } diff --git a/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx b/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx new file mode 100644 index 00000000..5c8d86cf --- /dev/null +++ b/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx @@ -0,0 +1,56 @@ +// @vitest-environment jsdom +// +// A crop's numbers count the picture as the browser reports it, its shown size, which differs from the +// file's own on a video with wide pixels. The size goes along, or the server cuts the wrong part. +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), +})); + +import VideoEditor from "./VideoEditor"; +import { api, type DownloadJob } from "../../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const job = { + id: 7, + display_name: "Talk", + asset: { duration_s: 100, width: 1920, height: 1080, title: "Talk" }, +} as unknown as DownloadJob; + +describe("VideoEditor crop", () => { + it("sends the size the crop was measured on with it", async () => { + const enqueue = vi + .spyOn(api, "enqueueEdit") + .mockResolvedValue({ id: 8 } as unknown as DownloadJob); + render( + + {}} /> + , + ); + fireEvent.click(screen.getByRole("button", { name: "editor.cropOff" })); + fireEvent.click(screen.getByRole("button", { name: "editor.create" })); + await waitFor(() => expect(enqueue).toHaveBeenCalledTimes(1)); + const crop = enqueue.mock.calls[0]?.[0].edit_spec.crop; + expect(crop).toMatchObject({ frame_w: 1920, frame_h: 1080, w: 1536, h: 864 }); + }); +}); diff --git a/frontend/src/modules/files/VideoEditor.tsx b/frontend/src/modules/files/VideoEditor.tsx index 0063e9ca..f2a2e016 100644 --- a/frontend/src/modules/files/VideoEditor.tsx +++ b/frontend/src/modules/files/VideoEditor.tsx @@ -226,14 +226,16 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos const willJoin = output === "join" && kept.length > 1; const reencode = cropOn || accurate; - const cropPx = (): { x: number; y: number; w: number; h: number } | undefined => { + const cropPx = (): EditSpec["crop"] => { const v = videoRef.current; const vw = v?.videoWidth || vdim.w; const vh = v?.videoHeight || vdim.h; if (!cropOn) return undefined; // crop fractions are relative to the picture (the overlay is anchored to it), so this is a // straight fraction→source-px scale; see cropGeometry for the even-rounding + in-bounds clamp. - return cropToSourcePx(crop, vw, vh) ?? undefined; + const px = cropToSourcePx(crop, vw, vh); + // The size they count in goes along: it is the shown one, and the file's own can differ. + return px ? { ...px, frame_w: vw, frame_h: vh } : undefined; }; const create = useMutation({ From 4c09a22addde7ca281105c543358263e1c09583c Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 4 Nov 2026 19:10:09 +0100 Subject: [PATCH 02/13] feat(share): a link's address says what it opens A video stays on /watch/, anything else is given out on /file/ and a folder on /folder/. The token is the whole key, so every path opens every link and the links already sent keep working. The new paths are kept out of search indexes like /watch/, boot the player alone, and the unfurl card calls a file a file. --- CLAUDE.md | 2 +- backend/app/downloads/links.py | 18 ++- backend/app/downloads/og.py | 17 ++- backend/app/main.py | 14 ++- backend/app/routes/downloads.py | 10 +- backend/tests/test_share_paths.py | 137 ++++++++++++++++++++++ frontend/public/robots.txt | 6 +- frontend/src/lib/api/downloads.ts | 2 +- frontend/src/watch/WatchPage.dom.test.tsx | 14 +++ frontend/src/watch/WatchPage.tsx | 10 +- frontend/src/watchEntry.contract.test.ts | 23 ++++ frontend/vite.config.ts | 6 +- 12 files changed, 233 insertions(+), 26 deletions(-) create mode 100644 backend/tests/test_share_paths.py diff --git a/CLAUDE.md b/CLAUDE.md index d81043fe..4b6b62a3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,7 +74,7 @@ only along a declared edge and writes them only through that unit's service; cor cannot (an ORM write on another unit's loaded row, SQL in `text()`, a commit by `begin()`) is review's to catch, listed in [`backend/app/README.md`](backend/app/README.md) with the conventions. Frontend, by folder under `src/`: `main.tsx` provider tree → `shell/` (`App`, header, nav rail, palette, -boot; a shared `/watch` link boots from its own entry, `watch.html` → `watchMain.tsx` → `watch/`, held +boot; a share link (`/watch`, `/file`, `/folder`) boots from its own entry, `watch.html` → `watchMain.tsx` → `watch/`, held app-free by a contract test) → **`modules//`** (one folder per page: the page, its rail, its store, its tests, and its `module.ts` definition — label, icon, gate, the lazy page and rail, its store — collected in nav order by `features/navigation/moduleRegistry`, which also mounts every store at the root; a new page is its folder, its id in diff --git a/backend/app/downloads/links.py b/backend/app/downloads/links.py index 54546761..68e5b6c5 100644 --- a/backend/app/downloads/links.py +++ b/backend/app/downloads/links.py @@ -127,14 +127,28 @@ def check_grant(token: str, grant: str | None, password_version: int) -> bool: # --- serialization ------------------------------------------------------------------------- -def owner_view(link: DownloadLink, base_url: str = "", token: str | None = None) -> dict: +def share_path(db: Session, link: DownloadLink, token: str) -> str: + """The address a link is given out on: its path says what it opens. Only a word for the reader, + the token is the whole key: every prefix of `main.SHARE_PREFIXES` opens every link, so the links + already sent on /watch/ work for good. A file still being made reads as the video it mostly is.""" + from app.downloads import mediakind + + if link.folder_id is not None: + return f"/folder/{token}" + job = db.get(DownloadJob, link.job_id) + asset = db.get(MediaAsset, job.asset_id) if job and job.asset_id else None + kind = asset.media_kind if asset else None + return f"/file/{token}" if kind and kind != mediakind.VIDEO else f"/watch/{token}" + + +def owner_view(db: Session, link: DownloadLink, token: str | None = None) -> dict: """What the file owner sees when managing their links. `token` is the one just made (it is in hand even where no copy is kept); else it is read back, and `url` is None when it cannot be.""" token = token or recover(link) return { "id": link.id, "token": token, - "url": f"{base_url}/watch/{token}" if token else None, + "url": share_path(db, link, token) if token else None, "allow_download": link.allow_download, "has_password": link.password_hash is not None, "expires_at": link.expires_at.isoformat() if link.expires_at else None, diff --git a/backend/app/downloads/og.py b/backend/app/downloads/og.py index f2d4d29c..626e4e1c 100644 --- a/backend/app/downloads/og.py +++ b/backend/app/downloads/og.py @@ -1,4 +1,4 @@ -"""Server-rendered Open Graph tags for the public /watch/{token} page. +"""Server-rendered Open Graph tags for the public page of a share link (/watch/, /file/, /folder/). The watch page is a client-side SPA, so a social crawler (facebookexternalhit, Twitterbot, …) that fetches /watch/{token} would otherwise only see the generic index.html — a blank/greyed link card. @@ -76,16 +76,21 @@ def _watch_tags(token: str, db: Session) -> str | None: link = linksmod.find(db, token) if link is None or linksmod.is_expired(link): return None - url = f"{settings.app_base}/watch/{token}" + url = f"{settings.app_base}{linksmod.share_path(db, link, token)}" folder = link.folder_id is not None + # What the card calls it, as its address does (`links.share_path`). + video = "/watch/" in url if link.password_hash: # Password-gated: don't leak the title/thumbnail in the unfurl. return "\n ".join( [ "Siftlode", - _tag("og:title", "Password-protected " + ("folder" if folder else "video")), + _tag( + "og:title", + "Password-protected " + ("folder" if folder else "video" if video else "file"), + ), _tag("og:description", "Shared via Siftlode"), - _tag("og:type", "website" if folder else "video.other"), + _tag("og:type", "video.other" if video else "website"), _tag("og:url", url), _tag("og:site_name", "Siftlode"), ] @@ -96,7 +101,7 @@ def _watch_tags(token: str, db: Session) -> str | None: asset = db.get(MediaAsset, job.asset_id) if job and job.asset_id else None if job is None or asset is None or asset.status != "ready": return None - title = (job.display_name or asset.title or "Video").strip() + title = (job.display_name or asset.title or ("Video" if video else "File")).strip() channel = (job.display_uploader or asset.uploader or "").strip() # Pick the link-preview image. Facebook's scraper REJECTS our own poster ("Corrupted Image" in the # Sharing Debugger): the poster is an ffmpeg-produced JPEG whose non-standard marker order FB's @@ -118,7 +123,7 @@ def _watch_tags(token: str, db: Session) -> str | None: f"{html.escape(title)}", _tag("og:title", title), _tag("og:description", channel or "Shared via Siftlode"), - _tag("og:type", "video.other"), + _tag("og:type", "video.other" if video else "website"), _tag("og:url", url), _tag("og:site_name", "Siftlode"), _tag("twitter:title", title, attr="name"), diff --git a/backend/app/main.py b/backend/app/main.py index 30905c8d..1f1702a4 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -368,6 +368,10 @@ INDEX_HTML = STATIC_DIR / "index.html" # A public share link's own page: the player alone, without the app's bundle (frontend/watch.html). WATCH_HTML = STATIC_DIR / "watch.html" INDEX_HEADERS = {"Cache-Control": "no-cache"} +# The paths a share link opens on, by what it opens (`links.share_path`). Any of them opens any link. +# A new one also goes in frontend/public/robots.txt, the Vite dev entry and WatchPage's token parsing +# (watchEntry.contract.test.ts). +SHARE_PREFIXES = ("watch/", "file/", "folder/") app.mount( "/assets", StaticFiles(directory=STATIC_DIR / "assets", check_dir=False), @@ -381,8 +385,10 @@ async def index() -> FileResponse: @app.get("/watch/{token}") -async def watch_page(token: str): - """Serve the public player page for a share link, with per-video Open Graph tags injected so the +@app.get("/file/{token}") +@app.get("/folder/{token}") +async def share_page(token: str): + """Serve the public page of a share link, with per-link Open Graph tags injected so the link unfurls richly (title/channel/thumbnail) in Messenger/social crawlers. A real browser ignores the extra tags and boots WatchPage; invalid/expired/password links fall back to the generic card.""" @@ -409,8 +415,8 @@ async def spa_fallback(full_path: str) -> FileResponse: # Real SPA-root assets (welcome images, favicon, robots.txt) rarely change and have # stable names, so a moderate cache is safe and satisfies the cache-lifetime audit. return FileResponse(candidate, headers={"Cache-Control": "public, max-age=604800"}) - # A /watch/ path the route above does not match (a trailing segment, no token) is still the + # A share path the route above does not match (a trailing segment, no token) is still the # player's: WatchPage reads the token off the path and says when a link is gone. - if full_path.startswith("watch/"): + if full_path.startswith(SHARE_PREFIXES): return FileResponse(WATCH_HTML, headers=INDEX_HEADERS) return FileResponse(INDEX_HTML, headers=INDEX_HEADERS) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 380bb337..4730f9d0 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -611,7 +611,7 @@ def list_links( job_id: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> list[dict]: own_job(db, user, job_id) - return [linksmod.owner_view(link) for link in queries.links_to(db, job_id=job_id)] + return [linksmod.owner_view(db, link) for link in queries.links_to(db, job_id=job_id)] class CreateLinkIn(BaseModel): @@ -653,7 +653,7 @@ def create_link( raise refused(409, "share_unfinished") link, token = _new_link(db, user, body, job_id=job_id) db.commit() - return linksmod.owner_view(link, token=token) + return linksmod.owner_view(db, link, token=token) def _shareable_job(db: Session, user: User, job_id: int) -> "DownloadJob": @@ -684,7 +684,7 @@ def list_folder_links( folder_id: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> list[dict]: own_folder(db, user, folder_id) - return [linksmod.owner_view(link) for link in queries.links_to(db, folder_id=folder_id)] + return [linksmod.owner_view(db, link) for link in queries.links_to(db, folder_id=folder_id)] @router.post("/folders/{folder_id}/links") @@ -697,7 +697,7 @@ def create_folder_link( _shareable_folder(db, user, folder_id) link, token = _new_link(db, user, body, folder_id=folder_id) db.commit() - return linksmod.owner_view(link, token=token) + return linksmod.owner_view(db, link, token=token) class UpdateLinkIn(BaseModel): @@ -724,7 +724,7 @@ def update_link( sent=frozenset(body.model_fields_set), ) db.commit() - return linksmod.owner_view(link) + return linksmod.owner_view(db, link) @router.delete("/links/{link_id}") diff --git a/backend/tests/test_share_paths.py b/backend/tests/test_share_paths.py new file mode 100644 index 00000000..3469a1df --- /dev/null +++ b/backend/tests/test_share_paths.py @@ -0,0 +1,137 @@ +"""A share link's address says what it opens: /watch/ a video, /file/ anything else, /folder/ a folder. + +Only a word for the reader: the token is the key, so every path opens every link, and the links +already sent out on /watch/ keep working for good. DB-lane. +""" + +import pytest +from cryptography.fernet import Fernet + +from app import main, security +from app.config import settings +from app.downloads import og +from app.downloads import probe as probemod + +_PAGE = """ + +""" + +_VIDEO = { + "streams": [ + {"codec_type": "video", "codec_name": "h264", "pix_fmt": "yuv420p"}, + {"codec_type": "audio", "codec_name": "aac"}, + ], + "format": {"format_name": "mov,mp4", "duration": "5"}, +} +_AUDIO = { + "streams": [{"codec_type": "audio", "codec_name": "aac"}], + "format": {"format_name": "mov,mp4", "duration": "5"}, +} + + +@pytest.fixture +def pages(tmp_path, monkeypatch): + index, watch = tmp_path / "index.html", tmp_path / "watch.html" + index.write_text(_PAGE.format(entry="app"), encoding="utf-8") + watch.write_text(_PAGE.format(entry="watch"), encoding="utf-8") + monkeypatch.setattr(main, "INDEX_HTML", index) + monkeypatch.setattr(main, "WATCH_HTML", watch) + + +@pytest.fixture +def key(monkeypatch): + """A link's address is read back from its encrypted token.""" + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) + + +@pytest.fixture +def shared(as_user, make_user, tmp_path, monkeypatch, upload_headers): + """Uploads a file ffprobe reads as `data` (None: it cannot read it) and shares it.""" + monkeypatch.setattr(settings, "files_root", str(tmp_path / "media")) + owner = as_user(make_user()) + + def share(name: str, data: dict | None, **link: object) -> dict: + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + job = owner.post( + "/api/files/upload", + content=b"some bytes", + headers=upload_headers(name, display_name=name), + ) + assert job.status_code == 200, job.text + made = owner.post(f"/api/downloads/{job.json()['id']}/links", json=link) + assert made.status_code == 200, made.text + return made.json() + + share.owner = owner # type: ignore[attr-defined] + return share + + +@pytest.mark.parametrize( + ("name", "data", "prefix"), + [ + ("clip.mp4", _VIDEO, "/watch/"), + ("song.m4a", _AUDIO, "/file/"), + ("backup.tar.gz", None, "/file/"), + ], +) +def test_a_link_is_given_out_on_the_path_of_what_it_opens(shared, name, data, prefix): + made = shared(name, data) + assert made["url"] == f"{prefix}{made['token']}" + + +def test_a_folder_link_is_given_out_on_folder(shared, key): + owner = shared.owner + folder = owner.post("/api/files/folders", json={"name": "Friends"}).json() + made = owner.post(f"/api/downloads/folders/{folder['id']}/links", json={}).json() + assert made["url"] == f"/folder/{made['token']}" + listed = owner.get(f"/api/downloads/folders/{folder['id']}/links").json() + assert [entry["url"] for entry in listed] == [made["url"]] + + +def test_the_list_gives_the_same_address_as_the_making(shared, key): + made = shared("backup.tar.gz", None) + owner = shared.owner + files = owner.get("/api/files").json() + job_id = next(f["id"] for f in files if f["display_name"] == "backup.tar.gz") + listed = owner.get(f"/api/downloads/{job_id}/links").json() + assert [entry["url"] for entry in listed] == [made["url"]] + + +@pytest.mark.parametrize("path", ["watch", "file", "folder"]) +@pytest.mark.parametrize( + ("name", "data"), [("clip.mp4", _VIDEO), ("backup.tar.gz", None)], ids=["video", "file"] +) +def test_every_path_opens_every_link(pages, shared, client, path, name, data): + """The links already sent on /watch/ open whatever they open, for good.""" + token = shared(name, data)["token"] + r = client.get(f"/{path}/{token}") + assert r.status_code == 200 + assert "/assets/watch.js" in r.text and "/assets/app.js" not in r.text + assert f'content="{name}"' in r.text + + +@pytest.mark.parametrize("path", ["/file/no-such-token", "/folder/no-such-token", "/file/a/b"]) +def test_a_new_path_without_a_live_link_still_gets_the_player(pages, client, path): + r = client.get(path) + assert r.status_code == 200 + assert "/assets/watch.js" in r.text and "/assets/app.js" not in r.text + + +def test_a_card_names_a_file_as_a_file(shared, db): + made = shared("backup.tar.gz", None) + tags = og._watch_tags(made["token"], db) + assert f'{made["url"]}"' in tags + assert "video.other" not in tags + + locked = shared("notes.tar.gz", None, password="pw") + hidden = og._watch_tags(locked["token"], db) + assert "Password-protected file" in hidden and "notes" not in hidden + assert "video.other" not in hidden + + +def test_a_card_of_a_video_stays_a_video(shared, db): + made = shared("clip.mp4", _VIDEO) + tags = og._watch_tags(made["token"], db) + assert f"/watch/{made['token']}" in tags and "video.other" in tags + locked = shared("other.mp4", _VIDEO, password="pw") + assert "Password-protected video" in og._watch_tags(locked["token"], db) diff --git a/frontend/public/robots.txt b/frontend/public/robots.txt index 5d456ccc..41b1e8d3 100644 --- a/frontend/public/robots.txt +++ b/frontend/public/robots.txt @@ -1,14 +1,16 @@ # Siftlode is mostly a private, login-gated app; only the public landing and legal # pages are worth indexing. Allow crawling but keep bots out of the API surface. -# Share links (/watch/) are unguessable capability URLs: kept out of search indexes, +# Share links (/watch/, /file/, /folder/) are unguessable capability URLs: kept out of search indexes, # but link-preview crawlers are allowed below so shared links unfurl in chat apps. User-agent: * Allow: / Disallow: /api/ Disallow: /auth/ Disallow: /watch/ +Disallow: /file/ +Disallow: /folder/ -# Link-preview / unfurl bots — allowed to fetch /watch/ so shared links render a rich +# Link-preview / unfurl bots — allowed to fetch share links so shared links render a rich # card (title/channel/thumbnail). They don't build a search index, so this doesn't # expose share links to search engines. User-agent: facebookexternalhit diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index acc06f26..93d4ee68 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -145,7 +145,7 @@ export interface ShareLink { // Null when the server cannot show this link's address again (no encrypted copy its key reads): the // link still opens for whoever has it; to share it anew, make a new link. token: string | null; - url: string | null; // path like "/watch/" — prepend window.location.origin to share + url: string | null; // path like "/watch/" or "/file/" — prepend window.location.origin to share allow_download: boolean; has_password: boolean; expires_at: string | null; diff --git a/frontend/src/watch/WatchPage.dom.test.tsx b/frontend/src/watch/WatchPage.dom.test.tsx index ff6883bc..3a47d563 100644 --- a/frontend/src/watch/WatchPage.dom.test.tsx +++ b/frontend/src/watch/WatchPage.dom.test.tsx @@ -24,6 +24,20 @@ afterEach(() => { localStorage.clear(); }); +describe("WatchPage share paths", () => { + it.each(["/watch/tok-p", "/file/tok-p", "/folder/tok-p", "/file/tok-p/extra"])( + "reads the token off %s", + async (path) => { + window.history.replaceState({}, "", path); + mockMeta({ needs_password: false, audio: false, file_url: "/f.mp4", title: "A clip" }); + render(); + await waitFor(() => + expect(fetch).toHaveBeenCalledWith("/api/public/watch/tok-p", expect.anything()), + ); + }, + ); +}); + describe("WatchPage background play", () => { it("hands its