New version db changes #47
Conversation
📝 WalkthroughWalkthroughConsolidates multiple DB migrations into an idempotent SQL script, updates Sequelize models (cascade FKs, schema fields, nullable changes), adds filesystem-safe media cleanup hooks and a safePath utility, and introduces a script to locate and optionally delete orphaned upload/thumbnail files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application
participant DB as Database (Media)
participant MediaModel as Media Model / Hook
participant FS as File System
participant Logger as Logger
Client->>DB: request delete media record
DB->>MediaModel: trigger beforeDestroy hook
activate MediaModel
MediaModel->>Logger: check UPLOADS_DIR / THUMBNAIL_DIR
alt UPLOADS_DIR not set
MediaModel->>Logger: log error, skip cleanup
MediaModel-->>DB: continue destroy
else
MediaModel->>MediaModel: resolve paths via safePath
alt path traversal detected
MediaModel->>Logger: log error, skip cleanup
MediaModel-->>DB: continue destroy
else
MediaModel->>FS: fsUnlink(originalFile)
FS-->>MediaModel: success / error (logged)
alt THUMBNAIL_DIR set
MediaModel->>FS: scan thumbnails dir recursively
loop for each matching thumbnail
FS->>MediaModel: found file
MediaModel->>FS: fsUnlink(thumbnail)
FS-->>MediaModel: success / error (logged)
end
end
MediaModel-->>DB: cleanup complete, allow destroy
end
end
deactivate MediaModel
DB->>DB: remove record
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@IMPLEMENTATION_SUMMARY.md`:
- Around line 340-346: The SUPPORT section in IMPLEMENTATION_SUMMARY.md
references an internal plan file `.claude/plans/iridescent-painting-quail.md`
which may not exist; either confirm and add that file to the repo or
remove/replace the reference. Open IMPLEMENTATION_SUMMARY.md, locate the
"Support" block containing `.claude/plans/iridescent-painting-quail.md`, and
either (a) point to an existing document or include the missing plan in
`.claude/plans/` with the same name, or (b) remove the broken path and replace
it with a valid internal doc or a generic troubleshooting link. Ensure the final
text no longer points to a non-existent
`.claude/plans/iridescent-painting-quail.md`.
In `@migrations/002_update_comments_foreign_key.sql`:
- Around line 5-7: Replace the hard-coded DROP FOREIGN KEY with a conditional
lookup of the actual FK name in INFORMATION_SCHEMA and drop it only if found:
query INFORMATION_SCHEMA.TABLE_CONSTRAINTS / KEY_COLUMN_USAGE for the constraint
on table `comments` referencing `users` (or specifically the `user_id` column),
capture the constraint_name, and execute an ALTER TABLE `comments` DROP FOREIGN
KEY <constraint_name> when present (mirroring the safe-to-rerun pattern used in
migrations 004 and 007) so the migration will succeed even if the FK has a
different name.
In `@migrations/003_add_post_id_to_media.sql`:
- Around line 1-7: Add a backfill step to the migration that populates
media.post_id from the legacy media_relations table before enforcing any FK:
query media_relations for rows linking media_id → post_id (and decide/mention
how to pick one if multiple relations exist), update the media table's post_id
for matching media.id where post_id is NULL using that value, and run this
inside a transaction (or with appropriate batching) so existing links are
preserved; reference the media table, the new post_id column, and the legacy
media_relations table/media_id/post_id columns when adding the update/backfill
and only add/enable the FK afterward.
In `@migrations/006_add_user_deletion_cascades.sql`:
- Around line 13-17: The ALTER TABLE adding foreign key fk_posts_user_id on
table posts is not idempotent and will fail if that constraint already exists;
update the migration to query INFORMATION_SCHEMA.TABLE_CONSTRAINTS
(CONSTRAINT_SCHEMA = DATABASE(), TABLE_NAME = 'posts', CONSTRAINT_NAME =
'fk_posts_user_id') and only run the ALTER TABLE ... ADD CONSTRAINT
fk_posts_user_id ... ON DELETE CASCADE ON UPDATE CASCADE when the count is 0
(use dynamic SQL with PREPARE/EXECUTE as in migration 007 pattern), otherwise
skip or log that the constraint already exists; apply the same existence-check
pattern for any other constraints added in this migration.
- Around line 9-61: Migration 006 adds FK constraints unconditionally; update it
to first check for and/or drop any existing foreign keys before adding each
constraint (follow the pattern used in migrations 002 and 004). For each
constraint (fk_posts_user_id on posts, fk_media_user_id on media,
fk_messages_user_id on messages, fk_messages_subscribers_user_id on
messages_subscribers, fk_groups_users_user_id on groups_users, fk_blog_author_id
on blog, fk_activations_user_id on activations) either (a) DROP FOREIGN KEY if
it exists, or (b) query
INFORMATION_SCHEMA.KEY_COLUMN_USAGE/REFERENTIAL_CONSTRAINTS to conditionally run
the ALTER TABLE ... ADD CONSTRAINT only when that constraint is absent; keep the
same ON DELETE/ON UPDATE CASCADE behavior and preserve the FOREIGN_KEY_CHECKS
wrapper. Ensure you reference the exact constraint names above when implementing
the checks/drops.
In `@migrations/007_remove_posts_image_id.sql`:
- Around line 11-45: The migration currently checks `@constraint_exists` and
attempts to drop a hard-coded FK `posts_image_id_media_id_fk`, which fails if
the FK has a different name; update the logic around `@constraint_exists` and
`@drop_fk_sql` to discover the actual constraint name for table `posts` and column
`image_id` (e.g., query INFORMATION_SCHEMA.KEY_COLUMN_USAGE where
TABLE_NAME='posts' AND COLUMN_NAME='image_id' AND REFERENCED_TABLE_NAME IS NOT
NULL), store that constraint name into a variable (instead of assuming
`posts_image_id_media_id_fk`), and build the ALTER TABLE `posts` DROP FOREIGN
KEY <discovered_name> statement for PREPARE/EXECUTE only when a name is found so
the subsequent DROP COLUMN `image_id` will not fail due to an existing FK.
In `@migrations/README.md`:
- Around line 31-34: Update the migrations README so it accurately describes the
actual migration files in this PR: replace the incorrect description of
"006_update_groups_users_notification_type.sql" with a summary of
006_add_user_deletion_cascades.sql (explain it adds CASCADE foreign keys for
user deletion), and add an entry for 007_remove_posts_image_id.sql describing
that it removes the image_id column and any related constraints; ensure the
filenames 006_add_user_deletion_cascades.sql and 007_remove_posts_image_id.sql
are referenced exactly and remove mention of the nonexistent enum change.
- Around line 43-44: The README example uses the wrong migration filename;
update the example command that references
006_update_groups_users_notification_type.sql to instead reference the actual
migration file 006_add_user_deletion_cascades.sql so the two example mysql
import commands match existing migration filenames (leave the other command
using 005_add_performance_indexes.sql unchanged).
In `@models/groups.js`:
- Line 6: The model change set the Groups model's description attribute
(models/groups.js -> description) to allowNull: false but there is no DB
migration; create a Sequelize migration that (1) backfills any NULL descriptions
in the groups table with a sensible default or computed value (e.g., empty
string or "No description"), (2) alters the description column to set NOT NULL
at the database level, and (3) include a down migration to revert the NOT NULL
constraint and restore previous values if needed; run the backfill before
applying the NOT NULL change and ensure the migration is referenced/deployed
alongside the models/groups.js change.
In `@models/index.js`:
- Around line 91-103: The hook using UPLOADS_DIR in
db.Media.addHook('beforeDestroy') lacks validation and can produce unexpected
paths; add a guard that verifies UPLOADS_DIR is a non-empty string before
calling path.resolve and attempting fsUnlink (or provide a clear, explicit
fallback directory), and if invalid log a distinct error via logger.error
(including UPLOADS_DIR value, media.id, media.user_id) and skip the file-delete
logic so DB deletion still proceeds; ensure the validation is performed either
at module initialization or at the start of the hook where path.resolve(...) and
fsUnlink(...) are invoked.
In `@routes/graphql/mutations/posts.js`:
- Around line 45-47: The slug is being generated from the raw args.title instead
of the sanitized title stored on the post; update the conditional that sets
post.slug to call generateSlug(Post, post.title) rather than generateSlug(Post,
args.title) so it uses the cleaned value produced by cleanContent(), referencing
post.slug, generateSlug, Post, args.title and post.title to locate the change.
In `@routes/graphql/typeDefs/types.graphql`:
- Around line 64-68: GraphQL type Post defines slug as nullable (slug: String)
but the Sequelize Post model enforces allowNull: false, causing a contract
mismatch; fix by either (A) making GraphQL non-nullable: change the Post.slug
field in types.graphql to slug: String! to match the model, or (B) allowing NULL
at the DB level: update the Sequelize Post model (the Post model definition that
sets allowNull for slug) to allowNull: true, create a migration that alters the
posts.slug column to NULLABLE, run the migration, and ensure any
resolver/validation logic that assumes non-null slug is adjusted accordingly.
In `@routes/graphql/types.js`:
- Around line 57-64: The content resolver currently masks valid text when
comment.user_id is null by returning "[deleted]"; change the content resolver
(the function assigned to content) to always return comment.content so stored
comment text is preserved, and leave the author resolver behavior as-is (author
returns DELETED_USER when comment.user_id is falsy and otherwise calls
User.findByPk(comment.user_id)); update only the content resolver logic and do
not alter DELETED_USER or User.findByPk usage.
In `@SCHEMA_UPDATE_SUMMARY.md`:
- Around line 9-10: Replace machine-specific absolute paths (e.g., the string
"/Users/spathon/Sites/cham/API/migrations/") with repo-relative or placeholder
paths throughout the file SCHEMA_UPDATE_SUMMARY.md; search for that exact path
and the other occurrences noted (lines referenced around 91-92, 132-133,
176-187) and update them to a consistent repo-relative form such as
"./migrations" or a placeholder like "${REPO_ROOT}/API/migrations" so the
document is portable for other developers.
- Around line 11-18: Update SCHEMA_UPDATE_SUMMARY.md to list seven migration
scripts (change "Six" to "Seven") and replace the current list so it includes
the actual filenames in order: 001_add_rate_limiting_fields_to_users.sql,
002_update_comments_foreign_key.sql, 003_add_post_id_to_media.sql,
004_add_posts_image_id_foreign_key.sql, 005_add_performance_indexes.sql,
006_add_user_deletion_cascades.sql, and 007_remove_posts_image_id.sql (remove or
reconcile the incorrect 006_update_groups_users_notification_type.sql entry);
also update Step 2 execution commands and any other references (lines ~89-103)
to reference the correct filenames and count so operators run migrations 001–007
in the proper order.
In `@scripts/cleanup-orphaned-files.js`:
- Around line 261-263: The summary log uses result.totalSize which can
overreport when some deletions fail; modify the deletion loop in
scripts/cleanup-orphaned-files.js to accumulate the size of only successfully
deleted files (e.g., maintain a deletedSize counter updated when a file deletion
returns success) and then, when DELETE_MODE is true, log deletedCount and
formatBytes(deletedSize) instead of result.totalSize; keep references to
DELETE_MODE, result.deletedCount and replace result.totalSize with the new
deletedSize that you compute from successful delete operations.
- Around line 153-160: The orphan-detection is using filenames only (creating
validFiles = new Set(mediaRecords.map(m => m.filename))) which causes false
negatives when different users share the same filename; change the logic to
build the Set of valid keys using the composite "{user_id}/{filename}" from
mediaRecords (e.g. map each m => `${m.user_id}/${m.filename}`), and then compare
against the same composite key produced by scanDirectory()/allFiles entries when
computing orphanedFiles; update any references to validFiles and the
orphanedFiles filter to use this composite key so files in user-specific
subdirectories are matched correctly.
🧹 Nitpick comments (3)
routes/graphql/queries.js (1)
204-217: Parameterize the raw SQL instead of string interpolation.
WHERE comments.user_id = ${me.id}should use replacements/bind parameters to avoid injection risk and improve query plan caching.🔧 Parameterized query
- const commentPosts = await sequelize.query(` + const commentPosts = await sequelize.query(` SELECT posts.id, posts.user_id, posts.slug, posts.group_id, posts.comments_count, posts.title, posts.created_at AS createdAt, EXISTS(SELECT id FROM media WHERE media.post_id = posts.id) AS hasMedia, COUNT(posts.id) AS commentsMade FROM posts JOIN comments ON comments.post_id = posts.id - WHERE comments.user_id = ${me.id} + WHERE comments.user_id = :userId AND posts.status = 'published' GROUP BY posts.id ORDER BY posts.id DESC LIMIT 200 - `, { type: sequelize.QueryTypes.SELECT }) + `, { replacements: { userId: me.id }, type: sequelize.QueryTypes.SELECT })models/index.js (1)
97-97: Uselogger.infoinstead ofconsole.logfor consistent structured logging.Errors are logged via
logger.error, but success messages useconsole.log. This creates inconsistency in log aggregation and observability.♻️ Suggested change
- console.log(`Deleted file: ${filePath}`) + logger.info('FILE_DELETED', { filePath, fileId: media.id, userId: media.user_id })Apply the same pattern at line 139:
- console.log(`Deleted thumbnail: ${fullPath}`) + logger.info('THUMBNAIL_DELETED', { path: fullPath })scripts/cleanup-orphaned-files.js (1)
275-280: Database connection not closed on error before exit.When an error occurs,
process.exit(1)is called without closing the database connection. While the process is terminating anyway, explicitly closing the connection ensures clean shutdown and prevents potential connection leaks in pooled environments.♻️ Proposed fix
} catch (err) { log(`Fatal error: ${err.message}`, 'ERROR') log(err.stack, 'ERROR') await writeLogFile() + await sequelize.close().catch(() => {}) // Best effort cleanup process.exit(1) }
…ion, and docs - Use sanitized title for slug generation in createPost and editPost - Parameterize raw SQL in postsCommented query to prevent SQL injection - Guard UPLOADS_DIR in Media beforeDestroy hook, replace console.log with logger - Fix orphan cleanup script to use user_id/filename composite key and track deletedSize - Add media.post_id backfill from media_relations in combined.sql - Update docs to reference combined.sql, remove absolute paths and stale references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routes/graphql/mutations/posts.js (1)
36-39:⚠️ Potential issue | 🟡 MinorNull-check order:
postcould benullbefore accessingpost.user_id.
Post.findByPk(args.id)returnsnullwhen the record doesn't exist, causing a crash on Line 39 (post.user_id). While this is pre-existing, the function is being modified in this PR — consider adding a guard.Proposed fix
async editPost(_, args, { me }) { - const post = await Post.findByPk(args.id) if (!me) throw new GraphQLError('You must be logged in.') + const post = await Post.findByPk(args.id) + if (!post) throw new GraphQLError('Post not found.') if (post.user_id !== me.id) throw new GraphQLError('You can\'t edit some one else post.')
🤖 Fix all issues with AI agents
In `@IMPLEMENTATION_SUMMARY.md`:
- Around line 69-87: Update the stale hook snippet to match the real
implementation in models/index.js: replace the console.log in the
db.Media.addHook('beforeDestroy', ...) example with logger.error usage for
failures, add the guard that checks UPLOADS_DIR exists before attempting
fsUnlink(…) and ensure the thumbnail cleanup logic using THUMBNAIL_DIR and
deleteThumbnailsRecursive(userThumbDir, media.filename) is present;
alternatively, remove the embedded snippet and add a clear reference/link to the
actual implementation in models/index.js so contributors are not misled by
outdated code.
- Around line 319-336: Update the IMPLEMENTATION_SUMMARY.md header counts and
lists so they match the actual PR contents: change "Code Changes (7 files)" to
"Code Changes (10 files)" and ensure the ten files listed include
routes/graphql/types.js, routes/graphql/queries.js, models/index.js,
models/posts.js, models/media.js, models/messages.js,
models/messagesSubscribers.js, models/groupsUsers.js, models/blog.js,
models/activation.js; change "New Files (3 files)" to "New Files (6 files)" and
list all new files including migrations/006_add_user_deletion_cascades.sql,
migrations/007_remove_posts_image_id.sql, scripts/cleanup-orphaned-files.js,
plus IMPLEMENTATION_SUMMARY.md, SCHEMA_UPDATE_SUMMARY.md, and
migrations/README.md so the counts and items are consistent.
- Around line 46-53: The doc example currently shows raw template interpolation
using ${me.id}; update it to show a parameterized SQL placeholder (e.g. a named
parameter like :meId) and explicitly state that the real implementation uses
Sequelize parameter binding/replacements, and describe passing the value (me.id)
via the replacements/bind options so readers do not mistake template-literal
interpolation for safe query construction.
In `@models/index.js`:
- Around line 90-107: The beforeDestroy hook on db.Media currently builds
filePath using media.filename allowing path traversal; update the hook around
db.Media.addHook('beforeDestroy') to validate/sanitize media.filename before
unlinking by either using a safe basename/sanitization and rejecting filenames
containing path separators or by resolving the candidate path and verifying it
is within the UPLOADS_DIR root (e.g., ensure path.resolve(UPLOADS_DIR,
String(media.user_id), media.filename).startsWith(path.resolve(UPLOADS_DIR) +
path.sep)); if validation fails, log a warning including fileId and userId and
skip fsUnlink to avoid deleting files outside the uploads dir.
In `@SCHEMA_UPDATE_SUMMARY.md`:
- Around line 195-199: The date under the "## Migration Completed" section is
inconsistent: change the line showing "2026-01-03" to the correct
implementation/migration date that matches the PR and combined.sql header
(February 2026) so the SCHEMA_UPDATE_SUMMARY.md is aligned; edit the date string
on the "Date:" line near the "## Migration Completed" heading accordingly.
In `@scripts/cleanup-orphaned-files.js`:
- Around line 160-163: The filter that builds the lookup key (in orphanedFiles)
assumes files live under a user subdirectory; add a short comment above the
block noting that files directly under UPLOADS_DIR (no user subdirectory) will
produce a key like `filename/filename` and thus be treated as orphaned and
removed, and if you want to preserve root files adjust the key generation: in
the orphanedFiles filter use the existing symbols (file.relativePath, parts,
key, validFiles) but derive key as either `${parts[0]}/${file.filename}` when
parts.length>1 or as `file.filename` when parts.length===1 so lookups match the
intended validFiles format—otherwise keep the comment documenting the
assumption.
🧹 Nitpick comments (6)
scripts/cleanup-orphaned-files.js (2)
251-287: Database connection not closed on fatal error path.If the
tryblock throws aftersequelize.authenticate()succeeds, thecatchblock logs and exits without callingsequelize.close(). Whileprocess.exit(1)will terminate the process, this can leave dangling connections in connection-pooled environments.Proposed fix — use finally
+ } finally { + try { await sequelize.close() } catch (_) { /* ignore */ } + } - await writeLogFile() - process.exit(1) - } + // Move process.exit into catch:Or more practically, wrap the cleanup:
} catch (err) { log(`Fatal error: ${err.message}`, 'ERROR') log(err.stack, 'ERROR') await writeLogFile() + try { await sequelize.close() } catch (_) { /* ignore close errors */ } process.exit(1) }
38-43:formatBytesreturnsInfinityorNaNfor negative input.Edge case: if
bytesis negative (e.g., from an unexpectedstats.size),Math.log(negative)yieldsNaN. This won't happen in practice but a guard would be defensive.migrations/combined.sql (2)
92-93:MODIFY COLUMNruns unconditionally on every re-run.Unlike the other steps that use
IF/prepared-statement guards, Line 93 always executes theALTER TABLEeven whenuser_idis already nullable. MySQL handles this gracefully (it's a no-op in effect), but on large tables theALTERmay still acquire a metadata lock and rebuild the table depending on the MySQL version/engine.For consistency with the rest of the script, consider wrapping this in a conditional check against
IS_NULLABLEinINFORMATION_SCHEMA.COLUMNS.
334-370: Blog FK replacement will drop-and-recreate on every re-run.The script finds the current FK by name (including the one it may have created on a previous run), drops it, then adds a new one. On repeated runs this performs unnecessary DDL. Not harmful, but a tighter guard (e.g., checking the current
DELETE_RULEinINFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS) would make it truly no-op on re-run.This is a minor observation — the current approach is functionally correct.
migrations/README.md (1)
7-7: Nit: "rate limiting" → "rate-limiting" (compound adjective).Per static analysis hint. As per coding guidelines, "rate-limiting" should be hyphenated when used as a compound modifier.
Fix
-1. Adds rate limiting fields to users table +1. Adds rate-limiting fields to users tableSCHEMA_UPDATE_SUMMARY.md (1)
114-125: Use a generic placeholder path consistently.Line 116 uses
cd /path/to/APIwhich is fine as a placeholder, but consider also noting that environment variables (UPLOADS_DIR, etc.) need to be set for the test scenarios described on Lines 122-124 (e.g., testing user deletion with file cleanup).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
routes/auth.js (2)
114-116:⚠️ Potential issue | 🟠 MajorBug:
&&should be||— validation only triggers when all three params are non-numeric.The current condition throws only if
userId,w, andhare all non-numeric simultaneously. If only one is non-numeric (e.g., a malicioususerId), execution continues unchecked. This weakens the downstream path construction on Line 128 (absThumbPath), which does not usesafePath.🐛 Proposed fix
if ( - !isNumber.test(userId) && !isNumber.test(w) && !isNumber.test(h) + !isNumber.test(userId) || !isNumber.test(w) || !isNumber.test(h) ) throw new Error('Has to be numeric')
127-128:⚠️ Potential issue | 🟠 Major
absThumbPathis constructed withpath.join— not guarded bysafePath.While
UPLOADS_DIRis now protected viasafePathon Line 124, theTHUMBNAIL_DIRpath on Line 128 uses rawpath.join. Combined with the&&/||bug on Line 115, a non-numericuserIdcould traverse outsideTHUMBNAIL_DIR. Consider applyingsafePathhere as well for consistency.🛡️ Proposed fix
const relThumbPath = path.join(userId, h, w) - const absThumbPath = path.join(process.env.THUMBNAIL_DIR, relThumbPath) + const absThumbPath = safePath(process.env.THUMBNAIL_DIR, userId, h, w)routes/graphql/mutations/media.js (1)
69-77:⚠️ Potential issue | 🟡 MinorDouble file deletion: explicit
fsUnlinkon Line 71, thenbeforeDestroyhook fires again on Line 77.
media.destroy({ force: true })triggers thebeforeDestroyhook inmodels/index.js, which also attempts to delete the same file. The second attempt will fail withENOENTand log aFILE_CLEANUP_ERRORon every successful delete — producing noisy false-alarm logs.Options:
- Remove the explicit
fsUnlinkhere and let the hook handle cleanup.- Pass an option to skip hook cleanup (e.g.,
{ hooks: false }), keeping the explicit delete.- Use
{ individualHooks: false }or a custom flag to signal the hook.♻️ Option 1: Let the hook handle it
async deleteFile(_, { id }, { me }) { const media = await Media.findByPk(id) if (!media) throw new GraphQLError('No file found') if (media.user_id !== me.id) throw new GraphQLError('No, just no!') - const filePath = safePath(UPLOADS_DIR, String(media.user_id), media.filename) - try { - await fsUnlink(filePath) - } catch (err) { - logger.error('DELETE_FILE_ERROR', { - error: err, filePath, fileId: media.id, userId: me.id, - }) - } await media.destroy({ force: true }) return id },
🧹 Nitpick comments (3)
models/index.js (2)
3-6: Minor: mixedpromisify(fs.unlink)andfs.promises.*APIs.Lines 114 and 155 use
fsUnlink(promisified), while lines 127 and 144 usefs.promises.stat/fs.promises.readdir. Consider usingfs.promises.unlinkthroughout for consistency.♻️ Proposed fix
-const { promisify } = require('util') const Sequelize = require('sequelize') -const fsUnlink = promisify(fs.unlink)Then replace
fsUnlink(...)calls withfs.promises.unlink(...).Also applies to: 114-114, 127-127, 144-144
141-165: Recursive thumbnail deletion could follow symlinks into unintended directories.
Dirent.isDirectory()in Node.js returnsfalsefor symlinks, so the current code won't follow symlinks by default — which is the safe behavior. However, iffollowSymlinksbehavior ever changes orreaddiroptions change, this could become a risk. A brief comment noting the symlink assumption would aid future maintainers.routes/graphql/mutations/media.js (1)
34-35:uploadFilestill uses string concatenation for paths instead ofsafePath.While
me.idis an authenticated integer andnewFilenameis UUID-generated (low risk), usingsafePathhere would be consistent with the safety pattern established across the rest of this PR.♻️ Proposed fix
- const absPath = `${UPLOADS_DIR}${me.id}/` - const newFilePath = `${absPath}${newFilename}` + const absPath = safePath(UPLOADS_DIR, String(me.id)) + const newFilePath = safePath(UPLOADS_DIR, String(me.id), newFilename)Note: The
fsMkdircall on Line 47 would needabsPathwithout the trailing separator, whichsafePathalready provides.
2 participants
Preparation for Next.js version
Summary by CodeRabbit
New Features
Bug Fixes
Database & Performance
Changes