Reader: mark groups read and toggle hiding of read items #24
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-18-read-unread"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements issue #18 (from the Todo.md conversion) with the design decisions recorded on the issue:
POST /aggregate/<id>/readmarks every member article read via the existing per-user read-status layer, then redirects back to the aggregate page (POST-redirect-GET — works from a plain form, no JavaScript). The group read state stays computed (all members read), so a group with a newly arrived article shows unread again automatically. The existing per-article read/unread endpoints remain JSON — they are the API #5 keyboard shortcuts will call.?hide_read=1query param with server-rendered Show/Hide-read links (default off). The reader index hides fully-read aggregates; the aggregate view hides read articles and shows an "All articles read" empty state. Toggle state carries through card links. Persistence as a default preference is deferred to #8 settings store.Controls styled with existing button classes; no new CSS variables needed. All behaviour covered by 5 new route tests (31 web tests total, all nox sessions green).
Closes #18
WuMing
Found 3 issue(s). See inline comments below.
@ -122,6 +129,7 @@ def view_aggregate(aggregate_id: int) -> str | tuple[str, int]:"""db = get_db()user_id = 1security [MEDIUM]
A01: The new state-changing endpoint hard-codes user_id=1 instead of deriving the current authenticated user from the session. Any caller can alter the default user's read state, and the aggregate is not checked against the caller's authorization. Use the authenticated user ID and verify access to the aggregate.
@ -198,6 +210,33 @@ def view_article(article_id: int) -> str | tuple[str, int]:)@bp.route("/aggregate/<int:aggregate_id>/read", methods=["POST"])security [LOW]
A01: This state-changing POST endpoint lacks CSRF protection. An attacker could trigger a cross-site request to mark an aggregate as read. Add CSRF token validation, e.g. Flask-WTF CSRFProtect, to all state-changing routes.
@ -201,0 +234,4 @@for article_id in agg_db.get_articles_in_aggregate(db, aggregate_id):users.mark_article_read(db, user_id, article_id)return redirect(url_for("reader.view_aggregate", aggregate_id=aggregate_id))code [MEDIUM]
The redirect after marking a group as read drops any hide_read query parameter. If the user submits the form from /aggregate/?hide_read=1, they are redirected to the default aggregate view and all the just-hidden read articles become visible again. Preserve the current filter, e.g. by reading hide_read from a hidden form field/request.args and passing hide_read=1 to url_for when appropriate; add a test for this path.
src/yunjin/web/routes/reader.py✅ Fixed — the form action now carries the current filter state, the endpoint reads it back and redirects with
hide_read=1preserved. Regression test asserts the redirect Location keepshide_read=1.src/yunjin/web/routes/reader.py🔴 Not actioned in this PR — the hard-coded
user_id = 1(annotated# Phase 6 will add multi-user) is the established project-wide pattern shared by every reader route, including the pre-existing per-article read/unread endpoints. There is no auth/session layer to derive a user from yet; adding authentication is the planned Phase 6 work and would be a project-level change, not something to introduce piecemeal in one endpoint. This endpoint deliberately matches its siblings.src/yunjin/web/routes/reader.py🔴 Not actioned in this PR — CSRF hardening would need to cover every state-changing route (the pre-existing per-article read/unread and feed-management POSTs have the same exposure), and with no auth/cookies in the app the cross-site forgery vector reduces to network reachability of a single-user, LAN-oriented tool. Flagged to the maintainer as a candidate project-level hardening issue to bundle with the Phase 6 multi-user work rather than patching one endpoint.
WuMing
Found 3 issue(s). See inline comments below.
@ -199,2 +211,4 @@@bp.route("/aggregate/<int:aggregate_id>/read", methods=["POST"])def mark_aggregate_read(aggregate_id: int) -> tuple[dict[str, str], int] | Any:code [HIGH]
The annotation uses
Any, but the shown import block only importsTypedDictfrom typing. IfAnyis not imported elsewhere (andfrom __future__ import annotationsis not active), this will raise NameError at import time. Alsotuple[dict[str, str], int] | Anycollapses toAny, disabling useful type checking; prefer a Flask response return type and import it.@ -201,0 +225,4 @@"""db = get_db()user_id = 1security [MEDIUM]
A01: The new POST route always acts as hard-coded user_id=1 and does not derive the user from an authenticated session or verify that the requester is authorised to modify the specified aggregate. Any client can mark any aggregate as read. Use the current authenticated user and check aggregate access.
@ -204,0 +257,4 @@users.mark_article_read(db_conn, user_id=1, article_id=sample_articles[0].id)shown_response = client.get(f"/aggregate/{sample_aggregate.id}")hidden_response = client.get(f"/aggregate/{sample_aggregate.id}?hide_read=1")code [LOW]
This verifies the aggregate endpoint directly with
?hide_read=1, but there is no test that the reader index card links actually carryhide_readthrough to the aggregate URL. Add a test that loads/?hide_read=1, extracts/follows the aggregate link, and asserts the aggregate view hides read articles.src/yunjin/web/routes/reader.py✅ Partially addressed — the NameError premise doesn't hold (
Anyis imported at the top of the module and used byteardown_request), but the substantive point was right:tuple[...] | Anycollapses toAny. The return annotation is nowflask.typing.ResponseReturnValue, the purpose-built Flask type for route returns (redirect +(json, status)tuple both satisfy it), verified with ty.src/yunjin/web/routes/reader.py⏸️ Standing decision — same finding as the 🔴 reply two comments above:
user_id = 1(annotated# Phase 6 will add multi-user) is the project-wide pattern in every reader route; there is no auth layer to derive a user from. Flagged to the maintainer as Phase 6 work, not piecemeal per-endpoint.tests/test_web.py✅ Fixed —
test_index_card_links_carry_hide_readasserts the card href on/?hide_read=1is/aggregate/<id>?hide_read=1, locking the filter carry-through.WuMing
Found 1 issue(s). See inline comments below.
@ -201,0 +226,4 @@"""db = get_db()user_id = 1security [MEDIUM]
A01: The new /aggregate/<aggregate_id>/read POST endpoint has no authentication or authorization check and hard-codes the acting user as user_id=1. Any unauthenticated client can mark every article in any aggregate as read for the default user. Derive the user from an authenticated session and add CSRF protection for this state-changing route.
src/yunjin/web/routes/reader.py⏸️ Standing decisions — this re-raises the two findings declined above with reasons: hard-coded
user_id = 1is the project-wide Phase 6 pattern present in every reader route, and CSRF hardening is a project-level change spanning all state-changing routes, flagged to the maintainer to bundle with Phase 6. No further action planned in this PR.Filed the project-level issue for the two declined findings: the auth/multi-user/CSRF work now lives in a dedicated hardening issue and will be picked up as Phase 6. This PR stays as-is.