feat(media): implement complete media management system (#28)#38
Open
temi-Dee wants to merge 8 commits into
Open
feat(media): implement complete media management system (#28)#38temi-Dee wants to merge 8 commits into
temi-Dee wants to merge 8 commits into
Conversation
…an detection - Add media library admin interface with grid view and filtering capabilities - Implement media upload functionality with drag-and-drop zone support - Add orphan media detection to identify unused assets in storage - Create cover image picker component for article integration - Add media detail modal for viewing and managing asset metadata - Implement media API routes for CRUD operations and orphan detection - Add Stellar hash anchoring support for media attestation - Configure Sharp image processing library for server-side optimization - Add media service layer for storage operations and migrations - Create database migration for media library schema - Update environment configuration with Stellar network settings - Add VSCode workspace settings for development consistency
- Add media_library table migration with Storage bucket policies and RLS - Add upload service with Sharp optimization pipeline (thumb/sm/md/lg/og variants) - Add media library admin UI with grid view, filters, bulk actions, and detail modal - Add image picker modal and cover image picker for article editor integration - Add media API routes for upload, CRUD, orphan detection, and article refs - Add Stellar SHA-256 hash anchoring with opt-in per upload - Add one-time migration script for existing external image URLs - Add root middleware for Supabase session refresh - Fix SSRF, path traversal, log injection, and OS command injection vulnerabilities - Configure next/image remotePatterns for Supabase Storage domain - Add VSCode workspace settings and extension recommendations
|
@temi-Dee is attempting to deploy a commit to the ACTA Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a Supabase-backed media library (upload, optimization, variants, DB + RLS, admin UI, migration, orphaning, optional Stellar anchoring) and a set of Claude agent prompts/workflow files plus workspace/editor config and Next.js middleware. ChangesMedia Management System
Claude AI Specification Workflow Framework
Sequence DiagramsequenceDiagram
participant Client
participant API_Route as /api/media/upload
participant UploadService as uploadMedia()
participant Optimize as generateVariants()
participant SupabaseStorage as Supabase Storage
participant Database as media_library
participant Stellar as Stellar (optional)
Client->>API_Route: POST /api/media/upload (FormData, anchorOnStellar?)
API_Route->>UploadService: validate + compute sha256 + get dimensions
UploadService->>Optimize: generateVariants(buffer)
Optimize-->>UploadService: variants[]
UploadService->>SupabaseStorage: upload original + variants
UploadService->>Database: insert media_library row
alt anchorOnStellar true
UploadService->>Stellar: anchorHashOnStellar(mediaId, contentHash)
Stellar-->>UploadService: txHash
UploadService->>Database: update stellar_tx_hash
end
UploadService-->>API_Route: UploadResponse (media, publicUrl)
API_Route-->>Client: 201 Created { media, publicUrl }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
src/lib/storage/migrate-images.ts-183-194 (1)
183-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing error handling for
media_libraryinsert.The insert operation returns
{ data, error }but the error is never checked. If the insert fails (e.g., constraint violation, network issue), the function still returns the Storage URL, leaving an orphan file with no tracking record.🐛 Proposed fix
// Insert media_library record - await supabase.from('media_library').insert({ + const { error: insertError } = await supabase.from('media_library').insert({ bucket, path, original_name: filename, mime_type: mimeType, size_bytes: buffer.byteLength, width: width || null, height: height || null, variants: variantPaths, content_hash: contentHash, uploaded_by: uploadedBy, }); + + if (insertError) { + throw new Error(`media_library insert failed: ${insertError.message}`); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 183 - 194, The insert into the media_library via supabase.from('media_library').insert(...) currently ignores the returned { data, error } — check that result and on error log the error and clean up the just-uploaded object (call supabase.storage.from(bucket).remove([path]) or equivalent) then throw or return a failure so the caller doesn't get a Storage URL for an untracked file; ensure the function only returns the Storage URL after the insert succeeds and include the error details in the thrown/logged message for debugging..claude/agents/kfc/spec-design.md-127-127 (1)
127-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDesign output path is contradictory (
.claudevs.kiro).Line 127 requires
.claude/specs/{feature_name}/design.md, but Line 133 requires.kiro/specs/{feature_name}/design.md. This creates nondeterministic behavior and breaks orchestration contracts with the starter workflow.Proposed fix
- - The model MUST create a detailed design document at '.kiro/specs/{feature_name}/design.md' + - The model MUST create a detailed design document at '.claude/specs/{feature_name}/design.md'Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/agents/kfc/spec-design.md at line 127, The spec-design doc contains two conflicting output paths ('.claude/specs/{feature_name}/design.md' vs '.kiro/specs/{feature_name}/design.md'); pick one canonical path and make all references consistent (e.g., replace the '.kiro/...' occurrence with '.claude/...' or vice versa) or centralize the path behind a single constant/variable (e.g., SPEC_PATH or SPEC_DIR) used by the checks that mention these paths so both Line 127 and Line 133 resolve to the same deterministic path..claude/agents/kfc/spec-judge.md-17-17 (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJudge input contract conflicts with workflow caller contract.
This file expects
documents(list), but the workflow starter definesdoc_pathfor judge calls. That mismatch will cause invocation failures or dropped candidates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/agents/kfc/spec-judge.md at line 17, The judge spec expects a parameter named "documents" but the workflow caller sends "doc_path", causing invocation mismatches; update the judge contract in .claude/agents/kfc/spec-judge.md to accept "doc_path" (or add "doc_path" as an alias) and ensure any referenced handler or entrypoint that reads "documents" (the judge input contract) is updated to read "doc_path" instead so the workflow starter and judge use the same parameter name..claude/agents/kfc/spec-requirements.md-48-49 (1)
48-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent requirements filename contract will break parallel workflow handoff.
Line 48–49 defines
requirements{output_suffix}.md(e.g.,requirements_v1.md), but Line 73 requiresrequirements_{output_suffix}.md. With suffixes like_v1, Line 73 yieldsrequirements__v1.md, which conflicts with the rest of the workflow and judge patterns.Proposed fix
- - The model MUST create a '.claude/specs/{feature_name}/requirements_{output_suffix}.md' file if it doesn't already exist + - The model MUST create '.claude/specs/{feature_name}/requirements{output_suffix}.md' when output_suffix is provided; otherwise '.claude/specs/{feature_name}/requirements.md'Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/agents/kfc/spec-requirements.md around lines 48 - 49, The codebase has two conflicting filename templates for the requirements file: "requirements{output_suffix}.md" and "requirements_{output_suffix}.md", causing double underscores when output_suffix already includes a leading underscore; pick one convention and make it consistent. Fix by normalizing how output_suffix is composed and used: either (A) require output_suffix to have no leading underscore and use the "requirements_{output_suffix}.md" template everywhere, or (B) allow output_suffix to include a leading underscore and use "requirements{output_suffix}.md" everywhere; update the occurrences of the templates (the places referencing output_suffix and the two templates) to the chosen single form and ensure validators/judge patterns expect that same pattern. Ensure all references to output_suffix, requirements{output_suffix}.md, and requirements_{output_suffix}.md are updated to the single agreed convention..claude/agents/kfc/spec-test.md-88-90 (1)
88-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
{module}is undefined in inputs, so test artifact naming is under-specified.The process and constraints require
{module}.md/{module}.test.ts, but no input providesmoduleand no derivation rule is specified. Different agents may produce inconsistent filenames.Proposed fix
## INPUT ... - spec_base_path: Spec document base path + - module_name: Target module/file stem for test artifacts (e.g., "media-service") ... - - First create test case documentation ({module}.md) - - Create corresponding test code ({module}.test.ts) based on test case documentation + - First create test case documentation ({module_name}.md) + - Create corresponding test code ({module_name}.test.ts) based on test case documentationAlso applies to: 103-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/agents/kfc/spec-test.md around lines 88 - 90, The spec references placeholder filenames `{module}.md` and `{module}.test.ts` without defining `module`, causing under-specified artifact naming; update the spec in .claude/agents/kfc/spec-test.md to require a concrete `module` input (or define a deterministic derivation rule such as using `artifactName`, `packageName`, or the primary feature identifier) and add validation steps: require `module` in inputs, or auto-derive it (e.g., from `task.name` or `targetFile`), and specify the exact filename template to use; ensure the spec text and any downstream generator/validator enforce and error if `module` is missing so produced filenames are consistent across agents..claude/system-prompts/spec-workflow-starter.md-196-204 (1)
196-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJudge invocation schema mismatches the judge agent interface.
This workflow defines
doc_path, butspec-judgerequiresdocuments(list of candidate paths). Parallel evaluation will fail or evaluate only one candidate unless the schema is aligned.Proposed fix
- - doc_path: Document path + - documents: List of document paths to evaluate🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/system-prompts/spec-workflow-starter.md around lines 196 - 204, The spec-judge invocation schema is using doc_path (single path) but the spec-judge agent expects documents (a list of candidate paths), so update the workflow schema: replace or augment the doc_path field with documents: string[] and ensure the judge block (named "Judge - spec-judge" / spec-judge invocation) uses the documents array; remove any single-path-only handling and adjust any consumers to iterate over documents for parallel evaluation..claude/agents/kfc/spec-judge.md-90-90 (1)
90-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScore aggregation logic is incorrect in evaluation pseudocode.
'total': sum(scores)sums the outer accumulator, not the current document’s criteria. This can produce invalid totals and wrong winner selection.Proposed fix
- 'total': sum(scores), + 'total': ( + evaluate_completeness(doc) + + evaluate_clarity(doc) + + evaluate_feasibility(doc) + + evaluate_innovation(doc) + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/agents/kfc/spec-judge.md at line 90, The 'total' field is wrongly computed with 'total': sum(scores) which sums the outer accumulator instead of the current document's criteria; change it to sum the per-document criteria scores (e.g., 'total': sum(criteria_scores) or 'total': sum(scores.values()) depending on whether per-document scores are stored in a list or dict) so that the 'total' reflects the current document's criteria sum; ensure you reference the per-document variable used in the surrounding loop (e.g., criteria_scores, perDocumentScores, or scores when it is the per-doc map) rather than the outer accumulator.src/lib/storage/media.service.ts-189-194 (1)
189-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThrow on failed
usage_countupdates inside the rescan loop.Each per-item
.update({ usage_count: count })inrecalculateUsageCounts()awaits the write but doesn’t check/throw on the returned Supabaseerror, so a failed update can leave staleusage_countwhile the function still resolves.Suggested change
for (const item of mediaItems ?? []) { const count = countOccurrences(allText, item.path); - await supabase + const { error } = await supabase .from('media_library') .update({ usage_count: count }) .eq('id', item.id); + if (error) throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/media.service.ts` around lines 189 - 194, In recalculateUsageCounts(), the per-item Supabase update inside the loop (for ... of mediaItems) awaits the .update({ usage_count: count }).eq('id', item.id) call but doesn't check the returned response for an error; modify the loop to capture the update response (e.g., const { error } = await supabase.from('media_library').update(...).eq(...)) and throw or propagate a new Error containing the item.id and the Supabase error if error is truthy so failed updates do not silently leave stale usage_count values.next.config.ts-5-13 (1)
5-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
next/imageremotePatterns to your project Supabase host.
next.config.tscurrently allowshttps://**.supabase.co/storage/v1/object/public/**.cover_image_urlfrom the DB is forwarded unmodified into<Image src={article.coverImageUrl} />(and the same pattern for monthly reviews), so any stored Supabase tenant URL matching that wildcard will be fetched/optimized. Narrow the hostname to your ownNEXT_PUBLIC_SUPABASE_URLhost.Suggested change
+const supabaseHostname = process.env.NEXT_PUBLIC_SUPABASE_URL + ? new URL(process.env.NEXT_PUBLIC_SUPABASE_URL).hostname + : undefined; + const nextConfig: NextConfig = { reactCompiler: true, images: { - remotePatterns: [ - { - protocol: 'https', - hostname: '**.supabase.co', - pathname: '/storage/v1/object/public/**', - }, - ], + remotePatterns: supabaseHostname + ? [ + { + protocol: 'https', + hostname: supabaseHostname, + pathname: '/storage/v1/object/public/**', + }, + ] + : [], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@next.config.ts` around lines 5 - 13, The images.remotePatterns in next.config.ts is too permissive (hostname '**.supabase.co')—restrict it to your project's Supabase host by reading NEXT_PUBLIC_SUPABASE_URL and using that exact hostname instead of the wildcard; update the images.remotePatterns entry (the images object in next.config.ts) to use the specific host derived from NEXT_PUBLIC_SUPABASE_URL so <Image src={article.coverImageUrl} /> only allows your tenant’s storage URLs.src/lib/storage/optimize.ts-30-40 (1)
30-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBypass SVG variants instead of returning raw SVG as
"thumb".
uploadMedia()currently uploads every variant with a.webpsuffix andcontentType: 'image/webp'. Returning the original SVG buffer here means SVG uploads will produce a*-thumb.webpobject that actually contains SVG bytes.Return no variants for SVGs, or carry per-variant format metadata through the pipeline so the uploader can preserve the real MIME type and extension.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/optimize.ts` around lines 30 - 40, The current SVG branch in optimize (the mimeType === 'image/svg+xml' block) returns a single "thumb" variant which later causes uploadMedia to save an SVG buffer with a .webp/contentType:image/webp; change this behavior by returning no variants for SVGs (i.e., return an empty array) or alternatively enrich the variant objects with explicit format/ mime metadata so uploadMedia can preserve the original MIME type and extension; update the optimize function's SVG branch (and any variant shape/type definitions it uses) and ensure uploadMedia reads per-variant format fields if you choose the metadata approach.src/lib/stellar/anchor.ts-96-115 (1)
96-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually verify the anchored hash, not just transaction success.
expectedHashis ignored, so any successful transaction hash currently returnsverified: true. That makes the verification badge a false positive whenever the tx exists but does not contain this media item's hash.Fetch the transaction operations and compare the
manage_dataentry's key/value against the expected media key and hash before returningverified: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/stellar/anchor.ts` around lines 96 - 115, The current verifyAnchor function returns verified based only on transaction success; modify verifyAnchor to fetch the transaction operations (use server.operations().forTransaction(txHash).call()), iterate the returned operations and locate any op where op.type === 'manage_data', then compare that operation's key/value against the expected media entry: decode op.value if it is base64-encoded and check value === expectedHash (and optionally check op.name/op.key equals the expected media key if your codebase has a specific key name); only return { verified: true, ledger } when a matching manage_data entry is found, otherwise return { verified: false }.src/lib/storage/upload.ts-130-185 (1)
130-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback uploaded objects when the metadata insert fails.
The original file and any successful variants are already in public storage before the
media_libraryinsert runs. If that insert fails, this throws and leaves orphaned objects behind with no DB record to manage them.Suggested fix
// 5. Upload original to Storage const originalPath = generatePath(typedMime); + const uploadedPaths = [originalPath]; const { error: uploadError } = await supabase.storage .from(bucket) .upload(originalPath, fileBuffer, { contentType: mimeType, upsert: false, @@ if (variantError) { // Non-fatal: log and continue const safeKey = variant.key.replace(/[\r\n]/g, '_'); console.error(`Failed to upload variant ${safeKey}:`, variantError.message); } else { variantPaths[variant.key] = variantPath; + uploadedPaths.push(variantPath); } } @@ if (insertError || !record) { + await supabase.storage.from(bucket).remove(uploadedPaths).catch(() => undefined); throw new Error(`Failed to record media metadata: ${insertError?.message}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/upload.ts` around lines 130 - 185, If the final insert into media_library (check insertError or !record) fails, remove the uploaded files to avoid orphaned objects: call supabase.storage.from(bucket).remove([...]) with the originalPath plus any values from variantPaths (only include successful variantPaths entries), log any removal errors, and then rethrow the original insertion error; update the error-handling branch that references insertError/record to perform this cleanup using the existing symbols originalPath, variantPaths, bucket, supabase.storage and then throw the same failure Error as before.src/app/api/media/route.ts-17-25 (1)
17-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp and validate pagination inputs before querying.
Number(...)acceptsNaN,0, negative values, and arbitrarily largepageSize. Since this endpoint is public, a crafted query can force malformed ranges or very large scans/responses. Reject non-finite values and cappageSizeserver-side.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/media/route.ts` around lines 17 - 25, The pagination inputs in the MediaListFilters construction (fields page and pageSize in src/app/api/media/route.ts) are currently parsed with Number(...) which can yield NaN, 0, negatives or huge values; update the parsing to validate and clamp: parse as integer, ensure isFinite and >= 1 for page (default to 1 if invalid) and ensure pageSize is an integer within an acceptable server-side cap (e.g. clamp to 1..100, default 24 if invalid); implement this validation before building the MediaListFilters object (refer to the page and pageSize properties and MediaListFilters) and return or sanitize inputs rather than relying on client-provided values.src/app/api/media/upload/route.ts-70-79 (1)
70-79:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove Stellar anchoring off the request path.
anchorHashOnStellar()is awaited inside the upload handler, so opted-in uploads still block on an external network call despite the “non-blocking” comment. Slow or flaky anchoring can turn a successful upload into a timeout. Persist the upload first, then anchor asynchronously with its own retry path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/media/upload/route.ts` around lines 70 - 79, The Stellar anchoring call is currently awaited inside the upload handler (anchorHashOnStellar and updateStellarTxHash), blocking the request; change this so the handler persists the upload and returns immediately, then kick off anchoring asynchronously (e.g., enqueue a background job or spawn a non-awaited worker using result.media.id and result.media.contentHash) that performs anchorHashOnStellar and updateStellarTxHash with its own retry/backoff and error logging; ensure the upload route does not await anchorHashOnStellar/updateStellarTxHash and still records the media ID so the background job can pick it up and store stellarTxHash when successful.src/app/admin/layout.tsx-8-33 (1)
8-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect the
/adminsubtree in the layout.There is no session/role check here, so
/admin/mediacan render for anonymous users and/admin/media/orphansis reachable until downstream fetching fails. The admin area needs a redirect or 403 at the layout boundary, using the same admin-role check as the mutating APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/admin/layout.tsx` around lines 8 - 33, AdminLayout currently renders the admin subtree without any auth check; update the AdminLayout component to perform the same admin-role/session validation used by your mutating APIs and, if the user is not an admin, either redirect them away or return a 403 response at the layout boundary. Implement this by making AdminLayout an async server component that calls your existing session/role check helper (the same function used by mutating APIs), and when the check fails use Next.js navigation redirect or throw the appropriate Response/redirect to stop rendering children (so /admin/media and /admin/media/orphans are protected).src/app/api/media/orphans/route.ts-14-22 (1)
14-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict orphan operations to admins.
Any authenticated user can enumerate orphaned assets and trigger a full usage-count rescan here. That exposes admin-only inventory data and lets non-admin users run an expensive maintenance operation.
Also applies to: 34-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/media/orphans/route.ts` around lines 14 - 22, The route currently allows any authenticated user to list or rescan orphans; after obtaining the user via createClient() and supabase.auth.getUser(), verify the user is an admin (e.g., check a role flag in user.app_metadata or via a dedicated isAdmin/getUserRole helper) and return a 403 NextResponse.json({ error: 'Forbidden' }, { status: 403 }) for non-admins; apply the same admin-check logic to the other orphan-related block (the second auth check) so only admin users can enumerate orphaned assets or trigger the rescan.src/app/api/media/upload/route.ts-27-35 (1)
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce admin authorization here, not just authentication.
This write endpoint only checks that a user exists. The PR contract says media writes are admin-only, so any authenticated account can currently upload files into all buckets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/media/upload/route.ts` around lines 27 - 35, The endpoint currently only verifies a user exists; enforce admin-only writes by checking the authenticated user's admin role before proceeding: after calling createClient() and awaiting supabase.auth.getUser(), inspect a clear admin flag on the returned user (e.g., user.role === 'admin' or user.user_metadata?.is_admin depending on your auth schema) and if not present return NextResponse.json({ error: 'Forbidden' }, { status: 403 }); update any downstream logic that assumes auth to only run for users who pass this admin check. Ensure you reference the existing supabase variable and the getUser() result when adding the role check.src/app/api/media/[id]/route.ts-19-27 (1)
19-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPATCH and DELETE need the same admin gate as upload.
Both handlers stop at
getUser(), which means any authenticated account can edit alt text or delete assets. That breaks the admin-only write/delete contract for the media library.Also applies to: 48-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/media/`[id]/route.ts around lines 19 - 27, PATCH and DELETE handlers currently only call createClient() and supabase.auth.getUser() which allows any authenticated user to modify or delete media; add the same admin gate used by the upload path by verifying the user's admin role before proceeding. After calling supabase.auth.getUser(), check the admin flag (for example user.app_metadata.role === 'admin' or via your existing isAdmin helper) and if the check fails return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); apply this check in both the PATCH and DELETE handlers in route.ts (the blocks using createClient, supabase.auth.getUser and NextResponse.json).src/components/modules/admin/media/ImagePickerModal.tsx-73-75 (1)
73-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the preferred variant after uploads too.
The library path uses
getVariantUrl(item, preferVariant), but the upload path returnspublicUrldirectly. InCoverImagePicker, that means selecting an existing cover stores thelgvariant while uploading a new cover stores the original asset, which bypasses the pre-generated variants on the upload flow.Suggested fix
- function handleUploaded(item: MediaItem, publicUrl: string) { - onSelect(item, publicUrl); + function handleUploaded(item: MediaItem) { + onSelect(item, getVariantUrl(item, preferVariant)); onClose(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/ImagePickerModal.tsx` around lines 73 - 75, handleUploaded currently calls onSelect(item, publicUrl) which returns the raw uploaded URL and bypasses variant selection; change it to resolve the preferred variant URL using getVariantUrl(item, preferVariant) (or fallback to publicUrl if variant missing) and pass that result to onSelect so uploads and existing selections use the same preferred variant behavior; update the handleUploaded function to compute const url = getVariantUrl(item, preferVariant) || publicUrl and call onSelect(item, url) before onClose().src/components/modules/admin/media/MediaLibraryClient.tsx-16-17 (1)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe initial fetch guard never runs.
filtersis aMediaListFiltersobject andinitialDatais aMediaListResponse, so the comparison on Line 28 is always false. This re-fetches/api/mediaon first paint, causes a loading flash, and can replace the server-rendered results with default page-1 state. Skip the first effect run explicitly, or hydratefiltersfrom the server query state instead.Suggested fix
-'use client'; +'use client'; -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; @@ export function MediaLibraryClient({ initialData }: MediaLibraryClientProps) { const [data, setData] = useState<MediaListResponse>(initialData); const [filters, setFilters] = useState<MediaListFilters>({ page: 1 }); + const didInitRef = useRef(false); @@ useEffect(() => { - if (filters === initialData) return; // Skip initial render + if (!didInitRef.current) { + didInitRef.current = true; + return; + } fetchMedia(filters); // eslint-disable-next-line react-hooks/exhaustive-deps }, [filters]);Also applies to: 27-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/MediaLibraryClient.tsx` around lines 16 - 17, The effect guard fails because you compare filters (MediaListFilters) to initialData (MediaListResponse), so the first render always triggers a refetch; in MediaLibraryClient either initialize filters from the server-provided query (use initialData.query / initialData.page) by seeding useState<MediaListFilters> with that value instead of { page: 1 }, or implement an explicit skip-first-run flag (e.g., skipInitialFetchRef checked inside the effect that calls fetch / setLoading) to prevent the initial effect execution; update references to filters, setFilters, and initialData accordingly so the initial client state matches the server-rendered query and the guard works.src/components/modules/admin/media/OrphanViewer.tsx-20-29 (1)
20-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset selection after replacing the orphan list.
handleScanupdatesitemson Lines 28-29 but keepsselectedIdsandconfirmDeleteintact. After a rescan, the bulk-delete UI can still target IDs that are no longer visible, so the next confirm may delete files from the previous result set. Clear the selection/confirm state when fresh scan results arrive.Suggested fix
async function handleScan() { setScanning(true); setError(null); setScanResult(null); try { const res = await fetch('/api/media/orphans', { method: 'POST' }); if (!res.ok) throw new Error('Scan failed'); const data = await res.json(); setItems(data.items ?? []); + setSelectedIds(new Set()); + setConfirmDelete(false); setScanResult(`Scan complete. Found ${data.orphanCount} orphaned file${data.orphanCount !== 1 ? 's' : ''}.`); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/OrphanViewer.tsx` around lines 20 - 29, handleScan currently updates setItems but leaves selectedIds and confirmDelete from the previous scan, allowing bulk-delete to target stale IDs; after successfully setting items in handleScan (in the try branch, where setItems(data.items ?? []) is called) also clear the selection and confirmation state by calling setSelectedIds([]) and setConfirmDelete(false) (or the appropriate state setters) so any previous selections or confirm dialogs are reset when fresh scan results arrive.src/components/modules/admin/media/MediaDetailModal.tsx-33-39 (1)
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat failed article lookups as “unused”.
On Lines 35-38, every response is parsed as a success. If
/api/media/${item.id}/articlesreturns a 4xx/5xx JSON body, this falls through todata.items ?? []and the modal shows “Not used in any articles,” which can drive an unsafe delete decision. Checkr.okbefore readingitemsand surface a dedicated load error instead.Suggested fix
+ const [articlesError, setArticlesError] = useState<string | null>(null); + useEffect(() => { setLoadingArticles(true); + setArticlesError(null); fetch(`/api/media/${item.id}/articles`) - .then((r) => r.json()) + .then(async (r) => { + if (!r.ok) { + throw new Error('Failed to load article references'); + } + return r.json(); + }) .then((data) => setArticles(data.items ?? [])) - .catch(() => setArticles([])) + .catch((err) => { + setArticles([]); + setArticlesError(err instanceof Error ? err.message : 'Failed to load article references'); + }) .finally(() => setLoadingArticles(false)); }, [item.id]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/MediaDetailModal.tsx` around lines 33 - 39, The fetch in the useEffect within MediaDetailModal.tsx currently treats any HTTP response as success; update the handler that calls fetch(`/api/media/${item.id}/articles`) so it checks the Response.ok before parsing JSON (e.g., in the first .then use if (!r.ok) throw new Error(`status ${r.status}`) ), and on error set a dedicated load error state (e.g., setArticlesLoadError / setArticlesError) instead of silently setting articles to [] — keep using setLoadingArticles(false) in finally. Ensure subsequent UI uses this error state to show a load error message rather than “Not used in any articles.” Reference: the useEffect, fetch(...), setArticles, setLoadingArticles and the existing .catch/.finally flow.
🟡 Minor comments (5)
src/lib/storage/migrate-images.ts-189-190 (1)
189-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFalsy coercion stores
nullinstead of0for SVG dimensions.
width || nullevaluates tonullwhenwidthis0. Per the upstreamgetImageDimensionscontract, SVGs return{ width: 0, height: 0 }. This coercion would storenullrather than the intended0.🔧 Proposed fix
- width: width || null, - height: height || null, + width: width ?? null, + height: height ?? null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 189 - 190, The code uses falsy coercion (width || null, height || null) which converts valid zero dimensions to null; change these to nullish checks so 0 is preserved (e.g. use the nullish coalescing operator or an explicit undefined check) when assigning width and height inside the migration logic in migrate-images.ts (look for the width and height assignments in the function that writes image metadata). Ensure undefined becomes null but numeric 0 remains 0..vscode/settings.json-20-23 (1)
20-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Tailwind
classRegexsocn()/clsx()multi-arg calls are parsed correctlyIn
.vscode/settings.jsonlines 20-23, the inner capture([^']*)stops only on', socn("bg-red-500", active && "text-white")gets matched as one block instead of separate class strings—breaking Tailwind IntelliSense. Exclude',", and`from the capture.Proposed fix
"tailwindCSS.experimental.classRegex": [ - ["clsx\\(([^)]*)\\)", "(?:'|\"|`)([^']*)(?:'|\"|`)"], - ["cn\\(([^)]*)\\)", "(?:'|\"|`)([^']*)(?:'|\"|`)"] + ["clsx\\(([^)]*)\\)", "(?:'|\"|`)([^'\"`]*)(?:'|\"|`)"], + ["cn\\(([^)]*)\\)", "(?:'|\"|`)([^'\"`]*)(?:'|\"|`)"] ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.vscode/settings.json around lines 20 - 23, The Tailwind VSCode classRegex entries for "tailwindCSS.experimental.classRegex" incorrectly use a capture that only excludes single quotes, causing multi-argument calls like cn("bg-red-500", active && "text-white") to be treated as one block; update both regexes for "clsx\\(([^)]*)\\)" and "cn\\(([^)]*)\\)" so the inner string-capture excludes single, double and backtick quotes (replace the inner capture with a character class that omits ', " and `) to allow separate quoted class strings to be matched correctly by Tailwind IntelliSense.src/components/modules/admin/media/UploadZone.tsx-67-71 (1)
67-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the reset timer before a later upload starts.
This timeout can fire during a second upload and flip the UI back to
idle, hiding the real progress/error state. Keep the timer id in a ref, clear it before starting a new upload, and cancel it on unmount.Suggested fix
export function UploadZone({ bucket, anchorOnStellar = false, onUploaded, onError }: UploadZoneProps) { const [state, setState] = useState<UploadState>({ status: 'idle', progress: 0 }); const [isDragging, setIsDragging] = useState(false); const inputRef = useRef<HTMLInputElement>(null); + const resetTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); const uploadFile = useCallback( async (file: File) => { + if (resetTimerRef.current) { + clearTimeout(resetTimerRef.current); + resetTimerRef.current = null; + } + // Client-side validation const limitBytes = BUCKET_SIZE_LIMITS[bucket]; ... - setTimeout(() => setState({ status: 'idle', progress: 0 }), 2000); + resetTimerRef.current = setTimeout(() => { + setState({ status: 'idle', progress: 0 }); + resetTimerRef.current = null; + }, 2000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/UploadZone.tsx` around lines 67 - 71, The success-state reset timeout can race with a subsequent upload and revert the UI; store the timeout id in a ref (e.g., resetTimerRef), clear it (clearTimeout) before starting any new upload (inside the upload start/handleUpload function), assign the new timer id when calling setTimeout after success, and cancel/clear the timer in a useEffect cleanup on unmount so the timeout never fires after the component is gone; update references around setState({ status: 'success', ... }) and the upload starter (handleUpload/onUpload) to implement this.src/components/modules/admin/media/MediaFilters.tsx-31-37 (1)
31-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the search field.
The input on Line 31 only has placeholder text, so assistive tech gets an unnamed control here while the other filters are labeled. Add a
<label>oraria-label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/MediaFilters.tsx` around lines 31 - 37, The search input in the MediaFilters component lacks an accessible name; update the input (in MediaFilters where filters and update are used) to provide an accessible label by either adding a visually associated <label> element for the search field or adding an aria-label/aria-labelledby attribute (e.g., "Search by filename") so assistive tech can identify the control; ensure the label text matches the placeholder and that the update({ search: ... }) handler remains unchanged.src/components/modules/admin/media/MediaLibraryClient.tsx-135-156 (1)
135-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBulk alt-text updates hide partial failures.
Lines 140-153 swallow both network and HTTP errors, then Lines 155-156 clear the input and selection anyway. That leaves the UI looking successful even when some items were not updated. Collect failures like
handleBulkDeletedoes, and keep failed IDs selected for retry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modules/admin/media/MediaLibraryClient.tsx` around lines 135 - 156, In handleBulkSetAltText, don’t swallow network/HTTP failures silently: mirror handleBulkDelete by tracking failed IDs and errors; for each id check res.ok and on failure push id into a failures array (and capture error details from thrown exceptions or non-ok responses), after the loop call setBulkActionError with a summary if failures.length>0, call handleAltTextSaved only for successful ids, call setSelectedIds to keep only failed IDs (so user can retry) and only clear setBulkAltText when there are no failures; use the existing symbols handleBulkSetAltText, handleAltTextSaved, handleBulkDelete (as reference), setBulkActionError, setSelectedIds, and setBulkAltText to implement this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/0002_media_library.sql`:
- Around line 53-58: The "authenticated can write media" policy on
public.media_library currently grants all mutations to the entire authenticated
role; change its target and predicates to restrict write/delete to admins only
by replacing the role "authenticated" and the unconditional using/with check
(true) with an admin-claim check (e.g., validate a JWT claim or auth.role
indicating admin) so only requests presenting that admin claim can
INSERT/UPDATE/DELETE; apply the same change to the other related policies in the
file (the block referenced around lines 117-160) so all mutation policies for
media_library and any bucket-object access use the admin claim check rather than
allowing every authenticated user.
---
Major comments:
In @.claude/agents/kfc/spec-design.md:
- Line 127: The spec-design doc contains two conflicting output paths
('.claude/specs/{feature_name}/design.md' vs
'.kiro/specs/{feature_name}/design.md'); pick one canonical path and make all
references consistent (e.g., replace the '.kiro/...' occurrence with
'.claude/...' or vice versa) or centralize the path behind a single
constant/variable (e.g., SPEC_PATH or SPEC_DIR) used by the checks that mention
these paths so both Line 127 and Line 133 resolve to the same deterministic
path.
In @.claude/agents/kfc/spec-judge.md:
- Line 17: The judge spec expects a parameter named "documents" but the workflow
caller sends "doc_path", causing invocation mismatches; update the judge
contract in .claude/agents/kfc/spec-judge.md to accept "doc_path" (or add
"doc_path" as an alias) and ensure any referenced handler or entrypoint that
reads "documents" (the judge input contract) is updated to read "doc_path"
instead so the workflow starter and judge use the same parameter name.
- Line 90: The 'total' field is wrongly computed with 'total': sum(scores) which
sums the outer accumulator instead of the current document's criteria; change it
to sum the per-document criteria scores (e.g., 'total': sum(criteria_scores) or
'total': sum(scores.values()) depending on whether per-document scores are
stored in a list or dict) so that the 'total' reflects the current document's
criteria sum; ensure you reference the per-document variable used in the
surrounding loop (e.g., criteria_scores, perDocumentScores, or scores when it is
the per-doc map) rather than the outer accumulator.
In @.claude/agents/kfc/spec-requirements.md:
- Around line 48-49: The codebase has two conflicting filename templates for the
requirements file: "requirements{output_suffix}.md" and
"requirements_{output_suffix}.md", causing double underscores when output_suffix
already includes a leading underscore; pick one convention and make it
consistent. Fix by normalizing how output_suffix is composed and used: either
(A) require output_suffix to have no leading underscore and use the
"requirements_{output_suffix}.md" template everywhere, or (B) allow
output_suffix to include a leading underscore and use
"requirements{output_suffix}.md" everywhere; update the occurrences of the
templates (the places referencing output_suffix and the two templates) to the
chosen single form and ensure validators/judge patterns expect that same
pattern. Ensure all references to output_suffix, requirements{output_suffix}.md,
and requirements_{output_suffix}.md are updated to the single agreed convention.
In @.claude/agents/kfc/spec-test.md:
- Around line 88-90: The spec references placeholder filenames `{module}.md` and
`{module}.test.ts` without defining `module`, causing under-specified artifact
naming; update the spec in .claude/agents/kfc/spec-test.md to require a concrete
`module` input (or define a deterministic derivation rule such as using
`artifactName`, `packageName`, or the primary feature identifier) and add
validation steps: require `module` in inputs, or auto-derive it (e.g., from
`task.name` or `targetFile`), and specify the exact filename template to use;
ensure the spec text and any downstream generator/validator enforce and error if
`module` is missing so produced filenames are consistent across agents.
In @.claude/system-prompts/spec-workflow-starter.md:
- Around line 196-204: The spec-judge invocation schema is using doc_path
(single path) but the spec-judge agent expects documents (a list of candidate
paths), so update the workflow schema: replace or augment the doc_path field
with documents: string[] and ensure the judge block (named "Judge - spec-judge"
/ spec-judge invocation) uses the documents array; remove any single-path-only
handling and adjust any consumers to iterate over documents for parallel
evaluation.
In `@next.config.ts`:
- Around line 5-13: The images.remotePatterns in next.config.ts is too
permissive (hostname '**.supabase.co')—restrict it to your project's Supabase
host by reading NEXT_PUBLIC_SUPABASE_URL and using that exact hostname instead
of the wildcard; update the images.remotePatterns entry (the images object in
next.config.ts) to use the specific host derived from NEXT_PUBLIC_SUPABASE_URL
so <Image src={article.coverImageUrl} /> only allows your tenant’s storage URLs.
In `@src/app/admin/layout.tsx`:
- Around line 8-33: AdminLayout currently renders the admin subtree without any
auth check; update the AdminLayout component to perform the same
admin-role/session validation used by your mutating APIs and, if the user is not
an admin, either redirect them away or return a 403 response at the layout
boundary. Implement this by making AdminLayout an async server component that
calls your existing session/role check helper (the same function used by
mutating APIs), and when the check fails use Next.js navigation redirect or
throw the appropriate Response/redirect to stop rendering children (so
/admin/media and /admin/media/orphans are protected).
In `@src/app/api/media/`[id]/route.ts:
- Around line 19-27: PATCH and DELETE handlers currently only call
createClient() and supabase.auth.getUser() which allows any authenticated user
to modify or delete media; add the same admin gate used by the upload path by
verifying the user's admin role before proceeding. After calling
supabase.auth.getUser(), check the admin flag (for example
user.app_metadata.role === 'admin' or via your existing isAdmin helper) and if
the check fails return NextResponse.json({ error: 'Unauthorized' }, { status:
401 }); apply this check in both the PATCH and DELETE handlers in route.ts (the
blocks using createClient, supabase.auth.getUser and NextResponse.json).
In `@src/app/api/media/orphans/route.ts`:
- Around line 14-22: The route currently allows any authenticated user to list
or rescan orphans; after obtaining the user via createClient() and
supabase.auth.getUser(), verify the user is an admin (e.g., check a role flag in
user.app_metadata or via a dedicated isAdmin/getUserRole helper) and return a
403 NextResponse.json({ error: 'Forbidden' }, { status: 403 }) for non-admins;
apply the same admin-check logic to the other orphan-related block (the second
auth check) so only admin users can enumerate orphaned assets or trigger the
rescan.
In `@src/app/api/media/route.ts`:
- Around line 17-25: The pagination inputs in the MediaListFilters construction
(fields page and pageSize in src/app/api/media/route.ts) are currently parsed
with Number(...) which can yield NaN, 0, negatives or huge values; update the
parsing to validate and clamp: parse as integer, ensure isFinite and >= 1 for
page (default to 1 if invalid) and ensure pageSize is an integer within an
acceptable server-side cap (e.g. clamp to 1..100, default 24 if invalid);
implement this validation before building the MediaListFilters object (refer to
the page and pageSize properties and MediaListFilters) and return or sanitize
inputs rather than relying on client-provided values.
In `@src/app/api/media/upload/route.ts`:
- Around line 70-79: The Stellar anchoring call is currently awaited inside the
upload handler (anchorHashOnStellar and updateStellarTxHash), blocking the
request; change this so the handler persists the upload and returns immediately,
then kick off anchoring asynchronously (e.g., enqueue a background job or spawn
a non-awaited worker using result.media.id and result.media.contentHash) that
performs anchorHashOnStellar and updateStellarTxHash with its own retry/backoff
and error logging; ensure the upload route does not await
anchorHashOnStellar/updateStellarTxHash and still records the media ID so the
background job can pick it up and store stellarTxHash when successful.
- Around line 27-35: The endpoint currently only verifies a user exists; enforce
admin-only writes by checking the authenticated user's admin role before
proceeding: after calling createClient() and awaiting supabase.auth.getUser(),
inspect a clear admin flag on the returned user (e.g., user.role === 'admin' or
user.user_metadata?.is_admin depending on your auth schema) and if not present
return NextResponse.json({ error: 'Forbidden' }, { status: 403 }); update any
downstream logic that assumes auth to only run for users who pass this admin
check. Ensure you reference the existing supabase variable and the getUser()
result when adding the role check.
In `@src/components/modules/admin/media/ImagePickerModal.tsx`:
- Around line 73-75: handleUploaded currently calls onSelect(item, publicUrl)
which returns the raw uploaded URL and bypasses variant selection; change it to
resolve the preferred variant URL using getVariantUrl(item, preferVariant) (or
fallback to publicUrl if variant missing) and pass that result to onSelect so
uploads and existing selections use the same preferred variant behavior; update
the handleUploaded function to compute const url = getVariantUrl(item,
preferVariant) || publicUrl and call onSelect(item, url) before onClose().
In `@src/components/modules/admin/media/MediaDetailModal.tsx`:
- Around line 33-39: The fetch in the useEffect within MediaDetailModal.tsx
currently treats any HTTP response as success; update the handler that calls
fetch(`/api/media/${item.id}/articles`) so it checks the Response.ok before
parsing JSON (e.g., in the first .then use if (!r.ok) throw new Error(`status
${r.status}`) ), and on error set a dedicated load error state (e.g.,
setArticlesLoadError / setArticlesError) instead of silently setting articles to
[] — keep using setLoadingArticles(false) in finally. Ensure subsequent UI uses
this error state to show a load error message rather than “Not used in any
articles.” Reference: the useEffect, fetch(...), setArticles, setLoadingArticles
and the existing .catch/.finally flow.
In `@src/components/modules/admin/media/MediaLibraryClient.tsx`:
- Around line 16-17: The effect guard fails because you compare filters
(MediaListFilters) to initialData (MediaListResponse), so the first render
always triggers a refetch; in MediaLibraryClient either initialize filters from
the server-provided query (use initialData.query / initialData.page) by seeding
useState<MediaListFilters> with that value instead of { page: 1 }, or implement
an explicit skip-first-run flag (e.g., skipInitialFetchRef checked inside the
effect that calls fetch / setLoading) to prevent the initial effect execution;
update references to filters, setFilters, and initialData accordingly so the
initial client state matches the server-rendered query and the guard works.
In `@src/components/modules/admin/media/OrphanViewer.tsx`:
- Around line 20-29: handleScan currently updates setItems but leaves
selectedIds and confirmDelete from the previous scan, allowing bulk-delete to
target stale IDs; after successfully setting items in handleScan (in the try
branch, where setItems(data.items ?? []) is called) also clear the selection and
confirmation state by calling setSelectedIds([]) and setConfirmDelete(false) (or
the appropriate state setters) so any previous selections or confirm dialogs are
reset when fresh scan results arrive.
In `@src/lib/stellar/anchor.ts`:
- Around line 96-115: The current verifyAnchor function returns verified based
only on transaction success; modify verifyAnchor to fetch the transaction
operations (use server.operations().forTransaction(txHash).call()), iterate the
returned operations and locate any op where op.type === 'manage_data', then
compare that operation's key/value against the expected media entry: decode
op.value if it is base64-encoded and check value === expectedHash (and
optionally check op.name/op.key equals the expected media key if your codebase
has a specific key name); only return { verified: true, ledger } when a matching
manage_data entry is found, otherwise return { verified: false }.
In `@src/lib/storage/media.service.ts`:
- Around line 189-194: In recalculateUsageCounts(), the per-item Supabase update
inside the loop (for ... of mediaItems) awaits the .update({ usage_count: count
}).eq('id', item.id) call but doesn't check the returned response for an error;
modify the loop to capture the update response (e.g., const { error } = await
supabase.from('media_library').update(...).eq(...)) and throw or propagate a new
Error containing the item.id and the Supabase error if error is truthy so failed
updates do not silently leave stale usage_count values.
In `@src/lib/storage/migrate-images.ts`:
- Around line 183-194: The insert into the media_library via
supabase.from('media_library').insert(...) currently ignores the returned {
data, error } — check that result and on error log the error and clean up the
just-uploaded object (call supabase.storage.from(bucket).remove([path]) or
equivalent) then throw or return a failure so the caller doesn't get a Storage
URL for an untracked file; ensure the function only returns the Storage URL
after the insert succeeds and include the error details in the thrown/logged
message for debugging.
In `@src/lib/storage/optimize.ts`:
- Around line 30-40: The current SVG branch in optimize (the mimeType ===
'image/svg+xml' block) returns a single "thumb" variant which later causes
uploadMedia to save an SVG buffer with a .webp/contentType:image/webp; change
this behavior by returning no variants for SVGs (i.e., return an empty array) or
alternatively enrich the variant objects with explicit format/ mime metadata so
uploadMedia can preserve the original MIME type and extension; update the
optimize function's SVG branch (and any variant shape/type definitions it uses)
and ensure uploadMedia reads per-variant format fields if you choose the
metadata approach.
In `@src/lib/storage/upload.ts`:
- Around line 130-185: If the final insert into media_library (check insertError
or !record) fails, remove the uploaded files to avoid orphaned objects: call
supabase.storage.from(bucket).remove([...]) with the originalPath plus any
values from variantPaths (only include successful variantPaths entries), log any
removal errors, and then rethrow the original insertion error; update the
error-handling branch that references insertError/record to perform this cleanup
using the existing symbols originalPath, variantPaths, bucket, supabase.storage
and then throw the same failure Error as before.
---
Minor comments:
In @.vscode/settings.json:
- Around line 20-23: The Tailwind VSCode classRegex entries for
"tailwindCSS.experimental.classRegex" incorrectly use a capture that only
excludes single quotes, causing multi-argument calls like cn("bg-red-500",
active && "text-white") to be treated as one block; update both regexes for
"clsx\\(([^)]*)\\)" and "cn\\(([^)]*)\\)" so the inner string-capture excludes
single, double and backtick quotes (replace the inner capture with a character
class that omits ', " and `) to allow separate quoted class strings to be
matched correctly by Tailwind IntelliSense.
In `@src/components/modules/admin/media/MediaFilters.tsx`:
- Around line 31-37: The search input in the MediaFilters component lacks an
accessible name; update the input (in MediaFilters where filters and update are
used) to provide an accessible label by either adding a visually associated
<label> element for the search field or adding an aria-label/aria-labelledby
attribute (e.g., "Search by filename") so assistive tech can identify the
control; ensure the label text matches the placeholder and that the update({
search: ... }) handler remains unchanged.
In `@src/components/modules/admin/media/MediaLibraryClient.tsx`:
- Around line 135-156: In handleBulkSetAltText, don’t swallow network/HTTP
failures silently: mirror handleBulkDelete by tracking failed IDs and errors;
for each id check res.ok and on failure push id into a failures array (and
capture error details from thrown exceptions or non-ok responses), after the
loop call setBulkActionError with a summary if failures.length>0, call
handleAltTextSaved only for successful ids, call setSelectedIds to keep only
failed IDs (so user can retry) and only clear setBulkAltText when there are no
failures; use the existing symbols handleBulkSetAltText, handleAltTextSaved,
handleBulkDelete (as reference), setBulkActionError, setSelectedIds, and
setBulkAltText to implement this behavior.
In `@src/components/modules/admin/media/UploadZone.tsx`:
- Around line 67-71: The success-state reset timeout can race with a subsequent
upload and revert the UI; store the timeout id in a ref (e.g., resetTimerRef),
clear it (clearTimeout) before starting any new upload (inside the upload
start/handleUpload function), assign the new timer id when calling setTimeout
after success, and cancel/clear the timer in a useEffect cleanup on unmount so
the timeout never fires after the component is gone; update references around
setState({ status: 'success', ... }) and the upload starter
(handleUpload/onUpload) to implement this.
In `@src/lib/storage/migrate-images.ts`:
- Around line 189-190: The code uses falsy coercion (width || null, height ||
null) which converts valid zero dimensions to null; change these to nullish
checks so 0 is preserved (e.g. use the nullish coalescing operator or an
explicit undefined check) when assigning width and height inside the migration
logic in migrate-images.ts (look for the width and height assignments in the
function that writes image metadata). Ensure undefined becomes null but numeric
0 remains 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/storage/optimize.ts (1)
68-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix pipeline order:
.rotate()must precede.withMetadata({})to preserve EXIF orientation.The current order clears metadata before auto-rotate reads the EXIF orientation tag, so images will not be rotated correctly.
🐛 Proposed fix
let pipeline = sharp(input) - // Strip EXIF metadata for privacy — use withMetadata({}) to clear all - .withMetadata({}) - .rotate(); // Auto-rotate based on EXIF orientation before stripping + .rotate() // Auto-rotate based on EXIF orientation first + .withMetadata({}); // Then strip EXIF metadata for privacy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/optimize.ts` around lines 68 - 71, The sharp pipeline currently calls .withMetadata({}) before .rotate(), which strips EXIF orientation before rotate can read it; in the pipeline built in optimize.ts (the variable pipeline created from sharp(input)), move the .rotate() call to come before .withMetadata({}) so auto-rotation runs while EXIF data is still present (i.e., chain .rotate() immediately after sharp(input) and then call .withMetadata({})).src/lib/stellar/anchor.ts (1)
58-59:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: 8-character mediaId prefix causes anchor collisions.
Truncating the UUID to 8 characters (32 bits) for the manageData key creates significant collision risk. Two media items with the same first 8 hex characters will overwrite each other's anchor data on the Stellar account. With ~65K media items, collision probability exceeds 50% (birthday paradox).
Use more of the UUID or hash both mediaId and contentHash together into the key.
🐛 Proposed fix: use full hash-based key
- // manageData key: "media:{first-8-chars-of-id}" (max 64 bytes for key) - const dataKey = `media:${mediaId.slice(0, 8)}`; + // manageData key: use content hash prefix to ensure uniqueness per file + // Key format: "m:{first 58 chars of hash}" (64 byte limit: 2 + 1 + 58 + null padding) + const dataKey = `m:${contentHash.slice(0, 58)}`;Alternatively, store the full mediaId in the value alongside the hash, or use a separate data entry per anchor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/stellar/anchor.ts` around lines 58 - 59, The current dataKey creation in anchor.ts uses an 8-char prefix (const dataKey = `media:${mediaId.slice(0, 8)}`), which causes collisions; change it to derive dataKey from a cryptographic hash of mediaId (and contentHash if available) instead: compute SHA-256(mediaId [+ contentHash]), hex-encode it and use that hex (or its first 64 bytes if needed to meet Stellar's 64-byte key limit) as the suffix (e.g., `media:${hashHex}`), and store the full mediaId (and contentHash) inside the manageData value so you can always recover the original identifiers.src/lib/storage/migrate-images.ts (2)
108-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: Missing MIME type and size validation.
The
downloadImagefunction accepts any MIME type from the remote server and doesn't enforce size limits before processing. This bypasses the bucket's intended restrictions:
- MIME type: According to PR objectives, only
image/jpeg, image/png, image/webp, image/gif, image/svg+xmlshould be allowed. A malicious server could returntext/html,application/javascript, or other non-image content.- Size limit: Buckets have 10 MB limits for article images, but the buffer is not checked before optimization and upload.
🛡️ Proposed fix: Add validation after download
+const ALLOWED_MIME_TYPES = [ + 'image/jpeg', + 'image/png', + 'image/webp', + 'image/gif', + 'image/svg+xml', +]; +const MAX_SIZE_BYTES = 10 * 1024 * 1024; // 10 MB + async function downloadImage(url: string): Promise<{ buffer: Buffer; mimeType: string; filename: string }> { if (!isSafeUrl(url)) { throw new Error(`Blocked URL (private/internal address not allowed): ${url}`); } const response = await fetch(url, { headers: { 'User-Agent': 'ACTA-News-Migration/1.0' }, signal: AbortSignal.timeout(30_000), }); if (!response.ok) { throw new Error(`HTTP ${response.status}: ${response.statusText}`); } const contentType = response.headers.get('content-type') ?? 'image/jpeg'; const mimeType = contentType.split(';')[0].trim(); + + if (!ALLOWED_MIME_TYPES.includes(mimeType)) { + throw new Error(`Unsupported MIME type: ${mimeType}`); + } const arrayBuffer = await response.arrayBuffer(); const buffer = Buffer.from(arrayBuffer); + + if (buffer.byteLength > MAX_SIZE_BYTES) { + throw new Error(`File too large: ${buffer.byteLength} bytes (max ${MAX_SIZE_BYTES})`); + } // Extract and sanitize filename from URL — strip directory components and // allow only safe characters to prevent path traversal / command injection. const rawName = basename(new URL(url).pathname); const filename = rawName.replace(/[^a-zA-Z0-9._-]/g, '_') || `image-${randomUUID()}.jpg`; return { buffer, mimeType, filename }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 108 - 134, The downloadImage function must enforce allowed MIME types and a size limit: after deriving mimeType (variable mimeType) validate it against the allowed set [image/jpeg, image/png, image/webp, image/gif, image/svg+xml] and throw a clear Error if not allowed; then check Buffer.byteLength (the buffer created from arrayBuffer) and reject if it exceeds 10 * 1024 * 1024 bytes (10 MB) with a descriptive Error before proceeding to filename sanitization and returning; ensure errors are thrown from downloadImage so callers don’t continue processing invalid content.
73-90:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: SSRF protection is incomplete—missing IPv6 and DNS rebinding defenses.
The current
isSafeUrlimplementation only blocks IPv4 private ranges and will not prevent SSRF attacks using:
- IPv6 loopback and link-local addresses:
http://[::1]/,http://[fe80::1]/,http://[::ffff:127.0.0.1]/- DNS rebinding: A malicious domain that resolves to
127.0.0.1or other private IPs will pass the hostname check- Additional cloud metadata endpoints: Alibaba Cloud (
100.100.x.x), Azure (169.254.169.254via IPv6), etc.This allows an attacker to migrate a malicious article with cover/content URLs pointing to internal services, which this script will fetch and potentially expose.
🔒 Recommended fix: Add IPv6 checks and use an allowlist
function isSafeUrl(url: string): boolean { try { const { hostname, protocol } = new URL(url); if (!['http:', 'https:'].includes(protocol)) return false; + + // Block IPv6 loopback and link-local + if ( + hostname === '::1' || + hostname.startsWith('fe80:') || + hostname.startsWith('[::1]') || + hostname.startsWith('[fe80:') + ) return false; + + // Block IPv4-mapped IPv6 addresses (::ffff:127.0.0.1) + if (/^\[?::ffff:(127\.|10\.|192\.168\.|172\.(1[6-9]|2\d|3[01])\.|169\.254\.)/i.test(hostname)) { + return false; + } + if ( hostname === 'localhost' || /^127\./.test(hostname) || /^10\./.test(hostname) || /^172\.(1[6-9]|2\d|3[01])\./.test(hostname) || /^192\.168\./.test(hostname) || /^169\.254\./.test(hostname) || + /^100\.100\./.test(hostname) || // Alibaba Cloud metadata hostname === '0.0.0.0' ) return false; + return true; } catch { return false; } }Note: This still doesn't prevent DNS rebinding. For production use, consider implementing a domain allowlist or using a dedicated SSRF protection library.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 73 - 90, isSafeUrl currently only checks URL.protocol and hostname text, missing IPv6, mapped IPv4-in-IPv6, cloud metadata ranges and DNS rebinding; update isSafeUrl (and any call sites that fetch remote URLs) to: 1) parse the URL in isSafeUrl, reject non-http(s); 2) detect IPv6 literal hosts and reject loopback (::1), link-local (fe80::/10), ULA (fc00::/7), IPv4-mapped addresses (::ffff:0:0/96 or ::ffff:127.0.0.1) and other reserved IPv6 ranges; 3) perform a DNS resolution (e.g. dns.lookup/dns.promises.lookup or equivalent) for hostnames and validate each resolved IP against a denylist of private/reserved ranges including 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16, 100.100.0.0/16 (Alibaba), and cloud metadata addresses, rejecting if any resolved address falls into those ranges; and 4) optionally add an explicit domain allowlist that short-circuits checks for trusted hosts. Update isSafeUrl to return false on any resolution failure or if any resolved address is private/mapped/reserved.
🧹 Nitpick comments (6)
src/lib/storage/optimize.ts (1)
90-91: 💤 Low valueMinor optimization: use
toBuffer({ resolveWithObject: true })to avoid re-parsing the buffer.The current code calls
sharp(buffer).metadata()aftertoBuffer(), which re-decodes the image. Sharp can return metadata alongside the buffer in one operation.♻️ Proposed refactor
- const buffer = await pipeline.toBuffer(); - const metadata = await sharp(buffer).metadata(); + const { data: buffer, info } = await pipeline.toBuffer({ resolveWithObject: true }); return { key: spec.key, buffer, - width: metadata.width ?? spec.width, - height: metadata.height ?? 0, + width: info.width ?? spec.width, + height: info.height ?? 0, sizeBytes: buffer.byteLength, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/optimize.ts` around lines 90 - 91, Replace the two-step decode (await pipeline.toBuffer(); then await sharp(buffer).metadata()) with a single call that returns both buffer and metadata by using pipeline.toBuffer({ resolveWithObject: true }); update references to use the returned object's .data (or similar) as the image buffer and .info (or .metadata) for metadata so you avoid re-parsing; locate the usage around pipeline.toBuffer and the subsequent sharp(buffer).metadata() calls in optimize.ts and adapt downstream variables accordingly.supabase/migrations/0002_media_library.sql (1)
26-26: 💤 Low valueConsider adding a foreign key to
auth.usersforuploaded_by.The
uploaded_bycolumn is defined asTEXTwithout a foreign key constraint. While this offers flexibility (e.g., storing user identifiers from external systems), it also means orphaned references can occur if users are deleted. Ifuploaded_byalways stores Supabaseauth.users.id, consider:uploaded_by uuid references auth.users(id) on delete set nullThis is optional and depends on whether you need to track uploaders from non-Supabase sources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/0002_media_library.sql` at line 26, The uploaded_by column is currently TEXT and lacks a foreign key to auth.users; change its type to uuid (or nullable uuid) and add a foreign key constraint referencing auth.users(id) with an appropriate ON DELETE behavior (e.g., ON DELETE SET NULL) so deleted users don’t leave invalid references; update the column definition for uploaded_by and add a REFERENCES auth.users(id) constraint (or keep TEXT if you intentionally need external identifiers).src/lib/stellar/anchor.ts (1)
110-118: ⚡ Quick winImprove type safety for Horizon operation response.
The type assertion
as { type: string; value: string }is fragile. Themanage_dataoperation response from Horizon has a specific shape. Consider using the SDK's types or a more defensive approach.♻️ Proposed refactor
for (const op of ops.records) { if (op.type !== 'manage_data') continue; - const manageDataOp = op as { type: string; value: string }; - // value is base64-encoded - const decoded = Buffer.from(manageDataOp.value, 'base64').toString('utf8'); + // Horizon returns manage_data ops with a `value` field (base64-encoded) + const value = (op as { value?: string }).value; + if (!value) continue; + const decoded = Buffer.from(value, 'base64').toString('utf8'); if (decoded === expectedHash.slice(0, 64)) { return { verified: true, ledger: (tx.ledger as unknown) as number }; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/stellar/anchor.ts` around lines 110 - 118, The loop over ops.records uses a brittle type assertion for manage_data ops; replace the cast on manageDataOp with a proper runtime/type-safe check or the SDK's typed response: import the Horizon/Operation response type for manage_data (e.g., the SDK's manage data operation record type) and use it for the variable, or keep a runtime guard that ensures 'value' exists and is a string before decoding; update the block that references ops.records, manageDataOp, expectedHash and tx.ledger to validate op.value and only decode when safe so you avoid unsafe assertions and improve type safety.src/lib/storage/migrate-images.ts (3)
273-273: 💤 Low valueMinor: Consider redacting URLs in console output.
Line 273 logs the first 60 characters of external image URLs, which could contain sensitive query parameters, tokens, or PII (e.g.,
https://cdn.example.com/img?key=SECRET&user=email@example.com). While the final report at line 319 redacts URLs, the real-time console output at line 273 does not.♻️ Proposed fix
- console.log(` Migrating content image: ${url.slice(0, 60)}…`); + const urlHost = new URL(url).hostname; + console.log(` Migrating content image from ${urlHost}…`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` at line 273, The console.log at the start of the image migration ("console.log(` Migrating content image: ${url.slice(0, 60)}…`)") prints raw URL fragments that may include sensitive query params; change this to print a redacted URL instead by reusing the same redaction logic used for the final report at line 319 (i.e., replace the url.slice call with the redacted form produced for the final report), ensuring you reference the existing redaction helper or variable that generates the redacted URL for consistency.
52-60: ⚡ Quick winRemove unused 'skipped' status from type definition.
The
MigrationResulttype defines a'skipped'status, but this value is never assigned in the actual migration logic (lines 230-296). All results are initialized to'failed'and only transition to'success'. The report at lines 303-313 will always show 0 skipped items.♻️ Proposed fix
interface MigrationResult { articleId: string; articleSlug: string; field: 'cover_image_url' | 'content'; originalUrl: string; newUrl?: string; - status: 'success' | 'skipped' | 'failed'; + status: 'success' | 'failed'; error?: string; }And remove the skipped report line:
console.log(`✓ Succeeded : ${succeeded.length}`); console.log(`✗ Failed : ${failed.length}`); - console.log(`→ Skipped : ${skipped.length}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 52 - 60, The MigrationResult type currently includes a 'skipped' status that is never assigned; update the MigrationResult interface to remove 'skipped' from the status union (leaving only 'success' | 'failed'), then remove the corresponding "skipped" row from the migration report generation so it no longer prints a skipped count; ensure any logic that might reference MigrationResult.status or build the report (functions/blocks around MigrationResult usage and the report summarization) is adjusted to expect only 'success' or 'failed'.
92-106: ⚡ Quick winExtend regex patterns to capture single-quoted and unquoted image URLs.
The current regex only matches
src="..."with double quotes. Valid HTML also allows single quotes (src='...') and unquoted values (src=https://...), which will be skipped by this migration.♻️ Proposed fix
function extractImageUrls(content: string): string[] { const urls: string[] = []; - // Match src="..." in img tags - const imgSrcRegex = /src="(https?:\/\/[^"]+)"/g; + // Match src with double quotes, single quotes, or no quotes + const imgSrcRegex = /src=["']?(https?:\/\/[^\s"'>]+)["']?/gi; let match; while ((match = imgSrcRegex.exec(content)) !== null) { urls.push(match[1]); } // Match markdown images  const mdImgRegex = /!\[[^\]]*\]\((https?:\/\/[^)]+)\)/g; while ((match = mdImgRegex.exec(content)) !== null) { urls.push(match[1]); } return [...new Set(urls)]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/storage/migrate-images.ts` around lines 92 - 106, The extractImageUrls function only matches src="..." double-quoted URLs; update the imgSrcRegex and its usage to also capture single-quoted and unquoted values (e.g., src='https://...' and src=https://...), for example by using an alternation that creates three capture groups (double-quoted, single-quoted, unquoted) and then pushing the first non-empty capture (instead of match[1]) into urls; keep the existing mdImgRegex logic as-is and preserve the Set de-duplication and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/admin/layout.tsx`:
- Around line 16-17: The current isAdmin check uses user.user_metadata which is
user-controlled and allows privilege escalation; update the logic in the admin
layout so isAdmin only checks trusted data (e.g., user.app_metadata?.role ===
'admin') and remove any use of user.user_metadata?.is_admin; ensure the redirect
still happens when the trusted isAdmin is false and consider adding a
server-side/DB-backed authorization check (RLS or endpoint) for further
enforcement in any server-rendered or API paths that serve admin data.
In `@src/app/api/media/`[id]/route.ts:
- Around line 17-22: The isAdmin function currently trusts user.user_metadata to
grant admin rights; remove that vector by only checking server-controlled
app_metadata. Update the isAdmin(user) implementation to return true only when
user.app_metadata?.role === 'admin' (remove the user.user_metadata?.is_admin
check), and ensure any callers (e.g., PATCH/DELETE authorization guards) rely on
this updated isAdmin to prevent user-editable metadata from granting privileges.
---
Outside diff comments:
In `@src/lib/stellar/anchor.ts`:
- Around line 58-59: The current dataKey creation in anchor.ts uses an 8-char
prefix (const dataKey = `media:${mediaId.slice(0, 8)}`), which causes
collisions; change it to derive dataKey from a cryptographic hash of mediaId
(and contentHash if available) instead: compute SHA-256(mediaId [+
contentHash]), hex-encode it and use that hex (or its first 64 bytes if needed
to meet Stellar's 64-byte key limit) as the suffix (e.g., `media:${hashHex}`),
and store the full mediaId (and contentHash) inside the manageData value so you
can always recover the original identifiers.
In `@src/lib/storage/migrate-images.ts`:
- Around line 108-134: The downloadImage function must enforce allowed MIME
types and a size limit: after deriving mimeType (variable mimeType) validate it
against the allowed set [image/jpeg, image/png, image/webp, image/gif,
image/svg+xml] and throw a clear Error if not allowed; then check
Buffer.byteLength (the buffer created from arrayBuffer) and reject if it exceeds
10 * 1024 * 1024 bytes (10 MB) with a descriptive Error before proceeding to
filename sanitization and returning; ensure errors are thrown from downloadImage
so callers don’t continue processing invalid content.
- Around line 73-90: isSafeUrl currently only checks URL.protocol and hostname
text, missing IPv6, mapped IPv4-in-IPv6, cloud metadata ranges and DNS
rebinding; update isSafeUrl (and any call sites that fetch remote URLs) to: 1)
parse the URL in isSafeUrl, reject non-http(s); 2) detect IPv6 literal hosts and
reject loopback (::1), link-local (fe80::/10), ULA (fc00::/7), IPv4-mapped
addresses (::ffff:0:0/96 or ::ffff:127.0.0.1) and other reserved IPv6 ranges; 3)
perform a DNS resolution (e.g. dns.lookup/dns.promises.lookup or equivalent) for
hostnames and validate each resolved IP against a denylist of private/reserved
ranges including 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8,
169.254.0.0/16, 100.100.0.0/16 (Alibaba), and cloud metadata addresses,
rejecting if any resolved address falls into those ranges; and 4) optionally add
an explicit domain allowlist that short-circuits checks for trusted hosts.
Update isSafeUrl to return false on any resolution failure or if any resolved
address is private/mapped/reserved.
In `@src/lib/storage/optimize.ts`:
- Around line 68-71: The sharp pipeline currently calls .withMetadata({}) before
.rotate(), which strips EXIF orientation before rotate can read it; in the
pipeline built in optimize.ts (the variable pipeline created from sharp(input)),
move the .rotate() call to come before .withMetadata({}) so auto-rotation runs
while EXIF data is still present (i.e., chain .rotate() immediately after
sharp(input) and then call .withMetadata({})).
---
Nitpick comments:
In `@src/lib/stellar/anchor.ts`:
- Around line 110-118: The loop over ops.records uses a brittle type assertion
for manage_data ops; replace the cast on manageDataOp with a proper
runtime/type-safe check or the SDK's typed response: import the
Horizon/Operation response type for manage_data (e.g., the SDK's manage data
operation record type) and use it for the variable, or keep a runtime guard that
ensures 'value' exists and is a string before decoding; update the block that
references ops.records, manageDataOp, expectedHash and tx.ledger to validate
op.value and only decode when safe so you avoid unsafe assertions and improve
type safety.
In `@src/lib/storage/migrate-images.ts`:
- Line 273: The console.log at the start of the image migration ("console.log(`
Migrating content image: ${url.slice(0, 60)}…`)") prints raw URL fragments that
may include sensitive query params; change this to print a redacted URL instead
by reusing the same redaction logic used for the final report at line 319 (i.e.,
replace the url.slice call with the redacted form produced for the final
report), ensuring you reference the existing redaction helper or variable that
generates the redacted URL for consistency.
- Around line 52-60: The MigrationResult type currently includes a 'skipped'
status that is never assigned; update the MigrationResult interface to remove
'skipped' from the status union (leaving only 'success' | 'failed'), then remove
the corresponding "skipped" row from the migration report generation so it no
longer prints a skipped count; ensure any logic that might reference
MigrationResult.status or build the report (functions/blocks around
MigrationResult usage and the report summarization) is adjusted to expect only
'success' or 'failed'.
- Around line 92-106: The extractImageUrls function only matches src="..."
double-quoted URLs; update the imgSrcRegex and its usage to also capture
single-quoted and unquoted values (e.g., src='https://...' and src=https://...),
for example by using an alternation that creates three capture groups
(double-quoted, single-quoted, unquoted) and then pushing the first non-empty
capture (instead of match[1]) into urls; keep the existing mdImgRegex logic
as-is and preserve the Set de-duplication and return behavior.
In `@src/lib/storage/optimize.ts`:
- Around line 90-91: Replace the two-step decode (await pipeline.toBuffer();
then await sharp(buffer).metadata()) with a single call that returns both buffer
and metadata by using pipeline.toBuffer({ resolveWithObject: true }); update
references to use the returned object's .data (or similar) as the image buffer
and .info (or .metadata) for metadata so you avoid re-parsing; locate the usage
around pipeline.toBuffer and the subsequent sharp(buffer).metadata() calls in
optimize.ts and adapt downstream variables accordingly.
In `@supabase/migrations/0002_media_library.sql`:
- Line 26: The uploaded_by column is currently TEXT and lacks a foreign key to
auth.users; change its type to uuid (or nullable uuid) and add a foreign key
constraint referencing auth.users(id) with an appropriate ON DELETE behavior
(e.g., ON DELETE SET NULL) so deleted users don’t leave invalid references;
update the column definition for uploaded_by and add a REFERENCES auth.users(id)
constraint (or keep TEXT if you intentionally need external identifiers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86835f32-6060-4f9c-8ae8-173892697f73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.env.example.vscode/settings.jsonpackage.jsonsrc/app/admin/layout.tsxsrc/app/api/media/[id]/route.tssrc/app/api/media/orphans/route.tssrc/app/api/media/route.tssrc/app/api/media/upload/route.tssrc/components/modules/admin/media/ImagePickerModal.tsxsrc/components/modules/admin/media/MediaDetailModal.tsxsrc/components/modules/admin/media/MediaFilters.tsxsrc/components/modules/admin/media/MediaLibraryClient.tsxsrc/components/modules/admin/media/OrphanViewer.tsxsrc/components/modules/admin/media/UploadZone.tsxsrc/lib/stellar/anchor.tssrc/lib/storage/media.service.tssrc/lib/storage/migrate-images.tssrc/lib/storage/optimize.tssrc/lib/storage/upload.tssrc/lib/supabase/database.types.tssupabase/migrations/0002_media_library.sql
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (11)
- .vscode/settings.json
- package.json
- src/app/api/media/upload/route.ts
- src/components/modules/admin/media/OrphanViewer.tsx
- src/app/api/media/orphans/route.ts
- src/lib/supabase/database.types.ts
- src/lib/storage/media.service.ts
- src/components/modules/admin/media/MediaLibraryClient.tsx
- src/lib/storage/upload.ts
- src/components/modules/admin/media/MediaDetailModal.tsx
- src/components/modules/admin/media/UploadZone.tsx
Comment on lines
+16
to
+17
| const isAdmin = user.app_metadata?.role === 'admin' || user.user_metadata?.is_admin === true; | ||
| if (!isAdmin) redirect('/'); |
There was a problem hiding this comment.
Do not trust user_metadata for admin authorization (privilege escalation).
src/app/admin/layout.tsx (Lines 16-17) authorizes admin access using user.user_metadata?.is_admin === true. In Supabase, user_metadata is user-controlled and not meant for security-sensitive authorization, so a user can set is_admin: true for themselves and bypass this UI gate. Restrict this check to app_metadata (or a DB-backed role enforced with RLS/server-side authorization).
Suggested fix
- const isAdmin = user.app_metadata?.role === 'admin' || user.user_metadata?.is_admin === true;
+ const isAdmin = user.app_metadata?.role === 'admin';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isAdmin = user.app_metadata?.role === 'admin' || user.user_metadata?.is_admin === true; | |
| if (!isAdmin) redirect('/'); | |
| const isAdmin = user.app_metadata?.role === 'admin'; | |
| if (!isAdmin) redirect('/'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/admin/layout.tsx` around lines 16 - 17, The current isAdmin check
uses user.user_metadata which is user-controlled and allows privilege
escalation; update the logic in the admin layout so isAdmin only checks trusted
data (e.g., user.app_metadata?.role === 'admin') and remove any use of
user.user_metadata?.is_admin; ensure the redirect still happens when the trusted
isAdmin is false and consider adding a server-side/DB-backed authorization check
(RLS or endpoint) for further enforcement in any server-rendered or API paths
that serve admin data.
Comment on lines
+17
to
+22
| function isAdmin(user: { app_metadata?: Record<string, unknown>; user_metadata?: Record<string, unknown> }): boolean { | ||
| return ( | ||
| user.app_metadata?.role === 'admin' || | ||
| user.user_metadata?.is_admin === true | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of the route handler
sed -n '1,120p' src/app/api/media/[id]/route.ts | cat -n
# Extract only the isAdmin helper and all usages of it in this file
rg -n "function isAdmin|isAdmin\(" src/app/api/media/[id]/route.ts
# Show the rest of the file where other handlers may exist
sed -n '120,240p' src/app/api/media/[id]/route.ts | cat -nRepository: ACTA-Team/news-acta
Length of output: 1960
🌐 Web query:
Supabase auth user_metadata user-editable unsafe for authorization app_metadata roles guidance
💡 Result:
In Supabase Auth, it is unsafe to use user_metadata for authorization logic or Row Level Security (RLS) policies because this field is user-editable [1][2]. Because users can modify their own user_metadata via the standard client API, relying on it for sensitive decisions—such as assigning roles, permissions, or access levels—poses a significant security risk [1][3][2]. For security-sensitive metadata, you must use app_metadata [2]. Key distinctions and guidance: 1. user_metadata: - Purpose: Storing non-sensitive user profile information (e.g., display name, avatar URL, preferences) [4]. - Security: User-editable via the client-side SDK (e.g., supabase.auth.updateUser) [4][2]. - Risk: Never use this for RLS or authorization logic, as users can escalate their own privileges [1][2]. 2. app_metadata: - Purpose: Storing system-level information, including roles, permissions, or organization IDs [4][5]. - Security: Admin-only access. It cannot be modified by the user directly; it must be updated via the Supabase Admin API using the service_role key (server-side) [4][3][5]. - Usage: This is the appropriate place to store authorization claims. When you update app_metadata, these claims are included in the user's JWT [5][2]. Implementation Guidance: - Use the Supabase Admin API (e.g., supabaseAdmin.auth.admin.updateUserById) from a secure server-side environment (like Edge Functions or a backend server) to assign roles to app_metadata [3][5]. - In your RLS policies, access these roles via the auth.jwt function, which reads from the JWT claims [5][2]. For example: using (auth.jwt -> 'app_metadata' ->> 'role' = 'admin'); - Be aware that JWTs are cached; updates to app_metadata will not reflect in RLS policies until the user's token is refreshed or expires [5][2].
Citations:
- 1: https://supabase.com/docs/guides/auth/users
- 2: https://supabase.com/docs/guides/database/postgres/row-level-security
- 3: https://asastandard.org/checks/rbac-role-claims-design
- 4: https://deepwiki.com/supabase/auth/8.2-user-metadata
- 5: https://github.com/jeremylongshore/claude-code-plugins-plus-skills/blob/main/plugins/saas-packs/supabase-pack/skills/supabase-enterprise-rbac/SKILL.md
Fix privilege escalation: remove user_metadata trust from isAdmin()
isAdmin() currently authorizes via user.user_metadata?.is_admin === true. Supabase documents user_metadata as user-editable and unsafe for authorization/RLS decisions, so any user who can update their own user_metadata could gain “admin” and pass PATCH/DELETE guards. Use only app_metadata (or server-owned roles) for this check. https://supabase.com/docs/guides/auth/users
Suggested fix
function isAdmin(user: { app_metadata?: Record<string, unknown>; user_metadata?: Record<string, unknown> }): boolean {
- return (
- user.app_metadata?.role === 'admin' ||
- user.user_metadata?.is_admin === true
- );
+ return user.app_metadata?.role === 'admin';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isAdmin(user: { app_metadata?: Record<string, unknown>; user_metadata?: Record<string, unknown> }): boolean { | |
| return ( | |
| user.app_metadata?.role === 'admin' || | |
| user.user_metadata?.is_admin === true | |
| ); | |
| } | |
| function isAdmin(user: { app_metadata?: Record<string, unknown>; user_metadata?: Record<string, unknown> }): boolean { | |
| return user.app_metadata?.role === 'admin'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/media/`[id]/route.ts around lines 17 - 22, The isAdmin function
currently trusts user.user_metadata to grant admin rights; remove that vector by
only checking server-controlled app_metadata. Update the isAdmin(user)
implementation to return true only when user.app_metadata?.role === 'admin'
(remove the user.user_metadata?.is_admin check), and ensure any callers (e.g.,
PATCH/DELETE authorization guards) rely on this updated isAdmin to prevent
user-editable metadata from granting privileges.
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
Closes #28
Implements the complete media management system as specified, using Supabase Storage as the primary layer with image optimization, a media library admin UI, and optional Stellar hash anchoring for verifiable integrity.
What was implemented
Supabase Storage Configuration
article-covers(10 MB),article-content(10 MB),author-avatars(5 MB)image/jpeg,image/png,image/webp,image/gif,image/svg+xmlsupabase/migrations/0002_media_library.sqlUpload Service
POST /api/media/upload{year}/{month}/{uuid}.{ext}media_libraryrecord; optionally anchors on StellarImage Optimization Pipeline (
src/lib/storage/optimize.ts)thumb(200px),sm(640px),md(1024px),lg(1920px),og(1200x630)Database Schema
media_librarytable with all 15 columns from the specbucket,created_at,usage_countMedia Library Admin UI (
/admin/media)Article Editor Integration
ImagePickerModalwith Library tab (search + grid) and Upload tab (drag-and-drop)CoverImagePickerreplaces raw URL input for cover image fieldNext.js Image Integration
NewsCardandNewsDetailusenext/imagewithfill, lazy loading, andsizesnext.config.tshasremotePatternsfor**.supabase.co/storage/v1/object/public/**Stellar Hash Anchoring (
src/lib/stellar/anchor.ts)anchorOnStellartoggle per uploadmanageDataoperation on StellarNEXT_PUBLIC_STELLAR_NETWORKenv varMigration Script (
src/lib/storage/migrate-images.ts)news_articlesfor externalcover_image_urland inline content imagesnpm run migrateOrphan Detection (
/admin/media/orphans)POST /api/media/orphansusage_countfor all media itemsSecurity Fixes
basename()/[a-zA-Z0-9]{1,5}$/console.errorOther
middleware.tswiring up Supabase session refresh.vscode/settings.jsonand.vscode/extensions.jsonfor development consistencytsxadded as devDependency;npm run migratescript addedAcceptance Criteria
next/imagewith lazy loading and responsive sizesSummary by CodeRabbit
New Features
Chores