Release: staging -> master - #11
Merged
Merged
Conversation
The rebuild targeted Next.js 15; the target audience is fullstack/backend roles, and a Next.js app whose backend is a directory of Server Actions doesn't show what those interviews probe — HTTP semantics, an explicit API contract, middleware ordering, integration tests against real routes. New spec (2026-07-16-express-react-rebuild-design.md) supersedes and replaces the 2026-07-12 Next.js design, which is deleted here along with its P1 plan; both remain in git history on staging. Everything worth keeping was migrated into the new spec rather than left behind a stale reference: the data model, Redis design, regression checklist, free-tier constraints, and Compose/CI topology all carry forward. Substantive changes beyond the stack swap: - apps/api serves the built SPA from one origin — no CORS, no cross-origin cookie problem, one Render service. apps/realtime stays separate and therefore keeps the signed-ticket handshake. - Sessions: express-session + connect-redis, httpOnly cookie. SameSite=Lax suffices for CSRF *because* of the single origin — recorded as a dependency, not an assumption. - Content gating survives: it moves from "Server Component omits the field" to "the API never serializes it". Strictly stronger than before, since dropping SEO also drops the spoofed-crawler bypass the old design accepted. - SEO/JSON-LD paywall markup: dropped, now an explicit non-goal. - Cloudinary replaces S3 + CloudFront (free forever). - Phases resplit: P1 API (demoable via curl alone), P2 client, P3 comments, P4 realtime, P5 media, P6 oauth. - Supertest integration layer added — where the regression checklist is enforced against real routes through the real middleware chain. Also rewrites CLAUDE.md and the deployment architecture doc for the new stack. dev/web-app-scaffold and dev/ci-cd-pipeline (PR #8) are abandoned unmerged, retained in history.
15 TDD tasks, each ending in an independently testable deliverable:
root/shared cleanup, app skeleton, session, guards, services, routes,
static catch-all, docker/compose, seed, CI, render.yaml.
Four spec divergences, each verified rather than assumed:
- §3 hazard 2's app.get('*') throws at startup on Express 5 (path-to-regexp
v8); the catch-all must be '/*splat'
- §5's requireOwner signature does not compile under noUncheckedIndexedAccess;
it is generic over the route params instead
- ConflictError/409 added to §10's four error types
- static.ts ships in P1 (no-ops without a build) to front-load the wildcard
and API-shadowing failures
Also verified: connect-redis@9 peer-depends on node-redis, not ioredis, and
exports RedisStore as a named export; tsup inlines the source-only shared
package into a bundle that runs on plain node; a 4-arity express error
handler fails the current eslint config without argsIgnorePattern.
…he REST design - typecheck fans out per workspace (no root tsc --build; see CLAUDE.md) - eslint allows _-prefixed unused args: Express error handlers need arity 4 - add ValidationError (spec §10 requires it) and ConflictError (409) - UpdatePostSchema drops postId and goes partial: the slug identifies the post - .env.example documents session vars, not the withdrawn Auth.js ones
- buildApp() composes helmet → json → session → routers → 404 → error handler - error handler is the single place typed errors become status codes - unexpected errors return an opaque 500: internal messages can carry secrets - env is Zod-validated at boot; SESSION_SECRET has no fallback - integration test asserts the chain, incl. Express 5 async rejection forwarding
- cookie is httpOnly + SameSite=Lax + Secure-in-prod, named sid - saveUninitialized: false — anonymous reads must not allocate redis keys - redis client cached on globalThis: tsx watch reloads would exhaust the 50-conn cap - BuildAppOptions.session groups store/secret/secure: no per-field fallback, no default secret in app.ts; buildTestApp() isolates the test-only secret in src/test/, unreachable from the production bundle - SessionData augmented with userId: the only source of caller identity
Identity always comes from req.session.userId, never from a body field — the root cause of all five legacy authorization holes. - requireAuth: 401 for anonymous callers - requireOwner(load): 403 unless the session identity is the author - requireOwner rejects anonymous callers before querying the database - a loader rejection forwards as itself, never as a 403
- useTestDb() extracts the mongodb-memory-server boilerplate - validate() replaces req.body with the parsed value: defaults applied, unknown keys stripped, so a client cannot smuggle fields into a service - userService keeps the bcrypt-12 and username-enumeration fixes - a duplicate-key race now surfaces as ConflictError, not an unhandled 500 - getPublicProfile whitelists fields: it cannot leak the hash or the email
- toDto() is the ONE place a Post becomes a response; a gated body is never copied into the returned object, so there is nothing to find in DevTools - list() always teases: a list endpoint never ships full bodies - like counts are derived via countDocuments, never stored (legacy drifted) - author comes from the caller id, never from input - slug collisions get a numeric suffix; delete cascades to likes
…rect Task 7 test count
- logout is POST + requireAuth; GET /logout is 404 (legacy: unauthenticated GET) - login returns one generic message for every failure — no username enumeration, asserted by comparing the wrong-password and unknown-user responses byte for byte - session id is regenerated on signup and login: session-fixation defense - GET /me answers 401, never a redirect - promisified regenerateSession/destroySession live in lib/session.ts, not inline
Regression tests for three legacy holes, against real routes: - non-owner edit is 403 (was: post.js:34 let any signed-in user edit) - non-owner delete is 403 (was: post.js:42 deleted any post) - an author field in the body is ignored; identity comes from the session Gating is asserted on the RAW response text, not the parsed body: the premium bytes must be absent from the payload, not merely hidden.
REGRESSION tests for three legacy holes: - a user cannot modify another user (user.js:73 — account takeover) - a user cannot delete another user (user.js:60) - a profile update no longer silently resets the password (user.js:79) requireSelf compares req.session.userId to req.params.id directly — a User has no author field, so requireOwner's shape doesn't fit here.
… session.ts split
- PUT/DELETE, never POST /toggle: repeating must not change the outcome - upsert + the unique (user, post) index make a double click a non-event - REGRESSION: like requires auth and uses session identity, not a body field - unlike is a no-op when absent; the count cannot go below zero
- EXPRESS 5: the catch-all is '/{*splat}'; a bare '*' throws at startup
(path-to-regexp v8), and a bare '/*splat' requires >=1 segment and
does not match '/' itself - the spec's section 3 example is the Express 4 form
- tsup: mongoose must be external - noExternal on @blog/shared pulls in
its transitive deps too, and the mongodb driver's CJS require() of
node builtins does not survive being bundled into ESM
- the catch-all excludes /api/ so it cannot shadow the API - asserted:
an unknown API route returns JSON 404, never index.html
- mountStatic no-ops when there is no build, which is the P1 default
- index.ts validates env before opening any connection and refuses to
boot without SESSION_SECRET
- runner target is non-root and copies from a dedicated prod-deps stage (npm ci --omit=dev), not the full deps install - no dev tooling ships - npm ci uses a cache mount, alongside the existing extra-ca secret mount - compose.yaml uses watch-sync, not bind mounts: a bind-mounted node_modules breaks across the Windows to Linux boundary and inotify is unreliable there - mongo/redis bind to 127.0.0.1 in dev, publish no ports at all in e2e: both run unauthenticated, so nothing outside the compose network may reach them - SESSION_SECRET is the only real secret here (mongo/redis URIs carry no credentials) - wired through Compose file-based secrets and a new SESSION_SECRET_FILE convention in env.ts, not inlined in the tracked yaml - compose.e2e.yaml builds the PROD image, so a broken prod build fails in CI - optional extra-ca build secret for TLS-intercepting AV; never baked in
Even throwaway, non-sensitive values should not sit as real files under a directory named secrets/ in a public repo - it invites exactly the kind of scanning/scraping the naming convention exists to signal against. The dev and e2e session-secret values are now .txt.example templates (committed); the real .txt files Compose actually reads are gitignored and created locally via the copy step documented in secrets/README.md.
Idempotent and destructive: wipes and rewrites the collections it owns. Seeds one premium post, so the gating rule is demoable with curl alone.
CLAUDE.md now states features must not be mixed into one branch.
Replaces the single environment-gated ci.yml (PR #8 design). GitHub Environments don't reliably interact with pull_request-triggered runs (branch policies match real branch refs, not the pull_request event's synthetic merge ref), so the human/source-branch gates move to branch protection + a validate-source-branch check instead: - pr-to-staging.yml: PRs into staging must come from dev/*; runs build/typecheck/lint/test/e2e-smoke. - staging-pipeline.yml: re-runs the same suite on every push to staging (i.e. every merged dev/* PR) to catch two branches breaking each other once combined, and opens/refreshes a single aggregating staging->master PR. - pr-to-master.yml: PRs into master must come from staging; relies on staging-pipeline.yml's checks via matching SHA rather than re-running the suite a third time. Branch protection (staging, master) requires these checks, requires a PR before merging, blocks force-push/delete, and enforces on admins too.
ci: three-workflow pipeline with branch-protection gates
feat(api): Express REST API foundation (P1)
pr-to-master.yml never ran for the first staging->master PR: it was opened by the default GITHUB_TOKEN, whose events don't trigger other workflows (GitHub's loop-prevention rule) - and since that was its only trigger attempt, GitHub never registered the workflow at all, so even reopening the PR by hand didn't help. Two fixes: - workflow_dispatch added to pr-to-master.yml, so it can be re-run/ registered manually if this ever recurs. - open-release-pr now authenticates with a RELEASE_PR_TOKEN PAT instead of github.token, so the PRs it opens are user-authored and trigger pr-to-master.yml normally going forward. Requires a RELEASE_PR_TOKEN repo secret (fine-grained PAT, Pull requests: read/write on this repo) to be added before the next auto-opened release PR.
Replaces the hand-rolled SHA-1 signature with the SDK's api_sign_request and cloudinary.url. The signing algorithm is Cloudinary's to change, not ours to reimplement, and the previous version was only ever verified against my own recomputation of it. Credentials move from a single CLOUDINARY_URL to CLOUDINARY_CLOUD_NAME / API_KEY / API_SECRET, passed to cloudinary.config() per the SDK's Node guide. All three or none: loadEnv rejects a partial set at boot, because a half-configured app looks fine and then fails at Cloudinary with an opaque error. Every value is read from the gitignored .env; API_SECRET also accepts a _FILE path like SESSION_SECRET. render.yaml carries all three as sync: false so they are prompted for, never committed.
Adds P6 OAuth. Passport handles the handshake only — every strategy runs with session: false, so Passport never serializes a user and identity is written to req.session.userId by the callback, exactly as password login does. One session model, not two. Optional per provider: with no credentials the button is absent rather than present and broken, and GET /auth/providers is what the sign-in page asks. Each credential pair is all-or-nothing at boot. Account linking is the security-critical part. A provider profile links to an existing local account ONLY when the provider asserts it verified the email; an unverified address is attacker-controlled, so matching on it would hand over the account. Facebook's Graph API exposes no per-address verification flag, so its email is never treated as verified — it can create an account but never silently link to one. A profile with no email is refused outright rather than becoming a half-account we can neither dedupe nor ever contact. Sessions are still regenerated before identity is written, so a planted session id cannot end up authenticated through the OAuth path either.
A Render deploy would otherwise keep building localhost OAuth callbacks and break sign-in for every user, with nothing failing at boot to warn you. Explicit PUBLIC_ORIGIN still wins, which is what a custom domain needs. Deliberately not derived from the request Host header: that is attacker-controlled, and an OAuth redirect built from it is an open-redirect primitive.
The strategy stays in the code and dormant, so enabling Facebook later is adding two variables rather than writing code.
CI caught four errors the local test run could not: indexed access into the pen and motif tables is string | undefined under this repo's tsconfig. Falls back rather than asserting non-null, so a genuinely out-of-range index would still surface instead of crashing at runtime.
The rail shows the bare username, 'New post' and 'Log out' — the old assertions were written against 'Welcome, <name>', 'New Post' and 'Logout'. Matched case-insensitively because the rail is uppercased in CSS and text-transform can reach the accessible name.
feat(client): editorial feed redesign with generated cover art
feat: optional cover images via signed Cloudinary uploads
feat(auth): Google sign-in via Passport
Caps the public demo so it cannot be filled or run up a bill on shared free-tier infrastructure: 20 users globally, 3 posts per author, 10 comments per post. Per-owner rather than a global pool for posts and comments. A shared pool lets one enthusiastic visitor consume all of it and every later visitor finds the app full; a per-account allowance cannot be spent on anyone else's behalf, which also makes the footprint a predictable product of the caps. Enforced in the service layer, never middleware, per the project's rule that routers stay thin. The two scoped counts are filtered queries on already-indexed fields, so each guard is an index lookup rather than a scan. authorId comes from the session and postId from the resolved slug, never from the request body — a body-supplied owner would let a caller spend someone else's allowance or dodge their own. 403 rather than 503: 503 reads as broken to a visitor and trips uptime monitoring, while this is a policy refusal of a well-formed request. Two messages, because telling someone who has used their 3 posts that the app is at capacity is false and invites a support email about a working app. Also fixes SignupPage, which showed a hardcoded "Signup failed. Please try again." A visitor hitting the global cap would have retried forever and never seen the message telling them how to make contact. seed.ts now wipes comments too. It previously cleared posts, likes and users only, so any reseed left every comment behind pointing at deleted posts. Accepted race, documented rather than fixed: concurrent writes at a limit can both pass the check. The overshoot is bounded by the concurrency level, and at demo traffic a transaction costs more than the overshoot.
Seeding is destructive, and a developer with production credentials in their shell could wipe the live database by typing `npm run seed`. That was reachable in one keystroke. The default path now IGNORES MONGODB_URI entirely and uses a local URI. It does not inspect the variable and bail on a risky-looking value — a check like that can be defeated by a URI that looks local but is not. Not reading it cannot be. `npm run seed:prod` is the only way to reach a remote database. It refuses when MONGODB_URI is unset, and refuses when it still points at a local host — seeding localhost while believing you seeded production looks like success and is the more dangerous of the two mistakes. Every run announces its target as host plus database, never the connection string; a test asserts the password cannot reach that line. Seeding also no longer calls loadEnv. It needs a Mongo URI and nothing else, and requiring SESSION_SECRET and REDIS_URL to reseed was friction with no safety value.
Names the web service blog-chat-app so a Blueprint deploy claims the existing subdomain instead of minting a new one, and pins branch: master so staging is never deployed.
Render discovers Blueprints at the repo root only, so infra/render.yaml would never have been found. Docs and CLAUDE.md updated to match — this is a deliberate exception to the rule that deploy config lives in infra/.
feat: demo capacity caps
…ounts Security fix, found by /security-review. oauthService.findOrCreate used to link a Google/Facebook identity to a pre-existing LOCAL account by email match whenever the provider claimed the email was verified. This app's local signup has no email-verification step at all, so that claim proved nothing about whether the local account genuinely belonged to the same person. Concrete attack: an attacker signs up locally with victim@gmail.com and a password only they know (nothing blocks this). The real victim later signs in with Google using that same, genuinely Google-verified address. findOrCreate found the attacker's account by email and silently attached the victim's Google identity to it, without touching the existing password field — the attacker's original credentials kept working, now against an account the victim also uses. A full, persistent account takeover requiring no further action from the victim beyond one Google sign-in. Fix: an email match to a pre-existing account is now refused unconditionally, regardless of what the provider claims. Linking still works via case 1 (a known provider id resolves to its account) and case 3 (no existing account at all creates a fresh passwordless one); only the vulnerable "link on email match" path is removed. emailVerified is dropped from OAuthProfile and the Passport strategies entirely, since a provider-asserted flag can no longer justify anything here. Also closes a related gap the same review flagged as an inconsistency (not independently exploitable, since nothing renders it today, but wrong by the same reasoning): the user profile's `image` field skipped both the PublicIdSchema regex and the publicIdFrom() re-validation coverImage gets. It now goes through both, matching how coverImage is handled on posts exactly. Regression tests added: the exact pre-registration attack scenario (attacker's password still works after the refused OAuth attempt, no account is mutated), refusal to cross-link a second provider by email, and the closed image-validation gap on both invalid and valid inputs.
fix(auth): stop auto-linking OAuth identities to unverified local accounts
Liking a post, then navigating back to the feed, showed the pre-like count until a hard refresh. useLikePost's optimistic update and invalidation only ever touched the post-detail query (queryKeys.posts.detail(slug)); the feed keeps its own separate cached copy of the same post under queryKeys.posts.list(params), and with a 5-minute staleTime, navigating back never triggered a refetch to pick up the real number. Adds queryKeys.posts.lists as a match target for every cached feed variant, alongside the existing queryKeys.posts.all — narrower than `all` so it can be used to patch list-shaped cache entries (Post[]) without also touching detail entries (a single Post), which would break if fed the same array-mapping updater. onMutate now patches both the detail cache and every cached list via setQueriesData, using getQueriesData first to snapshot the true prior state for rollback — setQueriesData's own return value is the data AFTER the updater runs, not before, so it cannot double as that snapshot. onSettled invalidates queryKeys.posts.all instead of just the one detail key, so every filtered feed variant refreshes too. Verified live: liked a post, navigated back via the nav link (no reload), and the feed showed the updated count immediately.
fix(client): sync the feed's cached like count after liking a post
master's only unique commit is bde9eb2, a revert of the abandoned pre-pivot "monorepo shared package" PR from the Next.js foundation attempt (2026-07-15, the day before the pivot to Express + React documented in CLAUDE.md). Reverting it restored the legacy CRA + Express app's files (server/, public/, src/) to master's tree — the exact files this rebuild replaces. A normal merge would reintroduce all 58 of those legacy files alongside apps/server, apps/client, and packages/zod-shared, plus reopen renamed files (packages/shared/* vs. their staging replacements) as spurious conflicts. None of that content is wanted: staging is the complete, deliberate replacement master's history predates, not a branch to reconcile file-by-file against dead scaffolding. Merged with -s ours: master's tip becomes an ancestor of staging (so the staging -> master PR stops reporting a conflict) with zero change to staging's actual tree - diff against pre-merge staging is empty.
YonatanHen
approved these changes
Jul 30, 2026
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.
Aggregates everything merged into staging since the last release.