fix(web): validate Server Action API responses at the trust boundary - #478
Conversation
serverAuthFetch/serverPublicFetch cast responses with `as T`, skipping runtime validation at a trust boundary (rule 8). Add an optional Zod schema parameter that safeParse()s the response body and throws a typed ApiClientError (502, code INVALID_RESPONSE_SCHEMA) on mismatch instead of letting mistyped data flow inward, then thread the matching @orbit/shared schemas through the highest-value habit + gamification Server Actions (suggestHabitSetup, bulk create/delete/log/skip, reportAchievementEvent). Each threaded schema was verified against the orbit-api DTO's real JSON (including explicit-null serialization). logHabitResponseSchema is left unthreaded: it marks xpEarned/newAchievementIds `.optional()` but the API serializes them as explicit `null`, so enforcing it as-is would reject a valid response — that schema needs a `.nullish()` fix in a follow-up. Refs #243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #478 — fix(web): validate Server Action API responses at the trust boundary
Scope: PR #478 in thomasluizon/orbit-ui-mobile (fix/web-server-fetch-zod-boundary → main)
Recommendation: NEEDS WORK
Summary
The PR adds optional Zod-schema validation to serverAuthFetch/serverPublicFetch (apps/web/lib/server-fetch.ts) and threads the matching @orbit/shared schemas through five high-value Server Actions (suggestHabitSetup, the four bulk habit actions, reportAchievementEvent). The web-side implementation is correct, well-tested, and matches the schemas already defined in packages/shared (verified index/nullable fields line up with the bulk-result and gamification schemas). The blocking issue is scope: the identical unvalidated JSON.parse(text) as T trust-boundary gap this PR closes on web is left completely untouched on apps/mobile/lib/api-client.ts, which the root parity rule treats as mandatory, not optional.
Findings
Critical
None.
High
[High] Web-only trust-boundary fix leaves mobile's identical gap open — parity violation
· dimension: Parity (web ↔ mobile), rubric dimension 9
· location: apps/mobile/lib/api-client.ts:132-156 (specifically line 155); web fix is apps/web/lib/server-fetch.ts:8-19
· issue: This PR adds .safeParse() validation and a typed ApiClientError(502, INVALID_RESPONSE_SCHEMA) on schema mismatch to web's serverAuthFetch/serverPublicFetch. Mobile's apiClient's parseApiResponse<T>() still does a bare return JSON.parse(text) as T with zero schema validation, and none of its callers (apps/mobile/hooks/use-habits.ts, apps/mobile/hooks/use-gamification.ts) pass a schema. The PR body states "Scope: Web-only by design — the mobile apiClient is a separate slice," but this was independently verified (including by an adversarial skeptic pass) to not be a legitimate platform-adapter difference — it is the same defect on both platforms, fixed on one.
· risk: Root CLAUDE.md's cross-platform parity rule is explicit and MANDATORY: "Every change lands in BOTH apps/web AND apps/mobile in the same task — logic, features, behavior, and error handling identical... Allowed differences: platform adapters only (BFF vs direct API, cookie vs SecureStore, shadcn vs NativeWind, next-intl vs i18next)." Schema-validation-at-the-trust-boundary is not on that allowed-differences list. A malformed or contract-drifted API response still flows into the mobile app as silently mistyped data — the exact failure mode this PR is meant to close. Additionally, the PR's "Refs #243" does not back a tracked mobile follow-up: #243 (per WORKFLOW.md) is the unrelated final-pre-launch-gate/Sonar campaign, not a mobile-Zod-validation issue — so there is no paper trail for the deferred work either.
· fix: Thread the same schemas (or their mobile-appropriate request/response counterparts) through apiClient/parseApiResponse in apps/mobile/lib/api-client.ts in this same PR, mirroring the schema?: ZodType<T> parameter pattern from server-fetch.ts, and update the mobile callers that have matching schemas today. If mobile genuinely needs to lag (e.g., broader blast radius touching the offline queue), open a dedicated tracked issue referencing this PR and note it explicitly in the PR body instead of asserting it is "by design" against a rule that says otherwise.
· reference: root CLAUDE.md "Cross-platform parity (MANDATORY)"; CLAUDE.md rule 8 (validate at trust boundaries); rubric.md dimension 9 (Parity)
Medium
[Medium] Hottest-path response (logHabit) diagnosed but left unvalidated, contra "fix immediately"
· dimension: No-workaround / root-cause, rubric dimension 5; also ties to Correctness (1)
· location: apps/web/app/actions/habits.ts:71-79 (logHabit); schema at packages/shared/src/types/habit.ts:339-346 (logHabitResponseSchema)
· issue: The PR body correctly diagnoses that logHabitResponseSchema's xpEarned/newAchievementIds/linkedGoalUpdates are .optional() while the orbit-api response serializes them as explicit null, so threading the schema as-is would make .optional().parse(null) reject a valid response on the single most-frequent mutation in the app (logging a habit). The fix is diagnosed precisely (.optional() → .nullish()) but not applied in this PR; it's left as a "follow-up," in packages/shared/src/types/habit.ts — a file well within this monorepo PR's reach, not a genuinely separate slice.
· risk: Root CLAUDE.md's Maximum Implementation rule states plainly: "see something broken, stale, or wrong? Fix it immediately, in the same PR — never report it as 'out of scope' or 'pre-existing.'" This is a small, already-scoped, already-diagnosed one-line schema fix (.nullish()) plus threading through the one remaining habit-mutation call site; deferring it leaves the highest-traffic Server Action as the one exception to the trust-boundary hardening this PR exists to deliver.
· fix: In the same PR (or a fast immediate follow-up before this merges), change the three fields in logHabitResponseSchema to .nullish() and thread logHabitResponseSchema through logHabit the same way the other five actions were threaded.
· reference: root CLAUDE.md "Maximum implementation"; CLAUDE.md rule 1 (root cause over workarounds)
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | MISSING — confirmed apps/mobile/lib/api-client.ts has no matching schema-validation update (see High finding above) |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — diff touches neither packages/shared/src/types/*/endpoints.ts nor orbit-api; gate not triggered |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no apps/* UI files touched (Server Actions/lib only) |
Validation
| Check | Result |
|---|---|
| Lint | N/A — not run (CI-wrapper scope skips Phase 7; manual console.log/comment-policy grep on changed files came back clean) |
| Type check | N/A — not run in this pass |
| Tests | N/A — not run in this pass; test additions were reviewed by inspection: bulkLogResultSchema/bulkSkipResultSchema both require index: z.number(), matching the fixture corrections in habits-extended.test.ts; other tests touching bulk actions (use-habits.test.ts, habits.test.ts) mock at the action/hook layer and bypass server-fetch, so they're unaffected |
| Build (api) | N/A — orbit-api not touched, sibling repo not checked out in this environment |
What's good
parseResponseBodyis a clean, minimal, correctly-shared helper (one implementation for bothserverAuthFetchandserverPublicFetch) rather than duplicated validation logic — good use of rule 10's "cross-function-in-file → local helper."- The 502/
INVALID_RESPONSE_SCHEMAerror is thrown as a typedApiClientError, not swallowed, and correctly routes throughgetFriendlyErrorKey's existingstatus >= 500branch to a generictoast.errors.serverkey — no Zod issue details leak into user-facing copy. - Every threaded schema was verified against its actual Zod definition in
packages/shared/src/types/*and lines up field-for-field with the new/updated test fixtures (theindexfield addition to bulk-log/bulk-skip fixtures is correct and necessary givenbulkLogResultSchema/bulkSkipResultSchemarequire it). - The
logHabitexclusion, while it should have been fixed in this PR (see Medium finding), shows real engineering judgment — the author caught a would-be regression (rejecting validnullresponses on the hottest path) before it shipped, rather than blindly threading every schema. - New tests cover the schema-valid, schema-invalid, empty/204, and 404 paths for both
serverAuthFetchandserverPublicFetch.
Recommendation
Close the mobile parity gap (High) before merge — either thread the equivalent Zod validation into apps/mobile/lib/api-client.ts in this PR, or replace the "by design" framing with an explicit, tracked follow-up issue since the current #243 reference does not cover it. Separately, consider folding in the one-line logHabitResponseSchema fix (.nullish()) and threading it through logHabit in the same PR per the Maximum Implementation rule — it's small and already fully diagnosed in the PR description.
Deferred — N/A dimensions & files not verdicted
- Contract drift + backward-compat (dimension 11): gate not triggered (no
packages/shared/src/types/*/endpoints.tsororbit-apichanges in this diff); additionally not independently verifiable in this environment since the siblingorbit-apirepo is not checked out here, so the PR's own claimed field-by-field DTO comparison could not be re-verified against real orbit-api source. No evidence of an actual mismatch was found, so no finding is made — noted here per the honesty clause. - Security (dimension 12, backend categories): N/A —
orbit-apinot touched. Frontend-facing categories (XSS, error-message leakage, auth-state) were reviewed inline and came back clean. - Backend hard rules (dimension 13): N/A —
orbit-apinot touched. - FEATURES.md parity (dimension 14): N/A — this is a bugfix/hardening change with no new screen, tool, gating, or platform-availability change.
- Lint / type-check / test execution: not run in this pass (CI-wrapper scope; Phase 7 Validate is skipped per the skill's CI instructions). Manual inspection of the changed files and the affected/updated test fixtures found no correctness gaps.
…sh schema orbit-api's LogHabitResponse defaults XpEarned/NewAchievementIds to null and the API serializes explicit nulls (no WhenWritingNull), so logHabitResponseSchema's `.optional()` on those fields (and linkedGoalUpdates) would reject a valid response. Switch them to `.nullish()` so the schema matches the real contract, then thread logHabitResponseSchema through the logHabit Server Action — closing the trust boundary on the app's most-frequent mutation. All consumers already read these fields with `?.`/`??`, so the added `| null` is behavior-preserving. Refs #243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both findings addressed in
|
…mobile parity) Mirror the web Server Action fix on mobile: `apiClient`/`parseApiResponse` now take an optional `schema?: ZodType<T>` that `.safeParse()`s the response body and throws a typed `ApiClientError(502, INVALID_RESPONSE_SCHEMA)` on mismatch. The schema stays optional, so the many un-threaded callers (and the offline-queue replay path) are behavior-unchanged. Threaded the shared response schemas through the two direct (non-queued) callers the review flagged: - useReportEvent -> reportEventResponseSchema - useHabitSuggestion -> habitSetupSuggestionSchema (replaces the ad-hoc `.parse()` that threw a bare ZodError; now surfaces the typed 502 boundary error) logHabit and the four bulk habit actions are intentionally left un-threaded: they run through `performQueuedApiMutation`, and queued mutations are replayed later by `processQueuedMutationFlush` via `apiClient<unknown>` from a persisted `QueuedMutation` that cannot carry a runtime schema function. Validating them consistently across the immediate and replay paths needs a `MutationType -> ZodType` registry consulted by both paths — a broader offline-queue change tracked as follow-up, kept out of this surgical response-boundary fix. Added mobile boundary tests mirroring server-fetch.test.ts (valid parse strips extras, malformed body -> typed 502, empty/204 skips validation). Refs #243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both findings addressed. [High] Mobile parity gap — fixed in this PR (commit 41374c1).
[Medium] logHabit left unvalidated — already addressed in commit ecf7f0b (pushed before this review's re-scan). Verification (all green): |
|
There was a problem hiding this comment.
Review Complete
Scope: PR #478 — "fix(web): validate Server Action API responses at the trust boundary" (diff spans web + mobile + shared)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. breaks-old-mobile-clients) | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
What it does
Adds an optional Zod schema param to serverAuthFetch/serverPublicFetch (apps/web/lib/server-fetch.ts) and apiClient (apps/mobile/lib/api-client.ts), validating responses at the trust boundary and throwing a typed ApiClientError(502, INVALID_RESPONSE_SCHEMA) on mismatch (mapped by getFriendlyErrorKey to a generic toast.errors.server, no schema leak to users). Threaded through 7 web Server Actions and 2 mobile hooks. Also fixes a real latent bug: logHabitResponseSchema's xpEarned/newAchievementIds/linkedGoalUpdates go from .optional() to .nullish() in packages/shared/src/types/habit.ts:343-345, since the API serializes explicit null — a strict, backward-compatible widening (verified: no field removed/renamed/newly-required, so no ⚠️ breaks old mobile clients marker applies).
The one finding (Medium, verified via adversarial skeptic pass)
Mobile omits trust-boundary schema validation for 5 of 7 threaded endpoints — apps/mobile/lib/queued-api-mutation.ts:26-33's hardcoded execute closure calls apiClient<TResult>(...) with no schema argument, and QueuedMutationBuildOptions (apps/mobile/lib/offline-mutations.ts) has no schema field at all, so useLogHabit, useBulkCreateHabits, useBulkDeleteHabits, useBulkLogHabits, useBulkSkipHabits (apps/mobile/hooks/use-habits.ts) stay unvalidated — confirmed by reading the files directly. This started as a draft High (rubric dimension 9: "missing parity" defaults to High) but was downgraded to Medium because: the PR body discloses this exact gap with a real technical reason (a runtime ZodType can't survive SQLite persistence/replay for queued mutations — closing it needs a serializable MutationType → schema registry, materially bigger than this PR), and it's already tracked in #479, not silently deferred. Recommend prioritizing #479 since it closes the same defect class on the harder (offline-replay) path.
Subagents
- parity-checker: PARTIAL (the Medium finding above; PAIRED on the core
server-fetch.ts↔api-client.tsplumbing and onsuggestHabitSetup/reportAchievementEvent) - contract-aligner: NOT VERIFIABLE IN CI (no
orbit-apisibling checkout in this job); local reasoning confirms no backward-compat break within this diff - i18n-syncer, security-reviewer, design-reviewer: N/A (no strings, no
orbit-apicode, no UI files touched)
Validation
Skipped per CI adaptation (Build / Unit Tests / SonarCloud run as separate required checks). PR body states the author ran lint, web/mobile type-check, @orbit/shared, web vitest, and React Doctor all green.
…es (#479, #451) (#498) * feat(client): validate API responses at the remaining client boundaries Extends the #478 trust-boundary validation seam to the two ingestion boundaries it left open. - Web client (#451): apps/web/lib/api-fetch.ts `apiFetch`/`fetchJson` gain an optional Zod `schema` param; a mismatch throws the typed ApiClientError(502, INVALID_RESPONSE_SCHEMA). Threaded through the gamification profile, streak, and notifications reads. - Mobile offline queue (#479): a MutationType->ZodType registry (mutation-response-schemas.ts) is consulted by BOTH the immediate execute path and the deferred flush replay, so a queued mutation's response is validated identically online or on flush. Registered logHabit + the four bulk habit mutations, each verified against its orbit-api DTO. Consolidated the safeParse/throw logic from #478's two copies (web server-fetch + mobile api-client) plus the two new seams into one shared `validateApiResponse` helper. Schemas tolerate additive API fields (unknown keys are stripped, never rejected) per the append-only contract. Tests cover, on both the web client seam and the mobile registry boundary: a valid response passes, a contract mismatch throws the typed 502, and an additive-unknown-field response still passes. Closes #479, Closes #451 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(shared): cover validateApiResponse at the shared-package level The web/mobile suites exercise the new validateApiResponse boundary helper, but their coverage reports under those projects, leaving the shared lines uncovered and dragging the PR's new-code coverage below the 80% gate. Add a direct unit test in the shared suite covering all branches: a matching body passes, additive unknown fields are stripped, a mismatch throws the typed 502 ApiClientError, and an absent schema returns the body unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(mobile): prove queue schema forwarding + mirror web read validation Addresses the two non-blocking review findings on #498. - Parity: mirror the web read-hook validation onto mobile — thread gamificationProfileSchema / streakInfoSchema / notificationsResponseSchema through mobile's useGamificationProfile, useStreakInfo, and useNotifications (same three reads changed on web), so the two platforms validate these responses identically. - Coverage of intent: add end-to-end tests that a registered mutation type (logHabit) forwards its real schema through BOTH offline-queue entry points — performQueuedApiMutation's default executor and processQueuedMutationFlush's replay — so a future refactor that drops the schema argument is caught, not only the unregistered (undefined) case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
serverAuthFetch/serverPublicFetch(web) andapiClient/parseApiResponse(mobile) previously cast API responses withJSON.parse(text) as T, skipping runtime validation at a trust boundary (code standard rule 8). A malformed or contract-drifted response flowed inward as mistyped data.This adds an optional Zod
schemaparameter to those helpers on both platforms. When supplied, the response body is.safeParse()d and, on mismatch, a typedApiClientError(502, code: INVALID_RESPONSE_SCHEMA)is thrown (not swallowed) —getFriendlyErrorKeymaps the 502 to a cleantoast.errors.servermessage with no schema leak.The matching
@orbit/sharedschemas are threaded through the highest-value habit + gamification Server Actions (web):suggestHabitSetup→habitSetupSuggestionSchema(AI endpoint — highest-value to validate)logHabit→logHabitResponseSchema(the app's most-frequent mutation)bulkCreateHabits/bulkDeleteHabits/bulkLogHabits/bulkSkipHabits→ their bulk result schemasreportAchievementEvent→reportEventResponseSchemaContract verification
Each threaded schema was checked field-by-field against the orbit-api DTO's real JSON serialization, including explicit-
nullhandling (the API does not setWhenWritingNull, so nullable fields serialize asnull). Threaded schemas use.nullable()/.nullish()for nullable leaves and Zod strips unknown keys, so enforcement is behavior-preserving.logHabit root-cause fix (was a latent schema bug)
logHabitResponseSchemamarkedxpEarned/newAchievementIds/linkedGoalUpdatesas.optional(), butLogHabitResponsein orbit-api defaults them tonulland the API serializes explicit nulls — so.optional().parse(null)would have rejected a valid response on the hottest path. Fixed in this PR by switching those three fields to.nullish()inpackages/shared/src/types/habit.ts(all consumers already read them with?./??, so the added| nullis behavior-preserving), then threading the schema throughlogHabit.Mobile parity
Included in this PR (the "web-only by design" framing was wrong — schema-validation-at-the-trust-boundary is not an allowed platform-adapter difference).
apps/mobile/lib/api-client.ts'sapiClient/parseApiResponsenow take the same optionalschema?: ZodType<T>parameter and throw the identical typedApiClientError(502, INVALID_RESPONSE_SCHEMA)on mismatch. The parameter is optional, so every un-threaded caller and the offline-queue replay path are behavior-unchanged. The shared response schemas are threaded through the two direct (non-queued) callers:useReportEvent→reportEventResponseSchemauseHabitSuggestion→habitSetupSuggestionSchema(replaces an ad-hoc.parse()that threw a bareZodError; now surfaces the typed 502 boundary error)logHabitand the four bulk habit actions run throughperformQueuedApiMutation, whose queued mutations are replayed later byprocessQueuedMutationFlushviaapiClient<unknown>reconstructed from a persistedQueuedMutationthat cannot carry a runtime schema function. Validating them consistently across the immediate and replay paths needs aMutationType → ZodTyperegistry consulted by both paths — a broader offline-queue change kept out of this surgical response-boundary fix and tracked in #479 (now narrowed to those queue-coupled callers).Tests
Web
__tests__/lib/server-fetch.test.tsgains cases for both helpers: valid payload parses (and strips extras), malformed payload rejects with the typed 502 error, empty/204 and 404 skip validation.habits-extended.test.tsbulk fixtures were corrected to include the always-presentindexfield they had been omitting. Mobile__tests__/lib/api-client.test.tsgains the mirrored boundary cases (valid parse strips extras, malformed → typed 502, empty/204 skips validation). lint + web/mobile type-check +@orbit/shared(1494) + web vitest (2239) + mobile vitest (1056) + React Doctor (--scope changed, 0/0) all green.Refs #243