patillacode/piruetas
patillacode/piruetas
1
0
Fork 0
Code Issues Pull requests Projects Releases 20 Packages 1 Wiki Activity Actions

Code review and clean up #18

Merged
patillacode merged 42 commits from chore/code-review-cleanup into main 2026-05-01 10:24:48 +02:00
Conversation 0 Commits 42 Files changed 51 +695 -565
patillacode commented 2026-05-01 10:14:41 +02:00
Owner
Copy link

This PR addresses all findings from the internal code review, covering security, correctness,
maintainability, i18n, accessibility, and tooling.

Security and correctness

  • Enforce ownership check on /uploads/{user_id}/{filename}, authenticated users can't read other users' files
  • Unlink image files from disk when bulk-deleting entries
  • Fix streak counter resetting to 0 on days with no entry yet
  • Fix rate-limit counter incrementing unconditionally on signup, only counts actual failures
  • Fix recovery code scan not stopping after first match
  • Return HTTP 400 on malformed date parameters in scope helpers
  • Add httponly=True to locale cookie
  • Use (user_id, filename) tuple for orphan image detection instead of bare filename

Refactoring and code quality

  • Extract _get_entry, _set_login_csrf_cookie, _count_unused_recovery_codes helpers to eliminate repeated blocks
  • Split app/export.py out of account.py
  • Extract shared theme toggle logic into theme.js, removes duplication between ui.js and landing.js
  • Rename app/auth.py to app/session_token.py to eliminate naming collision with app/routers/auth.py
  • Rename delete-modal to bulk-delete-modal in account/data page to remove duplicate IDs across templates
  • Extract scope_form Jinja macro to remove duplicated export/delete forms
  • Extract export CSS into app/static/css/export-bundle.css, loaded once at module import
  • Log exceptions on admin user delete instead of silently swallowing them

i18n

  • Drop dead i18n keys
  • calendar.js now reads month and weekday names from window.PIRUETAS, added SHORT_WEEKDAY_NAMES and
    get_short_weekday_names() to i18n.py
  • Replace hardcoded English strings in signup.html, admin/tasks.html, admin/users.html, and recovery-codes.js
    with t.* keys
  • Remove dead t is defined fallback guards from templates, t is always injected by ctx()

Accessibility

  • Replace aria-disabled anchor with a proper disabled button for the recovery codes continue action
  • Add aria-disabled="true" to mobile date next-arrow when on today
  • Add role="dialog" and aria-modal="true" to mobile sheet overlays

Tooling and infrastructure

  • Add ruff rules B, UP, and targeted S checks; enforce 99% coverage floor
  • Enable pyright basic type checking
  • Add lint and format check steps to CI
  • Add .dockerignore
  • Pin uv version in Dockerfile via official image copy
  • Add healthcheck and restart policy to compose.yml

Tests and test infrastructure

  • Add autouse fixture to clear get_settings cache between tests
  • Consolidate TEST_SECRET_KEY and _get_auth_cookies helper into tests/e2e/conftest.py
  • Wrap live_server fixture in try/finally to guarantee cleanup
  • Remove redundant networkidle waits after deterministic DOM signals in e2e tests

Minor fixes

  • Add noreferrer to external links in index.html
  • Document intentional breadth of share-link image src rewrite
This PR addresses all findings from the internal code review, covering security, correctness, maintainability, i18n, accessibility, and tooling. **Security and correctness** - Enforce ownership check on `/uploads/{user_id}/{filename}`, authenticated users can't read other users' files - Unlink image files from disk when bulk-deleting entries - Fix streak counter resetting to 0 on days with no entry yet - Fix rate-limit counter incrementing unconditionally on signup, only counts actual failures - Fix recovery code scan not stopping after first match - Return `HTTP 400` on malformed date parameters in scope helpers - Add `httponly=True` to locale cookie - Use (`user_id`, `filename`) tuple for orphan image detection instead of bare filename **Refactoring and code quality** - Extract `_get_entry`,` _set_login_csrf_cookie`,` _count_unused_recovery_codes` helpers to eliminate repeated blocks - Split `app/export.py` out of `account.py` - Extract shared theme toggle logic into `theme.js`, removes duplication between `ui.js` and `landing.js` - Rename `app/auth.py` to `app/session_token.py` to eliminate naming collision with `app/routers/auth.py` - Rename delete-modal to bulk-delete-modal in account/data page to remove duplicate IDs across templates - Extract scope_form Jinja macro to remove duplicated export/delete forms - Extract export CSS into `app/static/css/export-bundle.css`, loaded once at module import - Log exceptions on admin user delete instead of silently swallowing them **i18n** - Drop dead i18n keys - `calendar.js` now reads month and weekday names from `window.PIRUETAS`, added `SHORT_WEEKDAY_NAMES` and `get_short_weekday_names()` to `i18n.py` - Replace hardcoded English strings in `signup.html`, `admin/tasks.html`, `admin/users.html`, and `recovery-codes.js` with `t.*` keys - Remove dead `t` is defined fallback guards from templates, `t` is always injected by `ctx()` **Accessibility** - Replace aria-disabled anchor with a proper disabled button for the recovery codes continue action - Add `aria-disabled="true"` to mobile date next-arrow when on today - Add `role="dialog"` and `aria-modal="true"` to mobile sheet overlays **Tooling and infrastructure** - Add `ruff` rules B, UP, and targeted S checks; enforce 99% coverage floor - Enable pyright basic type checking - Add lint and format check steps to CI - Add `.dockerignore` - Pin `uv` version in `Dockerfile` via official image copy - Add `healthcheck` and restart policy to `compose.yml` **Tests and test infrastructure** - Add autouse fixture to clear get_settings cache between tests - Consolidate `TEST_SECRET_KEY` and `_get_auth_cookies` helper into `tests/e2e/conftest.py` - Wrap live_server fixture in try/finally to guarantee cleanup - Remove redundant `networkidle` waits after deterministic DOM signals in e2e tests **Minor fixes** - Add `noreferrer` to external links in `index.html` - Document intentional breadth of share-link image `src` rewrite
Remove unused imports (auto-fixed by ruff), reflow 6 long
translation strings in i18n.py, argument-wrap a long ctx() call
in account.py, and add # noqa: E501 to four unavoidable long
lines on the locked auth/test surface.
Remove 17 keys per locale (34 dict entries total) that have zero
references across templates and JS, surfaced by the code review:
share, copy_btn, save, written_with, more_entries, landing_more2_h,
landing_more2_p, landing_more5_h, landing_more5_p, landing_hosted_tag,
landing_hosted_h, landing_coming_soon, landing_pricing_hosted_tag,
landing_pricing_monthly, landing_pricing_yearly,
landing_pricing_yearly_note, landing_pricing_cta.
Any logged-in user could read another user's uploaded images by
requesting /uploads/{other_user_id}/{filename} directly. Added an
ownership check (current_user.id == user_id) on the authenticated
branch; raises 403 on mismatch. Added regression tests for cross-user
access and the happy-path own-file 200.
delete/confirm was removing Image DB rows but not the actual files on
disk, leaving them until the next scheduled cleanup run. Now mirrors
journal_delete: unlink each file before deleting the DB row.
Previously the streak walked backward from today, so any day without
an entry at time-of-request showed 0 even if every prior day was filled.
Now starts the walk from yesterday when today has no entry, preserving
the streak until the end of the current day.
Recovery code count query was duplicated four times across account_page,
recovery_codes_page, regenerate_recovery_codes, and change_password.
Entry lookup by user_id + date was duplicated five times across
journal_get, journal_save, journal_delete, journal_share, and
journal_revoke_share.
LOGIN_CSRF_COOKIE set-cookie block (httponly, samesite=strict, secure,
max_age) was duplicated nine times across login_page, login, signup_page,
signup, forgot_password_page, and forgot_password. One place to update
if samesite or max_age ever changes.
date.fromisoformat() in _get_entries_for_scope and _scope_label was
raising ValueError on invalid input, propagating to the 500 handler.
Both functions now catch ValueError and raise HTTPException(400).
Silent except: pass meant a failed shutil.rmtree left orphaned files on
disk with no trace in the logs. Now uses logger.exception(), matching
the pattern in tasks.py.
record_failed_attempt(ip) was called unconditionally before validation,
meaning any error (weak password, mismatched confirm, taken username)
permanently incremented the counter. On a shared NAT, 10 typos would
lock out unrelated users. Counter now lives inside render_error() so it
only fires on actual failed attempts.
CI was running pytest but never ruff, so formatting and lint drift could
land silently on any PR where a developer forgot to run just lint locally.
consume_code was iterating all 12 codes even after finding a match,
burning 11 unnecessary bcrypt rounds per successful recovery login.
Flat filename comparison across all users could theoretically skip
deletion of a file whose name collided with another user's DB record.
Also eliminates a redundant select query by deriving db_filenames from
the already-loaded all_images list.
Locale cookie is server-read only; missing httponly was inconsistent
with every other cookie in the codebase.
Added bugbear (B), pyupgrade (UP), and specific bandit rules covering
dangerous patterns: pickle/marshal, weak hashing, SSL bypass, and shell
injection. B008 (Depends in defaults) globally ignored as it is the
canonical FastAPI pattern. Fixed 9 B904 violations (raise from None in
except blocks) and 1 UP035 (Generator import from collections.abc).
Default typeCheckingMode is off; basic catches real bugs from
pydantic-settings/SQLModel without strict overhead.
Coverage had no fail_under threshold, so it could silently regress.
Set to 99 which matches current measured coverage.
Polls /health every 30s so Docker marks the container unhealthy if the
app becomes unresponsive, enabling dependent services and monitoring to
react correctly.
pip install uv pulled latest uv on every build, making builds
non-reproducible. Now copied directly from the official uv image at a
pinned version, which also removes the pip bootstrap step.
Build context was including tests/, data/, .env, __pycache__, and other
dev artifacts. The final image was fine (Dockerfile uses explicit COPY)
but builds were slower and .env in the context was a footgun.
get_settings() is lru_cache'd so env mutations in one test could leak
into the next. Autouse teardown fixture ensures the cache is always
cleared, making each test start from a fresh settings state.
Moved _get_auth_cookies() to tests/e2e/conftest.py and removed
the duplicate definitions and TEST_SECRET_KEY constant from
test_journal.py and test_data_management.py.
Startup failure or a RuntimeError before yield left os.environ
polluted and get_settings cache dirty. Wrapping in try/finally
ensures teardown always runs.
Inline CSS in _build_html_export made the function hard to read and
the styles impossible to edit without touching Python. Now loaded once
at module import from the static file.
t is always injected by ctx() so the fallback English strings
were unreachable dead code.
Avoided duplicate id="delete-modal" across journal.html and
account/data.html — each page now has a unique modal ID.
href='#' with no disabled state left the forward arrow keyboard-activatable
even when there's no valid next day to navigate to.
An anchor with aria-disabled is still Tab-reachable and does nothing
on Enter, which is confusing. A disabled button is skipped by Tab
and clearly non-interactive until the checkbox is checked.
Eliminated naming collision with app/routers/auth.py. Updated all
9 import sites across app/ and tests/.
Removed hardcoded NAMES dict from calendar.js. Added SHORT_WEEKDAY_NAMES
and get_short_weekday_names() to i18n.py, passed as short_weekdays via
ctx() and exposed as window.PIRUETAS.shortWeekdays in journal.html.
Removed duplicated applyTheme logic from ui.js and landing.js.
theme.js owns all theme state (apply, toggle, localStorage normalization)
and exposes window.PiruetasTheme.init(onApply) for page-specific
label callbacks. Also normalizes legacy 'light' stored value to ''.
account.py was 493 LOC with 6 concerns. Export helpers (HTML building,
markdown rewriting, HTML stripping) and scope query helpers now live in
app/export.py. account.py drops to 354 LOC with clear router-only focus.
The save-toast disappearing already confirms the save completed.
networkidle after wait_for_function can resolve before the fetch
even starts and is misleading. Kept networkidle only after goto().
The regex rewrites all /uploads/ srcs in entry content. Ownership is
enforced downstream by serve_upload, which validates the share_token
against the entry that actually owns the image.
The export and delete forms in account/data.html were identical except
for ID prefix, button class, and label. Extracted into scope_form macro
in macros.html.
Added task_last_run, task_never, task_result, task_run_now,
admin_users_nav, admin_tasks_nav keys to both en and es locales.
Removed hardcoded 'Copied!' and 'Copy all' English strings from
recovery-codes.js. Now read from window.RECOVERY configured in the
template using existing t.copied and t.recovery_codes_copy_all keys.
fix(a11y): add role=dialog and aria-modal=true to mobile sheet overlays
All checks were successful
Test and publish Docker image / test (pull_request) Successful in 36s
Details
Test and publish Docker image / e2e (pull_request) Successful in 3m41s
Details
Test and publish Docker image / build (pull_request) Has been skipped
Details
f98df5f260
patillacode deleted branch chore/code-review-cleanup 2026-05-01 10:24:49 +02:00
patillacode referenced this pull request from a commit 2026-05-01 10:24:51 +02:00
Code review and clean up (#18)
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
patillacode/piruetas!18
No description provided.