Repository navigation
chore: deep clean — ownership scoping, a dependency advisory, a type cycle and session tests - #92
Merged
Merged
Conversation
`deleteExpired` carries an owner check it does not strictly need, and says why: the ids come from one account's own rows, so the scope is redundant — until the day those rows come from somewhere else. Its comment claimed "every other write in this app carries that scope". The two writes in the same file did not. Nothing is reachable from a browser today: the only caller loads the list with `loadWatchlist(locals.user.id)`. This closes the gap while it is still theoretical, because the failure mode is a write that only misbehaves once its caller changes — a shared list or the calendar feed reusing the refresh would be enough — and that is not a bug anyone catches by reading. `ownedRow` moves up beside `watchedStamp`, which is the same shape of utility for the same reason: shared by the actions and by the refresh, and belonging to neither. `actions/shared.ts` keeps the one constant that is genuinely about actions. The isolation test mirrors the one `deleteExpired` already has. Verified it fails against the previous statement and passes against this one.
GHSA-9rgm-9g3h-6x36: devalue below 5.9.1 can be made to hang on malformed input. It arrives twice over, under @sveltejs/kit and under svelte. `npm audit --omit=dev` reports this project clean, and that reading is misleading here rather than reassuring: SvelteKit is a devDependency, but devalue is what serialises `load` data into every rendered page, so the code ships. An override is how this repository already handles a transitive advisory — `cookie` and `esbuild` are pinned the same way. Verified with a production build, not just the test run: devalue sits on the render path, so a type-check passing says little about it.
A throwaway script from the domain move (#62), committed by accident — its own name says `.tmp`. It launches a browser to compare the live OG image against `docs/brand/nextsode-banner.png`, which was a question worth asking once, on the day the domain changed. Nothing references it: no npm script, no workflow, no import. It is the only file in the root that is not configuration, so it reads as part of the project's setup to anyone opening the repository for the first time.
`upcoming` and `watchlist` imported each other. `watchlist` calls `hasUpcoming` and friends; `upcoming` took its input type back from `watchlist` with a `Pick`. The cycle was type-only, so it was erased at compile time and cost nothing at runtime. What it cost was elsewhere: it pointed the lower-level module at the one that consumes it, and it left every tool that looks for cycles with a permanent finding — the kind of standing warning that teaches people to stop reading the output. `UpcomingEntry` now declares its four fields, and `WatchlistEntry` extends it, the way `SavedTitle` already extends `WatchlistEntry`. One definition still, so the two cannot drift, and the dependency runs one way. madge reports no circular dependencies; svelte-check and all 497 tests pass.
`auth.ts` decides who somebody is and had no tests at all — the largest uncovered thing in the project, and the one where being wrong is worst. Thirteen cases over the properties the module claims rather than its calls: that the stored row is a hash and not the token the browser holds, that an expired session fails closed and takes its row with it, that a session whose account was deleted resolves to nobody, that renewal happens past the halfway point and not before, and that signing out on one device leaves the others alone. Against the in-memory engine, like the other server specs, because every one of those claims is about what ends up in a row. Checked they bite: storing the token in clear and skipping the expired-row delete each fail exactly one case and no others. Writing them turned up one harmless detail worth recording — `expiresAt` is a unix timestamp in seconds, so the row drops the milliseconds the returned Date keeps, and the cookie outlives its row by under a second.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A repository-wide quality pass. Most categories came back clean — the two
architecture reviews (#84, #86) did their job, and this reports that rather
than inventing work. Five things were worth changing, each its own commit.
What changed
Ownership scoping on the upkeep writes (
b798f8e)deleteExpiredcarries an owner check it does not strictly need and explainswhy, claiming "every other write in this app carries that scope". The two
writes in the same file did not. Nothing is reachable from a browser today —
the only caller loads the list with
loadWatchlist(locals.user.id)— so thisis defence in depth, closed while it is still theoretical.
ownedRowmoves upbeside
watchedStamp, the same shape of utility shared by the actions and therefresh. New isolation test, verified to fail against the previous statement.
devalue pinned past GHSA-9rgm-9g3h-6x36 (
7424a30)Moderate DoS on malformed input, arriving under both
@sveltejs/kitandsvelte. Worth spelling out whynpm audit --omit=devcalling this projectclean is misleading rather than reassuring: SvelteKit is a devDependency, but
devalue serialises
loaddata into every rendered page, so the code ships.Pinned via
overrides, which is howcookieandesbuildare alreadyhandled here. Verified with a production build, not just tests.
ogcheck.tmp.mjsremoved (67752a2)A throwaway from the domain move (#62), committed by accident — its own name
says
.tmp. No npm script, workflow or import references it. It was the onlynon-configuration file in the repository root.
Type cycle between
upcomingandwatchlistbroken (d1d8737)They imported each other:
watchlistcallshasUpcoming,upcomingtook itsinput type back with a
Pick. Type-only, so it cost nothing at runtime — whatit cost was a permanent finding for every tool that looks for cycles, and a
lower-level module pointing at its own consumer.
UpcomingEntrynow declaresits fields and
WatchlistEntryextends it, the waySavedTitlealreadyextends
WatchlistEntry. madge: no circular dependencies.Session tests (
1c5a4cd)auth.tsdecides who somebody is and had no tests — the largest uncoveredthing here. Thirteen cases over its claims: the stored row is a hash and not
the token, an expired session fails closed and takes its row with it, a
session whose account is gone resolves to nobody, renewal is lazy, signing out
on one device leaves the others. Mutation-checked: storing the token in clear
and skipping the expired-row delete each fail exactly one case.
What was reviewed and left alone
Secrets, auth, OAuth, IDOR, SSRF, injection, headers, cookies — no findings
beyond the one above. No hardcoded secrets in source or in git history;
.envwas never tracked. Every direct dependency is used. No dead exports: knip's
candidates were all false positives against SvelteKit conventions and were
checked by hand. No
any, no@ts-ignore,strict: true. No empty catches.No TODOs,
debuggeror skipped tests. Layer boundaries intact —domain/imports nothing from
server/or the framework. Listeners are cleaned up.The long explanatory comments were deliberately not trimmed: they explain why,
which is the kind this pass is meant to keep.
E2E absent from CI is documented as intentional (it needs local secrets), not
an oversight.
Verification
npm run lint,npm run check(0 errors),npm run test:unit— 510 pass, upfrom 497.
npm run buildsucceeds.npx playwright test— 17/17 (onecold-start flake on the first run, passing on every run since; noted below).
npm audit— 0 vulnerabilities. madge — no cycles. Home exercised in abrowser, no console errors.
No schema changes, so no Turso migration is needed before merging.
Left as debt
auth.tsnow has tests;oauth.ts,rate-limit.tsand the TMDB endpointmodules still have none.
oauth.tsis the next most valuable.e2e/home.e2e.ts:10can miss its 5s timeout when Vite compiles cold. Nottouched: one occurrence is not enough to tell a fragile assertion from a
slow machine, and raising a timeout on a guess hides the difference.
actions.spec.tssits beside theactions/directory rather than insideit. Cosmetic; renaming it would cost more diff than it returns.