diff --git a/apps/api/src/routes/people.ts b/apps/api/src/routes/people.ts index f8f2378..7bff95a 100644 --- a/apps/api/src/routes/people.ts +++ b/apps/api/src/routes/people.ts @@ -122,7 +122,31 @@ export async function peopleRoutes(fastify: FastifyInstance): Promise { tags: ['people'], summary: 'Update profile', params: { type: 'object', properties: { slug: { type: 'string' } }, required: ['slug'] }, - body: { type: 'object' }, + // Enumerate the editable fields and reject anything else, so privileged + // fields (e.g. accountLevel — see POST /api/people/:slug/account-level) + // can't be smuggled through the generic profile PATCH. Per + // specs/api/people.md → "PATCH /api/people/:slug". + body: { + type: 'object', + additionalProperties: false, + properties: { + fullName: { type: 'string' }, + firstName: { type: ['string', 'null'] }, + lastName: { type: ['string', 'null'] }, + bio: { type: ['string', 'null'] }, + slug: { type: 'string' }, + email: { type: 'string' }, + slackHandle: { type: ['string', 'null'] }, + tags: { + type: 'object', + additionalProperties: false, + properties: { + topic: { type: 'array', items: { type: 'string' } }, + tech: { type: 'array', items: { type: 'string' } }, + }, + }, + }, + }, }, }, async (request) => { const { slug } = request.params as { slug: string }; @@ -142,29 +166,6 @@ export async function peopleRoutes(fastify: FastifyInstance): Promise { return ok(await fastify.services.people.get(result.value.person.slug, caller)); }); - // DELETE /api/people/:slug (admin-only soft-delete — legacy, kept for backward compat) - fastify.delete('/api/people/:slug', { - schema: { - tags: ['people'], - summary: 'Soft-delete a person (admin only, legacy)', - params: { type: 'object', properties: { slug: { type: 'string' } }, required: ['slug'] }, - }, - }, async (request, reply) => { - const { slug } = request.params as { slug: string }; - const result = await fastify.store.transact( - buildTransactionOptions({ - request, - action: 'person.soft-delete', - subjectType: 'person', - subjectSlug: slug, - responseCode: 204, - }), - async (tx) => fastify.services.peopleWrite.softDelete(tx, slug, request.session), - ); - result.value.stateApply.apply(fastify.inMemoryState, fastify.fts); - return reply.code(204).send(); - }); - // POST /api/people/:slug/deactivate (self | staff) // Spec: specs/behaviors/person-lifecycle.md, specs/api/people.md fastify.post('/api/people/:slug/deactivate', { diff --git a/apps/api/src/services/person.write.ts b/apps/api/src/services/person.write.ts index 41e78ec..c3e541e 100644 --- a/apps/api/src/services/person.write.ts +++ b/apps/api/src/services/person.write.ts @@ -166,31 +166,6 @@ export class PersonWriteService { return { person: updated, stateApply }; } - async softDelete( - tx: DualStoreTx, - slug: string, - session: SessionContext, - ): Promise<{ stateApply: StateApply }> { - const existing = this.#personOrThrow(slug); - requireAuth('administrator', { session }); - - if (existing.deletedAt) { - return { stateApply: new StateApply() }; - } - - const now = nowIso(); - const updated: Person = PersonSchema.parse({ - ...existing, - deletedAt: now, - updatedAt: now, - }); - - await tx.public.people.upsert(updated); - - const stateApply = new StateApply().upsertPerson(updated); - return { stateApply }; - } - /** * Deactivate — self-service or staff. * Sets `deletedAt = now()`. The person can still sign in and reactivate. diff --git a/apps/api/src/store/commit-meta.ts b/apps/api/src/store/commit-meta.ts index 1bd631c..a3bb35e 100644 --- a/apps/api/src/store/commit-meta.ts +++ b/apps/api/src/store/commit-meta.ts @@ -8,6 +8,7 @@ * The User-Ip / User-Agent / Authorization / Cookie headers are deliberately * NOT included — the public commit log is forever-public. */ +import { STATUS_CODES } from 'node:http'; import type { Author, TransactionOptions } from 'gitsheets'; import type { FastifyRequest } from 'fastify'; import type { SessionContext } from '../auth/middleware.js'; @@ -61,6 +62,9 @@ export function buildTransactionOptions(ctx: CommitContext): TransactionOptions Host: request.hostname || 'unknown', 'Content-Type': String(request.headers['content-type'] ?? 'unknown'), 'Response-Code': String(responseCode), + // HTTP reason phrase for the response code, per + // specs/behaviors/storage.md#commit-message-shape. + 'Response-Message': STATUS_CODES[responseCode] ?? 'Unknown', }; if (subjectType) trailers['Subject-Type'] = subjectType; diff --git a/plans/spec-drift-fixes.md b/plans/spec-drift-fixes.md new file mode 100644 index 0000000..b22e407 --- /dev/null +++ b/plans/spec-drift-fixes.md @@ -0,0 +1,76 @@ +--- +status: done +depends: [] +specs: + - specs/behaviors/storage.md + - specs/api/people.md + - specs/api/conventions.md + - specs/behaviors/legacy-id-mapping.md +issues: [] +pr: 148 +--- + +# Plan: spec-drift audit fixes + +## Scope + +A spec-drift audit (two scoped `spec-drift-auditor` runs) compared `specs/` +against the implementation. This plan records the resolution of the real +findings — and the false positives that were verified away rather than +"fixed." + +## Real findings fixed + +**Code (apps/api):** + +- **Removed the legacy `DELETE /api/people/:slug` route + `softDelete` service + method.** It was an undocumented admin-only soft-delete running parallel to + the canonical self|staff `POST /deactivate` — two soft-delete paths with + diverging authz. Superseded by deactivate/reactivate/purge; deleted. +- **`commit-meta.ts`: added the `Response-Message` trailer** (HTTP reason + phrase via `node:http` `STATUS_CODES`) — `storage.md#commit-message-shape` + required it but only `Response-Code` was emitted. +- **`PATCH /api/people/:slug`: tightened the body schema** to enumerate the + editable fields + `additionalProperties: false`, so privileged fields (esp. + `accountLevel`) are rejected (422) rather than silently ignored — hardening + the "account-level only via its dedicated endpoint" rule. + +**Specs/docs:** + +- **`storage.md` public/private contradiction** — ground-truthed the repo + visibility (`codeforphilly-data` is **private** on GitHub today). Reconciled: + it's a private repo holding **public-by-design** content; the dangerous table + claim that it "contains emails, real names, IPs" (contradicting the whole + redaction section) was corrected — PII lives in the private store. +- `people.md` Person response shape: documented `slackHandle` and `deletedAt` + (both returned by the serializer; `deletedAt` gated to self/staff). +- `people.md`: clarified `?accountLevel=` returns an empty 200 to non-staff + (not 403); corrected the PATCH note to point at the dedicated admin endpoint. +- `legacy-id-mapping.md`: tightened the `byLegacyId` claim — runtime indices + exist only for people/projects/blog-posts; tags/buzz/updates carry `legacyId` + for import idempotence only. +- `storage.md`: fixed the `scrub-data.ts` path; clarified that commit bodies + carry only the caller `summary` (request-body PII redaction is moot). +- `conventions.md`: marked Idempotency-Key as built-but-not-yet-wired (status + note) — the actual per-route wiring is a follow-up. + +## Verified false positives (NOT changed) + +- **`reloadInMemoryState` vs `swapPublic`** — the hot-reload path *does* call + `store.swapPublic` (`reload.ts:76`); the spec was correct. Auditor only read + `internal.ts`. +- **`perPage` out-of-range "returns 400"** — this app remaps Fastify validation + errors to **422** (`errors.ts:179`). Spec is correct. +- **account-level `Previous-Account-Level: unknown` "race"** — the 404 throws + inside the transaction before any write, so no commit (or trailer) persists. + +## Validation + +- [x] `type-check` + `lint` clean. +- [x] people-account-level 10/10, people-lifecycle 14/14, write-api 28/28 + (52 total across affected suites). + +## Follow-ups + +- Wire `Idempotency-Key` into at-risk mutating endpoints (starting with + `POST /api/projects/:slug/updates`) + per-route tests. diff --git a/specs/api/conventions.md b/specs/api/conventions.md index 650f9dd..dd2b6c9 100644 --- a/specs/api/conventions.md +++ b/specs/api/conventions.md @@ -170,6 +170,8 @@ Exceeded → `429 rate_limited`, `Retry-After` header in seconds. Mutating endpoints accept an optional `Idempotency-Key` header (any client-generated string). The API caches the response in-memory keyed by `(personId, idempotencyKey)` for 24 hours; repeat requests with the same key return the same response. In-memory by design — single replica, restart-tolerant: a key that hasn't seen a duplicate within 24h won't see one after a restart either. This matters for cases like "post project update" where a double-tap shouldn't create two updates. +> **Status:** the idempotency plugin (`apps/api/src/plugins/idempotency.ts`) is implemented and registered, but not yet wired into any route handler — mutating endpoints don't honor the header *yet*. Wiring the at-risk endpoints (starting with `POST /api/projects/:slug/updates`) and the per-route tests are tracked as a follow-up; until then, treat this section as the target contract, not current behavior. + ## Logging and trace IDs Every request has a `traceId` (UUIDv7). It's included in logs and surfaced in error responses' `error.traceId`. If a user reports a problem, the traceId is the link to the server logs. diff --git a/specs/api/people.md b/specs/api/people.md index dfe707f..c1cfd64 100644 --- a/specs/api/people.md +++ b/specs/api/people.md @@ -26,7 +26,7 @@ See [data-model.md](../data-model.md#person). | ----- | ---- | ----- | | `q` | string | Full-text on `fullName`, `bio`. | | `tag` | string | Repeatable. Tag handle (e.g., `tech.python`). AND across repeats. | -| `accountLevel` | enum | `user` \| `staff` \| `administrator`. Staff-only filter. | +| `accountLevel` | enum | `user` \| `staff` \| `administrator`. Staff-only filter — for a non-staff caller it returns an empty list (a 200 with no items), not a `403`, so the filter's existence isn't a signal. | | `sort` | sort | Default `-createdAt`. Allowed: `createdAt`, `fullName`. | | `page`, `perPage` | int | Default `perPage = 30`. | @@ -80,7 +80,9 @@ See [data-model.md](../data-model.md#person). "avatarUrl": "https://...", "bio": "Markdown source...", "bioHtml": "

...

", + "slackHandle": "janedoe", // null if unset "accountLevel": "staff", // visible to self and staff only; "user" otherwise + "deletedAt": null, // ISO timestamp if deactivated; null otherwise. Visible to self + staff only (always null to others) "tags": { "topic": [Tag, ...], "tech": [Tag, ...] }, "memberships": [ { @@ -102,6 +104,7 @@ Fields visible only to self or staff: - `email` (not in the shape above; added only when authorized) - `firstName`, `lastName` (visible to all but editable only by self/staff) - `accountLevel` value beyond a generic "user" — public callers always see `"user"` regardless of true level +- `deletedAt` — the real timestamp is shown to self + staff; everyone else always sees `null` ### Errors @@ -109,7 +112,7 @@ Fields visible only to self or staff: ## PATCH /api/people/:slug -Self or staff. Self cannot change their own `accountLevel`; only administrators can change account levels (and only via a staff-only sub-endpoint, not by passing `accountLevel` in a generic PATCH). +Self or staff. Self cannot change their own `accountLevel`; only administrators can change account levels (and only via the dedicated [`POST /api/people/:slug/account-level`](#post-apipeopleslugaccount-level) endpoint — `accountLevel` passed in a generic PATCH body is rejected by the schema, not silently applied). ### Request diff --git a/specs/behaviors/legacy-id-mapping.md b/specs/behaviors/legacy-id-mapping.md index 7dbd2f5..ffd79db 100644 --- a/specs/behaviors/legacy-id-mapping.md +++ b/specs/behaviors/legacy-id-mapping.md @@ -25,7 +25,7 @@ If we ever need to re-import (e.g., catching up on changes made after cutover), ## Lookup -`legacyId` lookups go through the in-memory `byLegacyId.` indices documented in [data-model.md](../data-model.md). These indices are built at boot by iterating the sheet and skipping records where `legacyId` is absent. +`legacyId` lookups that back a live redirect go through in-memory `byLegacyId` indices, built at boot by iterating the sheet and skipping records where `legacyId` is absent. These indices exist only for the entities whose laddr URLs actually referenced numeric IDs — **`people`, `projects`, and `blog-posts`**. The other migrated sheets (`tags`, `project-buzz`, `project-updates`) carry `legacyId` for *import idempotence* but have no runtime `byLegacyId` index, because no live URL resolves them by numeric ID (their legacy URLs were always slug-based). If a future redirect ever needs one of those by numeric ID, add the index then. Uniqueness of `legacyId` per sheet is enforced by the API's write mutex: a record's `legacyId` is checked against the index before commit, just like `slug` and `email`. diff --git a/specs/behaviors/storage.md b/specs/behaviors/storage.md index 2e7feb9..2ff0edd 100644 --- a/specs/behaviors/storage.md +++ b/specs/behaviors/storage.md @@ -2,7 +2,7 @@ ## Rule -**Public** persistent data lives in a **gitsheets-backed git repository** — TOML records in templated paths, committed atomically, pushed to a publicly cloneable GitHub remote. There is no relational database. At runtime the API loads all records into typed in-memory structures and serves reads from there; mutations write to the gitsheets repo and update the in-memory state synchronously. +**Public-by-design** persistent data lives in a **gitsheets-backed git repository** — TOML records in templated paths, committed atomically, pushed to the `codeforphilly-data` GitHub remote. The repo is access-controlled (private) today, but its contents are curated to be publicly shareable (civic transparency), and the commit log is redacted of PII accordingly — treat everything in it as forever-public (see [PII-aware redaction](#pii-aware-redaction)). There is no relational database. At runtime the API loads all records into typed in-memory structures and serves reads from there; mutations write to the gitsheets repo and update the in-memory state synchronously. **Private** data — emails, password hashes during migration, newsletter subscription state — lives in a separate S3-compatible bucket. See [behaviors/private-storage.md](private-storage.md). This spec covers only the public side. @@ -54,7 +54,7 @@ There are two git repositories: | Repo | Purpose | Visibility | | ---- | ------- | ---------- | | `codeforphilly-rewrite` | The application code (this repo) | Public | -| `codeforphilly-data` | The live gitsheets data | **Private** — contains emails, real names, IPs | +| `codeforphilly-data` | The live gitsheets data | **Private repo today**, but **public-by-design** content — member names + projects are public-facing; the commit log is redacted (no emails, IPs, or password hashes — those live in the private store). Treat as forever-public. | The code repo references the data repo by env (`CFP_DATA_REPO_PATH`). They are not git submodules — too much friction. Sibling clones, locally and in production. @@ -81,7 +81,7 @@ A scrubbed snapshot of the data repo is published as a public tag (e.g., `snapsh - `slackHandle` → null - `bio`, `overview`, `body` content → unchanged (assumed safe; staff may flag specific records for redaction) -A `scripts/scrub-data.ts` in the code repo produces the snapshot. The contributor bootstrap is: +An `apps/api/scripts/scrub-data.ts` in the code repo produces the snapshot. The contributor bootstrap is: ```bash git clone https://github.com/CodeForPhilly/codeforphilly-rewrite.git @@ -264,12 +264,12 @@ Rules: ### PII-aware redaction -The public data repo is, well, public — redaction is essential, not defense-in-depth: +The data repo is public-by-design (and may be made public outright) — redaction is essential, not defense-in-depth: - **No real emails as commit authors.** Author = `@users.noreply.codeforphilly.org`, never the user's actual email. - **No `User-Ip` or `User-Agent` trailers in public commits** (rule above). - `Authorization` and `Cookie` request headers are never embedded anywhere. -- Any request body field matching `/email|password|token|secret/i` is replaced with `[REDACTED]` before embedding in the commit message body. +- **Commit message bodies carry only the caller-provided `summary` string — never the raw request body** — so request PII can't reach the commit message in the first place. If a `summary` is ever derived from request input, fields matching `/email|password|token|secret/i` must be `[REDACTED]` before embedding. - `Set-Cookie` response headers and JWT bodies are not embedded. - Trailers for actions on the private store (`PrivateProfile`, `LegacyPasswordCredential`) appear in the public log as semantic action only — `Action: private-profile.update`, with no field-level diff. The actual change details live in the private bucket's object-version history (see [private-storage.md](private-storage.md)).