Display settings: theme (system/light/dark), font family and size, custom CSS #27
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-8-display-settings-dark-mode"
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 #8 and #6 together on the new per-user settings store.
Settings store (#8 groundwork)
settingskey/value table keyed by(user_id, key)— mirroring the per-userread_statuspattern — via alembic migration +db/settings.pyaccessorsTheme (#6, with the scope update recorded on the issue)
color-scheme: darkincluded) applies via@media (prefers-color-scheme: dark)unless the user forces light, and viadata-theme="dark"when explicitly selected — no JavaScriptdata-themeattribute emitted on<html>through a context processorTypography & custom CSS (#8)
static/fonts/with its SIL OFL 1.1 license file, verified against google/fonts (OFL-confirmed source); works offline/LAN-only, no CDNhtmlfont-size so the whole rem scale followsAll 201 tests green (13 new display-settings tests), all 9 nox sessions pass.
Closes #8
Closes #6
WuMing
Found 8 issue(s). See inline comments below.
@ -34,0 +50,4 @@# Needed for flash messages (session cookies). Override via the environment# in production; a per-installation secret becomes mandatory with the# authentication work tracked in issue #25.app.config["SECRET_KEY"] = os.environ.get("YUNJIN_SECRET_KEY", "yunjin-insecure-dev-secret")code [HIGH]
Using a hard-coded default SECRET_KEY when YUNJIN_SECRET_KEY is unset allows anyone to forge Flask session cookies. Even in the current app flash messages can be tampered with; once auth lands this becomes critical. Generate a random secret during first setup or refuse to start without an explicit key.
security [HIGH]
A02: Hard-coded default Flask SECRET_KEY. Deployments that do not set YUNJIN_SECRET_KEY use a public secret, allowing session cookie forgery. Generate a strong random secret and fail closed when unset.
@ -43,6 +54,43 @@ def manage_tags() -> str:)@bp.route("/display", methods=["POST"])code [LOW]
This new state-changing POST endpoint has no CSRF protection. A malicious website could submit the display-settings form on the user's behalf. Add Flask-WTF CSRF protection or equivalent for all POST routes.
@ -46,0 +63,4 @@"""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [MEDIUM]
A01: The route hard-codes user_id=1 and has no authentication/authorisation check, so any requester can modify the fixed user's settings. Use the authenticated user's ID and verify ownership before applying changes.
@ -46,0 +82,4 @@custom_css = request.form.get("custom_css", "")settings_db.set_setting(db, user_id=user_id, key="theme", value=theme)code [MEDIUM]
Each settings_db.set_setting call commits independently. If one of the later writes fails, the earlier settings persist and the request returns a 500 with a partial update. Wrap all four writes in a single transaction or add a save_settings helper that commits once.
@ -46,0 +71,4 @@return redirect(url_for("settings.manage_tags"))font_family = request.form.get("font_family", "")if font_family not in _VALID_FONT_FAMILIES:code [LOW]
The invalid font_family validation branch is not covered by any test in TestDisplaySettingsRoutes. Add a test analogous to test_save_invalid_theme_rejected to ensure unknown font families are rejected and not stored.
@ -46,0 +80,4 @@flash("Invalid base font size. Use a value like 16px or 1.125rem.", "error")return redirect(url_for("settings.manage_tags"))custom_css = request.form.get("custom_css", "")security [HIGH]
A03: User-controlled custom_css is stored and rendered verbatim without sanitisation. Raw CSS can break out of a style element and execute script (stored XSS). Escape or sanitise the CSS, or reject custom CSS unless a strict allow-list is enforced.
@ -46,0 +85,4 @@settings_db.set_setting(db, user_id=user_id, key="theme", value=theme)settings_db.set_setting(db, user_id=user_id, key="font_family", value=font_family)settings_db.set_setting(db, user_id=user_id, key="font_size", value=font_size)settings_db.set_setting(db, user_id=user_id, key="custom_css", value=custom_css)code [LOW]
Custom CSS is stored and later injected without sanitisation (the test asserts it appears verbatim). This is documented for a trusted single-user deployment, but if multi-user access is ever added or the value becomes attacker-influenced, it enables CSS injection/exfiltration. Consider scoping to the owning user and adding a restrictive CSP or sanitizing.
tests/test_web.py✅ Fixed —
test_save_invalid_font_family_rejectedasserts an unknown font family is rejected with a flash message and nothing stored.src/yunjin/web/templates/base.html✅ Fixed — the real vector was
</style>inside the value closing the style element early;</style>sequences (whitespace/case tolerant) are now neutralised to the CSS escape\3C/styleon save, so the stored CSS can never break out of its<style>element. Regression test feeds a</style><script>payload and asserts no breakout. Threat-model note: with a single operator the input is self-provided (self-XSS at worst); the guard makes it safe regardless. Full sanitisation/user-scoping stays tied to #25 multi-user work as noted in the related comment.src/yunjin/web/routes/settings.py🔴 Not actioned in this PR — same standing decision as PR #24:
user_id = 1is the project-wide Phase 6 pattern (there is no auth layer to derive from); tracked in #25 and applied project-wide there, not piecemeal per endpoint.src/yunjin/web/routes/settings.py✅ Fixed — new
settings_db.save_settings()writes all keys in one transaction (executemany + single commit, rollback on failure) and the route validates every input before saving, so a rejected value persists nothing and an accepted save is all-or-nothing.src/yunjin/web/routes/settings.py✅ Addressed by the breakout guard from the related HIGH finding (see that reply); settings are already stored per-user, so the Phase 6 multi-user work in #25 inherits correct scoping.
src/yunjin/web/routes/settings.py🔴 Not actioned in this PR — CSRF spans every state-changing route in the app and belongs to the #25 auth hardening (it becomes meaningful together with sessions), tracked there rather than patched per endpoint.
src/yunjin/web/__init__.py✅ Fixed — same finding as the duplicate A02 comment. The key is now a strong random value (
secrets.token_hex(32)) generated on first start and persisted next to the database (stable across restarts, unique per installation);YUNJIN_SECRET_KEYoverrides it. Tests cover generation, stability, per-installation uniqueness, and env precedence.src/yunjin/web/__init__.py✅ Fixed — duplicate of the previous finding (identical marker); see the ✅ reply above.
WuMing
Found 5 issue(s). See inline comments below.
@ -13,0 +46,4 @@def _inject_display_settings() -> dict[str, str | None]:"""Expose the current user's display settings to all templates."""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [LOW]
Template context processor always reads display settings for hard-coded user_id=1, so every rendered page uses the same user's settings regardless of authenticated identity. This breaks per-user access control when multi-user support is added. A01: Broken Access Control.
@ -13,0 +39,4 @@return secret_file.read_text(encoding="utf-8")secret_file.parent.mkdir(parents=True, exist_ok=True)secret_file.write_text(secrets.token_hex(32), encoding="utf-8")security [MEDIUM]
The Flask SECRET_KEY file is created with default file permissions (often 0644 depending on umask), which can allow other local users to read the signing key and forge session cookies. Create the file with 0600 permissions using os.open or chmod immediately after writing. A05: Security Misconfiguration.
@ -43,6 +55,52 @@ def manage_tags() -> str:)@bp.route("/display", methods=["POST"])security [MEDIUM]
State-changing settings endpoint has no CSRF token or Origin/SameSite protection. A malicious website can submit a form to change the victim's display settings, including custom CSS. Add CSRF protection and/or validate Origin. A01: Broken Access Control.
@ -46,0 +64,4 @@"""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [LOW]
Settings are stored per-user, but this route always writes to hard-coded user_id=1 without deriving the current user from an authenticated session. Once multiple users exist, this becomes an insecure direct object reference and access control failure. Use the authenticated current user and enforce ownership. A01: Broken Access Control.
@ -46,0 +84,4 @@custom_css = request.form.get("custom_css", "")# Neutralise </style> sequences so the stored CSS can never close its# <style> element and inject markup (CSS backslash escape for '<').custom_css = _STYLE_BREAKOUT_PATTERN.sub(lambda _match: "\\3C/style", custom_css)security [MEDIUM]
User-controlled custom CSS is only escaped for breakout and then rendered in a style block. CSS injection can still load external resources via @import or exfiltrate data with attribute selectors. Use a CSS sanitize/allow-list approach or a dedicated CSS parser. A03: Injection.
src/yunjin/web/__init__.py✅ Fixed — the key file is now
chmod 0600after writing, with a regression test asserting owner-only permissions.🔴 Not actioned in this PR — with a single operator the CSS is self-authored (no attacker-controlled input path), and the remaining vectors (@import, attribute-selector beacons) are inherent to allowing arbitrary user CSS at all; a strict allow-list would remove the feature. The durable fix is a Content-Security-Policy with
style-src 'self' 'unsafe-inline'; img-src 'self' data:— which becomes possible once #21 serves article images locally instead of hotlinking (a CSP restricting images to self would break every remote article image today). That tightening is the natural follow-up to #21; the breakout escape from the earlier HIGH finding remains in place.src/yunjin/web/routes/settings.py⏸️ Standing decision — CSRF across all state-changing routes is tracked in #25 (it becomes meaningful together with sessions), not patched per endpoint.
src/yunjin/web/routes/settings.py⏸️ Standing decision — the
user_id = 1pattern (# Phase 6 will add multi-user) is project-wide across every route and is exactly what #25 will replace with session-derived identity.src/yunjin/web/__init__.py⏸️ Standing decision — same as the route-level comment: single context processor + single route will both switch to session-derived identity in #25.
WuMing
Found 4 issue(s). See inline comments below.
@ -0,0 +28,4 @@conn.commit()def save_settings(conn: sqlite3.Connection, user_id: int, values: dict[str, str]) -> None:tests [LOW]
Coverage gap: the new multi-key write helper
save_settings(used by the /settings/display route) is never called directly from tests —tests/test_db.py::TestSettingsonly exercisesset_setting,get_setting,get_settingsanddelete_setting. In particular the documented all-or-nothing guarantee (theexcept/conn.rollback()path) is unverified. Add a test that forces a failure partway through the batch (e.g. patchcursor.executemanyto raise) and asserts the exception propagates and no partial writes were persisted viaget_settings.@ -13,0 +47,4 @@def _inject_display_settings() -> dict[str, str | None]:"""Expose the current user's display settings to all templates."""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [LOW]
A01: Display settings are always read for hard-coded user_id=1 without checking the authenticated user. Once multi-user is introduced, this becomes an IDOR exposing one user’s settings to all requests. Use the current authenticated user ID.
@ -13,0 +39,4 @@return secret_file.read_text(encoding="utf-8")secret_file.parent.mkdir(parents=True, exist_ok=True)secret_file.write_text(secrets.token_hex(32), encoding="utf-8")security [MEDIUM]
A02: The Flask SECRET_KEY is written with default file permissions before chmod 0600 is applied. Depending on umask the key file may be briefly world-readable, exposing the session signing key and allowing session forgery. Create the file atomically with restrictive permissions, e.g. os.open(..., 0o600).
@ -147,1 +148,3 @@assert row["featured_image"] == sample_media[0].url# Articles order newest-first (issue #19), so the featured image# comes from the newest article's media.assert row["featured_image"] == sample_media[2].urltests [MEDIUM]
This PR changes an existing expectation in a test unrelated to the display-settings work (the aggregate/feed listing test), re-pointing
featured_imagefromsample_media[0]tosample_media[2], and the diff contains no corresponding article-ordering source change. Adapting an assertion to whatever the implementation now produces can mask a real ordering regression. Either revert this edit or move the ordering fix plus this test update into its own change, and pin the intended behaviour explicitly (e.g. assert the returned article order itself) rather than only the derived featured image.src/yunjin/web/__init__.py✅ Fixed — the key file is now created atomically via
os.open(..., 0o600)(write + close), so it is never readable by others at any point.tests/test_db.py✅ Fixed —
TestSaveSettingsnow covers the happy path (all keys persisted), the all-or-nothing rollback (a deliberate mid-writesqlite3.ProgrammingErrorvia acasttype violation leaves the earlier row unwritten), and overwrite semantics.tests/test_web.py🔴 Not a defect — the article-ordering source change landed in merged PR #26 (
get_articles_in_aggregatenow orders newest-first), which post-dates this branch's base for test_web.py purposes. This PR only updates that one stale assertion so it matches the merged behaviour, and the test comment cites issue #19. Nothing here adapts assertions 'to whatever the implementation now produces' beyond that: the value is deterministically produced by #19's ORDER BY.src/yunjin/web/__init__.py⏸️ Standing decision — same as the route-level findings: session-derived identity arrives with #25 (Phase 6); both this context processor and the save route switch to it then.
Bug fix: web app now migrates the database on startup
Found in real use (thanks for the fast report — reproduced exactly):
create_appnever ran Alembic migrations — only the CLIinitcommand does — so an existing deployment's database lacks the newsettingstable and every page render 500s via the theme context processor. Tests missed it because the fixture callsinit_dbbeforecreate_app.create_appnow runsinit_db(db_path)(idempotent Alembic upgrade) at startup, so existing deployments self-migrate when the web app starts/with 200After merging, simply starting the web app upgrades the database; no manual
yunjin initneeded.WuMing
Found 4 issue(s). See inline comments below.
@ -0,0 +98,4 @@)rows = cursor.fetchall()return {row[0]: row[1] for row in rows}code [LOW]
settings.value is nullable and get_setting returns str | None, but get_settings constructs dict[str, str]. If any row has a NULL value, this returns a dict containing None contrary to its type. Either make value NOT NULL or annotate/handle dict[str, str | None].
@ -13,1 +20,4 @@_VALID_THEMES = frozenset({"system", "light", "dark"})_VALID_FONT_FAMILIES = frozenset({"", "atkinson"})_FONT_SIZE_PATTERN = re.compile(r"^\d+(\.\d+)?(px|rem|%)$")code [LOW]
CSS length units are ASCII case-insensitive, so valid input such as "16PX" or "1.5REM" is rejected. Add re.IGNORECASE or normalize the unit before validating.
@ -46,0 +71,4 @@flash("Invalid theme selection.", "error")return redirect(url_for("settings.manage_tags"))font_family = request.form.get("font_family", "")code [MEDIUM]
Missing form keys default to empty strings, so a partial POST (e.g. disabled input, old cached form, or API call) will overwrite existing settings for font_family (and similarly font_size/custom_css) with empty values. Use the current saved settings as defaults for missing fields, or only update keys present in request.form.
@ -429,0 +521,4 @@"""Test that a failure mid-write leaves no partial update behind."""first = users.create_user(db_conn, "dave")with pytest.raises(sqlite3.ProgrammingError):code [HIGH]
Unsupported SQLite parameter types such as object() raise sqlite3.InterfaceError, not sqlite3.ProgrammingError. This test will fail with the current expectation. Change to pytest.raises(sqlite3.InterfaceError) (or sqlite3.Error) to actually exercise the rollback path.
tests/test_db.py✅ Addressed — on this build (sqlite3 2.6+ / CPython 3.13) the observed exception is indeed
sqlite3.ProgrammingError(the test was green locally and in CI), but the version-dependent distinction is real. Widened the expectation tosqlite3.Error, the common base of both, so the rollback path is exercised regardless of sqlite version.alembic/versions/20260912_add_settings_table.py✅ Fixed —
valueis nowTEXT NOT NULLin the migration. The migration is unreleased (no deployed database has the table yet — that was the startup crash fixed in this PR), so amending it in place is safe. Accessors only ever write strings, including empty ones.src/yunjin/web/routes/settings.py✅ Fixed — the route now updates only keys present in
request.form(validated individually); a partial POST leaves every omitted setting untouched. Regression test: posting onlyfont_sizeafter settingtheme=darkkeeps the theme.src/yunjin/web/routes/settings.py✅ Fixed — the pattern is compiled with
re.IGNORECASE, with a test for16PX.Bug fix 2: settings save fails on the user foreign key
Second real-deployment crash, same family: the routes write
user_id = 1, but nothing ever created that user outside the test fixtures — theuserstable is empty in real deployments, so the first user-keyed write (settings, enforced by its foreign key) fails withIntegrityError. The pre-existing article read/unread buttons carried the same latent bug (first click would have 500'd identically).default, id 1) when absent, after migrations — matching the hard-codeduser_id = 1assumption used by every route until #25 replaces itAfter pulling, restart the web app and the settings save will work.
WuMing
Found 3 issue(s). See inline comments below.
@ -43,6 +55,57 @@ def manage_tags() -> str:)@bp.route("/display", methods=["POST"])security [MEDIUM]
A01: State-changing POST route /settings/display has no CSRF protection. A malicious website can force a victim's browser to submit display settings, including custom CSS, for the hard-coded default user. Add CSRF token validation (Flask-WTF CSRFProtect) or otherwise enforce same-site/origin checks.
@ -46,0 +67,4 @@user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [HIGH]
A01/A07: The handler hard-codes user_id = 1 and performs no authentication or session authorization. Any client able to reach the web UI can modify the default user's settings. Derive user_id from a verified authenticated session and enforce ownership checks.
@ -46,0 +98,4 @@custom_css = request.form["custom_css"]# Neutralise </style> sequences so the stored CSS can never close its# <style> element and inject markup (CSS backslash escape for '<').values["custom_css"] = _STYLE_BREAKOUT_PATTERN.sub(lambda _match: "\\3C/style", custom_css)security [MEDIUM]
A03: User-controlled custom CSS is stored with only a breakout regex and later rendered raw in a
⏸️ Standing decisions (no new substance in this wave)
CSRF·user_id=1 / no auth·custom CSS injection surfaceAll three re-raise findings already addressed in detail above: CSRF and authentication/hard-coded-user are tracked in #25 (Phase 6) where they become meaningful together; the custom-CSS surface carries the documented single-operator trade-off with the
</style>breakout guard in place and the CSP tightening queued behind #21. Not re-actioning in this PR.Bug fix 3: dark mode readability (hardcoded colours swept into variables)
Reported: front-page group cards rendered light-on-light in dark mode. Root cause: the stylesheet carried 26 hardcoded colour literals in rule bodies (unread-card background
#f0f5ff, read-status badge#e6f2ff, alert tints, button hover colours, white text on accent backgrounds, tag borders) which the dark variable blocks could never reach — light surfaces stayed under light text. The planned variable sweep had not been done; it is now.--color-on-primary/secondary/accent,--color-primary-hover,--color-secondary-hover,--color-accent-border,--color-unread-bg,--color-info-bg,--color-danger-bg,--color-warning-bg,--focus-ring-color), defined in light, system-dark, and forced-dark blocks#1f2a3d) under light text (~12:1 contrast); info/warning/danger alert tints and the focus ring have proper dark variantsBug fix 4: tag / accent-badge text unreadable in dark mode
Reported: tags (yellow chip backgrounds) showed near-white text in dark mode. Cause:
.tagand.badge-accentpair the yellow accent background withcolor: var(--color-text)— which becomes near-white in dark mode. The sweep converted literal colours but missed these variable pairings.color: var(--color-on-accent)(dark text in dark mode, unchanged near-black in light)--color-primary/secondary/accentbackground must set the matching--color-on-*text colour — this catches the variable-pairing class of bug the literal-colour guard cannot seeWuMing
Found 2 issue(s). See inline comments below.
@ -46,0 +64,4 @@"""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [MEDIUM]
The display-settings save handler hard-codes user_id=1 and performs no authentication or authorization check. The POST endpoint also has no CSRF token, so any website or client that can reach the app can modify the default user's settings. Add authentication, server-side ownership checks, and CSRF protection before applying writes (A01).
@ -46,0 +95,4 @@values["font_size"] = font_sizeif "custom_css" in request.form:custom_css = request.form["custom_css"]security [MEDIUM]
User-controlled custom_css is stored with only sequences neutralised and rendered as inline CSS. This permits arbitrary CSS injection, including UI redress, external resource loading, and potential attribute-based data exfiltration. Use an allowlist/CSS sanitizer, add CSP, and restrict this feature to authenticated/trusted users (A03).
⏸️ Standing decisions
user_id=1 / auth·CSRF·custom CSS surfaceRe-raises of findings addressed in detail earlier in this PR: auth/CSRF are tracked in #25 (Phase 6) and the custom-CSS trade-off is documented with the CSP tightening queued behind #21. Not re-actioning.