From 5f82950d7c2b709a6cee9cef697b8f98aa9ab6a4 Mon Sep 17 00:00:00 2001 From: mwiegand Date: Sun, 17 May 2026 16:29:55 +0200 Subject: [PATCH] feat(files): delete /files/content endpoint + extract _apply_optional_rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 12/12 of docs/superpowers/plans/2026-05-17-files-overlay-rewrite.md. End of Phase C — end of the rewrite plan. Two cleanups in one commit: 1. Delete GET /overlays//files/content. The legacy openEditorForFile in files-overlay.js was its only caller, and Step 9 deleted that code path. grep confirms no remaining live callers (the matches in .claude/worktrees/* are other in-flight branches; matches in docs/ are plan/spec text describing the route's history). Removed: * The @bp.get route function (was already a thin wrapper around _load_file_for_editing from Step 11) * The endpoint's mention in the module docstring * test_content_returns_text * test_content_returns_415_for_binary * test_content_404_for_non_files_overlay * The /files/content entry in the batched "non-files-overlay 404s everywhere" test The _load_file_for_editing helper from Step 11 becomes single-caller (only the edit route uses it now). Kept because the function name gives the prelude a useful named concept and inlining would add ~17 lines of low-density logic into overlay_file_edit_page. 2. Extract _apply_optional_rename. overlay_file_save and overlay_file_replace had near-identical rename branches: safe_resolve_for_move → 422-on-traversal, 409-if-dst-exists, mkdir-parents, os.rename → echo_path = new_path. Extracted into _apply_optional_rename(overlay, path, new_path) → (write_target, echo_path) | Response. The helper handles both cases: * Rename: atomic rename, returns (dst, new_path) * No rename: safe_resolve_for_write, mkdir parents, returns (write_target, path) Save's "destination is not a file" 409 (creation branch) stays inline in overlay_file_save — it's save-specific behavior that doesn't apply to /replace (which assumes a file exists or creates one). Subtle behavior change in /save: the prior code called safe_resolve_for_write(new_path or path) upfront and then potentially overrode write_target via safe_resolve_for_move. The new code only calls one validator per branch. Confirmed equivalent: per overlay_files.py:36-58 (safe_resolve_for_write) vs. lines 76-106 (safe_resolve_for_move), the dst-side checks are identical (root escape, symlink refuse, parent-is-dir) and safe_resolve_for_move adds strictly more (src must exist, cycle check for directory moves). pytest covers the save/replace paths and stays green. pytest: 580 → 577 passed, 1 skipped, 3 deselected. The -3 is the 3 deleted /files/content tests. files_routes.py: ended at 735 lines. The plan estimated ~450 — the delta is the new /files/new route (~43 lines, Step 5), the binary template branch (Step 7), the _load_file_for_editing helper (Step 11), and module-header expansions. The structural goal (no dead routes, shared helpers across paths) is met. End-of-plan summary: * 1091-line files-overlay.js → 4 focused modules totaling 1191 lines (core.js 247, editor.js 309, dialogs.js 212, uploads.js 423) * Editor flows (text edit, binary replace, create new) all run through URL-addressable modals (?modal= deep-linkable) * Legacy deleted * /files/content deleted (dead endpoint) * Shared helpers _load_file_for_editing + _apply_optional_rename * pytest stayed green at every step * Chromium-verified every step Co-Authored-By: Claude Opus 4.7 (1M context) --- l4d2web/l4d2web/routes/files_routes.py | 111 +++++++++------------ l4d2web/tests/test_overlay_files_routes.py | 42 -------- 2 files changed, 48 insertions(+), 105 deletions(-) diff --git a/l4d2web/l4d2web/routes/files_routes.py b/l4d2web/l4d2web/routes/files_routes.py index 44a3c2c..2d4340c 100644 --- a/l4d2web/l4d2web/routes/files_routes.py +++ b/l4d2web/l4d2web/routes/files_routes.py @@ -10,8 +10,6 @@ Read-only endpoints (any overlay): is refused. Mutating endpoints (only `overlay.type == 'files'`, owner or admin): -- `GET /overlays//files/content?path=` — JSON `{path, content}` for - an editable text file, 415 if not editable. - `POST /overlays//files/save` — JSON `{path, content, new_path?}`, text-mode write with optional atomic rename. - `POST /overlays//files/replace` — multipart `path`, `file`, @@ -243,24 +241,6 @@ def _load_file_for_editing(overlay_id: int, sub_path: str, user): return overlay, target, content, False, len(content.encode("utf-8")) -@bp.get("/overlays//files/content") -@require_login -def overlay_file_content(overlay_id: int): - """Return `{path, content}` for an editable text file.""" - user = current_user() - assert user is not None - sub_path = request.args.get("path", "") - - result = _load_file_for_editing(overlay_id, sub_path, user) - if isinstance(result, Response): - return result - _overlay, _target, content, is_binary, _byte_count = result - - if is_binary: - return Response("not editable", status=415) - return jsonify({"path": sub_path, "content": content}) - - @bp.get("/overlays//files/edit") @require_login def overlay_file_edit_page(overlay_id: int): @@ -355,6 +335,40 @@ def _validate_save_content(content: str) -> Response | None: return None +def _apply_optional_rename(overlay, path: str, new_path: str | None): + """Resolve the write target for save / replace, atomically renaming + when `new_path` differs from `path`. Returns: + * (write_target, echo_path) on success — write_target is the + resolved Path the caller should write to; echo_path is the + canonical relative path to echo back in the response + * a Flask Response on failure: 422 (path validation) or 409 + (rename destination already exists) + + The atomic rename happens INSIDE this helper. Save's "destination + is not a file" 409 (creation branch) stays in the caller, since + /replace doesn't enforce that check.""" + if new_path is not None and new_path != path: + try: + src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path) + except ValueError as exc: + return Response(str(exc), status=422) + if dst_path.exists(): + return Response("destination already exists", status=409) + dst_path.parent.mkdir(parents=True, exist_ok=True) + os.rename(src_path, dst_path) + return dst_path, new_path + try: + write_target = safe_resolve_for_write(overlay.path, path) + except ValueError as exc: + return Response(str(exc), status=422) + write_target.parent.mkdir(parents=True, exist_ok=True) + return write_target, path + + +def _is_existing_file(path) -> bool: + return path.is_file() and not path.is_symlink() + + @bp.post("/overlays//files/save") @require_login def overlay_file_save(overlay_id: int): @@ -380,38 +394,23 @@ def overlay_file_save(overlay_id: int): return result overlay = result - try: - write_target = safe_resolve_for_write(overlay.path, new_path or path) - except ValueError as exc: - return Response(str(exc), status=422) + rename_result = _apply_optional_rename(overlay, path, new_path) + if isinstance(rename_result, Response): + return rename_result + write_target, echo_path = rename_result - # Rename branch: source must exist, dst must not collide. - if new_path is not None and new_path != path: - try: - src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path) - except ValueError as exc: - return Response(str(exc), status=422) - if dst_path.exists(): - return Response("destination already exists", status=409) - dst_path.parent.mkdir(parents=True, exist_ok=True) - os.rename(src_path, dst_path) - write_target = dst_path - else: - write_target.parent.mkdir(parents=True, exist_ok=True) - # Creation branch: must not collide with an existing path. - if write_target.exists() and not _is_existing_file(write_target): - return Response("destination is not a file", status=409) + # Creation branch (no rename): must not collide with an existing + # non-file (e.g. a directory at the same path). + if (new_path is None or new_path == path) \ + and write_target.exists() and not _is_existing_file(write_target): + return Response("destination is not a file", status=409) try: write_target.write_text(content, encoding="utf-8") except OSError as exc: return Response(f"write failed: {exc}", status=500) - return jsonify({"path": new_path or path}) - - -def _is_existing_file(path) -> bool: - return path.is_file() and not path.is_symlink() + return jsonify({"path": echo_path}) @bp.post("/overlays//files/replace") @@ -433,24 +432,10 @@ def overlay_file_replace(overlay_id: int): return result overlay = result - if new_path and new_path != path: - try: - src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path) - except ValueError as exc: - return Response(str(exc), status=422) - if dst_path.exists(): - return Response("destination already exists", status=409) - dst_path.parent.mkdir(parents=True, exist_ok=True) - os.rename(src_path, dst_path) - write_target = dst_path - echo_path = new_path - else: - try: - write_target = safe_resolve_for_write(overlay.path, path) - except ValueError as exc: - return Response(str(exc), status=422) - write_target.parent.mkdir(parents=True, exist_ok=True) - echo_path = path + rename_result = _apply_optional_rename(overlay, path, new_path) + if isinstance(rename_result, Response): + return rename_result + write_target, echo_path = rename_result return _stream_upload_into(upload, write_target, echo_path) diff --git a/l4d2web/tests/test_overlay_files_routes.py b/l4d2web/tests/test_overlay_files_routes.py index b17c967..d6b90b3 100644 --- a/l4d2web/tests/test_overlay_files_routes.py +++ b/l4d2web/tests/test_overlay_files_routes.py @@ -582,47 +582,6 @@ def _csrf_headers(): return {"X-CSRF-Token": "test-token"} -# ---- /content ------------------------------------------------------------- - - -def test_content_returns_text(app, left4me_root: Path) -> None: - user_id = _make_user() - overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="files") - overlay_dir = left4me_root / "overlays" / str(overlay_id) - (overlay_dir / "motd.txt").write_text("welcome") - - client = _client_for(app, user_id) - r = client.get(f"/overlays/{overlay_id}/files/content?path=motd.txt") - - assert r.status_code == 200 - assert r.get_json() == {"path": "motd.txt", "content": "welcome"} - - -def test_content_returns_415_for_binary(app, left4me_root: Path) -> None: - user_id = _make_user() - overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="files") - overlay_dir = left4me_root / "overlays" / str(overlay_id) - (overlay_dir / "image.bin").write_bytes(b"\x00\x01\x02") - - client = _client_for(app, user_id) - r = client.get(f"/overlays/{overlay_id}/files/content?path=image.bin") - - assert r.status_code == 415 - - -def test_content_404_for_non_files_overlay(app, left4me_root: Path) -> None: - """`content` is gated to type=='files' to keep the new editor scoped.""" - user_id = _make_user() - overlay_id = _make_overlay(left4me_root, user_id=user_id, name="ws") # type='script' helper - overlay_dir = left4me_root / "overlays" / str(overlay_id) - (overlay_dir / "motd.txt").write_text("hi") - - client = _client_for(app, user_id) - r = client.get(f"/overlays/{overlay_id}/files/content?path=motd.txt") - - assert r.status_code == 404 - - # ---- /save --------------------------------------------------------------- @@ -1064,7 +1023,6 @@ def test_mutating_endpoints_404_for_workshop_overlay(app, left4me_root: Path) -> ("post", f"/overlays/{overlay_id}/files/delete", {"data": {"path": "x", "csrf_token": "test-token"}}), ("get", f"/overlays/{overlay_id}/files/download_zip?path=", {}), - ("get", f"/overlays/{overlay_id}/files/content?path=x.txt", {}), ] for method, url, kwargs in paths: kwargs = dict(kwargs)