Reader: FreshRSS-style keyboard shortcuts and mark-groups-read-on-view setting #28
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-5-keyboard-shortcuts"
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 #5 plus the companion reading setting requested in tandem.
Keyboard shortcuts (#5)
Progressive enhancement only — a single static
shortcuts.js; every action drives links/forms/routes that already work without JavaScript (the sanctioned minimal exception to the no-JS rule, recorded in the issue findings).j/k(andn/p) — move between items; visible focus indicator, scrolls into viewh— jump to next unread articler— toggle read state of the current article (fetch POST to the existing read/unread routes, badge and state update in place)space— open the current article on its original website (new tab)o/enter— open the current itemc— collapse/expand the combined summary on aggregate pages1/2/3— go to Reader / Feeds / Settingsf1or?— help overlay;escapecloses itq(no refresh route — fetching stays CLI-side),m(no pagination),f/l/s/a/u(no favourites/labels/sharing/search)Server-rendered HTML contract (data attributes for items, article URLs, read state) covered by route tests; JS itself has no test infrastructure — manual checklist in the comments below.
Mark-groups-read-on-view setting (companion, Marvin8 request)
h, hide-read, and the reader index a true unread-workflow226 tests, all 9 nox sessions green.
Closes #5
Manual verification checklist (no JS test infrastructure in this project)
Server-side behaviour is covered by 12 new tests (HTML contract: data attributes, script inclusion, static asset, read-on-view semantics). The JavaScript itself needs eyeballs:
j/kmove the blue outline between group cards, scrolling follows;o/enteropens the focused group;1/2/3jump between pages.j/kwalk the article list;spaceopens the focused article's original site in a new tab;o/enteropens the article detail;rtoggles Read/Unread badge in place (persists after reload);ccollapses/expands the Combined Summary card.hskips to the next group with unread articles.f1or?opens the help overlay;escapeor clicking the backdrop closes it.WuMing
Found 6 issue(s). See inline comments below.
@ -160,0 +187,4 @@if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":for article_id in article_ids:if not users.is_article_read(db, user_id, article_id):users.mark_article_read(db, user_id, article_id)security [LOW]
This causes a database state change during a GET request when mark_read_on_view is enabled. A cross-site image or fetch can trigger /aggregate/ and mark groups read for a logged-in user. State changes should be made via POST with CSRF protection, not as a side effect of GET navigation.
@ -106,6 +108,29 @@ def save_display_settings() -> ResponseReturnValue:return redirect(url_for("settings.manage_tags"))@bp.route("/reading", methods=["POST"])security [MEDIUM]
The new state-changing POST route does not validate a CSRF token. An attacker-controlled page can auto-submit a form to /settings/reading and change the victim's mark_read_on_view setting. Add CSRF protection to all state-changing routes.
@ -109,0 +117,4 @@"""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [LOW]
Settings are read and written using a hard-coded user_id=1 rather than the authenticated session's user identity. This is an insecure direct object reference/missing authorization once multiple users exist, or if any user context is supplied. Use the current authenticated user ID.
@ -0,0 +1,243 @@/**tests [LOW]
New non-trivial source file (key/state handling, fetch-based read toggle, overlay construction, index/
data-*bookkeeping) is added with no automated tests. The route tests added in tests/test_web.py only assert the server-rendered HTML contract (data-shortcut-item,data-article-url,data-read), not any of the JS behaviour (key dispatch, text-entry/modifier guards, move/nextUnread boundaries, toggleRead state flips). If a browser/JS test harness is unavailable, at minimum extract the pure helpers (move/bounds, isTextEntryTarget, help row rendering) so they can be unit-tested in isolation, and record the manual checklist as a tracked artifact.@ -0,0 +125,4 @@if (!element || !element.dataset.articleUrl) {return;}window.open(element.dataset.articleUrl, "_blank", "noopener");security [MEDIUM]
element.dataset.articleUrl is derived from feed/article data and is opened without validating the URL scheme. A malicious feed item could use a javascript: or data: URL, enabling script execution or phishing via the space shortcut. Restrict to http/https schemes.
@ -0,0 +138,4 @@element.getAttribute("href") ||element.dataset.articleUrl;if (url) {window.location.href = url;security [HIGH]
Unsanitized dataset value is assigned to window.location.href. If a feed item can supply a javascript: or other dangerous URL, pressing o/Enter executes script in the application origin (DOM XSS). Validate that the target URL uses http/https before navigation.
src/yunjin/web/static/shortcuts.js✅ Fixed —
isSafeUrl()parses the target with the URL constructor and only navigates forhttp:/https:protocols;javascript:/data:and unparseable values are inert. Applies to botho/enterand the original-site open. Related note: the server-rendered<a href>links carry the same feed-supplied URLs (Jinja-escaped but not scheme-checked) — scheme validation at article ingest is a sensible follow-up beyond this PR's scope.src/yunjin/web/static/shortcuts.js✅ Fixed — same
isSafeUrl()guard applied to thewindow.openpath (spaceshortcut); duplicate of the navigation finding above.src/yunjin/web/routes/reader.py🔴 Not actioned — the state change on view is the feature's defining semantic ("viewed = read"), standard in feed readers; requiring a POST to view a page would break it. Harm is low and reversible (unread toggle), and with no sessions today a cross-site image is indistinguishable from a direct request — once #25 adds authenticated sessions with SameSite cookies, the cross-site-image vector closes automatically. Noted there.
src/yunjin/web/routes/settings.py🔴 Not actioned in this PR — CSRF protection spans every state-changing route and is tracked in #25 (it becomes meaningful together with sessions), consistent with the standing decisions on PRs #24/#27.
src/yunjin/web/routes/settings.py🔴 Not actioned in this PR — the
user_id = 1Phase 6 pattern is project-wide across every route (session-derived identity arrives with #25); this route deliberately matches its siblings.src/yunjin/web/static/shortcuts.js🔴 Acknowledged limitation — the project has no JavaScript test infrastructure, so this PR ships a manual verification checklist (posted above) covering every shortcut. Introducing a JS runner (node/vitest) is a project-level tooling decision to be made separately; the server-rendered HTML contract the JS depends on is under automated test.
WuMing
Found 2 issue(s). See inline comments below.
@ -47,11 +47,13 @@ def manage_tags() -> str:db = get_db()all_tags = tags_db.get_all_tags(db)ignored_tags = tags_db.get_ignored_tags(db)mark_read_on_view = settings_db.get_setting(db, user_id=1, key="mark_read_on_view") == "true"security [MEDIUM]
A01 Broken Access Control: the settings page reads mark_read_on_view for hard-coded user_id=1 instead of the current authenticated user. Any user will see/use user 1's settings once multi-user is added. Use the session user ID.
@ -0,0 +1,257 @@/**tests [LOW]
New ~257-line source file with non-trivial logic (key dispatch, read-state toggling via /article//read and /unread, URL allow-listing, DOM focus management) has no automated tests. The added TestKeyboardShortcutContracts tests only assert the server-rendered HTML data attributes; nothing verifies that the routes the script fetches actually exist/behave as the script expects, nor the guard behaviour (text-entry/modifier keys, unsafe-URL rejection) on the JS side. Consider adding at least route-level tests for the endpoints the shortcuts depend on, or make the manual-only coverage explicit as a documented, tracked gap.
⏸️ Standing positions
user_id=1·no JS test infrastructureBoth re-raise points already answered above: session-derived identity lands with #25 (Phase 6) and applies project-wide; the JavaScript testing gap is a documented project-level tooling decision with a manual verification checklist covering every shortcut. No further action in this PR.
Group-level shortcuts + reading checkbox layout (review feedback)
Shortcuts now work on groups (the reader index is the primary surface):
data-aggregate-id+ group read state (unread_count == 0)ron a focused group toggles it: anything unread → all articles marked read; all read → all marked unread (newPOST /aggregate/<id>/unreadroute); the page reloads so counts, badges, and hide-read filtering re-render server-sidehjumps to the next unread group on the index (and still works per-article inside a group)j/k/o/enteralready worked on groups; aggregate pages keep the article-levelj/k/r/spaceReading settings layout: the checkbox now sits to the left of the title + explanation text (flex row), no longer stacked on top.
Tests: mark-aggregate-unread route (+404), index cards expose group state; all 229 tests and 9 nox sessions green.
WuMing
Found 5 issue(s). See inline comments below.
@ -160,0 +187,4 @@if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":for article_id in article_ids:if not users.is_article_read(db, user_id, article_id):users.mark_article_read(db, user_id, article_id)security [MEDIUM]
A01: When mark_read_on_view is enabled, a plain GET to /aggregate/ modifies read state. This state-changing GET can be triggered cross-site via image/link/prefetch and lacks CSRF protection. Use POST or require an explicit user action with CSRF protection.
@ -241,6 +272,33 @@ def mark_aggregate_read(aggregate_id: int) -> ResponseReturnValue:return redirect(url_for("reader.view_aggregate", aggregate_id=aggregate_id))code [HIGH]
When
mark_read_on_viewis enabled, this redirect causes the unread action to be immediately undone:view_aggregatesees the setting and marks every article in the aggregate read again. Thergroup shortcut and this route therefore become no-ops for unread whenever that setting is on. Redirect to a target that suppresses mark-on-view after unmarking (e.g. a query flag), or otherwise ensureview_aggregatedoes not re-mark the group just after unreading it, and add a regression test.@ -244,0 +287,4 @@"""db = get_db()user_id = 1security [HIGH]
A01: The new unauthenticated route hardcodes user_id=1 and only checks that the aggregate exists, without ownership or session authorization. Any caller can mark all articles in any aggregate unread for the default user. Derive the user from the session and verify aggregate ownership.
@ -0,0 +1,283 @@/**tests [LOW]
New non-trivial client-side logic (~280 lines of keyboard handling, read-state toggling, URL-safety checks, and help overlay) ships with no automated tests. The added tests only assert the server-rendered HTML contract and that the asset is served; the actual key handling,
isSafeUrlguard, and toggle/fetch branches are never exercised. The PR notes there is no JS test infrastructure, but at minimum the pure helpers (isSafeUrl, the move/nextUnread index logic) could be extracted and unit-tested (e.g. with a lightweight runner) rather than relying solely on a manual checklist.@ -0,0 +222,4 @@if (event.defaultPrevented || event.ctrlKey || event.metaKey || event.altKey) {return;}if (isTextEntryTarget(event.target)) {code [MEDIUM]
The keydown handler only ignores inputs, textareas, and selects. If a link or button has focus, pressing Enter or Space is intercepted by the shortcuts (openCurrent/openOriginal),
preventDefault()runs, and the focused control cannot be activated via keyboard. Add a guard for interactive elements such asa,button,summary, and[role="button"](e.g.target.closest("a, button, summary, [role='button']")).src/yunjin/web/routes/reader.py✅ Fixed — exactly right, and a sharp catch.
mark_aggregate_unreadnow redirects to the reader index (preserving thehide_readfilter) instead of the aggregate view, so the freshly-unread group is not immediately re-marked read; under hide-read it reappears, which is the desired workflow. Regression test posts the unread action with mark-read-on-view enabled, follows the redirect, and asserts the articles remain unread.src/yunjin/web/static/shortcuts.js✅ Fixed —
spaceando/enterare no longer intercepted while a link/button (or[role=button]) has focus; the browser's native activation runs instead. Movement and read-toggle keys still work.src/yunjin/web/routes/reader.py⏸️ Standing — as answered for the earlier GET-side-effect comment: the state change on view is the feature's defining semantic ("viewed = read", as in FreshRSS); a POST-to-view model would break it. Low harm, reversible, and the cross-site trigger vector closes with #25 sessions + SameSite cookies.
src/yunjin/web/routes/reader.py⏸️ Standing — the
user_id = 1Phase 6 pattern is project-wide across every route (tracked in #25); this route deliberately matches its siblings.src/yunjin/web/static/shortcuts.js⏸️ Standing — acknowledged limitation; manual verification checklist covers every shortcut and the server-rendered HTML contract is under automated test. A JS runner remains a separate tooling decision.
WuMing
Found 2 issue(s). See inline comments below.
@ -244,0 +287,4 @@"""db = get_db()user_id = 1security [HIGH]
A01: The new route hard-codes the user identity to 1 and performs no authentication or authorization check. Any caller can mark all articles in any aggregate unread for that user. Use the authenticated session's user ID and authorize that the aggregate belongs to that user.
@ -0,0 +1,291 @@/**tests [LOW]
This is a substantial new source file (~291 lines) with non-trivial logic (focus management, read-state toggling via fetch, group read/unread toggling, safe-URL checks, help overlay, key routing). The tests added in this diff only assert the server-rendered data-attribute contract (data-shortcut-item, data-article-id, data-read, etc.) and that the static asset is served; none exercise the JS behavior itself. The PR notes there is no JS test infrastructure and relies on a manual checklist, but a unit test harness (e.g., jsdom + a small test for move/nextUnread/toggleRead/isSafeUrl handlers) would lock in the key handling and the http(s)-only URL guard. Consider adding minimal automated coverage or tracking the manual checklist as a follow-up.
⏸️ Standing positions (both re-raises, no new substance)
user_id=1 / no auth— tracked in #25 (Phase 6), project-wide pattern.no automated JS tests— acknowledged limitation; manual checklist covers every shortcut; runner is a separate tooling decision.Not re-actioning in this PR.
New commits pushed, approval review dismissed automatically according to repository settings
gshortcut: open next unread group (review discussion)Per Marvin8's request,
g(single key, "next Group") opens the next unread group directly — no detour through the reader index, and no capital-letter two-key chord.data-next-unread-urlplus a visible "Next unread group →" button next to the other controls — works without JavaScript, clickable without keyboardgnavigates there; when no unread groups remain, a small toast says "No unread groups" instead of a silent no-opFinal key map:
h= next unread article (within the open group) ·g= open next unread group ·r= toggle read (article, or whole group on the index) ·j/k/n/p= move ·space= original site ·o/enter= open item ·c= summary ·1/2/3= pages ·f1/?= help ·escape= closeNew db query (
get_next_aggregate_with_unread) with ordering/coverage tests; route contract tests for the link and its absence when everything is read.WuMing
Found 4 issue(s). See inline comments below.
@ -160,0 +184,4 @@# renders with the read states captured above, so what you are reading# does not change under you; the group simply counts as read afterwards# (reader index, hide-read filter, next-unread navigation).if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":security [MEDIUM]
A01/A04: Enabling mark_read_on_view makes a GET request mutate read state for all articles. GET state changes can be triggered cross-site via links, prefetch, or crawlers. State changes should be POST-only with CSRF protection or an explicit user action.
@ -241,6 +282,40 @@ def mark_aggregate_read(aggregate_id: int) -> ResponseReturnValue:return redirect(url_for("reader.view_aggregate", aggregate_id=aggregate_id))@bp.route("/aggregate/<int:aggregate_id>/unread", methods=["POST"])security [MEDIUM]
A01: New state-changing POST route lacks visible CSRF protection. An attacker can forge a request to mark any aggregate unread if the user is authenticated. Add CSRF token validation.
@ -244,0 +297,4 @@"""db = get_db()user_id = 1security [HIGH]
A01: Hard-coded user_id=1 with no authenticated session and no ownership check on aggregate_id allows any caller to modify any aggregate's read state (IDOR). Use the current authenticated user and scope aggregate lookup to that user.
@ -0,0 +1,312 @@/**tests [LOW]
This ~313-line keyboard-shortcut script contains the bulk of the new non-trivial logic (key dispatch, modifier/typing guards, read/unread fetch toggle, in-place badge updates, next-unread navigation, help overlay) but has no automated coverage. The only tests touching it verify that the asset is served and that
shortcuts.jsis referenced by templates, which would still pass if every handler were broken. The PR acknowledges this (no JS test infra, manual checklist only) and the server-rendered data attributes are contract-tested, which is good, but consider adding at least a minimal DOM-level test (e.g. jsdom/node or Playwright) for the key-to-action mapping and the input/modifier guard, since these are the most regression-prone parts.⏸️ Standing decisions (all four re-raises, no new substance)
user_id=1 / IDOR·GET state change·CSRF·no JS testsEach was answered in detail earlier in this PR: auth/session identity and CSRF protection are tracked in #25 (Phase 6, where they become meaningful together with sessions); the GET-side-effect is the defining semantic of mark-read-on-view (FreshRSS-standard, low harm, reversible) and its cross-site vector closes with #25 SameSite cookies; the JS test gap is a documented tooling decision with a manual checklist covering every shortcut. Not re-actioning further in this PR — the maintainer has already approved.