Skip to content

refactor: pre-v3.0.0 over-engineering audit (-3,094 lines) - #224

Open
InterN0te wants to merge 4 commits into
mainfrom
code/ponytail-audit-v3-7175c3
Open

InterN0te wants to merge 4 commits into
mainfrom
code/ponytail-audit-v3-7175c3

Conversation

@InterN0te

@InterN0te InterN0te commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

Two repo-wide over-engineering passes before v3.0.0: dead code, duplicated helpers and hand-rolled stdlib removed across Go (both modules), the Vue frontend, the e2e fixtures and the tooling, followed by the release documentation checked against v2.0.0. Net -3,094 lines over 192 files, no dependency added or removed. Behaviour is unchanged except for the fixes listed below.

First pass (4a1f090)

pkg

  • datevalidation: the offline holiday year list was unreachable from production, and the Coverage type only ever mirrored err != nil. Both are removed (-207 lines).
  • chi's RequestSize replaces the copied LimitRequestSize. ValidateVar, database.Close / CloseRedis / RedisConfig and the package-level logger.Debug are removed.
  • The pool metrics collector is table-driven.
  • New httputil.Validate(w, v) replaces 24 identical validation blocks. New middleware.RequireUserUUID replaces 10 identical user-ID blocks.

internal

  • Shared authorization helpers: ownedCalendar replaces 7 copies, participantOn replaces 9. Same errors and status codes as before.
  • Shared column lists and scan helpers: userColumns / scanUser (9 copies) and passkeyColumns / scanPasskey.
  • Notify: one chat-channel loop and a postJSON helper. A golden test pins the HTML emails byte for byte.
  • robots.txt and the sitemap are built from a const and a table (output byte-identical). Unreachable ICS code paths are removed. Token generators are now one randomHex per package, with the same hex format.

frontend

  • Removed useBuildType, dead store actions and types, and the week-start-day localStorage override that nothing wrote.
  • Views reuse the cached intlFormatters and useEventListener.

Makefile

  • Removed dead targets and the --build-arg BUILD_TYPE the Dockerfile silently ignored.

Second pass (efe55df)

pkg / cmd

  • httputil.DecodeValid replaces 23 decode-then-validate blocks; Response and ValidationError are unexported.
  • Removed database.RedisPinger (a pingFunc in cmd/main.go), the unused locale validator, broadcast.memoryBroker, pkg/models/enums.go and nagerResult.
  • strings.ContainsAny replaces the password regexes; email.Service keeps its Config; test-only constructors are unexported.
  • RegisterQuotaRoutes lives once in cmd/services.go; route limits use middleware.RateLimitConfig directly.

internal

  • writeCalendarError for the calendar error chains; shared issueSessionTokens for login, magic link and password reset; shared completeLogin for password and passkey; ParseNotifyConfig / ParticipantEmailOn.
  • Removed the dead SetEmailAsVerified, GetByHash from the TokenRepository interface, always-nil error returns and unused parameters.

frontend

  • Removed never-rendered spinners, the dead store/API updatePassword, unused props, types and store returns, pass-through wrappers (now storeToRefs), and 8 orphan i18n keys.
  • Native atob / btoa in passkey.ts; reuse of intlFormatters, minutesToTime, apiClient and the @vueuse helpers.
  • The orphan-key test now matches whole quoted keys, so a key can no longer survive as a substring of another.

tooling

  • One scripts/check-compose-env.py replaces three scripts; stale references are removed from the Makefile, CI, .golangci.yml and the docs.

Session fix and test stability (34b041d)

  • A tab opened while another tab was refreshing the session adopted the broadcast token mid-restore, had its own replay voided as stale, and was sent to /login while still signed in. The voided restore now waits for the adoption's /auth/me before the guard decides, with no extra request.
  • The e2e fixtures renew the shared owner token once it nears expiry: a full backend run outlasts the 15-minute token that globalSetup leaves behind.
  • Vitest keeps root-relative asset URLs as strings (file:///logo.png was rejected on Windows), and the locale checks use POSIX paths.

Release documentation (9c40f49)

  • CHANGELOG.md [Unreleased] checked against v2.0.0:
    • adds the missing user-facing changes (make dev, the removed Makefile targets, the password minimum counted in characters, GET /api/v1/auth/status and BOOTSTRAP_REQUIRED, the root Dockerfile in fix(build): restore the migrate-builder stage truncated in two Dockerfiles #124, the webhook URL prefix no longer logged);
    • drops or folds the entries that fixed features new in this release;
    • rewrites the reminder bullets in user terms;
    • puts Fixed before Security, as in v2.0.0.
  • THIRD-PARTY-LICENSES.md:
    • versions are taken from go.mod and package-lock.json;
    • date-fns, date-fns-tz, i18n-iso-countries and world-countries are removed;
    • the Inter font (OFL-1.1) and the new dev dependencies are added.
  • Swagger @version is now 3.0.0. A regenerated spec differs only by the version.

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)

Fixes and the breaking change, all user-facing ones recorded in the CHANGELOG:

  • Breaking: TOTP_PERIOD / TOTP_DIGITS are removed. Any non-default value made MFA enrollment impossible, because validation always used 30s and 6 digits. The unused WEBAUTHN_TIMEOUT is removed too.
  • A passkey login's refresh cookie follows JWT_REFRESH_EXPIRY instead of a fixed 7 days.
  • A tab opened during another tab's refresh no longer lands on the login page while signed in.
  • Deleting a missing user now returns 404 instead of 500.
  • The notify log no longer prints the first 20 characters of webhook URLs. That slice could also panic on a short URL.
  • make dev runs ./cmd/ (./cmd/main.go alone did not compile).
  • The dev preview pages declare their favicon, removing the /favicon.ico 404 that made month grid › renders without console errors flaky.
  • TestSetRefreshToken no longer fails when a second boundary passes during the test.

Related Issue

None. This is pre-release cleanup.

Testing

  • Unit tests added/updated (httputil.Validate, RequireUserUUID, DeleteUser 404, notify HTML golden, cold-tab session adoption, owner-token renewal)
  • Manual testing performed
  • Tested with self-hosted build (go build -tags selfhosted)

Run locally on Windows on the final head:

  • Go: go build, go vet and golangci-lint report 0 issues for both modules on selfhosted and cloud. go test passes against real Postgres 16 and Redis 7 with 0 skips; no -race, as this machine has no C compiler.
  • Swagger: the regenerated spec is identical to main.
  • Frontend: Prettier, vue-tsc and ESLint are clean. Vitest: 978/979. The remaining failure is weekStart › CLDR data, specific to this machine's Node 24.13 ICU (Iceland on Monday); Edge 157 and CI agree with the app's table (Sunday).
  • Playwright, mocked suite: 88/88; the console-errors test passes 20/20 when repeated.
  • Playwright, real backend: 29/29 on two consecutive full runs, with the CI env and both backends (5173 and short-TTL 5174). Acceptance 4 passes 10/10 when repeated in parallel (9/10 failed before the session fix). Run on Edge, because Playwright's Chromium isn't installed locally.

Checklist

  • My code follows the project's code style
  • I have performed a self-review
  • I have added tests that prove my fix/feature works
  • New and existing tests pass locally
  • I have updated documentation if needed

Notes for reviewers

  • The riskiest parts are the shared SQL projections (userColumns, passkeyColumns), the trimmed ICS eventsAboveThresholdQuery, and the initializeAuth change in stores/auth.ts. All are covered by tests, but they deserve a careful look.
  • Small behaviour changes:
    • UpdateAvailability used to skip the minimum-duration check if a redundant second calendar read failed. That read is gone, so the check now always runs.
    • CreateCalendar rejects an invalid user ID before decoding the body.
    • VerifyEmail shows the generic message for a 400 without the JSON error envelope, which the backend always sends on that endpoint.
  • Deliberately not changed: handleAvailabilityError and handleRecurrenceError stay separate, so ErrWeekdayNotAllowed still answers 500 on recurrences; and skip2/go-qrcode stays.
  • Left out on purpose in the first pass:
    • scripts/init-db.sh (documented as hardened)
    • JWT parsing and Redis hook refactors (security- and outage-sensitive)
    • making slog's global default JSON from init
    • cmd.Services (referenced by the ADRs)
    • the email-template loader consolidation (would cross modules)

Someone added 3 commits November 4, 2026 20:58
Repo-wide pass removing dead code, duplicated helpers and hand-rolled
stdlib, behaviour unchanged unless noted below. Net -2,130 lines.

- pkg/datevalidation: drop the offline holiday year list (unreachable from
  production) and the Coverage type, which only ever mirrored err != nil.
- pkg: chi's RequestSize replaces LimitRequestSize; remove ValidateVar,
  database.Close/CloseRedis/RedisConfig and logger.Debug; table-driven
  pool collector; httputil.Validate replaces 24 identical validation blocks.
- internal: shared ownedCalendar / participantOn checks, userColumns and
  passkeyColumns scan helpers, one chat-channel loop and postJSON in notify,
  table-built robots.txt and sitemap, unreachable ICS paths removed.
- frontend: remove useBuildType and dead store actions/types, the unused
  week-start localStorage override; reuse the cached date formatter and
  useEventListener.
- Makefile: drop dead targets and the BUILD_TYPE build-arg the Dockerfile
  ignored.

Fixes found along the way:
- Remove TOTP_PERIOD/TOTP_DIGITS: any non-default value made MFA
  enrollment impossible. Remove the unused WEBAUTHN_TIMEOUT.
- Deleting a missing user returns 404 instead of 500.
- Stop logging the first 20 characters of webhook URLs.
- Make TestSetRefreshToken tolerant of a second boundary.
Second repo-wide pass, behaviour unchanged unless noted below.
Net -1,196 lines.

- pkg/httputil: DecodeValid replaces 23 decode-then-validate blocks;
  Response and ValidationError are unexported.
- internal: writeCalendarError for the calendar error chains, shared
  issueSessionTokens for login, magic link and password reset, shared
  completeLogin for password and passkey, ParseNotifyConfig and
  ParticipantEmailOn; drop dead SetEmailAsVerified, GetByHash from the
  TokenRepository interface, always-nil error returns and unused params.
- pkg: remove RedisPinger (a pingFunc in cmd/main.go), the locale validator,
  memoryBroker, pkg/models/enums.go and nagerResult; strings.ContainsAny
  replaces the password regexes; email.Service keeps its Config; unexport
  test-only constructors.
- cmd: RegisterQuotaRoutes lives once in services.go; route limits use
  middleware.RateLimitConfig directly.
- frontend: drop never-rendered spinners, dead store/API updatePassword,
  unused props, types and store returns, pass-through wrappers (storeToRefs),
  8 orphan i18n keys; native atob/btoa in passkey.ts; reuse intlFormatters,
  minutesToTime, apiClient and @vueuse helpers. The orphan-key test now
  matches whole quoted keys.
- tooling: one check-compose-env.py replaces three scripts; stale refs
  removed from Makefile, CI, .golangci.yml and docs.

Fixes:
- Passkey login sets its refresh cookie through sessioncookie, so it
  follows JWT_REFRESH_EXPIRY instead of a fixed 7 days.
- make dev runs ./cmd/ (./cmd/main.go alone did not compile).
- The dev preview pages declare their favicon, removing the /favicon.ico
  404 that made the console-errors e2e test flaky.
A tab opened while another tab was refreshing the session adopted the
broadcast token mid-restore. Its own replay was then voided as stale,
initializeAuth resolved with no user, and the guard sent a signed-in tab
to /login. The voided restore now waits for the adoption's /auth/me before
the guard decides.

Test fixes so every suite is green:
- e2e fixtures renew the shared owner token once it nears expiry: a full
  backend run outlasts the 15-minute token globalSetup leaves behind.
- Vitest keeps root-relative asset URLs as strings (file:///logo.png was
  rejected on Windows) and the locale checks use POSIX paths.
@InterN0te InterN0te changed the title refactor: pre-v3.0.0 over-engineering audit (-2,130 lines) refactor: pre-v3.0.0 over-engineering audit (-3,088 lines) Nov 7, 2026
CHANGELOG [Unreleased]:
- add the missing user-facing changes: make dev fix, removed Makefile
  targets, password minimum counted in characters, GET /auth/status and
  BOOTSTRAP_REQUIRED, root Dockerfile build (#124), webhook URL prefix
  no longer logged
- drop or fold entries that fixed features new in this release
- rewrite the reminder bullets in user terms
- note that the bundled Compose file does not forward DB_*
- put Fixed before Security, as in v2.0.0

THIRD-PARTY-LICENSES: versions from go.mod and package-lock.json, remove
date-fns, date-fns-tz, i18n-iso-countries and world-countries, add the
Inter font (OFL-1.1) and the new dev dependencies.

Swagger @Version: 3.0.0.
@InterN0te InterN0te changed the title refactor: pre-v3.0.0 over-engineering audit (-3,088 lines) refactor: pre-v3.0.0 over-engineering audit (-3,094 lines) Nov 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant