Audit 1: Frontend Implementation Document Review
Review of all documents in the 3-frontend-implementation/ project directory
and referenced parent project context. Identifies inconsistencies, ambiguities,
missing requirements, security vulnerabilities, and deficiencies.
Findings
Section titled “Findings”| # | File | Lines | Issue Summary | Recommendation |
|---|---|---|---|---|
| 1 | BFF spec (bff-specification.md) vs requirements.md vs goal.md | bff-spec:52-71, req:REQ-FE-001/010, goal:119-136 | Upload endpoint path contradiction. BFF spec and TD-13 specify entity-scoped POST /api/item/<itemEId>/image-upload-url, but REQ-FE-001 specifies POST /v1/item/image-upload (module-scoped, no entity ID), and goal.md explicitly states “the endpoint is module-scoped (not entity-scoped)”. The 3.5 spec T-5 re-export path uses entity-scoped (src/app/api/item/[itemEId]/image-upload-url/route.ts), contradicting the requirements. The api-proxy preparation doc confirms module-scoped. | Align all documents to one path. The preparation doc and REQ-FE-001 say module-scoped; TD-13 says entity-scoped. Resolve which is correct and update BFF spec, 3.5-bff-routes T-5, and SPA spec interface table. |
| 2 | SPA spec (spa-specification.md) | Line 44 (SPA-FR-009) | Stale requirement: copyright checkbox vs. inline text. SPA-FR-009 says “Present copyright acknowledgment checkbox. Block upload until accepted.” But goal.md (lines 242-245) and the UX implementation state the checkbox was removed and replaced with inline footer text — confirm is always enabled. | Update SPA-FR-009 to reflect the current design: inline copyright text in confirm footer, no blocking checkbox. Reference TD-04 and the UX post-implementation spec D-03. |
| 3 | analysis.md vs specification.md | analysis.md throughout, specification.md lines 107-218 | Phase naming inconsistency. analysis.md uses old naming (Phase 3a, 3b, 4a, 4b, 4c). specification.md uses 3.1-3.7. Both are in the same project directory. A reader cannot easily map between them. | Update analysis.md to use the 3.1-3.7 numbering, or add a mapping table. |
| 4 | goal.md | Lines 366-421 | Duplicate deliverable numbering. Deliverable #12 appears twice (BFF route tests, then lifecycle types). Deliverables #28/#29 appear twice (CDN provider + form fields, then grid + form). This makes the deliverable table unreliable as a checklist. | Re-number deliverables sequentially without gaps or duplicates. |
| 5 | specification.md | Lines 289-303 (Open Questions table) | Stale open questions. Questions #3 (ImageFormField prop naming), #4 (TypeaheadCellEditor), and #5 (CloudFront key loading) are listed as “Pending”, but the sub-phase specs and decision log (FD-10) already resolved them. | Update the specification.md open questions table to reflect decisions already made in sub-phase specs and FD-10. |
| 6 | decision-log.md (FD-10) vs 3.5-bff-routes spec (T-4) | decision-log:108, 3.5:415-419 | Env var name contradiction. FD-10 says CLOUDFRONT_SIGNING_KEY_SECRET_ARN is an env var. But 3.5 T-4 says “The CLOUDFRONT_SIGNING_KEY_SECRET_ARN env var is no longer needed — the secret name is sufficient.” The T-4 unit test table (line 448) still references this removed env var. | Update FD-10 to reflect the naming convention approach. Remove the stale env var reference from the T-4 unit test table. |
| 7 | 3.5-bff-routes spec (T-8) and requirements.md (REQ-FE-017/018) | 3.5:614-619, req:REQ-FE-017/018 | Missing Max-Age/Expires on CDN cookies. Cookie attributes are specified as Secure; HttpOnly; SameSite=Lax; Domain; Path but no Max-Age or Expires is set. Without these, cookies become session cookies and survive indefinitely until browser close, regardless of the 30-min CloudFront policy TTL. Users could accumulate stale cookies, and the browser won’t self-clean them. | Add Max-Age (matching cookie TTL, e.g. 1800 for 30 min) to the cookie attributes in T-8, REQ-FE-017, and BFF spec. |
| 8 | 3.5-bff-routes spec (T-7) | Lines 559-593 | No Content-Type validation on fetch-url response. The route fetches arbitrary URLs and streams the response without verifying it’s actually an image. An attacker could trick the BFF into proxying HTML, JavaScript, or other content types to the SPA. The check-url route (T-6) has a similar issue — it returns the remote server’s claimed Content-Type without validation. | Add Content-Type validation to both routes: reject responses that don’t match image/* MIME types. Return 422 with a descriptive error for non-image content. |
| 9 | 3.5-bff-routes spec (T-2) | Lines 277-279 | DNS rebinding TOCTOU vulnerability acknowledged but untracked. The SSRF validator documents a DNS rebinding risk (DNS resolves safe at check time, private at connect time) as a “known limitation” but creates no ticket or follow-up item. | Create a tracking ticket for IP pinning (passing resolved IPs to fetch() via custom agent/lookup option). Reference it in the spec as a known-risk follow-up. |
| 10 | 3.5-bff-routes spec (T-1c) | Lines 175-240 | Secret cache has no TTL or invalidation. getSecret() caches indefinitely. If a CloudFront signing key is rotated in Secrets Manager, the BFF serves stale keys until process restart. The BFF spec’s CdnCookiesKeyRotationTest (line 239) implies key rotation support but the implementation doesn’t support it. | Add a configurable cache TTL (e.g., 1 hour) to the secrets utility, or expose a clearSecretCache() for key rotation scenarios. Document the expected rotation workflow. |
| 11 | 3.6-spa-integration spec (T-8, Open Question #1) | Lines 241-259 | 403 recovery gap for non-grid image display contexts. Option B was chosen (grid wrapper handles 403), but ImageFormField and ImageInspectorOverlay also render CDN images and have no 403 recovery specified. The spec acknowledges this (“those contexts need their own wiring”) without specifying how or when. | Add concrete 403 recovery specifications for ImageFormField and ImageInspectorOverlay, or create a follow-up ticket. Consider a shared useImageWithCdnRecovery hook that all image-displaying contexts can use. |
| 12 | goal.md | Lines 301-302 | Accessibility explicitly deferred with no timeline. “Accessibility/WCAG requirements — documented as NFR-022 through NFR-025, not in V1.” No keyboard navigation for upload dialog, no screen reader announcements for upload progress/errors, no alt text strategy for CDN images. Shipping an image feature without any accessibility is a significant gap. | At minimum, add keyboard support for the upload dialog (focus trap, Escape to close) and meaningful alt attributes. Create a follow-up ticket for full WCAG compliance with a target timeline. |
| 13 | 3.5-bff-routes spec (T-6) | Lines 531-536 | check-url fallback from HEAD to GET downloads content. The check-url route is supposed to verify reachability only, but on HEAD failure it performs a GET with Range: bytes=0-4095, downloading up to 4 KB of potentially malicious content through the BFF. | Consider using only HEAD for check-url (report unreachable if HEAD fails). Reserve GET-with-Range for fetch-url only. Or explicitly document why partial GET is necessary and what happens with the downloaded bytes (discarded after inspection). |
| 14 | 32-lifecycle-framework spec (T-4) | Interface definition, ~line 70 | useDraft resets on initialValue reference change — identity vs. structural equality. “Resets draft when initialValue reference changes” uses reference equality. If a parent re-renders with a structurally identical but new reference (common in React), the draft resets unexpectedly, losing user edits. | Specify whether reset uses reference or deep equality. If reference, document the parent’s obligation to memoize initialValue. Consider offering an optional isEqual parameter. |
| 15 | 32-lifecycle-framework spec (T-3) | setNestedField signature | setNestedField accepts value: unknown, losing type safety. In a strict TypeScript codebase, accepting unknown means any field can be set to any value without compile-time validation. | Consider a type-safe overload using template literal types or mapped types to constrain the value type based on the path. Or document this as a deliberate trade-off with the recommendation to use update() with a typed updater function for type safety. |
| 16 | 3.5-bff-routes spec (T-3) | Lines 346-351 | Rate limiter returns user-facing error messages from the BFF. The error message is written “for non-technical users” and surfaced directly in the SPA. This couples the BFF to UI copy — localization, wording changes, and A/B testing all require BFF changes. | Return a machine-readable error code from the BFF (e.g., { code: "RATE_LIMITED", retryAfterMs }) and let the SPA own the user-facing message text. |
| 17 | specification.md | Lines 39-43 | component-preparation prerequisite references non-existent directory. The spec links to ../component-preparation/goal.md as a mandatory prerequisite, but the project directory listing shows no component-preparation directory under item-image-upload/. | Verify this prerequisite project exists and is correctly linked. If it’s in a different location, fix the path. If it hasn’t been created yet, note it as a blocker. |
| 18 | Directory name | 3-frontend-implementation/ | Typo in directory name — “implementatoin” instead of “implementation”. This propagates through every link and reference in the project. | Rename the directory to 3-frontend-implementation/ and update all references. Better to fix early before more documents reference this path. |
| 19 | verification.md vs requirements.md | verification:28 (REQ-FE-020) | REQ-FE-020 (“BFF shall not buffer file bytes”) verified only by “Code review”. This is an architectural constraint that’s easy to violate in future changes. There’s no automated enforcement. | Add a structural test or lint rule that checks src/server/routes/ files don’t use Buffer, arrayBuffer(), or stream accumulation patterns. Or add an architectural decision record noting this must be validated on every BFF PR. |
| 20 | 3.6-spa-integration spec (T-2) | Lines 57-64 | SPA duplicates api-proxy types instead of sharing. The spec says ImageUploadUrlRequest/ImageUploadUrlResponse are defined in src/api/image-upload.ts (SPA) separately from api-proxy, even though “shapes are identical today.” The rationale (independent evolution, boundary enforcement) is reasonable but creates a maintenance risk — if the Backend contract changes, two locations need updating. | Add a comment/test that validates the SPA types match the Backend contract (e.g., a type compatibility test or contract test). Or document the sync obligation in the maintenance section. |
| 21 | requirements.md (REQ-FE-073) vs 3.6 spec (T-3) | req:REQ-FE-073, 3.6:88-90 | Single upload enforcement scope ambiguity. REQ-FE-073 says “per browser session.” 3.6 T-3 says “mutation is disabled while another is pending.” These are different scopes — “per browser session” implies cross-tab enforcement, while “while pending” is per-React-tree. TanStack mutation state is per QueryClient (per tab). | Clarify scope: if per-tab is sufficient (likely), update REQ-FE-073 to say “per browser tab” or “per application instance.” If cross-tab is actually needed, a BroadcastChannel or localStorage lock would be required. |
| 22 | goal.md | Line 300 | Orphaned S3 objects explicitly deferred with no mitigation. If upload succeeds but entity persist fails, S3 objects are orphaned. No compensating action, no S3 lifecycle rule, no cleanup job. Over time this accumulates storage cost and stale data. | Add an S3 lifecycle rule (e.g., delete objects older than 24h that aren’t referenced by any entity). Or specify a deferred-cleanup mechanism with a tracking ticket. |
| 23 | BFF spec (bff-specification.md) | Lines 207, 211 | Module location mismatch. BFF spec says cloudfront-signer goes in src/lib/cloudfront-signer.ts, but the 3.5-bff-routes spec (which is the implementation spec) puts it in src/server/lib/cloudfront-signer.ts per FD-02. | Update BFF specification to reflect the FD-02 directory structure (src/server/lib/). |
| 24 | Not in any document | — | No Content-Security-Policy guidance for CDN domain. The SPA loads images from <partition>.<infra>.assets.arda.cards, but no document mentions updating the CSP img-src directive to allow this domain. A missing CSP entry would block all CDN images in production. | Add a task or note in Phase 3.6 or 3.7 to update the CSP headers to include the assets.arda.cards domain pattern for img-src. |
| 25 | 3.1-api-proxy-publish spec | T-0, RequestContext | X-oidc-subject header added in api-proxy but not referenced in BFF routes. The api-proxy RequestContext includes userId (maps to X-oidc-subject), but none of the BFF route specs (T-5 through T-8) mention populating or sending this header. REQ-FE-010 also omits it. | Either add X-oidc-subject to the BFF route specifications and REQ-FE-010, or document why it’s intentionally omitted from the image upload routes. |
| 26 | 33-component-updates spec (T-2) | Lines ~67-72 | console.warn in production for missing hooks. The spec says ImageCellEditor should console.warn when hooks aren’t provided. In production, this warn would fire during SSR or if column definitions are assembled before provider tree is ready. | Use process.env.NODE_ENV === 'development' guard on the console.warn, or use a different signaling mechanism (e.g., TypeScript-only — make hooks required in the type and optional only in the Storybook story config). |
| 27 | 3.5-bff-routes spec (T-6, step 6) | Lines 534-536 | check-url GET fallback leaks Range header capability. Step 6 says “attempt a GET with a Range: bytes=0-4095 header.” Not all servers support Range requests; some will return the full response. The BFF should abort/discard after reading the headers, but this isn’t specified. | Specify that the GET response body is discarded (only headers are inspected for Content-Type/Content-Length). Add AbortController.abort() after reading headers. |
| 28 | 33-component-updates spec (T-1), infrastructure | ImagePreviewEditor, ImageStorageStack | Cross-origin canvas tainting on edit-existing-image flow. When a user re-crops an existing CDN image, the editor draws it onto a <canvas>. The CDN (assets.arda.cards) and SPA (app.arda.cards) are different origins. Without CORS headers from CloudFront, the canvas is tainted and toBlob() throws SecurityError. The SPA must set crossOrigin="use-credentials" (signed cookies required), which triggers CORS preflight that CloudFront doesn’t currently support. | Add CloudFront Response Headers Policy with Access-Control-Allow-Origins: https://*.arda.cards and Access-Control-Allow-Credentials: true. In Phase 3.3, set crossOrigin="use-credentials" on the crop <img> only for CDN URLs. |
Severity Classification
Section titled “Severity Classification”Critical (resolve before implementation)
Section titled “Critical (resolve before implementation)”- #1 — Upload endpoint path contradiction
- #7 — Missing cookie
Max-Age - #8 — No Content-Type validation on fetch
High (security/correctness)
Section titled “High (security/correctness)”- #9 — DNS rebinding TOCTOU untracked
- #10 — No secret cache TTL
- #13 —
check-urldownloads content - #22 — Orphaned S3 objects
- #28 — Cross-origin canvas tainting on edit-existing-image
Medium (consistency/maintainability)
Section titled “Medium (consistency/maintainability)”- #2 — Stale SPA-FR-009
- #3 — Phase naming mismatch
- #4 — Duplicate deliverable numbers
- #5 — Stale open questions
- #6 — Env var name contradiction
- #11 — 403 recovery gap for non-grid contexts
- #14 —
useDraftreference equality - #17 — Missing prerequisite directory
- #18 — Directory name typo
- #20 — Duplicated types without sync mechanism
- #21 — Single upload scope ambiguity
- #23 — Module location mismatch
- #25 —
X-oidc-subjectheader gap - #26 —
console.warnin production - #27 — GET fallback body not discarded
Low (improvement opportunities)
Section titled “Low (improvement opportunities)”- #12 — Accessibility deferred
- #15 —
setNestedFieldtype safety - #16 — User-facing text in BFF
- #19 — REQ-FE-020 enforcement
Decisions and Resolutions
Section titled “Decisions and Resolutions”| # | Decision | Resolution | Status |
|---|---|---|---|
| 1 | OpenAPI confirms module-scoped POST /v1/item/image-upload (no entity ID). Update all docs. | Updated: BFF spec, SPA spec, 3.5-bff-routes T-5, decision-log TD-13, requirements. | Done |
| 2 | Update SPA-FR-009 to inline text, no blocking checkbox. | Updated: SPA spec SPA-FR-009 + event table + scenario mapping. | Done |
| 3 | Add phase naming mapping note to analysis.md. | Updated: analysis.md with :::note admonition after Executive Summary. | Done |
| 4 | Re-number deliverables sequentially. | Updated: goal.md — ux-prototype #13-21, Phase 4 #22-35. | Done |
| 5 | Update stale open questions in specification.md. | Already resolved in sub-phase specs. No change needed. | Done |
| 6 | Update FD-10 env var, fix T-4 test table. | Updated: decision-log FD-10, 3.5-bff-routes T-4 test setup. | Done |
| 7 | Add Max-Age=1800 to CDN cookie attributes. | Updated: 3.5-bff-routes T-8, requirements REQ-FE-017. BFF spec already had it. | Done |
| 8 | Add Content-Type image/* validation to fetch-url and check-url. | Updated: 3.5-bff-routes T-6 and T-7 with validation steps + tests. BFF spec already had it. | Done |
| 9 | Create tracking ticket for DNS rebinding IP pinning. | Created: arda-frontend-app#738. | Done |
| 10 | Add 8-hour default cache TTL to secrets utility. | Updated: 3.5-bff-routes T-1c with TTL eviction logic + tests. | Done |
| 11 | Add shared useImageWithCdnRecovery hook for all image display contexts. | Updated: 3.6-spa-integration T-7b (new task), T-8 and T-9 wiring, Open Question #1 mitigation. | Done |
| 12 | Ignore (accessibility). | No action. | N/A |
| 13 | Document GET fallback body discarding. Combined with #27. | Updated: 3.5-bff-routes T-6 step 6 clarification. | Done |
| 14 | Add isEqual parameter to useDraft with memoization docs. | Updated: 32-lifecycle-framework T-4 interface + tests. | Done |
| 15 | Document as accepted limitation. Add comment to ux-prototype#77. | Comment added: ux-prototype#77. | Done |
| 16 | Machine-readable error codes from BFF. SPA owns user-facing text. | Updated: 3.5-bff-routes T-3, 3.6-spa-integration T-2. | Done |
| 17 | Fix prerequisite path to roadmap/completed/component-preparation. | Updated: specification.md prerequisites table link. | Done |
| 18 | Ignore (directory name typo). | No action — renaming would break existing links. | N/A |
| 19 | Add to Decision Log as FD-11 (code review obligation). | Updated: decision-log.md FD-11. | Done |
| 20 | SPA types re-exported from api-proxy through shared barrel. | Updated: 3.6-spa-integration T-2 type ownership section. | Done |
| 21 | Clarify: per browser tab (per application instance). | Updated: requirements.md REQ-FE-073. | Done |
| 22 | Ignore (orphaned S3 objects). | No action — tracked as deferred NFR-012. | N/A |
| 23 | Acknowledge server/lib in FD-02, open door to future client/lib. | Updated: decision-log.md FD-02 outcome text. | Done |
| 24 | No action needed. No CSP exists; CDN images use <img> tags (not restricted). Advisory for future CSP adoption. | No change. Downgraded from Critical to advisory note. | N/A |
| 25 | X-oidc-subject extracted from JWT sub claim by BFF. | Updated: requirements REQ-FE-010, 3.5-bff-routes T-5, 3.1-api-proxy T-0. | Done |
| 26 | Make hooks required in type; optional only in Storybook story config. | Updated: 33-component-updates T-2 interface + tests. | Done |
| 27 | Combined with #13. | See #13. | Done |
| 28 | Add CloudFront CORS policy (infrastructure) + crossOrigin="use-credentials" in Phase 3.3. | Infrastructure: infrastructure#439. SPA: 33-component-updates T-1 updated with crossOrigin spec and tests. | Done |
Copyright: (c) Arda Systems 2025-2026, All rights reserved
Copyright: © Arda Systems 2025-2026, All rights reserved