1ce3eec7cb
Tutor questions require owned selected attempts; similarity context filters eligibility before ranking. Uploads move to a permission-aware boundary with reference ACLs, canonical legacy aliases, pre-mutation attachment checks and card-aware moderator rules. Nginx stops caching media and supplies native byte ranges. Verified 37 deployed-image backend tests, 69 frontend tests/build, real pgvector/Nginx/browser checks, and two independent reviews.
11 lines
No EOL
3.7 KiB
Markdown
11 lines
No EOL
3.7 KiB
Markdown
## Review — follow-up validation (static, read-only)
|
||
|
||
- **Finding 1 — RESOLVED.** `owns_source` (`backend/app/utils/upload_access.py:115–127`) no longer treats every file as moderator-owned. The moderator grant is now `user.is_moderator and not cards and path.startswith('questions/')` (line 121), so a live card reference (`cards` non-empty from `card_deck_ids`) forces the card ACL path: `card_access` (lines 130–134) requires `user_id == user.id` / `is_shared == 1` / admin — a non-owner moderator fails. Reads (`can_read_upload`, lines 137–152) and attachments (`validate_image_attachments`, lines 183–188) both fall through to `references()` only when the `questions/` and `images/doc_` grants do not apply, so moderator attachment of a card-only file returns 403. Admin still succeeds via `owns_source:116`. An independent question reference grants moderator access because `require_question_access` (`backend/app/utils/quiz_access.py:69–90`) returns for moderators, and `validate_image_attachments:186–187` uses `bool(ids)` for moderators. Covered by `test_card_only_moderator_denial_admin_success_and_question_grant` (`backend/tests/test_related_privacy.py`).
|
||
|
||
- **Finding 2 — RESOLVED.** `stored_upload_path` (`upload_access.py:60–85`) canonicalizes case (`_authority` lowercases host), default port 80/443, query/fragment (dropped via `urlsplit`), percent (`unquote` on path/host), backslash (`replace('\\','/')`), and `..` (`posixpath.normpath`), and rejects anything that does not normalize back under `/uploads/` — so hostile out-of-root paths return `None`. Both classification (`references`/`card_deck_ids`/`document_for_file` compare canonicalized stored values to the route path) and permission checks use this. Anonymous access hits `get_current_user("", db)` → 401 (`uploads.py:37–41`); owner success and peer 404 covered by `test_legacy_aliases_are_canonical_for_classification_and_permissions`. New-write strictness is unchanged: `validate_image_attachments:168–175` rejects query/fragment, `%` in netloc, backslash, and bad ports on absolute URLs, and `local_upload_path:24–31` rejects `\ % ? # :`, control chars, and `..`/empty parts. The image chooser dedupes via `stored_upload_path(row.path) or row.path` in a set (`questions.py:445–447`).
|
||
|
||
- **Orphan/LMS handling — RESOLVED.** `protected` (`uploads.py:35–36`) defaults non-LMS-prefix paths to private, and `document_for_file`/`questions`/`cards` force protection even inside a legacy prefix. Unreferenced `course_files|course_thumbnails|scorm|certificates` files stay anonymous (test `test_orphaned_files...`). Hostile paths can't escape: `upload_file:34–47` rejects symlinks and resolves `is_relative_to(root)`; `stored_upload_path` normalizes `..` before the `/uploads/` prefix check.
|
||
|
||
- **Note (not a blocker):** `owns_source:123–126` grants moderators the `images/doc_N/` branch without checking `cards`, unlike the card-aware `questions/` clause. Currently unreachable as a leak because no code path writes `Flashcard.image_path` (generation at `tasks/quiz_tasks.py:655–660` sets none; `CardEdit` has only front/back), and moderators already hold document access. One-line hardening when cards get images: `if match and not cards:`.
|
||
|
||
- **Residual risks (unverified by me):** I ran no tests/commands/live DB — static only. Parent re-verified 37 backend tests, 69 frontend tests/build, Nginx 206/HEAD/416 with no-store/Vary/SVG sandbox, and pgvector eligibility-before-LIMIT; those remain parent-attested, not re-run here. Full-table scans in `references`/`card_deck_ids`/`document_for_file` per request are an acknowledged perf ceiling (`upload_access.py:88`). The `pedshub_media` cookie is not HttpOnly (deliberate, `frontend/src/utils/token.js`). No blockers remain for the two P1 findings. |