Harden core platform: data ownership, upload/auth safety, reader reliability, and perf - #26
Merged
Merged
Conversation
…k schema The search_vector tsvector column and four GIN/trgm indexes exist in the database via a raw SQL migration but were never declared in the Drizzle schema. As a result any future `db:push` (a wired script) would treat them as drift and silently drop them, degrading or breaking search. Declare the generated column and indexes in `links.ts` and omit `searchVector` from the public select schemas so it cannot leak into API responses. The migration snapshot is reconciled so `db:generate` is a clean no-op rather than emitting duplicate DDL. No database change intended; this only aligns the schema source with the existing database.
…/search The server create-link schema required a tag `id` that the web client never sent and the handler never used (tags are resolved by name + group). Remove the dead field and add a colocated contract test that fails on web/server request-shape drift. Also stop selecting `textContent` from the list and search queries and omit it from the list response schema. The heavy column is only needed by the reader detail view; returning it on every list/search row inflated payloads for no consumer.
…ions The highlight and tag-link enrichment queries inside findUpcoming and search loaded related rows by link id alone, so a request for one user could surface another user's highlights or tag associations. Add the matching userId filter to every enrichment query, matching findMany. getHomeSuggestions issued six sequential reads, two of which (short/long counts) selected every matching readingTime row only to count and sum them in JS. Collapse those into a single conditional-aggregation query and run the four independent reads concurrently via Promise.all so home-view latency no longer grows with the sum of round-trips. Return shape is unchanged. Add repository tests covering cross-user enrichment scoping and the short/long count split.
…p per-image delay Store extracted article body images under user-scoped keys (user-<id>/articles/...) and ownership-check them through the files route, so one user can never read another's extracted images. Block the legacy shared/articles/* prefix at the files route while leaving other shared uploads readable. Remove the unconditional 500ms sleep inside the per-image upload loop; it burned ~25s of the job budget on a 50-image article with no benefit. Make processing resumable: stamp processingStartedAt on the pending to processing flip, and reset a stale processing row back to pending on job start instead of returning skipped, so a hard-crashed worker no longer leaves a link stranded. Add behavior tests for user-scoped uploads, the removed delay, stale-processing recovery, and the legacy shared gate.
…n first admin atomically Rate limiting keyed client identity on forwarded headers unconditionally, so a spoofable x-forwarded-for let a single client evade limits (or frame others). Trust x-forwarded-for/x-real-ip only behind an explicit BEHIND_PROXY flag or a TRUSTED_PROXIES allowlist, falling back to the socket peer IP otherwise. Replace the unused RATE_LIMIT_WINDOW_MS/MAX env vars, which no limiter read, with the new proxy-trust flags and document them for self-hosters. Validate uploaded images by sniffing the buffer magic bytes rather than trusting the client filename or Content-Type, and stop trusting response Content-Type when downloading article images. Make first-admin assignment atomic: the initial role check plus insert runs in a serializable transaction with retry, so concurrent first registrations settle on exactly one admin instead of racing to create several. Add tests for header trust in both modes, image-byte rejection, and concurrent first-user registration.
…ent retries Reading progress was saved only through the react-query PATCH mutation, which the browser can cancel during page unload, silently dropping the final position. Route the unload save through a dedicated fetch PATCH with keepalive: true and no mutation-cache work, while normal in-app navigation keeps the cached mutation path. Stop retrying non-idempotent mutations: react-query mutations no longer retry by default, and the HTTP client retries only on 429/5xx, excluding 401/403 so a failed auth fails immediately instead of cascading. Add reader hook tests for throttling, localStorage persistence, and the unload save contract, and paragraph-tracker tests with a mocked IntersectionObserver.
The article-url-guard and browser-request-guard wrappers looked like duplicates but are intentionally separate: article navigation reports a typed failure to the extraction workflow, while browser subresource interception needs a boolean per request. Document that layering inline so the two are not collapsed by mistake. Add a `pnpm audit --prod --audit-level=high` step to CI. It runs non-blocking for now while the existing dependency baseline is remediated; flip it to gating once that baseline is clean.
Existing deployments have article body images stored under the shared/articles/* prefix from before per-user storage. The hard 403 introduced for ownership safety would break those articles on upgrade. Keep the secure default (deny shared/articles/*) but add an ALLOW_LEGACY_SHARED_ARTICLES env flag that serves the legacy prefix while on, so older articles keep rendering during migration. New extractions always use the ownership-checked user-<id>/articles/* path regardless of the flag; the two are separate key prefixes and code paths. Extract the access decision into an env-free predicate (isFileAccessForbidden) following the rate-limit convention so both flag states are unit-tested without touching process env. Document the flag for self-hosters.
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
1 participant
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.
Summary
A cross-stack hardening pass over security, correctness, and performance. Each change is an independent, verifiable fix split into concern-based commits so they can be reviewed and reverted individually. No product behavior is added; this makes existing behavior safer, more correct, and faster.
What changed
Data integrity & ownership
search_vectorcolumn and four GIN/trgm indexes in the Drizzle link schema. They existed only via raw SQL, so any futuredb:pushwould have silently dropped them and broken search. Migration snapshot reconciled sodb:generateis a clean no-op.findUpcoming/searchbyuserId. A request for one user could previously surface another user's highlights or tag associations.idthe client never sent and the handler never used. Added a colocated web↔server contract test that fails on request-shape drift.Performance
textContentfrom list/search responses (heavy column needed only by the reader detail view) across the query, response schema, and web types.getHomeSuggestionsnow runs its independent reads concurrently (Promise.all) and collapses the short/long counts into one conditional-aggregation query instead of two full scans counted in JS.Extraction worker
user-<id>/articles/...) and ownership-checked by the files route; the legacyshared/articles/*prefix is blocked.processingrow is reset topendingon job start instead of returningskipped, so a crashed worker no longer strands a link.Auth & upload security
x-forwarded-for/x-real-iponly behind an explicitBEHIND_PROXYflag orTRUSTED_PROXIESallowlist; otherwise falls back to the socket peer IP. (Replaces the unusedRATE_LIMIT_WINDOW_MS/MAXenv vars.)Content-Type; article-image download no longer trusts responseContent-Type.Reader reliability
keepalivePATCH (previously dropped if the browser cancelled the mutation during teardown); normal navigation keeps the cached mutation path.retry: 0); the HTTP client retries only on 429/5xx, excluding 401/403.DX / CI
article-url-guardvsbrowser-request-guardlayering inline.pnpm audit --prod --audit-level=highto CI (report-only for now — see Risks).Verification
pnpm typecheck,pnpm lint,pnpm build— green.pnpm --filter server test— 42 files, 286 tests.pnpm --filter web test— 31 files, 141 tests.Risks & deployment notes
BEHIND_PROXY/TRUSTED_PROXIESdefault to untrusted — rate limiting will key on the socket peer IP, so if you run behind a reverse proxy you must set the flag or all clients will share one bucket. Documented inREADME.mdanddocs/SELF_HOSTING.md.shared/articles/*article body images (from before per-user storage) are denied by default. New images always use the ownership-checkeduser-<id>/articles/*path. To keep older articles rendering during migration, setALLOW_LEGACY_SHARED_ARTICLES=trueuntil those assets are backfilled, then unset it. Orphaned-asset cleanup remains out of scope.continue-on-error) until the existing dependency baseline (17 high, 1 critical) is remediated in a follow-up; flip to gating once clean.Deferred / follow-ups
chore(deps)PR after this merges).