Display settings: theme (system/light/dark), font family and size, custom CSS #27

Manually merged
marvin8 merged 12 commits from feat/issue-8-display-settings-dark-mode into main 2026-09-12 03:39:03 +00:00
Collaborator

Implements #8 and #6 together on the new per-user settings store.

Settings store (#8 groundwork)

  • settings key/value table keyed by (user_id, key) — mirroring the per-user read_status pattern — via alembic migration + db/settings.py accessors

Theme (#6, with the scope update recorded on the issue)

  • Three modes: Follow system (default), Light, Dark — an explicit user choice overrides the OS preference
  • Implemented in pure CSS: the dark palette (same deuteranopia-safe hues, tuned for dark surfaces, color-scheme: dark included) applies via @media (prefers-color-scheme: dark) unless the user forces light, and via data-theme="dark" when explicitly selected — no JavaScript
  • Theme select on the settings page; data-theme attribute emitted on <html> through a context processor

Typography & custom CSS (#8)

  • Font family: system default or Atkinson Hyperlegible — bundled as woff2 (Regular + Bold, ~24 KB each) in static/fonts/ with its SIL OFL 1.1 license file, verified against google/fonts (OFL-confirmed source); works offline/LAN-only, no CDN
  • Base font size: validated CSS length (px/rem/%), emitted as the html font-size so the whole rem scale follows
  • Custom CSS: stored verbatim and injected after the built-in stylesheet (trusted single-user, no sanitisation — documented decision)
  • Invalid values rejected server-side with flash messages

All 201 tests green (13 new display-settings tests), all 9 nox sessions pass.

Closes #8
Closes #6

Implements #8 and #6 together on the new per-user settings store. ## Settings store (#8 groundwork) - `settings` key/value table keyed by `(user_id, key)` — mirroring the per-user `read_status` pattern — via alembic migration + `db/settings.py` accessors ## Theme (#6, with the scope update recorded on the issue) - Three modes: **Follow system** (default), **Light**, **Dark** — an explicit user choice overrides the OS preference - Implemented in pure CSS: the dark palette (same deuteranopia-safe hues, tuned for dark surfaces, `color-scheme: dark` included) applies via `@media (prefers-color-scheme: dark)` unless the user forces light, and via `data-theme="dark"` when explicitly selected — no JavaScript - Theme select on the settings page; `data-theme` attribute emitted on `<html>` through a context processor ## Typography & custom CSS (#8) - Font family: system default or **Atkinson Hyperlegible** — bundled as woff2 (Regular + Bold, ~24 KB each) in `static/fonts/` with its SIL OFL 1.1 license file, verified against google/fonts (OFL-confirmed source); works offline/LAN-only, no CDN - Base font size: validated CSS length (px/rem/%), emitted as the `html` font-size so the whole rem scale follows - Custom CSS: stored verbatim and injected after the built-in stylesheet (trusted single-user, no sanitisation — documented decision) - Invalid values rejected server-side with flash messages All 201 tests green (13 new display-settings tests), all 9 nox sessions pass. Closes #8 Closes #6
Add display settings: font family, base font size, custom CSS
All checks were successful
/ gitleaks (pull_request) Successful in 18s
/ checks (pull_request) Successful in 1m17s
/ pr-review (pull_request) Successful in 2m52s
9d9be10cce
forgejo-actions left a comment

WuMing

Found 8 issue(s). See inline comments below.

## 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.

**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. <!-- wuming:sha256:269d0746dd7e78643587b866cd29ff4980b118eb8cfb00ae580b4deacbe1821f -->

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.

**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. <!-- wuming:sha256:269d0746dd7e78643587b866cd29ff4980b118eb8cfb00ae580b4deacbe1821f -->
@ -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.

**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. <!-- wuming:sha256:4680a981823d9dd69ed504489c3aab70c18c63a303049f1bc1109dc5253a0fe2 -->
@ -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.

**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. <!-- wuming:sha256:23b71e809afa8342eb95f6ac04582b0b47392f4d0b2d1997c645dadeb8381ca7 -->
@ -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.

**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. <!-- wuming:sha256:698c8876435d1c1a69f768577694a0dd73f22253c965a958ff4306879ef568e6 -->
@ -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.

**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. <!-- wuming:sha256:32070953e02eb0dd3ce3df228a201044b4130409c22815604a02bbcc04a1362e -->
@ -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.

**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. <!-- wuming:sha256:173c41e027bbb72b73fcfd811f6dd496b578c0eede37d8f16dc3ce0434970ef8 -->
@ -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.

**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. <!-- wuming:sha256:fa624f5a778273cdcd6584c3f44db23ad780082441eac8e112f38303b12e9470 -->
🔒 Harden display settings: secret key, CSS breakout guard, atomic saves
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 1m19s
/ pr-review (pull_request) Successful in 3m53s
a442180cb5
Author
Collaborator

tests/test_web.py

The invalid font_family validation branch is not covered by any test…

Fixed — test_save_invalid_font_family_rejected asserts an unknown font family is rejected with a flash message and nothing stored.

[`tests/test_web.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2688) > The invalid font_family validation branch is not covered by any test… ✅ Fixed — `test_save_invalid_font_family_rejected` asserts an unknown font family is rejected with a flash message and nothing stored.
Author
Collaborator

src/yunjin/web/templates/base.html

A03: User-controlled custom_css is stored and rendered verbatim without sanitisation…

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/style on 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/templates/base.html`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2690) > A03: User-controlled custom_css is stored and rendered verbatim without sanitisation… ✅ 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/style` on 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

A01: The route hard-codes user_id=1…

🔴 Not actioned in this PR — same standing decision as PR #24: user_id = 1 is 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`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2691) > A01: The route hard-codes user_id=1… 🔴 Not actioned in this PR — same standing decision as PR #24: `user_id = 1` is 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

Each settings_db.set_setting call commits independently…

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`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2685) > Each settings_db.set_setting call commits independently… ✅ 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

Custom CSS is stored and later injected without sanitisation…

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`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2686) > Custom CSS is stored and later injected without sanitisation… ✅ 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

This new state-changing POST endpoint has no CSRF protection…

🔴 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/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2687) > This new state-changing POST endpoint has no CSRF protection… 🔴 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.
Author
Collaborator

src/yunjin/web/__init__.py

Using a hard-coded default SECRET_KEY when YUNJIN_SECRET_KEY is unset allows anyone to forge Flask session cookies…

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_KEY overrides it. Tests cover generation, stability, per-installation uniqueness, and env precedence.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2684) > Using a hard-coded default SECRET_KEY when YUNJIN_SECRET_KEY is unset allows anyone to forge Flask session cookies… ✅ 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_KEY` overrides it. Tests cover generation, stability, per-installation uniqueness, and env precedence.
Author
Collaborator

src/yunjin/web/__init__.py

A02: Hard-coded default Flask SECRET_KEY…

Fixed — duplicate of the previous finding (identical marker); see the reply above.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2689) > A02: Hard-coded default Flask SECRET_KEY… ✅ Fixed — duplicate of the previous finding (identical marker); see the ✅ reply above.
forgejo-actions left a comment

WuMing

Found 5 issue(s). See inline comments below.

## 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.

**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. <!-- wuming:sha256:213c7bce62abfad520ccac8867fb323cc3ca85e5c95bd307f8d5ebb80202ef7e -->
marvin8 marked this conversation as resolved
@ -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.

**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. <!-- wuming:sha256:f753a9d65f8927e369a855aa14cdfffa18c45c94f4a8386dfd09373550a48e99 -->
@ -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.

**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. <!-- wuming:sha256:1a96327d8599dfe17f9f3c715f9d652b6d06090774409ea0ade8cf72a3b9ec2f -->
@ -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.

**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. <!-- wuming:sha256:b7af1ba06698f955882002440bc870521e363b5beb2a983307ca9eb59224c424 -->
@ -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.

**security** [MEDIUM] User-controlled custom CSS is only escaped for </style> 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. <!-- wuming:sha256:9230c011e5ab0959307ba3bcbca795423507f869340c127a9d82a01c48bc266f -->
🔒 Create secret key file with owner-only permissions
All checks were successful
/ gitleaks (pull_request) Successful in 21s
/ checks (pull_request) Successful in 1m56s
/ pr-review (pull_request) Successful in 3m50s
79e2d22a77
Author
Collaborator

src/yunjin/web/__init__.py

The Flask SECRET_KEY file is created with default file permissions…

Fixed — the key file is now chmod 0600 after writing, with a regression test asserting owner-only permissions.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2711) > The Flask SECRET_KEY file is created with default file permissions… ✅ Fixed — the key file is now `chmod 0600` after writing, with a regression test asserting owner-only permissions.
Author
Collaborator

User-controlled custom CSS is only escaped for breakout…

🔴 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.

> User-controlled custom CSS is only escaped for </style> breakout… 🔴 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

State-changing settings endpoint has no CSRF token or Origin/SameSite protection…

⏸️ 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`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2708) > State-changing settings endpoint has no CSRF token or Origin/SameSite protection… ⏸️ Standing decision — CSRF across all state-changing routes is tracked in #25 (it becomes meaningful together with sessions), not patched per endpoint.
Author
Collaborator

src/yunjin/web/routes/settings.py

Settings are stored per-user, but this route always writes to hard-coded user_id=1…

⏸️ Standing decision — the user_id = 1 pattern (# 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/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2709) > Settings are stored per-user, but this route always writes to hard-coded user_id=1… ⏸️ Standing decision — the `user_id = 1` pattern (`# Phase 6 will add multi-user`) is project-wide across every route and is exactly what #25 will replace with session-derived identity.
Author
Collaborator

src/yunjin/web/__init__.py

Template context processor always reads display settings for hard-coded user_id=1…

⏸️ Standing decision — same as the route-level comment: single context processor + single route will both switch to session-derived identity in #25.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2710) > Template context processor always reads display settings for hard-coded user_id=1… ⏸️ Standing decision — same as the route-level comment: single context processor + single route will both switch to session-derived identity in #25.
forgejo-actions left a comment

WuMing

Found 4 issue(s). See inline comments below.

## 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::TestSettings only exercises set_setting, get_setting, get_settings and delete_setting. In particular the documented all-or-nothing guarantee (the except/conn.rollback() path) is unverified. Add a test that forces a failure partway through the batch (e.g. patch cursor.executemany to raise) and asserts the exception propagates and no partial writes were persisted via get_settings.

**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::TestSettings` only exercises `set_setting`, `get_setting`, `get_settings` and `delete_setting`. In particular the documented all-or-nothing guarantee (the `except`/`conn.rollback()` path) is unverified. Add a test that forces a failure partway through the batch (e.g. patch `cursor.executemany` to raise) and asserts the exception propagates and no partial writes were persisted via `get_settings`. <!-- wuming:sha256:a331e706869656e6632ac50b20146327b2a8e2bda958372230dbf4f40c84ae82 -->
@ -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.

**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. <!-- wuming:sha256:d6772ad0dfb6f7b95f5f4869a13572f845f39f9ad4bc3c312c1e706980a7db63 -->
marvin8 marked this conversation as resolved
@ -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).

**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). <!-- wuming:sha256:ea2341ab2d99310d62c028e1f65a32a28613bfe3ad892a7036c3068a8b5c1d4a -->
@ -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].url

tests [MEDIUM]

This PR changes an existing expectation in a test unrelated to the display-settings work (the aggregate/feed listing test), re-pointing featured_image from sample_media[0] to sample_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.

**tests** [MEDIUM] This PR changes an existing expectation in a test unrelated to the display-settings work (the aggregate/feed listing test), re-pointing `featured_image` from `sample_media[0]` to `sample_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. <!-- wuming:sha256:7a99cf6e137ec732c12b673e6166929000b0512ef78c28c5af163809639b7a98 -->
🔒 Create secret key atomically with 0600, cover save_settings rollback
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 1m31s
/ pr-review (pull_request) Successful in 3m54s
4c5805a0c9
Author
Collaborator

src/yunjin/web/__init__.py

The Flask SECRET_KEY is written with default file permissions before chmod 0600 is applied…

Fixed — the key file is now created atomically via os.open(..., 0o600) (write + close), so it is never readable by others at any point.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2723) > The Flask SECRET_KEY is written with default file permissions before chmod 0600 is applied… ✅ Fixed — the key file is now created atomically via `os.open(..., 0o600)` (write + close), so it is never readable by others at any point.
Author
Collaborator

tests/test_db.py

The new multi-key write helper save_settings is never called directly from tests…

Fixed — TestSaveSettings now covers the happy path (all keys persisted), the all-or-nothing rollback (a deliberate mid-write sqlite3.ProgrammingError via a cast type violation leaves the earlier row unwritten), and overwrite semantics.

[`tests/test_db.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2726) > The new multi-key write helper save_settings is never called directly from tests… ✅ Fixed — `TestSaveSettings` now covers the happy path (all keys persisted), the all-or-nothing rollback (a deliberate mid-write `sqlite3.ProgrammingError` via a `cast` type violation leaves the earlier row unwritten), and overwrite semantics.
Author
Collaborator

tests/test_web.py

This PR changes an existing expectation in a test unrelated to the display-settings work…

🔴 Not a defect — the article-ordering source change landed in merged PR #26 (get_articles_in_aggregate now 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.

[`tests/test_web.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2725) > This PR changes an existing expectation in a test unrelated to the display-settings work… 🔴 Not a defect — the article-ordering source change landed in merged PR #26 (`get_articles_in_aggregate` now 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.
Author
Collaborator

src/yunjin/web/__init__.py

A01: Display settings are always read for hard-coded user_id=1…

⏸️ 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.

[`src/yunjin/web/__init__.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2724) > A01: Display settings are always read for hard-coded user_id=1… ⏸️ 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.
Author
Collaborator

Bug fix: web app now migrates the database on startup

Found in real use (thanks for the fast report — reproduced exactly): create_app never ran Alembic migrations — only the CLI init command does — so an existing deployment's database lacks the new settings table and every page render 500s via the theme context processor. Tests missed it because the fixture calls init_db before create_app.

  • create_app now runs init_db(db_path) (idempotent Alembic upgrade) at startup, so existing deployments self-migrate when the web app starts
  • Regression test simulates a pre-settings database (table dropped, alembic version pinned to the previous revision) and asserts the app migrates it and serves / with 200

After merging, simply starting the web app upgrades the database; no manual yunjin init needed.

## Bug fix: web app now migrates the database on startup Found in real use (thanks for the fast report — reproduced exactly): `create_app` never ran Alembic migrations — only the CLI `init` command does — so an existing deployment's database lacks the new `settings` table and **every page render** 500s via the theme context processor. Tests missed it because the fixture calls `init_db` before `create_app`. - `create_app` now runs `init_db(db_path)` (idempotent Alembic upgrade) at startup, so existing deployments self-migrate when the web app starts - Regression test simulates a pre-settings database (table dropped, alembic version pinned to the previous revision) and asserts the app migrates it and serves `/` with 200 After merging, simply starting the web app upgrades the database; no manual `yunjin init` needed.
🐛 Run database migrations on web app startup
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 1m21s
/ pr-review (pull_request) Successful in 4m17s
47d1027240
forgejo-actions left a comment

WuMing

Found 4 issue(s). See inline comments below.

## 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].

**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]. <!-- wuming:sha256:52fa6f18029bb94195e4ac483919d8f8dd4edbbc236343d8fc63dcdb52e60fe1 -->
@ -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.

**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. <!-- wuming:sha256:1afbee67668e598630f78d4138361f473bcb98c7ffcaf8350d3cc88c7d5d35a9 -->
@ -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.

**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. <!-- wuming:sha256:5f5cb295c683c7717099afc91e37129b11bbe3142dd996b64457e23353ac4650 -->
@ -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.

**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. <!-- wuming:sha256:feacc2aa1b9506de76f4ad160ee4b1848b80f6c7cf5288e017475f996b156bbc -->
🔧 Address WuMing review: settings robustness fixes
All checks were successful
/ gitleaks (pull_request) Successful in 18s
/ checks (pull_request) Successful in 1m17s
/ pr-review (pull_request) Successful in 4m20s
b38705accd
Author
Collaborator

tests/test_db.py

Unsupported SQLite parameter types such as object() raise sqlite3.InterfaceError, not sqlite3.ProgrammingError…

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 to sqlite3.Error, the common base of both, so the rollback path is exercised regardless of sqlite version.

[`tests/test_db.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2739) > Unsupported SQLite parameter types such as object() raise sqlite3.InterfaceError, not sqlite3.ProgrammingError… ✅ 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 to `sqlite3.Error`, the common base of both, so the rollback path is exercised regardless of sqlite version.
Author
Collaborator

alembic/versions/20260912_add_settings_table.py

settings.value is nullable and get_setting returns str | None, but get_settings constructs dict[str, str]…

Fixed — value is now TEXT NOT NULL in 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.

[`alembic/versions/20260912_add_settings_table.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2740) > settings.value is nullable and get_setting returns str | None, but get_settings constructs dict[str, str]… ✅ Fixed — `value` is now `TEXT NOT NULL` in 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.
Author
Collaborator

src/yunjin/web/routes/settings.py

Missing form keys default to empty strings, so a partial POST… will overwrite existing settings…

Fixed — the route now updates only keys present in request.form (validated individually); a partial POST leaves every omitted setting untouched. Regression test: posting only font_size after setting theme=dark keeps the theme.

[`src/yunjin/web/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2738) > Missing form keys default to empty strings, so a partial POST… will overwrite existing settings… ✅ Fixed — the route now updates only keys present in `request.form` (validated individually); a partial POST leaves every omitted setting untouched. Regression test: posting only `font_size` after setting `theme=dark` keeps the theme.
Author
Collaborator

src/yunjin/web/routes/settings.py

CSS length units are ASCII case-insensitive, so valid input such as "16PX" or "1.5REM" is rejected…

Fixed — the pattern is compiled with re.IGNORECASE, with a test for 16PX.

[`src/yunjin/web/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2741) > CSS length units are ASCII case-insensitive, so valid input such as "16PX" or "1.5REM" is rejected… ✅ Fixed — the pattern is compiled with `re.IGNORECASE`, with a test for `16PX`.
🐛 Seed default user on web startup for user-keyed writes
All checks were successful
/ gitleaks (pull_request) Successful in 22s
/ checks (pull_request) Successful in 1m20s
/ pr-review (pull_request) Successful in 3m59s
48617e2d41
Author
Collaborator

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 — the users table is empty in real deployments, so the first user-keyed write (settings, enforced by its foreign key) fails with IntegrityError. The pre-existing article read/unread buttons carried the same latent bug (first click would have 500'd identically).

  • Web startup now seeds the default user (default, id 1) when absent, after migrations — matching the hard-coded user_id = 1 assumption used by every route until #25 replaces it
  • Regression tests: startup seeds user id 1; a settings save on a freshly migrated database succeeds

After pulling, restart the web app and the settings save will work.

## 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 — the `users` table is empty in real deployments, so the first user-keyed write (`settings`, enforced by its foreign key) fails with `IntegrityError`. The pre-existing article read/unread buttons carried the same latent bug (first click would have 500'd identically). - Web startup now seeds the default user (`default`, id 1) when absent, after migrations — matching the hard-coded `user_id = 1` assumption used by every route until #25 replaces it - Regression tests: startup seeds user id 1; a settings save on a freshly migrated database succeeds After pulling, restart the web app and the settings save will work.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## 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.

**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. <!-- wuming:sha256:3f7d6a66bb5627571aa3a0f72773accdebf49bd4da61f4984e639b3b4682c0be -->
@ -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.

**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. <!-- wuming:sha256:6638c2ae56d26abfae3404512a316d56cb8bb562172ba07e4c827875ac984aea -->
@ -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

**security** [MEDIUM] A03: User-controlled custom CSS is stored with only a </style> breakout regex and later rendered raw in a <style> block. This leaves CSS injection intact: CSS can load external resources and exfiltrate attribute values via selector matching. Use a strict CSS sanitizer/allowlist, add a restrictive CSP, or disable custom CSS. <!-- wuming:sha256:81dfcb8275cc4ed3efbc68aa22a35e021ac07f7a7a019883d47ae7ae85fda162 -->
Author
Collaborator

⏸️ Standing decisions (no new substance in this wave)

CSRF · user_id=1 / no auth · custom CSS injection surface

All 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.

### ⏸️ Standing decisions (no new substance in this wave) [`CSRF`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2751) · [`user_id=1 / no auth`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2752) · [`custom CSS injection surface`](https://forge.marvin8.zone/marvin8/yunjin/puls/27#issuecomment-2753) All 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.
🐛 Sweep hardcoded colours into theme variables for dark mode
All checks were successful
/ gitleaks (pull_request) Successful in 20s
/ checks (pull_request) Successful in 1m20s
/ pr-review (pull_request) Successful in 5m44s
7df4ecf3d2
Author
Collaborator

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.

  • Every colour literal now lives in a custom property; 10 new semantic variables added (--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
  • Dark unread cards are now dark blue-tinted (#1f2a3d) under light text (~12:1 contrast); info/warning/danger alert tints and the focus ring have proper dark variants
  • Contrast fixes in both themes: secondary (orange) buttons/badges switch from white text (2.3:1 — failing) to dark text (6.4:1); dark-mode accent backgrounds use dark text (~8-10:1)
  • Deuteranopia: the palette keeps the blue/orange/yellow hue strategy of the approved light theme; the dark trio separates orange/yellow by lightness, and accent-paired text is near-black in both themes
  • New static regression test fails on any future hardcoded colour literal outside variable definitions (print block exempt — white paper is correct there)
## 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. - Every colour literal now lives in a custom property; 10 new semantic variables added (`--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 - Dark unread cards are now dark blue-tinted (`#1f2a3d`) under light text (~12:1 contrast); info/warning/danger alert tints and the focus ring have proper dark variants - Contrast fixes in **both** themes: secondary (orange) buttons/badges switch from white text (2.3:1 — failing) to dark text (6.4:1); dark-mode accent backgrounds use dark text (~8-10:1) - Deuteranopia: the palette keeps the blue/orange/yellow hue strategy of the approved light theme; the dark trio separates orange/yellow by lightness, and accent-paired text is near-black in both themes - New static regression test fails on any future hardcoded colour literal outside variable definitions (print block exempt — white paper is correct there)
🐛 Tag and accent badges use on-accent text colour
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 1m24s
/ pr-review (pull_request) Successful in 6m27s
8abedece6b
Author
Collaborator

Bug fix 4: tag / accent-badge text unreadable in dark mode

Reported: tags (yellow chip backgrounds) showed near-white text in dark mode. Cause: .tag and .badge-accent pair the yellow accent background with color: var(--color-text) — which becomes near-white in dark mode. The sweep converted literal colours but missed these variable pairings.

  • Both rules now use color: var(--color-on-accent) (dark text in dark mode, unchanged near-black in light)
  • New static regression test: any rule with a --color-primary/secondary/accent background must set the matching --color-on-* text colour — this catches the variable-pairing class of bug the literal-colour guard cannot see
## Bug fix 4: tag / accent-badge text unreadable in dark mode Reported: tags (yellow chip backgrounds) showed near-white text in dark mode. Cause: `.tag` and `.badge-accent` pair the yellow accent background with `color: var(--color-text)` — which becomes near-white in dark mode. The sweep converted literal colours but missed these *variable* pairings. - Both rules now use `color: var(--color-on-accent)` (dark text in dark mode, unchanged near-black in light) - New static regression test: any rule with a `--color-primary/secondary/accent` background must set the matching `--color-on-*` text colour — this catches the variable-pairing class of bug the literal-colour guard cannot see
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing 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).

**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). <!-- wuming:sha256:042a5b5b03275c7cf97d5bf43f0d048cda282e6395b07d2486297f18a9eae087 -->
@ -46,0 +95,4 @@
values["font_size"] = font_size
if "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).

**security** [MEDIUM] User-controlled custom_css is stored with only </style> 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). <!-- wuming:sha256:e2038f20d645b0d22e0783667a8d915a8045e7b2bfa472849081086774e9f63d -->
Author
Collaborator

⏸️ Standing decisions

user_id=1 / auth · CSRF · custom CSS surface

Re-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.

### ⏸️ Standing decisions [`user_id=1 / auth`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2762) · [`CSRF`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2762) · [`custom CSS surface`](https://forge.marvin8.zone/marvin8/yunjin/pulls/27#issuecomment-2763) Re-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.
🙈 Ignore the generated secret key file
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 1m30s
/ pr-review (pull_request) Successful in 4m57s
9838f7ee60
marvin8 approved these changes 2026-09-12 03:36:26 +00:00
Dismissed
marvin8 approved these changes 2026-09-12 03:37:12 +00:00
marvin8 manually merged commit 1328fec70a into main 2026-09-12 03:39:03 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
marvin8/yunjin!27
No description provided.