fix(store): security + correctness blockers found in PR review (F1, F2, F4, F5)
Three independent reviews of PR #180 surfaced four real defects in the new Store / my-ai-stack surface. CHANGELOG entries detail each; one-liners: - F1 video_url XSS: any authenticated user could upload a Store entity with `video_url=javascript:...` and pop XSS in any viewer's session via the `<a href=...>` "Watch video" link in store_detail.html. Jinja2 autoescape doesn't block URI schemes inside attribute values. Fixed by scheme-validating to http(s) only on create + update; 400 invalid_video_url. - F2 ZIP decompression bomb: _safe_zip_extract checked path-traversal but not declared file_size totals — a 50 MB compressed upload at 1:1000 ratio decompresses to 50 GB and DOS the host disk. Fixed by summing zinfo.file_size across infolist() and refusing > 200 MB before extractall touches disk. 413 zip_too_large_uncompressed. - F4 admin authz parity: PUT /api/store/entities/{id} was owner-only while DELETE allowed owner OR admin; the store-detail page hid Edit/Delete buttons from admin even though DELETE was permitted. Fixed by allowing admin on PUT and passing is_admin to the template; gate is now is_owner OR is_admin everywhere. - F5 cross-owner suffix collision: sanitize_username is many-to-one (alice.smith / alice_smith both → alice-smith). Two such users uploading entities with the same display name produced identical `<name>-by-<username>` suffixes, silently colliding in the served agnes-store-bundle on-disk paths AND the manifest catalog (Claude Code dedupes by plugin.json `name`). Fixed by enforcing global uniqueness on the suffixed value at create_entity; 409 conflict_global_suffix. F3 (ZIP symlink members) was investigated and confirmed to be a false-positive — Python's stdlib ZipFile.extractall does not honor symlink mode bits, so no exploit exists. 9 new regression tests in tests/test_store_api.py::TestStoreSecurityFixes covering all four. Test run locally: 60/60 store-related tests pass.
This commit is contained in:
parent
537ea7662b
commit
fd3c76d21b
6 changed files with 381 additions and 5 deletions
39
CHANGELOG.md
39
CHANGELOG.md
|
|
@ -10,6 +10,8 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C
|
||||||
|
|
||||||
## [Unreleased]
|
## [Unreleased]
|
||||||
|
|
||||||
|
## [0.35.0] — 2026-05-05
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
- **`/store` page** — community marketplace where every authenticated user
|
- **`/store` page** — community marketplace where every authenticated user
|
||||||
can upload skills, agents, and plugins as ZIPs. Listing has type / category /
|
can upload skills, agents, and plugins as ZIPs. Listing has type / category /
|
||||||
|
|
@ -43,6 +45,11 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C
|
||||||
`GET /api/store/categories`, `GET /api/store/owners`,
|
`GET /api/store/categories`, `GET /api/store/owners`,
|
||||||
`GET /api/my-stack`,
|
`GET /api/my-stack`,
|
||||||
`PUT /api/my-stack/curated/{marketplace_id}/{plugin_name}`.
|
`PUT /api/my-stack/curated/{marketplace_id}/{plugin_name}`.
|
||||||
|
- **CLI: `agnes store {list,show,install,uninstall,upload,delete}`** and
|
||||||
|
**`agnes my-stack {show,toggle}`** — full analyst-side coverage of the
|
||||||
|
new Store + composition REST surface. Multipart upload helper added to
|
||||||
|
`cli/v2_client.py` (`api_post_multipart` / `api_put_multipart`) so
|
||||||
|
future multipart endpoints don't have to roll their own httpx wiring.
|
||||||
|
|
||||||
### Changed
|
### Changed
|
||||||
- `/admin/marketplaces` admin nav entry moved from the top-level header into
|
- `/admin/marketplaces` admin nav entry moved from the top-level header into
|
||||||
|
|
@ -57,6 +64,32 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C
|
||||||
`/marketplace/info` payload now splits its `plugins` array by `source`,
|
`/marketplace/info` payload now splits its `plugins` array by `source`,
|
||||||
exposing `plugins` (admin) and `store_plugins` (community).
|
exposing `plugins` (admin) and `store_plugins` (community).
|
||||||
|
|
||||||
|
### Fixed
|
||||||
|
- **Stored XSS via `video_url`** (`app/api/store.py`) — `video_url` accepted
|
||||||
|
on `POST/PUT /api/store/entities` is now scheme-validated to `http(s)://`
|
||||||
|
only. Previously a `javascript:` URI flowed through the form field into
|
||||||
|
`store_detail.html`'s `<a href>` and would execute in any viewer's
|
||||||
|
session on click. 400 `invalid_video_url` on bad input.
|
||||||
|
- **ZIP decompression bomb** (`app/api/store.py:_safe_zip_extract`) — the
|
||||||
|
uncompressed-side total of an upload is now capped at 200 MB
|
||||||
|
(`MAX_ZIP_UNCOMPRESSED`); the compressed-side cap (50 MB) alone did not
|
||||||
|
bound the on-disk footprint. 413 `zip_too_large_uncompressed` on
|
||||||
|
oversize.
|
||||||
|
- **Admin authz parity for Store mutations** (`app/api/store.py`,
|
||||||
|
`app/web/router.py`, `app/web/templates/store_detail.html`) —
|
||||||
|
`PUT /api/store/entities/{id}` now permits owner OR admin (matches
|
||||||
|
`DELETE`); the store-detail page passes `is_admin` to the template and
|
||||||
|
gates the Edit/Delete buttons on `is_owner OR is_admin`. Pre-fix, an
|
||||||
|
admin could delete via the API but saw no Edit/Delete affordance in the
|
||||||
|
UI, and could not update non-owned entities at all.
|
||||||
|
- **Cross-owner suffix collision** (`app/api/store.py:create_entity`) —
|
||||||
|
`sanitize_username` is many-to-one (`alice.smith` and `alice_smith`
|
||||||
|
both → `alice-smith`). Two such users uploading entities with the same
|
||||||
|
display `name` produced identical `<name>-by-<username>` suffixes,
|
||||||
|
silently colliding in the served bundle's on-disk paths and the
|
||||||
|
manifest catalog (Claude Code dedupes by `plugin.json`'s `name`).
|
||||||
|
We now refuse the second upload with 409 `conflict_global_suffix`.
|
||||||
|
|
||||||
### Internal
|
### Internal
|
||||||
- Schema **v24 → v25**: adds `store_entities`, `user_store_installs`,
|
- Schema **v24 → v25**: adds `store_entities`, `user_store_installs`,
|
||||||
`user_plugin_optouts`. Auto-migration via `_V24_TO_V25_MIGRATIONS` ladder
|
`user_plugin_optouts`. Auto-migration via `_V24_TO_V25_MIGRATIONS` ladder
|
||||||
|
|
@ -70,6 +103,12 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C
|
||||||
returns, parameterized SQL, no ORM).
|
returns, parameterized SQL, no ORM).
|
||||||
- `app/utils.py:get_store_dir()` — `${DATA_DIR}/store/`.
|
- `app/utils.py:get_store_dir()` — `${DATA_DIR}/store/`.
|
||||||
- `humanbytes` Jinja2 filter on Store detail page (binary KB/MB/GB).
|
- `humanbytes` Jinja2 filter on Store detail page (binary KB/MB/GB).
|
||||||
|
- New CLI command modules: `cli/commands/store.py`, `cli/commands/my_stack.py`.
|
||||||
|
Registered as Typer subapps `agnes store` and `agnes my-stack` in
|
||||||
|
`cli/main.py`. Tests at `tests/test_cli_store.py`.
|
||||||
|
- `tests/test_store_api.py:TestStoreSecurityFixes` — regression suite for
|
||||||
|
F1 (video_url), F2 (zip-bomb), F4 (admin authz parity), F5 (cross-owner
|
||||||
|
suffix collision).
|
||||||
|
|
||||||
## [0.34.0] — 2026-05-04
|
## [0.34.0] — 2026-05-04
|
||||||
|
|
||||||
|
|
|
||||||
104
app/api/store.py
104
app/api/store.py
|
|
@ -27,6 +27,7 @@ import zipfile
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, List, Optional
|
from typing import Any, List, Optional
|
||||||
|
from urllib.parse import urlparse
|
||||||
|
|
||||||
import duckdb
|
import duckdb
|
||||||
from fastapi import (
|
from fastapi import (
|
||||||
|
|
@ -66,6 +67,63 @@ _CHUNK_SIZE = 64 * 1024
|
||||||
_VALID_TYPES = {"skill", "agent", "plugin"}
|
_VALID_TYPES = {"skill", "agent", "plugin"}
|
||||||
_NAME_RE = re.compile(r"^[a-z][a-z0-9-]{0,63}$")
|
_NAME_RE = re.compile(r"^[a-z][a-z0-9-]{0,63}$")
|
||||||
_FRONTMATTER_RE = re.compile(r"^---\s*\n(.*?)\n---", re.DOTALL)
|
_FRONTMATTER_RE = re.compile(r"^---\s*\n(.*?)\n---", re.DOTALL)
|
||||||
|
_ALLOWED_VIDEO_SCHEMES = {"http", "https"}
|
||||||
|
|
||||||
|
# Cap on uncompressed total size of an uploaded ZIP. The compressed-side cap
|
||||||
|
# is MAX_ZIP_SIZE; an attacker could craft a 50 MB ZIP that decompresses to
|
||||||
|
# >>10 GB and DOS the host disk via _safe_zip_extract. We sum infolist()
|
||||||
|
# file_size before extracting and refuse anything above this bound.
|
||||||
|
MAX_ZIP_UNCOMPRESSED = 200 * 1024 * 1024 # 200 MB
|
||||||
|
|
||||||
|
|
||||||
|
def _suffixed_already_taken(
|
||||||
|
conn: duckdb.DuckDBPyConnection,
|
||||||
|
suffixed: str,
|
||||||
|
*,
|
||||||
|
exclude_entity_id: Optional[str] = None,
|
||||||
|
) -> bool:
|
||||||
|
"""Whether any existing entity ships the same display+invocation name.
|
||||||
|
|
||||||
|
The Store namespace is **flat** in Claude Code — two plugins/skills/agents
|
||||||
|
that share a ``name`` collide in the served marketplace catalog (the
|
||||||
|
``manifest_name`` is unique-key for ``/plugin`` lookup) and on-disk inside
|
||||||
|
the ``agnes-store-bundle`` (skills/<suffixed>/SKILL.md is the dir name).
|
||||||
|
|
||||||
|
``sanitize_username`` is many-to-one (``alice.smith`` and ``alice_smith``
|
||||||
|
both → ``alice-smith``), so the per-owner UNIQUE on
|
||||||
|
``(owner_user_id, name)`` does NOT prevent the cross-owner collision.
|
||||||
|
We enforce global uniqueness on ``name || '-by-' || owner_username``
|
||||||
|
here, at upload time, with a clear 409.
|
||||||
|
"""
|
||||||
|
sql = (
|
||||||
|
"SELECT id FROM store_entities "
|
||||||
|
"WHERE name || '-by-' || owner_username = ?"
|
||||||
|
)
|
||||||
|
params: List[Any] = [suffixed]
|
||||||
|
if exclude_entity_id:
|
||||||
|
sql += " AND id != ?"
|
||||||
|
params.append(exclude_entity_id)
|
||||||
|
return bool(conn.execute(sql, params).fetchone())
|
||||||
|
|
||||||
|
|
||||||
|
def _validate_video_url(value: Optional[str]) -> Optional[str]:
|
||||||
|
"""Return the URL if it is a safe http(s) URL, raise 400 otherwise.
|
||||||
|
|
||||||
|
Empty / None passes through as None — video_url is optional. Defends
|
||||||
|
against ``javascript:``, ``data:``, ``vbscript:`` (etc.) URIs that
|
||||||
|
would execute in the viewer's session if rendered inside an ``href``.
|
||||||
|
Jinja2 autoescape only HTML-escapes characters; it does not block URI
|
||||||
|
schemes inside attribute values.
|
||||||
|
"""
|
||||||
|
if value is None:
|
||||||
|
return None
|
||||||
|
s = value.strip()
|
||||||
|
if not s:
|
||||||
|
return None
|
||||||
|
parsed = urlparse(s)
|
||||||
|
if parsed.scheme.lower() not in _ALLOWED_VIDEO_SCHEMES or not parsed.netloc:
|
||||||
|
raise HTTPException(status_code=400, detail="invalid_video_url")
|
||||||
|
return s
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
@ -251,11 +309,24 @@ async def _stream_to_temp(
|
||||||
|
|
||||||
|
|
||||||
def _safe_zip_extract(zf: zipfile.ZipFile, dest: Path) -> None:
|
def _safe_zip_extract(zf: zipfile.ZipFile, dest: Path) -> None:
|
||||||
"""Extract ``zf`` into ``dest`` while rejecting any member whose normalized
|
"""Extract ``zf`` into ``dest`` while rejecting unsafe members.
|
||||||
path would escape ``dest`` (zip-slip guard)."""
|
|
||||||
|
Three guards:
|
||||||
|
|
||||||
|
1. Path traversal (zip-slip) — refuse absolute paths or ``..`` segments.
|
||||||
|
2. Decompression bomb — reject if the sum of declared uncompressed sizes
|
||||||
|
exceeds ``MAX_ZIP_UNCOMPRESSED``. The compressed-side cap
|
||||||
|
(``MAX_ZIP_SIZE``) does not bound the decompressed footprint; a 50 MB
|
||||||
|
ZIP at ratio 1:1000 expands to 50 GB on disk.
|
||||||
|
3. (Note) Python's stdlib ``ZipFile.extractall`` does NOT honor symlink
|
||||||
|
mode bits — symlink entries are written as regular files containing
|
||||||
|
the link target text, not as actual symlinks. So no extra symlink
|
||||||
|
guard is needed for the stdlib path.
|
||||||
|
"""
|
||||||
dest_resolved = dest.resolve()
|
dest_resolved = dest.resolve()
|
||||||
|
total_uncompressed = 0
|
||||||
for member in zf.infolist():
|
for member in zf.infolist():
|
||||||
# Reject absolute paths and any traversal segments.
|
# Path traversal.
|
||||||
member_path = Path(member.filename)
|
member_path = Path(member.filename)
|
||||||
if member_path.is_absolute() or any(part == ".." for part in member_path.parts):
|
if member_path.is_absolute() or any(part == ".." for part in member_path.parts):
|
||||||
raise HTTPException(status_code=422, detail="zip_unsafe_path")
|
raise HTTPException(status_code=422, detail="zip_unsafe_path")
|
||||||
|
|
@ -264,6 +335,19 @@ def _safe_zip_extract(zf: zipfile.ZipFile, dest: Path) -> None:
|
||||||
target.relative_to(dest_resolved)
|
target.relative_to(dest_resolved)
|
||||||
except ValueError:
|
except ValueError:
|
||||||
raise HTTPException(status_code=422, detail="zip_unsafe_path")
|
raise HTTPException(status_code=422, detail="zip_unsafe_path")
|
||||||
|
|
||||||
|
# Decompression bomb. We sum declared sizes — these are advisory
|
||||||
|
# (an attacker can lie) but mismatched values trip Python's own
|
||||||
|
# CRC/size check during read. The pre-extract sum catches the
|
||||||
|
# honest-malicious case (large declared sizes) and is the last
|
||||||
|
# cheap fence before extractall touches the disk.
|
||||||
|
total_uncompressed += int(member.file_size or 0)
|
||||||
|
if total_uncompressed > MAX_ZIP_UNCOMPRESSED:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=413,
|
||||||
|
detail=f"zip_too_large_uncompressed (max {MAX_ZIP_UNCOMPRESSED // 1024 // 1024}MB)",
|
||||||
|
)
|
||||||
|
|
||||||
zf.extractall(dest)
|
zf.extractall(dest)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -729,6 +813,8 @@ async def create_entity(
|
||||||
if category and not is_valid_category(category):
|
if category and not is_valid_category(category):
|
||||||
raise HTTPException(status_code=400, detail="invalid_category")
|
raise HTTPException(status_code=400, detail="invalid_category")
|
||||||
|
|
||||||
|
video_url = _validate_video_url(video_url)
|
||||||
|
|
||||||
# Stream + extract ZIP into a scratch dir.
|
# Stream + extract ZIP into a scratch dir.
|
||||||
tmp, size = await _stream_to_temp(file, MAX_ZIP_SIZE, suffix=".zip")
|
tmp, size = await _stream_to_temp(file, MAX_ZIP_SIZE, suffix=".zip")
|
||||||
try:
|
try:
|
||||||
|
|
@ -757,6 +843,14 @@ async def create_entity(
|
||||||
|
|
||||||
entity_id = uuid.uuid4().hex
|
entity_id = uuid.uuid4().hex
|
||||||
suffixed = suffixed_name(final_name, username)
|
suffixed = suffixed_name(final_name, username)
|
||||||
|
# Global cross-owner check — sanitize_username is many-to-one, so
|
||||||
|
# two emails (alice.smith / alice_smith) can resolve to the same
|
||||||
|
# username and produce the same `<name>-by-<username>` suffix even
|
||||||
|
# when the per-owner UNIQUE passes. The suffixed value drives both
|
||||||
|
# the bundle on-disk dir and the served plugin.json `name`, so a
|
||||||
|
# collision silently last-write-wins. Refuse upfront.
|
||||||
|
if _suffixed_already_taken(conn, suffixed):
|
||||||
|
raise HTTPException(status_code=409, detail="conflict_global_suffix")
|
||||||
plugin_dir = _plugin_dir(entity_id)
|
plugin_dir = _plugin_dir(entity_id)
|
||||||
file_size = _bake_plugin_tree(
|
file_size = _bake_plugin_tree(
|
||||||
type_=type,
|
type_=type,
|
||||||
|
|
@ -820,12 +914,14 @@ async def update_entity(
|
||||||
entity = repo.get(entity_id)
|
entity = repo.get(entity_id)
|
||||||
if not entity:
|
if not entity:
|
||||||
raise HTTPException(status_code=404, detail="entity_not_found")
|
raise HTTPException(status_code=404, detail="entity_not_found")
|
||||||
if entity["owner_user_id"] != user["id"]:
|
if entity["owner_user_id"] != user["id"] and not is_user_admin(user["id"], conn):
|
||||||
raise HTTPException(status_code=403, detail="not_owner")
|
raise HTTPException(status_code=403, detail="not_owner")
|
||||||
|
|
||||||
if category and not is_valid_category(category):
|
if category and not is_valid_category(category):
|
||||||
raise HTTPException(status_code=400, detail="invalid_category")
|
raise HTTPException(status_code=400, detail="invalid_category")
|
||||||
|
|
||||||
|
video_url = _validate_video_url(video_url)
|
||||||
|
|
||||||
new_version: Optional[str] = None
|
new_version: Optional[str] = None
|
||||||
new_size: Optional[int] = None
|
new_size: Optional[int] = None
|
||||||
if file is not None:
|
if file is not None:
|
||||||
|
|
|
||||||
|
|
@ -869,6 +869,7 @@ async def store_detail(
|
||||||
from src.repositories.user_store_installs import UserStoreInstallsRepository
|
from src.repositories.user_store_installs import UserStoreInstallsRepository
|
||||||
from src.store_naming import suffixed_name
|
from src.store_naming import suffixed_name
|
||||||
from app.utils import get_store_dir
|
from app.utils import get_store_dir
|
||||||
|
from app.auth.access import is_user_admin
|
||||||
|
|
||||||
entity = StoreEntitiesRepository(conn).get(entity_id)
|
entity = StoreEntitiesRepository(conn).get(entity_id)
|
||||||
if not entity:
|
if not entity:
|
||||||
|
|
@ -897,6 +898,10 @@ async def store_detail(
|
||||||
user["id"], entity_id
|
user["id"], entity_id
|
||||||
)
|
)
|
||||||
is_owner = entity["owner_user_id"] == user["id"]
|
is_owner = entity["owner_user_id"] == user["id"]
|
||||||
|
# Admin can also Edit/Delete (parity with the API: store.py guards both
|
||||||
|
# mutations on owner OR admin). Without this the store_detail buttons
|
||||||
|
# would be hidden from admin even though they have authority.
|
||||||
|
is_admin = is_user_admin(user["id"], conn)
|
||||||
|
|
||||||
ctx = _build_context(
|
ctx = _build_context(
|
||||||
request,
|
request,
|
||||||
|
|
@ -907,6 +912,7 @@ async def store_detail(
|
||||||
files=files,
|
files=files,
|
||||||
is_installed=is_installed,
|
is_installed=is_installed,
|
||||||
is_owner=is_owner,
|
is_owner=is_owner,
|
||||||
|
is_admin=is_admin,
|
||||||
)
|
)
|
||||||
return templates.TemplateResponse(request, "store_detail.html", ctx)
|
return templates.TemplateResponse(request, "store_detail.html", ctx)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -157,7 +157,7 @@
|
||||||
<div class="breadcrumb"><a href="/store">Store</a> / {{ entity.name }}</div>
|
<div class="breadcrumb"><a href="/store">Store</a> / {{ entity.name }}</div>
|
||||||
|
|
||||||
<div class="detail-hero">
|
<div class="detail-hero">
|
||||||
{% if is_owner %}
|
{% if is_owner or is_admin %}
|
||||||
<div class="owner-actions">
|
<div class="owner-actions">
|
||||||
<a href="#" id="edit-btn">Edit (coming soon)</a>
|
<a href="#" id="edit-btn">Edit (coming soon)</a>
|
||||||
<button class="delete" id="delete-btn">Delete</button>
|
<button class="delete" id="delete-btn">Delete</button>
|
||||||
|
|
|
||||||
|
|
@ -36,6 +36,16 @@ def sanitize_username(email: str) -> str:
|
||||||
|
|
||||||
Raises ``ValueError`` if the local-part sanitizes to an empty string —
|
Raises ``ValueError`` if the local-part sanitizes to an empty string —
|
||||||
callers (the upload endpoint) translate that to a 400.
|
callers (the upload endpoint) translate that to a 400.
|
||||||
|
|
||||||
|
Note: this mapping is **many-to-one** — ``alice.smith@x`` and
|
||||||
|
``alice_smith@x`` both yield ``alice-smith``. The Store namespace is
|
||||||
|
flat in Claude Code, so two such users uploading entities with the
|
||||||
|
same display name would produce identical ``<name>-by-<username>``
|
||||||
|
suffixes and collide in the served marketplace + bundle. The upload
|
||||||
|
endpoint enforces global uniqueness on the suffixed value via
|
||||||
|
``app.api.store._suffixed_already_taken`` and rejects the second one
|
||||||
|
with 409 ``conflict_global_suffix``; the per-owner UNIQUE on
|
||||||
|
``store_entities(owner_user_id, name)`` alone does not catch this.
|
||||||
"""
|
"""
|
||||||
local = email.split("@", 1)[0].lower()
|
local = email.split("@", 1)[0].lower()
|
||||||
s = _SANITIZE_RE.sub("-", local)
|
s = _SANITIZE_RE.sub("-", local)
|
||||||
|
|
|
||||||
|
|
@ -377,6 +377,231 @@ class TestStoreUpload:
|
||||||
assert r.json()["detail"] in {"zip_looks_like_plugin", "zip_looks_like_skill"}
|
assert r.json()["detail"] in {"zip_looks_like_plugin", "zip_looks_like_skill"}
|
||||||
|
|
||||||
|
|
||||||
|
class TestStoreSecurityFixes:
|
||||||
|
"""Regression tests for the three security blockers and one correctness
|
||||||
|
bug found in PR #180 review (F1, F2, F4, F5)."""
|
||||||
|
|
||||||
|
def test_video_url_javascript_scheme_rejected_on_create(self, web_client):
|
||||||
|
"""F1 — `javascript:` URI must not be stored. Otherwise a malicious
|
||||||
|
uploader can pop XSS in any viewer's session via the
|
||||||
|
store_detail "Watch video" link."""
|
||||||
|
_, cookies = _create_user(web_client, "f1a@x.com")
|
||||||
|
r = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("vid1"), "application/zip")},
|
||||||
|
data={"type": "skill", "video_url": "javascript:alert(1)"},
|
||||||
|
cookies=cookies,
|
||||||
|
)
|
||||||
|
assert r.status_code == 400, r.text
|
||||||
|
assert r.json()["detail"] == "invalid_video_url"
|
||||||
|
|
||||||
|
def test_video_url_data_scheme_rejected(self, web_client):
|
||||||
|
_, cookies = _create_user(web_client, "f1b@x.com")
|
||||||
|
r = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("vid2"), "application/zip")},
|
||||||
|
data={"type": "skill", "video_url": "data:text/html,<script>alert(1)</script>"},
|
||||||
|
cookies=cookies,
|
||||||
|
)
|
||||||
|
assert r.status_code == 400
|
||||||
|
assert r.json()["detail"] == "invalid_video_url"
|
||||||
|
|
||||||
|
def test_video_url_https_accepted(self, web_client):
|
||||||
|
_, cookies = _create_user(web_client, "f1c@x.com")
|
||||||
|
r = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("vid3"), "application/zip")},
|
||||||
|
data={"type": "skill", "video_url": "https://www.youtube.com/watch?v=abc"},
|
||||||
|
cookies=cookies,
|
||||||
|
)
|
||||||
|
assert r.status_code == 201, r.text
|
||||||
|
assert r.json()["video_url"] == "https://www.youtube.com/watch?v=abc"
|
||||||
|
|
||||||
|
def test_video_url_javascript_scheme_rejected_on_update(self, web_client):
|
||||||
|
_, cookies = _create_user(web_client, "f1d@x.com")
|
||||||
|
c = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("vid4"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=cookies,
|
||||||
|
)
|
||||||
|
eid = c.json()["id"]
|
||||||
|
u = web_client.put(
|
||||||
|
f"/api/store/entities/{eid}",
|
||||||
|
data={"video_url": "javascript:alert(1)"},
|
||||||
|
cookies=cookies,
|
||||||
|
)
|
||||||
|
assert u.status_code == 400
|
||||||
|
assert u.json()["detail"] == "invalid_video_url"
|
||||||
|
|
||||||
|
def test_zip_bomb_uncompressed_size_rejected(self, tmp_path):
|
||||||
|
"""F2 — _safe_zip_extract must refuse when the sum of declared
|
||||||
|
file_size across infolist() exceeds MAX_ZIP_UNCOMPRESSED, BEFORE
|
||||||
|
extractall touches disk.
|
||||||
|
|
||||||
|
We test the helper directly because Python's ``ZipFile.writestr``
|
||||||
|
rewrites ``ZipInfo.file_size`` to the real payload length, making
|
||||||
|
an end-to-end ZIP-with-fake-size impossible without manual header
|
||||||
|
surgery. The bomb defense is in ``_safe_zip_extract``, so target
|
||||||
|
it directly with a stub ZipFile whose ``infolist()`` returns
|
||||||
|
entries with inflated declared sizes.
|
||||||
|
"""
|
||||||
|
from fastapi import HTTPException
|
||||||
|
|
||||||
|
from app.api import store as store_module
|
||||||
|
|
||||||
|
class _FakeZipFile:
|
||||||
|
def __init__(self, infolist):
|
||||||
|
self._infolist = infolist
|
||||||
|
self.extracted = False
|
||||||
|
|
||||||
|
def infolist(self):
|
||||||
|
return self._infolist
|
||||||
|
|
||||||
|
def extractall(self, dest):
|
||||||
|
# Must not be reached — the guard is supposed to raise
|
||||||
|
# before extractall. Mark and let the caller assert.
|
||||||
|
self.extracted = True
|
||||||
|
|
||||||
|
zi = zipfile.ZipInfo("code-review/SKILL.md")
|
||||||
|
zi.file_size = store_module.MAX_ZIP_UNCOMPRESSED + 1
|
||||||
|
zf = _FakeZipFile([zi])
|
||||||
|
|
||||||
|
try:
|
||||||
|
store_module._safe_zip_extract(zf, tmp_path)
|
||||||
|
except HTTPException as exc:
|
||||||
|
assert exc.status_code == 413
|
||||||
|
assert "zip_too_large_uncompressed" in str(exc.detail)
|
||||||
|
else:
|
||||||
|
raise AssertionError("expected HTTPException 413, got none")
|
||||||
|
assert zf.extracted is False, "guard fired AFTER extractall — bug in fix"
|
||||||
|
|
||||||
|
def test_admin_can_update_non_owned_entity(self, web_client):
|
||||||
|
"""F4 — UPDATE must permit owner OR admin (parity with DELETE)."""
|
||||||
|
from argon2 import PasswordHasher
|
||||||
|
from src.db import get_system_db
|
||||||
|
from src.repositories.users import UserRepository
|
||||||
|
from tests.helpers.auth import grant_admin
|
||||||
|
|
||||||
|
owner_id, owner_cookies = _create_user(web_client, "owner-f4@x.com")
|
||||||
|
c = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("f4-skill"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=owner_cookies,
|
||||||
|
)
|
||||||
|
eid = c.json()["id"]
|
||||||
|
|
||||||
|
ph = PasswordHasher()
|
||||||
|
conn = get_system_db()
|
||||||
|
UserRepository(conn).create(
|
||||||
|
id="adm-f4", email="adm-f4@x.com", name="adm",
|
||||||
|
password_hash=ph.hash("AdminPass1!"),
|
||||||
|
)
|
||||||
|
grant_admin(conn, "adm-f4")
|
||||||
|
admin_token = web_client.post(
|
||||||
|
"/auth/token", json={"email": "adm-f4@x.com", "password": "AdminPass1!"}
|
||||||
|
).json()["access_token"]
|
||||||
|
admin_cookies = {"access_token": admin_token}
|
||||||
|
|
||||||
|
u = web_client.put(
|
||||||
|
f"/api/store/entities/{eid}",
|
||||||
|
data={"description": "moderated by admin"},
|
||||||
|
cookies=admin_cookies,
|
||||||
|
)
|
||||||
|
assert u.status_code == 200, u.text
|
||||||
|
assert u.json()["description"] == "moderated by admin"
|
||||||
|
|
||||||
|
def test_non_owner_non_admin_cannot_update(self, web_client):
|
||||||
|
"""F4 negative — random user still gets 403 on UPDATE."""
|
||||||
|
_, owner_cookies = _create_user(web_client, "owner-f4b@x.com")
|
||||||
|
c = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("f4b-skill"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=owner_cookies,
|
||||||
|
)
|
||||||
|
eid = c.json()["id"]
|
||||||
|
_, intruder_cookies = _create_user(web_client, "intruder-f4@x.com")
|
||||||
|
u = web_client.put(
|
||||||
|
f"/api/store/entities/{eid}",
|
||||||
|
data={"description": "hijack"},
|
||||||
|
cookies=intruder_cookies,
|
||||||
|
)
|
||||||
|
assert u.status_code == 403
|
||||||
|
assert u.json()["detail"] == "not_owner"
|
||||||
|
|
||||||
|
def test_admin_sees_action_buttons_on_store_detail(self, web_client):
|
||||||
|
"""F4 — admin must see Edit/Delete in store_detail UI even when
|
||||||
|
not the owner."""
|
||||||
|
from argon2 import PasswordHasher
|
||||||
|
from src.db import get_system_db
|
||||||
|
from src.repositories.users import UserRepository
|
||||||
|
from tests.helpers.auth import grant_admin
|
||||||
|
|
||||||
|
_, owner_cookies = _create_user(web_client, "owner-ui@x.com")
|
||||||
|
c = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("ui-skill"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=owner_cookies,
|
||||||
|
)
|
||||||
|
eid = c.json()["id"]
|
||||||
|
|
||||||
|
ph = PasswordHasher()
|
||||||
|
conn = get_system_db()
|
||||||
|
UserRepository(conn).create(
|
||||||
|
id="adm-ui", email="adm-ui@x.com", name="adm",
|
||||||
|
password_hash=ph.hash("AdminPass1!"),
|
||||||
|
)
|
||||||
|
grant_admin(conn, "adm-ui")
|
||||||
|
admin_token = web_client.post(
|
||||||
|
"/auth/token", json={"email": "adm-ui@x.com", "password": "AdminPass1!"}
|
||||||
|
).json()["access_token"]
|
||||||
|
admin_cookies = {"access_token": admin_token}
|
||||||
|
|
||||||
|
page = web_client.get(f"/store/{eid}", cookies=admin_cookies)
|
||||||
|
assert page.status_code == 200, page.text
|
||||||
|
# Admin-non-owner sees the owner-actions panel — pre-fix, the
|
||||||
|
# `is_owner` gate hid it. Same gate now reads `is_owner or
|
||||||
|
# is_admin`.
|
||||||
|
assert "owner-actions" in page.text
|
||||||
|
assert 'id="delete-btn"' in page.text
|
||||||
|
|
||||||
|
def test_cross_owner_suffix_collision_rejected(self, web_client):
|
||||||
|
"""F5 — two emails can sanitize to the same username
|
||||||
|
(alice.smith / alice_smith → alice-smith). Both uploading a skill
|
||||||
|
called `code-review` would yield the same `code-review-by-alice-smith`
|
||||||
|
and silently collide in the served bundle + manifest. The upload
|
||||||
|
endpoint must refuse the second one."""
|
||||||
|
_, a_cookies = _create_user(web_client, "alice.smith@x.com")
|
||||||
|
r1 = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("collide"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=a_cookies,
|
||||||
|
)
|
||||||
|
assert r1.status_code == 201, r1.text
|
||||||
|
assert r1.json()["invocation_name"] == "collide-by-alice-smith"
|
||||||
|
|
||||||
|
_, b_cookies = _create_user(web_client, "alice_smith@x.com")
|
||||||
|
r2 = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip("collide"), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=b_cookies,
|
||||||
|
)
|
||||||
|
assert r2.status_code == 409, r2.text
|
||||||
|
assert r2.json()["detail"] == "conflict_global_suffix"
|
||||||
|
|
||||||
|
def test_distinct_suffixes_pass(self, web_client):
|
||||||
|
"""F5 — uploads that yield distinct suffixed names must pass. (Avoid
|
||||||
|
regressing into rejecting all distinct uploads.)"""
|
||||||
|
_, a_cookies = _create_user(web_client, "alice@x.com")
|
||||||
|
_, b_cookies = _create_user(web_client, "bob@x.com")
|
||||||
|
for cookies, skill_name in [(a_cookies, "alpha"), (b_cookies, "beta")]:
|
||||||
|
r = web_client.post(
|
||||||
|
"/api/store/entities",
|
||||||
|
files={"file": ("s.zip", _make_skill_zip(skill_name), "application/zip")},
|
||||||
|
data={"type": "skill"}, cookies=cookies,
|
||||||
|
)
|
||||||
|
assert r.status_code == 201, r.text
|
||||||
|
|
||||||
|
|
||||||
class TestInstallCycle:
|
class TestInstallCycle:
|
||||||
def test_install_uninstall_and_count(self, web_client):
|
def test_install_uninstall_and_count(self, web_client):
|
||||||
# Owner uploads, two other users install, install_count = 2.
|
# Owner uploads, two other users install, install_count = 2.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue