Add persistent split logging with rotation and retention - #81
Add persistent split logging with rotation and retention#81wickedyoda wants to merge 41 commits into
Conversation
…ovements Feature/ghcr ci and upload improvements
…ovements Add upload cancellation, retention config, and GHCR publish workflow
…ovements Add failed-upload retention cleanup and legal warning banners
…ovements Upgrade GitHub Actions to Node 24 compatible versions
…ovements Fix test script for Node 24 test discovery
…ovements Upload reliability, download link fixes, and legal banner updates
…ovements Upload reliability, HTTPS links, and WickedYoda title updates
…ovements Improve public download links and failed upload cleanup config
…ovements Fix upload URL test for sanitized filenames
…rovements title update
Add configurable TERMS_LINK and update docs
Fix missing getTermsLink helper in config
Update index.html
Fix rename modal positioning
WalkthroughAdds short-link download routing, configurable public-domain and terms links, file retention and failed-upload cleanup, optional file-based and access logging, client-side upload cancellation with fail reporting, Docker/CI image and logging config updates, expanded docs, and tests for retention/upload/download behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Server as DumbDrop Server
participant FS as Filesystem
participant Cleanup as Retention Cleanup Scheduler
Client->>Server: GET /:shortPath
Server->>Server: resolve path -> config.uploadDir
Server->>Server: validate via isPathWithinUploadDir
Server->>FS: stat(path)
alt file exists & is regular file
FS-->>Server: file stream
Server->>Client: 200 + streamed file\nContent-Disposition header
else missing/invalid/reserved
Server->>Client: 403/404 or pass to other routing
end
Client->>Server: POST /api/upload/init and /api/upload/chunk
Server->>FS: write partials + metadata
Client->>Server: cancel -> POST /api/upload/fail/:uploadId
Server->>FS: set metadata.failedAt
Cleanup->>FS: periodic cleanupExpiredFiles checks retention
Cleanup->>FS: delete expired partials and metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces persistent, host-mountable file logging with category-split log files, rotation/retention controls, and adds an HTTP access logging middleware for operational visibility.
Changes:
- Added file-backed logging in
src/utils/logger.jswith configurable log directory, rotation windows, and retention pruning. - Added request/response access logging middleware in
src/app.jsbehind anACCESS_LOG_ENABLEDtoggle. - Updated Docker/compose and README to support bind-mounting
/logsand documenting logging env vars.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/logger.js | Implements file logging, category splitting, rotation naming, and retention pruning logic. |
| src/app.js | Adds middleware to emit structured access logs on response completion. |
| docker-compose.yml | Mounts host ./logs into container /logs and sets logging env vars. |
| Dockerfile | Ensures /logs directory exists in dev/prod images. |
| README.md | Documents new logging environment variables. |
Comments suppressed due to low confidence (1)
src/app.js:65
- The PR description says “Console logging remains unchanged”, but this middleware introduces new console output (via
logger.access(..., console.log, ...)) for every request whenACCESS_LOG_ENABLEDis true. If the intention is file-only access logs, consider sending access logs only to files (or add a separate toggle for console access logs) and update the PR description accordingly.
const RESERVED_SHORT_LINK_PREFIXES = ['api/', 'assets/', 'toastify/'];
// Create Express app
const app = express();
const PORT = process.env.PORT || 3000;
const BASE_URL = process.env.BASE_URL || `http://localhost:${PORT}`;
// Configure proxy trust based on environment (security-sensitive)
if (config.trustProxy) {
if (config.trustedProxyIps && config.trustedProxyIps.length > 0) {
// Trust only specific proxy IPs
app.set('trust proxy', config.trustedProxyIps);
logger.warn(`Proxy trust enabled for specific IPs: ${config.trustedProxyIps.join(', ')}`);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const categories = getCategoryPrefixes(level); | ||
| for (const category of categories) { | ||
| const filePath = getLogFilePath(category); | ||
| try { | ||
| fs.appendFileSync(filePath, `${line}\n`, 'utf8'); | ||
| } catch (err) { |
| function initializeFileLogging() { | ||
| if (!LOG_TO_FILE) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| fs.mkdirSync(LOG_DIR, { recursive: true }); | ||
| pruneOldLogs(); | ||
| fileLoggingReady = true; | ||
| } catch (err) { |
| const fileWindowEnd = fileWindowStart.getTime() + (LOG_ROTATION_DAYS * DAY_MS); | ||
| if (fileWindowEnd < cutoff) { |
| fs.appendFileSync(filePath, `${line}\n`, 'utf8'); | ||
| } catch (err) { | ||
| // Fall back silently to console logging if file writing fails. | ||
| fileLoggingReady = false; | ||
| console.error(`[ERROR] ${new Date().toISOString()} - Failed to write log file (${filePath}): ${err.message}`); | ||
| break; | ||
| } |
| function getRotationWindowStart(date = new Date()) { | ||
| const windowSpanMs = LOG_ROTATION_DAYS * DAY_MS; | ||
| const windowStartMs = Math.floor(date.getTime() / windowSpanMs) * windowSpanMs; | ||
| return new Date(windowStartMs); | ||
| } | ||
|
|
||
| function formatDateStamp(date) { | ||
| const year = date.getUTCFullYear(); | ||
| const month = `${date.getUTCMonth() + 1}`.padStart(2, '0'); | ||
| const day = `${date.getUTCDate()}`.padStart(2, '0'); | ||
| return `${year}${month}${day}`; | ||
| } | ||
|
|
||
| function parseDateStamp(stamp) { | ||
| if (!/^\d{8}$/.test(stamp)) { | ||
| return null; | ||
| } | ||
|
|
||
| const year = Number.parseInt(stamp.slice(0, 4), 10); | ||
| const month = Number.parseInt(stamp.slice(4, 6), 10) - 1; | ||
| const day = Number.parseInt(stamp.slice(6, 8), 10); | ||
| const date = new Date(Date.UTC(year, month, day)); | ||
|
|
||
| if (Number.isNaN(date.getTime())) { | ||
| return null; | ||
| } | ||
|
|
||
| return date; | ||
| } | ||
|
|
||
| function getLogFilePath(prefix, timestamp = new Date()) { | ||
| const windowStart = getRotationWindowStart(timestamp); | ||
| const stamp = formatDateStamp(windowStart); | ||
| return path.join(LOG_DIR, `${prefix}-${stamp}.log`); | ||
| } |
| | APPRISE_MESSAGE | Notification message template | New file uploaded {filename} ({size}), Storage used {storage} | No | | ||
| | APPRISE_SIZE_UNIT | Size unit for notifications (B, KB, MB, GB, TB, or Auto) | Auto | No | | ||
| | AUTO_UPLOAD | Enable automatic upload on file selection | false | No | | ||
| | SHOW_FILE_LIST | Enable file listing with download and delete functionality | false | No | | ||
| | CLIENT_MAX_RETRIES | Maximum client retries for chunk upload failures | 5 | No | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/utils/logger.js (1)
16-17: Function used before declaration. Works, but only because JavaScript is polite.
LOG_RETENTION_DAYSandLOG_ROTATION_DAYS(lines 16–17) callgetPositiveInteger, which is declared at line 22. This works only because function declarations are hoisted — if anyone ever "modernizes" this toconst getPositiveInteger = (…) => …, the module will crash at load time with a TDZReferenceError, and no test will catch it until production boots.Either move the function declaration above the constants, or add a comment flagging the hoisting dependency so future refactoring doesn't blow up in someone's face.
Also applies to: 22-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.js` around lines 16 - 17, LOG_RETENTION_DAYS and LOG_ROTATION_DAYS call getPositiveInteger before its declaration (relying on function hoisting), which is fragile; either move the getPositiveInteger function declaration above the constants (so getPositiveInteger is defined before LOG_RETENTION_DAYS and LOG_ROTATION_DAYS are initialized) or add a clear comment above the constants naming getPositiveInteger and explaining the intentional hoisting dependency to prevent future refactors (e.g., changing getPositiveInteger to an arrow function) from breaking module load.src/app.js (1)
56-65:finishonly. Aborted requests become ghosts.
res.on('finish', …)fires only when the response completes successfully. If the client bails mid-upload, the connection is killed, or Node emits an error,finishnever runs and you log nothing — which is exactly the sort of thing ops people want to see in an access log. Standard practice is to also hookcloseand guard against double-logging.🔧 Suggested pattern
- res.on('finish', () => { + let logged = false; + const emit = () => { + if (logged) return; + logged = true; const durationMs = Date.now() - startedAt; const contentLength = res.getHeader('content-length') || 0; const ip = req.ip || req.socket?.remoteAddress || 'unknown'; const userAgent = req.get('user-agent') || '-'; logger.access( `${req.method} ${req.originalUrl || req.url} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${ip} ua="${userAgent}"`, ); - }); + }; + res.on('finish', emit); + res.on('close', emit);Alternatively — and frankly, the adult move — just install
morganand stop reinventing access logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.js` around lines 56 - 65, The current access-log uses only res.on('finish') so aborted/closed connections never get logged; modify the middleware around res.on('finish', …) to also attach res.on('close', …) and share the same logging logic, using a local boolean flag (e.g., let logged = false) to prevent double-logging, and ensure you still compute durationMs, contentLength, ip and userAgent the same way before calling logger.access (keep references to req.method, req.originalUrl || req.url, res.statusCode, etc.); optionally mark the close-event log with a reason like "aborted" if res.finished is false.Dockerfile (1)
35-36: Congratulations, you made a directory. Did you remember who owns it?
mkdir -p uploads /logscreates/logsowned byroot. Right now this container runs as root so nobody notices, but the moment someone gets clever and adds aUSER nodedirective (which, spoiler, is best practice), the logger'sfs.mkdirSync/appendFileSyncinsrc/utils/logger.jswill start throwingEACCES, flipfileLoggingReady = false, and your shiny new split logs silently vanish into the void. Consider pre-seeding ownership now so future-you doesn't spend an evening debugging "why no logs".🔧 Optional hardening
-# Create upload and logs directories -RUN mkdir -p uploads /logs +# Create upload and logs directories +RUN mkdir -p uploads /logs && chown -R node:node uploads /logsAlso worth noting: bind-mounting
./logsfrom the host (perdocker-compose.yml) will override the container's/logspermissions with whatever the host directory has, so the real fix for rootless deploys is to match UIDs on the host too.Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 35 - 36, The Dockerfile creates uploads and /logs as root (RUN mkdir -p uploads /logs) which will cause EACCES when the container runs as a non-root user; update the Dockerfile to create and chown those directories to the intended runtime user (e.g., node) in the same RUN step—for example: create the directories and then run chown -R node:node uploads /logs (and apply the same pattern to the other creation sites referenced around lines 54-55) so that src/utils/logger.js can write logs when a USER node directive or a rootless deployment is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Line 20: The environment variable ACCESS_LOG_ENABLED is defined as an unquoted
YAML boolean which will become invalid in future Docker Compose releases; update
the docker-compose.yml environment entry for ACCESS_LOG_ENABLED to use a quoted
string value (e.g., "true" or "false") so the variable is passed as a string,
ensuring compatibility with Compose and environment parsers.
In `@src/app.js`:
- Around line 52-68: The access-log middleware is writing unsanitized fields
(req.get('user-agent') and req.originalUrl/req.url) directly into logger.access,
allowing log injection; fix this by adding a single reusable sanitizer (e.g.,
sanitizeLogField) and a module-level precompiled regex (e.g., SANITIZE_RE =
/[\r\n"]/g) that replaces control chars with safe placeholders or escapes (strip
or replace \r, \n, and " ), then call sanitizeLogField on req.get('user-agent')
and on req.originalUrl || req.url before interpolating into the logger.access
message; keep other logic the same and avoid allocating the regex inside the
middleware so it’s reused per request.
In `@src/utils/logger.js`:
- Around line 31-44: The current pruneOldLogs() is only called once in
initializeFileLogging(), so logs never get pruned after startup; modify
initializeFileLogging() to schedule recurring pruning by creating a timer like
setInterval(() => { try { pruneOldLogs(); } catch (e) { console.error(...) } },
DAY_MS).unref() immediately after the initial pruneOldLogs() call (only when
LOG_TO_FILE is true and after fileLoggingReady is set), and ensure the interval
callback is wrapped in try/catch so filesystem errors don't crash the process;
reference pruneOldLogs, initializeFileLogging, and DAY_MS when making this
change.
- Around line 82-102: getCategoryPrefixes currently always adds 'container'
causing every log (ACCESS, ERROR, DEBUG, etc.) to be duplicated into
container-*.log; remove 'container' from the default fan-out in
getCategoryPrefixes so each level maps only to its own category(s) (e.g., ACCESS
-> ['access'], ERROR -> ['app','error'], DEBUG -> ['debug']), and if a catch-all
mirror is required introduce an explicit config flag (e.g.,
ENABLE_CONTAINER_MIRROR) checked in getCategoryPrefixes before adding
'container' so duplication is opt-in rather than automatic.
- Around line 46-50: The rotation window logic in getRotationWindowStart anchors
windows to the Unix epoch which yields odd calendar dates; change it to align to
calendar boundaries by first computing the UTC midnight for the given date (use
date.getUTCFullYear(), getUTCMonth(), getUTCDate() to build a UTC-midnight Date)
and then compute the bucket start relative to a calendar anchor (for example,
the UTC-midnight of Jan 1 of the same year or another explicit calendar-based
reference) using LOG_ROTATION_DAYS and DAY_MS to floor into N-day buckets;
update getRotationWindowStart to use that calendar-aligned anchor instead of
date.getTime() / windowSpanMs so rotation files start on predictable calendar
days.
- Around line 136-153: The writeToFile function sets fileLoggingReady = false
permanently on the first appendFileSync error, which kills file logging until
restart; change this to implement retry/backoff and periodic probes instead of a
one-shot disable: replace the immediate permanent toggle with logic that counts
consecutive failures (e.g., consecutiveFileWriteFailures), marks
fileLoggingReady false only after a threshold, and schedule reinitialization
attempts by calling initializeFileLogging (or re-running the file setup using
getLogFilePath) with exponential backoff (via setTimeout) to reset
fileLoggingReady on success; ensure the appendFileSync error handler logs the
error but does not permanently disable logging on a single transient error.
- Around line 136-153: The writeToFile function currently blocks the event loop
by calling fs.appendFileSync for each category; replace the synchronous writes
with cached append streams: create a Map to cache fs.createWriteStream(filePath,
{ flags: 'a', encoding: 'utf8' }) per category (use getCategoryPrefixes(level)
to derive keys), call stream.write(`${line}\n`) instead of appendFileSync, and
on stream 'error' set fileLoggingReady = false and console.error the error
(preserving the existing error message text), ensuring streams are created
lazily and reused to avoid per-request blocking.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 35-36: The Dockerfile creates uploads and /logs as root (RUN mkdir
-p uploads /logs) which will cause EACCES when the container runs as a non-root
user; update the Dockerfile to create and chown those directories to the
intended runtime user (e.g., node) in the same RUN step—for example: create the
directories and then run chown -R node:node uploads /logs (and apply the same
pattern to the other creation sites referenced around lines 54-55) so that
src/utils/logger.js can write logs when a USER node directive or a rootless
deployment is used.
In `@src/app.js`:
- Around line 56-65: The current access-log uses only res.on('finish') so
aborted/closed connections never get logged; modify the middleware around
res.on('finish', …) to also attach res.on('close', …) and share the same logging
logic, using a local boolean flag (e.g., let logged = false) to prevent
double-logging, and ensure you still compute durationMs, contentLength, ip and
userAgent the same way before calling logger.access (keep references to
req.method, req.originalUrl || req.url, res.statusCode, etc.); optionally mark
the close-event log with a reason like "aborted" if res.finished is false.
In `@src/utils/logger.js`:
- Around line 16-17: LOG_RETENTION_DAYS and LOG_ROTATION_DAYS call
getPositiveInteger before its declaration (relying on function hoisting), which
is fragile; either move the getPositiveInteger function declaration above the
constants (so getPositiveInteger is defined before LOG_RETENTION_DAYS and
LOG_ROTATION_DAYS are initialized) or add a clear comment above the constants
naming getPositiveInteger and explaining the intentional hoisting dependency to
prevent future refactors (e.g., changing getPositiveInteger to an arrow
function) from breaking module load.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 640df3ac-2d15-4bb3-92d4-493ef4ceb8fc
📒 Files selected for processing (5)
DockerfileREADME.mddocker-compose.ymlsrc/app.jssrc/utils/logger.js
| LOG_DIR: /logs | ||
| LOG_ROTATION_DAYS: 30 | ||
| LOG_RETENTION_DAYS: 90 | ||
| ACCESS_LOG_ENABLED: true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Docker Compose v2 environment variables unquoted boolean warning
💡 Result:
In Docker Compose (both v1 and v2), when defining environment variables in the environment section of a compose file, unquoted values like true, false, yes, no, TRUE, etc., are parsed by YAML as booleans rather than strings. Environment variables must be strings, so Docker Compose issues a warning: "Warning: There is a boolean value, [value] in the 'environment' key. Environment variables can only be strings. Please add quotes to any boolean values to make them string (eg, 'True'). This warning will become an error in a future release." To fix this, quote the values to force them to be treated as strings, e.g.: environment: DEBUG: "true" ENABLED: 'false' This behavior has existed since at least Docker Compose 1.5.0 (2015) and persists in v2, as YAML parsing occurs before Compose validation. No specific change or new warning was introduced in v2; it's the standard YAML boolean handling. Official docs on environment variables (https://docs.docker.com/compose/how-tos/environment-variables/) do not explicitly mention this, but GitHub issues and PRs confirm quoting is required for string preservation. Example from compose file: services: app: environment: FEATURE_FLAG: "true" # Correct, passes "true" as string # FEATURE_FLAG: true # Wrong, YAML parses as boolean, triggers warning
Citations:
- 1: Warning: There is a boolean value, True in the 'environment' key. docker/compose#2146
- 2: docker-compose does not pass environment variables true/false unchanged docker/compose#1788
- 3: Disallow booleans in environment docker/compose#2000
- 4: https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/
- 5: https://docs.docker.com/compose/how-tos/environment-variables/
- 6: https://docs.docker.com/compose/environment-variables/env-file/
- 7: https://docs.docker.com/compose/env-file/
- 8: https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation
Your unquoted booleans will break in the next Docker Compose release. Quote them now.
Look, YAML treats true and false as actual booleans, not strings. Docker Compose has been warning about this since 2015 and has finally had enough—it'll become an actual error in a future release instead of just a warning. Yes, it works today, but that doesn't make it right.
Environment variables must be strings. Quote your boolean and numeric values. It's a one-second fix that saves you from a production headache later.
🔧 Fix
- LOG_ROTATION_DAYS: 30
- LOG_RETENTION_DAYS: 90
- ACCESS_LOG_ENABLED: true
+ LOG_ROTATION_DAYS: "30"
+ LOG_RETENTION_DAYS: "90"
+ ACCESS_LOG_ENABLED: "true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` at line 20, The environment variable ACCESS_LOG_ENABLED
is defined as an unquoted YAML boolean which will become invalid in future
Docker Compose releases; update the docker-compose.yml environment entry for
ACCESS_LOG_ENABLED to use a quoted string value (e.g., "true" or "false") so the
variable is passed as a string, ensuring compatibility with Compose and
environment parsers.
| // Structured request/response access log for operational visibility. | ||
| app.use((req, res, next) => { | ||
| const startedAt = Date.now(); | ||
|
|
||
| res.on('finish', () => { | ||
| const durationMs = Date.now() - startedAt; | ||
| const contentLength = res.getHeader('content-length') || 0; | ||
| const ip = req.ip || req.socket?.remoteAddress || 'unknown'; | ||
| const userAgent = req.get('user-agent') || '-'; | ||
|
|
||
| logger.access( | ||
| `${req.method} ${req.originalUrl || req.url} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${ip} ua="${userAgent}"`, | ||
| ); | ||
| }); | ||
|
|
||
| next(); | ||
| }); |
There was a problem hiding this comment.
Log injection, served hot and unsanitized.
You're taking a raw User-Agent string straight from the client and dropping it into your log line wrapped in quotes — with zero escaping. A mildly curious user can send a header like:
User-Agent: Mozilla/5.0" status=200 bytes=0 durationMs=0 ip=127.0.0.1 ua="legitimate
[ERROR] 2026-04-25T00:00:00Z - Admin password changed to hunter2
…and congratulations, your access-*.log (and app-*.log, since getCategoryPrefixes fans ACCESS into container+access only — but the shape is still corrupted) now contains forged entries that will fool any downstream log parser, SIEM, or poor human grepping the file. Same concern applies to req.originalUrl (attackers can put newlines in the raw URL via %0A which some proxies decode).
Strip or escape control characters (at minimum \r, \n, ") before writing. And since this loops through every single response, please don't allocate a fresh regex per call.
🔒 Proposed fix
+const CONTROL_CHARS = /[\r\n\t\x00-\x1f\x7f"]/g;
+const sanitizeLogField = (value) => String(value).replace(CONTROL_CHARS, '?');
+
// Structured request/response access log for operational visibility.
app.use((req, res, next) => {
const startedAt = Date.now();
res.on('finish', () => {
const durationMs = Date.now() - startedAt;
const contentLength = res.getHeader('content-length') || 0;
const ip = req.ip || req.socket?.remoteAddress || 'unknown';
const userAgent = req.get('user-agent') || '-';
logger.access(
- `${req.method} ${req.originalUrl || req.url} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${ip} ua="${userAgent}"`,
+ `${req.method} ${sanitizeLogField(req.originalUrl || req.url)} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${sanitizeLogField(ip)} ua="${sanitizeLogField(userAgent)}"`,
);
});
next();
});📝 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.
| // Structured request/response access log for operational visibility. | |
| app.use((req, res, next) => { | |
| const startedAt = Date.now(); | |
| res.on('finish', () => { | |
| const durationMs = Date.now() - startedAt; | |
| const contentLength = res.getHeader('content-length') || 0; | |
| const ip = req.ip || req.socket?.remoteAddress || 'unknown'; | |
| const userAgent = req.get('user-agent') || '-'; | |
| logger.access( | |
| `${req.method} ${req.originalUrl || req.url} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${ip} ua="${userAgent}"`, | |
| ); | |
| }); | |
| next(); | |
| }); | |
| const CONTROL_CHARS = /[\r\n\t\x00-\x1f\x7f"]/g; | |
| const sanitizeLogField = (value) => String(value).replace(CONTROL_CHARS, '?'); | |
| // Structured request/response access log for operational visibility. | |
| app.use((req, res, next) => { | |
| const startedAt = Date.now(); | |
| res.on('finish', () => { | |
| const durationMs = Date.now() - startedAt; | |
| const contentLength = res.getHeader('content-length') || 0; | |
| const ip = req.ip || req.socket?.remoteAddress || 'unknown'; | |
| const userAgent = req.get('user-agent') || '-'; | |
| logger.access( | |
| `${req.method} ${sanitizeLogField(req.originalUrl || req.url)} status=${res.statusCode} bytes=${contentLength} durationMs=${durationMs} ip=${sanitizeLogField(ip)} ua="${sanitizeLogField(userAgent)}"`, | |
| ); | |
| }); | |
| next(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.js` around lines 52 - 68, The access-log middleware is writing
unsanitized fields (req.get('user-agent') and req.originalUrl/req.url) directly
into logger.access, allowing log injection; fix this by adding a single reusable
sanitizer (e.g., sanitizeLogField) and a module-level precompiled regex (e.g.,
SANITIZE_RE = /[\r\n"]/g) that replaces control chars with safe placeholders or
escapes (strip or replace \r, \n, and " ), then call sanitizeLogField on
req.get('user-agent') and on req.originalUrl || req.url before interpolating
into the logger.access message; keep other logic the same and avoid allocating
the regex inside the middleware so it’s reused per request.
| function initializeFileLogging() { | ||
| if (!LOG_TO_FILE) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| fs.mkdirSync(LOG_DIR, { recursive: true }); | ||
| pruneOldLogs(); | ||
| fileLoggingReady = true; | ||
| } catch (err) { | ||
| // Do not crash app startup if file logging cannot be initialized. | ||
| console.error(`[ERROR] ${new Date().toISOString()} - Failed to initialize log directory (${LOG_DIR}): ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
pruneOldLogs runs exactly once, at boot. That is not retention.
You call pruneOldLogs() inside initializeFileLogging(), which runs once at module load (line 183). A container that stays up for six months will happily accumulate six months of log files regardless of what LOG_RETENTION_DAYS=90 says, because nothing ever prunes again after startup.
Schedule it: setInterval(pruneOldLogs, DAY_MS).unref() (the .unref() is important so it doesn't keep the process alive during shutdown). Also wrap the interval callback in try/catch so a single bad readdirSync doesn't take the process down.
🔧 Proposed fix
function initializeFileLogging() {
if (!LOG_TO_FILE) {
return;
}
try {
fs.mkdirSync(LOG_DIR, { recursive: true });
pruneOldLogs();
fileLoggingReady = true;
+ setInterval(() => {
+ try { pruneOldLogs(); } catch (err) {
+ console.error(`[ERROR] ${new Date().toISOString()} - log prune failed: ${err.message}`);
+ }
+ }, DAY_MS).unref();
} catch (err) {
// Do not crash app startup if file logging cannot be initialized.
console.error(`[ERROR] ${new Date().toISOString()} - Failed to initialize log directory (${LOG_DIR}): ${err.message}`);
}
}Also applies to: 104-134, 183-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.js` around lines 31 - 44, The current pruneOldLogs() is only
called once in initializeFileLogging(), so logs never get pruned after startup;
modify initializeFileLogging() to schedule recurring pruning by creating a timer
like setInterval(() => { try { pruneOldLogs(); } catch (e) { console.error(...)
} }, DAY_MS).unref() immediately after the initial pruneOldLogs() call (only
when LOG_TO_FILE is true and after fileLoggingReady is set), and ensure the
interval callback is wrapped in try/catch so filesystem errors don't crash the
process; reference pruneOldLogs, initializeFileLogging, and DAY_MS when making
this change.
| function getRotationWindowStart(date = new Date()) { | ||
| const windowSpanMs = LOG_ROTATION_DAYS * DAY_MS; | ||
| const windowStartMs = Math.floor(date.getTime() / windowSpanMs) * windowSpanMs; | ||
| return new Date(windowStartMs); | ||
| } |
There was a problem hiding this comment.
Rotation windows are aligned to the Unix epoch. Your "30-day" files will not start on the 1st of the month.
Math.floor(date.getTime() / windowSpanMs) * windowSpanMs anchors every window boundary to midnight UTC on Jan 1 1970 (a Thursday, in case you're curious). With LOG_ROTATION_DAYS=30, the actual rotation day is whatever epoch + N*30days happens to land on — operators expecting monthly rotation on the 1st of each calendar month are going to be very confused when the file stamp says 20260107 or whatever.
Either:
- Document that rotation is "rolling windows of N days aligned to 1970-01-01" (accurate but unhelpful), or
- Anchor to calendar day/week/month (e.g., floor the UTC date to midnight, then compute which N-day bucket since a fixed reference within the current year).
Not a bug, just a UX landmine in the docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.js` around lines 46 - 50, The rotation window logic in
getRotationWindowStart anchors windows to the Unix epoch which yields odd
calendar dates; change it to align to calendar boundaries by first computing the
UTC midnight for the given date (use date.getUTCFullYear(), getUTCMonth(),
getUTCDate() to build a UTC-midnight Date) and then compute the bucket start
relative to a calendar anchor (for example, the UTC-midnight of Jan 1 of the
same year or another explicit calendar-based reference) using LOG_ROTATION_DAYS
and DAY_MS to floor into N-day buckets; update getRotationWindowStart to use
that calendar-aligned anchor instead of date.getTime() / windowSpanMs so
rotation files start on predictable calendar days.
| function getCategoryPrefixes(level) { | ||
| const categories = ['container']; | ||
|
|
||
| if (level === 'ERROR') { | ||
| categories.push('app', 'error'); | ||
| return categories; | ||
| } | ||
|
|
||
| if (level === 'DEBUG') { | ||
| categories.push('debug'); | ||
| return categories; | ||
| } | ||
|
|
||
| if (level === 'ACCESS') { | ||
| categories.push('access'); | ||
| return categories; | ||
| } | ||
|
|
||
| categories.push('app'); | ||
| return categories; | ||
| } |
There was a problem hiding this comment.
Every access log gets copy-pasted into the container log. Why.
getCategoryPrefixes sends ACCESS to both container and access. Same for ERROR (container + app + error) and DEBUG (container + debug). So container-*.log is basically "everything, twice". With file-backed access logs on a busy server that doubles your disk usage for no operational benefit, since operators who want "everything" can just tail all the split files anyway.
If the intent of container-*.log is "catch-all mirror of console output", fine — document it. But then note in the README that enabling split logs roughly doubles on-disk size. Otherwise, consider dropping container from the fan-out list and letting each category own its file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.js` around lines 82 - 102, getCategoryPrefixes currently
always adds 'container' causing every log (ACCESS, ERROR, DEBUG, etc.) to be
duplicated into container-*.log; remove 'container' from the default fan-out in
getCategoryPrefixes so each level maps only to its own category(s) (e.g., ACCESS
-> ['access'], ERROR -> ['app','error'], DEBUG -> ['debug']), and if a catch-all
mirror is required introduce an explicit config flag (e.g.,
ENABLE_CONTAINER_MIRROR) checked in getCategoryPrefixes before adding
'container' so duplication is opt-in rather than automatic.
| function writeToFile(level, line) { | ||
| if (!fileLoggingReady) { | ||
| return; | ||
| } | ||
|
|
||
| const categories = getCategoryPrefixes(level); | ||
| for (const category of categories) { | ||
| const filePath = getLogFilePath(category); | ||
| try { | ||
| fs.appendFileSync(filePath, `${line}\n`, 'utf8'); | ||
| } catch (err) { | ||
| // Fall back silently to console logging if file writing fails. | ||
| fileLoggingReady = false; | ||
| console.error(`[ERROR] ${new Date().toISOString()} - Failed to write log file (${filePath}): ${err.message}`); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
One hiccup and file logging stays dead until restart. Extremely dramatic.
On the first appendFileSync failure (disk full for a second, transient EIO, someone chmods the mount), you set fileLoggingReady = false forever. There's no retry, no re-initialization, no re-check on the next write. A momentary issue permanently silences your file logs for the rest of the container's lifetime — which may be weeks.
At minimum, reset fileLoggingReady periodically (e.g., every N minutes, re-run initializeFileLogging), or track consecutive failures with backoff instead of a one-strike-you're-out flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.js` around lines 136 - 153, The writeToFile function sets
fileLoggingReady = false permanently on the first appendFileSync error, which
kills file logging until restart; change this to implement retry/backoff and
periodic probes instead of a one-shot disable: replace the immediate permanent
toggle with logic that counts consecutive failures (e.g.,
consecutiveFileWriteFailures), marks fileLoggingReady false only after a
threshold, and schedule reinitialization attempts by calling
initializeFileLogging (or re-running the file setup using getLogFilePath) with
exponential backoff (via setTimeout) to reset fileLoggingReady on success;
ensure the appendFileSync error handler logs the error but does not permanently
disable logging on a single transient error.
🧩 Analysis chain
🌐 Web query:
Node.js fs.appendFileSync performance event loop blocking
💡 Result:
fs.appendFileSync blocks the Node.js event loop (and therefore blocks processing of other JavaScript callbacks) for the duration of the synchronous filesystem operation. Use the async form (fs.appendFile or fs.promises.appendFile) in request paths to avoid degraded throughput / event-loop blocking. If your concern is “performance” vs “data durability/flush”: - For performance/event-loop health: avoid sync APIs (appendFileSync) in hot paths; the Node.js docs explicitly say synchronous file system APIs block the Node.js event loop and further JavaScript execution until completion. - For async append durability: Node’s fs.appendFile supports an options.flush flag; when flush is true, the underlying file descriptor is flushed before closing (default false). This does not remove the cost of flushing, but it keeps your JS thread from being synchronously blocked. Practical guidance: - In servers: prefer fs.appendFile(...) / await fs.promises.appendFile(...), not fs.appendFileSync. - If you need to reduce “lost last writes on crash”: use flush:true with appendFile, or write via an open file descriptor and explicitly fdatasync/fync-like behavior (but that adds complexity and overhead). Example (async + flush): const fs = require('node:fs/promises'); await fs.appendFile('/tmp/test.d', 'message\n', { flush: true });
Citations:
- 1: https://nodejs.org/learn/asynchronous-work/dont-block-the-event-loop
- 2: https://nodejs.org/api/fs.html
- 3: https://stackoverflow.com/questions/52098850/does-fs-readfilesync-block-the-thread-node-is-running
- 4: https://nodejs.org/docs/latest/api/fs.html
- 5: https://medium.com/@bhagyarana80/node-file-i-o-10-sync-calls-hiding-in-async-code-38786965095a
- 6: https://stackoverflow.com/questions/42277513/how-to-make-fs-appendfile-method-of-node-js-flush-to-disk-right-away/42279022
🏁 Script executed:
sed -n '82,102p' src/utils/logger.jsRepository: DumbWareio/DumbDrop
Length of output: 451
You're blocking the entire server on every single HTTP request. Here's why that's bad.
fs.appendFileSync is synchronous, which means every time you call logger.access, it freezes the Node.js event loop waiting for a disk write to complete. On every. Single. Request. The web search results are crystal clear: the Node.js docs explicitly say synchronous file APIs block the event loop and halt all JavaScript execution until they finish.
And because getCategoryPrefixes('ACCESS') returns ['container', 'access'], each access log triggers two blocking disk writes instead of one. Spinning disks, network mounts, or any I/O contention will directly tank your throughput and add latency to every user.
Node.js has had async file APIs for years. You have two choices:
- Use cached write streams: Create an append stream per log category (using
fs.createWriteStreamwithflags: 'a'), cache them in a Map, and call.write()non-blocking. - Use a real logging library:
pinoorwinstonsolved this problem years ago—use them instead.
🔧 Quick fix sketch with cached streams
-function writeToFile(level, line) {
+const writeStreams = new Map();
+function getWriteStream(category) {
+ const filePath = getLogFilePath(category);
+ const existing = writeStreams.get(category);
+ if (existing && existing.path === filePath) return existing.stream;
+ existing?.stream.end();
+ const stream = fs.createWriteStream(filePath, { flags: 'a' });
+ stream.on('error', (err) => {
+ fileLoggingReady = false;
+ console.error(`[ERROR] ${new Date().toISOString()} - log stream error (${filePath}): ${err.message}`);
+ });
+ writeStreams.set(category, { path: filePath, stream });
+ return stream;
+}
+
+function writeToFile(level, line) {
if (!fileLoggingReady) {
return;
}
-
- const categories = getCategoryPrefixes(level);
- for (const category of categories) {
- const filePath = getLogFilePath(category);
- try {
- fs.appendFileSync(filePath, `${line}\n`, 'utf8');
- } catch (err) {
- // Fall back silently to console logging if file writing fails.
- fileLoggingReady = false;
- console.error(`[ERROR] ${new Date().toISOString()} - Failed to write log file (${filePath}): ${err.message}`);
- break;
- }
- }
+ for (const category of getCategoryPrefixes(level)) {
+ getWriteStream(category).write(`${line}\n`);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.js` around lines 136 - 153, The writeToFile function
currently blocks the event loop by calling fs.appendFileSync for each category;
replace the synchronous writes with cached append streams: create a Map to cache
fs.createWriteStream(filePath, { flags: 'a', encoding: 'utf8' }) per category
(use getCategoryPrefixes(level) to derive keys), call stream.write(`${line}\n`)
instead of appendFileSync, and on stream 'error' set fileLoggingReady = false
and console.error the error (preserving the existing error message text),
ensuring streams are created lazily and reused to avoid per-request blocking.
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
LICENSE (1)
154-612:⚠️ Potential issue | 🔴 CriticalDon't rewrite GPLv3. Seriously.
Every clause heading from
2.through17.got squashed to1.(lines 154, 179, 195, 208, 245, 343, 407, 435, 446, 471, 540, 552, 563, 589, 600, 612). The actual license body still talks about "section 4", "section 7", "section 10", "sections 15 and 16", etc. — those references now all point at headings that read "1.", which is gibberish to any compliance reviewer and is a modification of the GNU GPLv3 verbatim text, which the license itself forbids changing.This almost certainly came from a Markdown renderer auto-numbering an ordered list. The fix is to paste the canonical GPLv3 text from https://www.gnu.org/licenses/gpl-3.0.txt back in and not touch it again.
🔧 Quick way to restore the file
curl -fsSL https://www.gnu.org/licenses/gpl-3.0.txt -o LICENSE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LICENSE` around lines 154 - 612, The LICENSE file has been mangled: every section heading (originally 2.–17.) was converted to "1." (likely by Markdown auto-numbering), which modifies the GPLv3 verbatim text; restore the canonical GPLv3 text exactly as published so headings and section numbers match (do not reformat or renumber), e.g. replace the current LICENSE contents with the authoritative text from https://www.gnu.org/licenses/gpl-3.0.txt (or run the provided curl command) and commit that exact file so references like "section 4", "section 7", "sections 15 and 16", etc., point to the correct numbered headings.test/security.test.js (1)
163-168:⚠️ Potential issue | 🟡 MinorThe test description says "preserve safe filenames" but it now asserts the opposite.
Input:
my-file_123.txt→ expected:my_file_123.txt. That is the literal opposite of "preserving" — the hyphen got rewritten. The assertion matches the implementation insrc/utils/fileUtils.js(the[+\-\s]+/greplacement at line 179), but that means the test description is now misleading and you've also baked into your test suite the assumption that hyphens — a character that is legal in every modern filesystem and is heavily used in URL slugs — will be mangled.Two paths:
- Rename the test (e.g., "should normalize hyphens to underscores") so future readers aren't misled, OR
- Reconsider whether
sanitizeFilenameSafeshould be transforming hyphens at all. They're not dangerous and stripping them silently rewrites user filenames and any external links built from them.At minimum, fix the test name — the current one actively misrepresents the behavior under test.
♻️ Minimum fix (rename only)
- it('should preserve safe filenames', () => { + it('should normalize hyphens and spaces to underscores in filenames', () => { const safe = 'my-file_123.txt'; const sanitized = sanitizeFilenameSafe(safe); assert.strictEqual(sanitized, 'my_file_123.txt'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/security.test.js` around lines 163 - 168, The test description is misleading: update the it(...) description in test/security.test.js to reflect the actual behavior of sanitizeFilenameSafe (which replaces hyphens with underscores) — e.g., change "should preserve safe filenames" to "should normalize hyphens to underscores" — so the test name matches the implementation of sanitizeFilenameSafe; alternatively, if you prefer to keep hyphens, adjust sanitizeFilenameSafe's regex (the [+\-\s]+/g replacement) to not replace '-' and then update tests accordingly.test/files.test.js (1)
267-274:⚠️ Potential issue | 🟡 MinorFix the sanitizer to stop mangling hyphens in filenames.
Look at line 179 of
src/utils/fileUtils.js:.replace(/[+\-\s]+/g, '_')replaces hyphens with underscores. That's the bug. Your test at lines 267-274 sendsnewName: 'renamed-file.txt'but then expectsrenamed_file.txt—you're just baking in the broken behavior instead of testing the right thing.Hyphens are legal filename characters on every OS this app supports. Remove the hyphen from that regex (line 179) and update the test to expect
renamed-file.txt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/files.test.js` around lines 267 - 274, The filename sanitizer is incorrectly replacing hyphens; update the regex in the sanitizer (e.g., the sanitizeFileName function in src/utils/fileUtils.js that currently uses .replace(/[+\-\s]+/g, '_')) to stop treating '-' as a character to replace (remove '-' from that character class so hyphens are preserved) and then update the test expectation in test/files.test.js (the assertion around newName 'renamed-file.txt' / newPath) to expect 'renamed-file.txt' instead of 'renamed_file.txt'.public/index.html (1)
316-339:⚠️ Potential issue | 🟡 MinorTiny but real race window in your abort plumbing.
At line 317 you create a fresh per-chunk
controller. At line 320 you forwardthis.abortController.signal's abort into it viaaddEventListener('abort', onAbort, { once: true }). ButaddEventListeneron an already-abortedAbortSignaldoes not fire the listener — the event has already happened. So if the user hits the cancel button in the tiny window between line 307 (if (this.cancelRequested) throw) and line 320 (addEventListener), then:
this.abortController.signal.aborted === true- the new
controlleris not aborted- the
fetchat line 324 proceeds normally and runs for up to 30 seconds before the timeout tripsThe
cancelRequestedflag will kick in on the next loop iteration, so it's not fatal — the chunk just wastes bandwidth and time instead of cancelling instantly like the UI promises. Tighten it by checkingsignal.abortedright after wiring up the listener:⚡ Close the window
const controller = new AbortController(); const onAbort = () => controller.abort(); if (this.abortController) { - this.abortController.signal.addEventListener('abort', onAbort, { once: true }); + if (this.abortController.signal.aborted) { + controller.abort(); + } else { + this.abortController.signal.addEventListener('abort', onAbort, { once: true }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 316 - 339, There’s a race where a newly-created per-chunk AbortController (controller) can miss an already-fired this.abortController.signal abort event because addEventListener won’t run for past events; after you attach the listener (this.abortController.signal.addEventListener('abort', onAbort, { once: true })), immediately check if this.abortController.signal.aborted and if so call controller.abort() (and skip the fetch), and ensure you still clear the timeout and remove the listener only when appropriate; apply this change around the controller/onAbort wiring before calling fetch so the fetch is aborted instantly when cancelRequested/this.abortController has already been triggered.src/routes/upload.js (1)
390-409:⚠️ Potential issue | 🟡 Minor
uploadedAtis lying on one of these paths — pick a story.On the zero-byte path (line 395) you call
getUploadedFilePayload(req, finalFilePath)with no third argument, souploadedAtdefaults toDate.now(). On the chunk-completion path (line 541) you passmetadata.createdAt, which is the moment the upload was initialized, possibly hours earlier for a large slow upload. So two identical-lookingfileobjects in the API mean wildly different things depending on which code path built them, andexpiresAtis inconsistent with it.Pick one — either "when the upload started" or "when it finished" — and use it on both paths. "When it finished" (
Date.now()) is generally the more useful one for a retention/expiry field.♻️ Suggested alignment
- completedFile = getUploadedFilePayload(req, metadata.filePath, metadata.createdAt); + completedFile = getUploadedFilePayload(req, metadata.filePath);(and leave line 395 as-is)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/upload.js` around lines 390 - 409, The uploadedAt/expiresAt timestamps are inconsistent: getUploadedFilePayload(req, finalFilePath) on the zero-byte path uses the default Date.now(), while the chunk-completion path passes metadata.createdAt. Make them consistent by choosing "when finished" and ensure both paths call getUploadedFilePayload with the same uploadedAt value (e.g., pass Date.now() explicitly instead of metadata.createdAt). Update the chunk-completion code that currently uses metadata.createdAt to pass Date.now() so uploadedAt and derived expiresAt match the actual completion time across both paths (references: getUploadedFilePayload and metadata.createdAt).
🟡 Minor comments (14)
src/config/index.js-35-35 (1)
35-35:⚠️ Potential issue | 🟡 MinorRe-branding the upstream default to "WickedYoda's DumbDrop" is out of scope.
This PR is about logging. Changing the default site title on upstream
DumbWareio/DumbDropfromDumbDropto"WickedYoda's DumbDrop"(lines 35 and 279) is unrelated branding for a fork's preferences and will rebrand the upstream project for every user who hasn't setDUMBDROP_TITLE. Revert to'DumbDrop'and keep the rename in your fork's local config.Also applies to: 279-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.js` at line 35, Revert the upstream default site title change: restore the DEFAULT_SITE_TITLE constant value back to 'DumbDrop' (undo the change from "WickedYoda's DumbDrop") and remove or do not modify any other occurrences that set a default title (also revert the similar change referenced near the second occurrence, e.g., the fallback used when process.env.DUMBDROP_TITLE is not set). Keep any custom branding only in your fork's local configuration rather than changing DEFAULT_SITE_TITLE or the fallback logic used by the codebase.LICENSE-660-660 (1)
660-660:⚠️ Potential issue | 🟡 MinorMissing space —
andis glued to the backtick.Line 660 reads
andshow c'instead of `` andshow c' ``. Same root cause as the heading renumbering — please restore from the canonical source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LICENSE` at line 660, The sentence contains a missing space around the inline code: replace the malformed fragment "and`show c'" with a properly spaced inline code sequence "and `show c'` (or restore the canonical source wording) so that backticks are separated from surrounding words; locate the occurrence of "and`show c'" in the LICENSE content and insert the missing space (or copy the correct phrase from the canonical source) to produce "and `show c'` exactly as intended..env.example-80-80 (1)
80-80:⚠️ Potential issue | 🟡 MinorQuote the value — apostrophes break naive parsers.
docker --env-fileand POSIXset -a; source .envboth break on unquoted apostrophes. dotenv-linter flagged the same thing. Wrap it in quotes:🛡️ Proposed fix
-DUMBDROP_TITLE=WickedYoda's DumbDrop +DUMBDROP_TITLE="WickedYoda's DumbDrop"(Separately, the question of whether the upstream default should be rebranded at all is raised on
src/config/index.js.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 80, The DUMBDROP_TITLE value in .env.example contains an unquoted apostrophe which breaks naive env parsers; update the DUMBDROP_TITLE entry in .env.example to wrap the value in quotes (e.g., "WickedYoda's DumbDrop") so docker --env-file and POSIX sourcing work correctly, and then verify any code that reads this variable (e.g., the default in src/config/index.js or places that reference DUMBDROP_TITLE) handles quoted values unchanged.src/routes/files.js-75-75 (1)
75-75:⚠️ Potential issue | 🟡 Minor
expiresAtlies if retention cleanup is disabled.You unconditionally compute
expiresAt = mtime + config.fileRetentionMsfor every file/directory, but cleanup is only honored if the retention scheduler is actually running (andDISABLE_BATCH_CLEANUPexists, used all over the test suite). Returning an "expires at 2026‑05‑25" value when in reality the file lives forever is a UX trap — clients will display it, users will believe it.Either (a) only emit
expiresAtwhen retention cleanup is genuinely active, or (b) document loudly that this field reflects configured policy, not a guarantee. (a) is what users actually want.Also applies to: 186-186, 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/files.js` at line 75, The code unconditionally sets expiresAt = new Date(stats.mtime.getTime() + config.fileRetentionMs) which misleads users when the cleanup scheduler is disabled; change the logic in the places that set expiresAt (the expiresAt assignment around the expiresAt symbol in src/routes/files.js and the other two occurrences) to only add the expiresAt property when batch cleanup is actually enabled (i.e., when the DISABLE_BATCH_CLEANUP flag is not set) and when config.fileRetentionMs is present — otherwise omit the expiresAt field entirely; apply the same conditional pattern to all three spots so clients only see expiresAt when retention cleanup is active.test/cleanup.test.js-16-26 (1)
16-26:⚠️ Potential issue | 🟡 MinorTests share
config.uploadDirwithtest/files.test.js— cross-test interference is a footgun.Both this suite and
test/files.test.jswrite directly into the realconfig.uploadDir, andfiles.test.js'safterhook iterates the directory and deletes every regular file it finds. If the runner ever executes these in parallel (or test ordering changes),fresh-test.txthere can be wiped out by the other test file's cleanup, producing flaky failures with no obvious cause.Use a per-test temporary upload dir (e.g. via
process.env.LOCAL_UPLOAD_DIR=os.tmpdir()/dumbdrop-cleanup-…set before requiring../src/config), or at minimum stop letting unrelated suites garbage-collect each other's files. The 5-minute fix today saves you a 4-hour debugging session in three months.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cleanup.test.js` around lines 16 - 26, The tests write to the shared config.uploadDir causing cross-test interference; change this cleanup.test.js to use a per-test temporary upload dir by setting process.env.LOCAL_UPLOAD_DIR to a unique tmp path (e.g. path.join(os.tmpdir(), `dumbdrop-cleanup-${Date.now()}-${Math.random()}`)) before requiring ../src/config, then recreate config by requiring it after that change, and update oldFile/freshFile to use the new config.uploadDir; also ensure the before hook creates that temp dir and the after hook removes only the files created by this test (oldFile and freshFile) rather than relying on sweeping the whole directory so files from test/files.test.js are not removed.public/login.html-68-71 (1)
68-71:⚠️ Potential issue | 🟡 MinorCosmetic flicker: the anchor renders with
href="{{TERMS_LINK}}"until the script runs.When
TERMS_LINKis unset, the markup contains a literalhref="{{TERMS_LINK}}"and the surrounding text "By using this site, you agree to the Terms and Conditions." until the inline script hides the<p>. Users on slow devices will briefly see, and possibly click, a broken link. If you adopt the suggestion in the comment above (data attribute / inline style hidden by default, unhidden when valid), this also goes away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/login.html` around lines 68 - 71, The anchor renders a literal href="{{TERMS_LINK}}" until the script runs causing a cosmetic flicker; update the markup and script so the <p id="termsNotice"> (and/or <a id="termsLink">) is hidden by default (e.g., add a hidden class or inline style) and only unhide and set the anchor's href/text when the TERMS_LINK value is validated, or store the link in a data- attribute instead of the href initially; specifically, locate elements with id termsNotice and termsLink, prevent rendering a raw href="{{TERMS_LINK}}" in the DOM, and have your client-side code populate termsLink.href and remove the hidden state only when TERMS_LINK is present and valid.src/app.js-39-53 (1)
39-53:⚠️ Potential issue | 🟡 MinorReserved list is hard-coded and out of sync with what you actually serve.
This list is supposed to enumerate "URLs that aren't downloads." A few you're missing:
'login'(the route at line 200 only matches/login.html, not/login, but users type/login).'logout'.'.well-known'(anything using ACME/Let's Encrypt HTTP-01 challenges via this app will break).- Anything else you add to
app.get('/...', …)later — and there's no link between that file and this set, so it's a pure landmine for the next contributor.Either (a) drive this list off the actual registered routes (introspect
app._router) at startup, or (b) flip the defaults: only match paths that match an existing file in the upload dir and otherwise callnext()immediately. (b) is what you arguably want anyway, and it removes the maintenance burden entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.js` around lines 39 - 53, The hard-coded RESERVED_SHORT_LINK_PATHS and RESERVED_SHORT_LINK_PREFIXES are out of sync with actual routes; update the logic so short-link detection no longer relies on that static set: either at startup derive reserved paths from the Express router (inspect app._router.stack / route paths and include route names like 'login' and 'logout' and prefixes such as '.well-known') or—preferred—change the short-link matcher to only treat a path as a download when a corresponding file exists in the upload directory (check the filesystem for the requested path in the upload dir and otherwise call next()); update any code that references RESERVED_SHORT_LINK_PATHS and RESERVED_SHORT_LINK_PREFIXES to use the new router-derived set or the file-existence check (look for usages in functions handling short-link resolution).test/files.test.js-191-203 (1)
191-203:⚠️ Potential issue | 🟡 MinorThis test confirms a public, unauthenticated download path. See the bigger comment in
src/app.js.Worth noting that this test exists because the short-link route deliberately serves files without going through PIN auth. That's a behavior change worth its own scrutiny, not just a green checkmark in the test suite — see the dedicated comment on the
/*handler insrc/app.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/files.test.js` around lines 191 - 203, The test "should download existing file from short link path" currently validates a download but doesn't document or assert the deliberate unauthenticated behavior; update this test (in the describe block that uses makeRequest and path '/test-file.txt') to (1) add an inline comment referencing the short-link behavior in src/app.js and its /* handler, and (2) explicitly assert that the route bypasses PIN/auth (e.g., ensure no Authorization/WWW-Authenticate/401 is returned or that no auth header was sent and status is 200) so the test both documents and enforces the intended public short-link behavior.test/files.test.js-107-113 (1)
107-113:⚠️ Potential issue | 🟡 MinorConditional assertion = silent pass when there's nothing to test.
if (listedFile) { … }means the moment the listing returns no files (concurrent test wiping the dir, ordering change, etc.), this entiredownloadUrlcheck is skipped and the test still passes. That's not a test, that's a placebo.Assert the file exists, then assert its shape:
🧪 Proposed fix
- const listedFile = response.data.items.find(item => item.type === 'file'); - if (listedFile) { - assert.ok(typeof listedFile.downloadUrl === 'string'); - assert.ok(listedFile.downloadUrl.endsWith('/test-file.txt')); - } + const listedFile = response.data.items.find(item => item.name === 'test-file.txt'); + assert.ok(listedFile, 'expected test-file.txt to be present in listing'); + assert.strictEqual(typeof listedFile.downloadUrl, 'string'); + assert.ok(listedFile.downloadUrl.endsWith('/test-file.txt'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/files.test.js` around lines 107 - 113, The test currently conditionally skips assertions when no file is returned; change it to assert that the file exists first (e.g., assert.ok(listedFile, 'expected a file item in response.data.items')), then perform the existing shape checks on listedFile.downloadUrl (assert typeof string and assert endsWith '/test-file.txt'); locate the `listedFile` assignment and `response.data.items` usage in the test and replace the `if (listedFile) { ... }` guard with a direct existence assertion followed by the downloadUrl assertions.docker-compose.yml-26-26 (1)
26-26:⚠️ Potential issue | 🟡 Minor
PUBLIC_DOMAIN: http://localhost:3000will hand out broken links to everyone who isn't you.This value is what
buildDownloadUrlinsrc/routes/files.jsstamps onto everydownloadUrlreturned by/api/filesand/api/files/info/*. If a user opens this in another machine/network, every "download link" they get back resolves to their own localhost. As a default in a sample compose file that's fine; as documentation it's misleading. Add a comment that this must be replaced with the public hostname users will actually browse to (e.g.https://drop.example.com), otherwise short-link sharing — the entire selling point of this PR — silently produces unusable URLs.📝 Suggested clarification
- PUBLIC_DOMAIN: http://localhost:3000 # Public domain used in generated file links + # IMPORTANT: PUBLIC_DOMAIN is embedded into every generated downloadUrl. + # Set this to the externally-resolvable URL users will actually use, + # e.g. https://drop.example.com. Leaving it as localhost will produce + # download links that only work on the host running the container. + PUBLIC_DOMAIN: "http://localhost:3000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 26, The PUBLIC_DOMAIN env value in docker-compose.yml currently points to http://localhost:3000 which produces broken download links for remote users; update the docker-compose comment next to PUBLIC_DOMAIN to clearly state that this value must be replaced with the actual public hostname used by users (for example https://drop.example.com) because buildDownloadUrl in src/routes/files.js uses PUBLIC_DOMAIN to stamp every downloadUrl returned by /api/files and /api/files/info/*; make the comment explicit about the requirement and consequences (shareable short-links will be unusable if left as localhost).LOCAL_DEVELOPMENT.md-27-33 (1)
27-33:⚠️ Potential issue | 🟡 MinorFix the list indentation — markdownlint is yelling at you for a reason.
Line 27 indents by 4 spaces while siblings under the same numbered item use 3, tripping
MD005. Pick three OR four and stick with it; mixing them renders inconsistently in some markdown viewers.📐 Proposed fix
- Open `.env` in your editor and review the variables. - - At minimum, set: - - `PORT=3000` - - `LOCAL_UPLOAD_DIR=./local_uploads` - - `MAX_FILE_SIZE=1024` - - `DUMBDROP_PIN=` (optional, for PIN protection) - - `TERMS_LINK=` (optional, add your Terms URL) - - `APPRISE_URL=` (optional, for notifications) + - At minimum, set: + - `PORT=3000` + - `LOCAL_UPLOAD_DIR=./local_uploads` + - `MAX_FILE_SIZE=1024` + - `DUMBDROP_PIN=` (optional, for PIN protection) + - `TERMS_LINK=` (optional, add your Terms URL) + - `APPRISE_URL=` (optional, for notifications)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LOCAL_DEVELOPMENT.md` around lines 27 - 33, The indented list under the numbered item is inconsistent (one line uses 4 spaces while the others use 3) causing MD005; change the leading indentation of the block that lists PORT, LOCAL_UPLOAD_DIR, MAX_FILE_SIZE, DUMBDROP_PIN, TERMS_LINK, and APPRISE_URL so all items use the same number of spaces (pick 3 to match the siblings) and ensure each bullet line has exactly three leading spaces before the dash.src/app.js-175-180 (1)
175-180:⚠️ Potential issue | 🟡 MinorAdd a comment clarifying the
req.pathassumption, or refactor for resilience.The current check on
req.pathis correct at its present mount point: Express 4.18 strips the/api/filesprefix before the middleware sees the request, soGET /api/files/download/foo.txtarrives asreq.path = '/download/foo.txt'. However, the check is fragile—if this middleware moves higher in the stack (e.g., attached directly toapp), the prefix strip disappears,req.pathbecomes/api/files/download/foo.txt, and the bypass silently fails, leaving all endpoints unprotected.Either add a comment documenting this assumption, or match against
req.baseUrl + req.pathto survive future refactoring without breaking silently.Suggested defensive change
app.use('/api/files', (req, res, next) => { - if (req.path.startsWith('/download/')) { + // req.path is relative to the '/api/files' mount; this check assumes the + // middleware stays mounted here. If moved, req.path will include '/api/files/'. + if (req.path.startsWith('/download/')) { return next(); } return filesPinMiddleware(req, res, next); }, downloadLimiter, fileRoutes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.js` around lines 175 - 180, The current bypass check relies on req.path being '/download/...' which only holds because this router is mounted at '/api/files' (risking silent break if moved); update the condition to be resilient by matching against req.baseUrl + req.path (e.g., check (req.baseUrl + req.path).startsWith('/api/files/download/') or otherwise normalize/join baseUrl and path) so the bypass works regardless of mount point, and/or add a short inline comment next to the middleware registration explaining the current mount assumption; adjust the check around filesPinMiddleware/ downloadLimiter/fileRoutes accordingly.src/routes/upload.js-225-227 (1)
225-227:⚠️ Potential issue | 🟡 MinorThis interval starts automatically on
require, and your tests didn't disable it.
test/upload.test.jssetsDISABLE_BATCH_CLEANUP=truebut notDISABLE_FAILED_UPLOAD_CLEANUP, so every test run spins up a real 60-second timer that scans the metadata directory. Yes,.unref()keeps the process from hanging on exit, and yes, the timer probably won't fire within a typical test run — but "probably" is a fun word to put in a test suite. If some future soul adds a long-running test or the cleanup starts deleting things with realDate.now(), you'll have a flaky test that's impossible to reproduce locally.Either gate this behind the same env flag pattern as the tests already use, or set
DISABLE_FAILED_UPLOAD_CLEANUP=trueintest/upload.test.js.🧪 Tiny fix in the test file
// Disable batch cleanup for tests process.env.DISABLE_BATCH_CLEANUP = 'true'; +process.env.DISABLE_FAILED_UPLOAD_CLEANUP = 'true';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/upload.js` around lines 225 - 227, The module currently calls startFailedUploadCleanup() unconditionally on require, which spawns a real interval during tests; update the module to guard that call with the same env-flag logic used elsewhere (check process.env.DISABLE_BATCH_CLEANUP or add/consult process.env.DISABLE_FAILED_UPLOAD_CLEANUP) so the cleanup interval is only started when the flag is not set, or alternatively set DISABLE_FAILED_UPLOAD_CLEANUP=true in the test setup; locate the unconditional invocation of startFailedUploadCleanup and wrap it with a conditional that reads the appropriate environment variable to prevent the timer from starting during test runs.public/index.html-739-772 (1)
739-772:⚠️ Potential issue | 🟡 Minor
renderRecentUploadLinkssilently drops everything that doesn't havedownloadUrl.You build the array
uploadedFilesfrom every completed upload (line 1345–1347 instartUploads), then in the renderer at line 754 you skip every entry with!file.downloadUrl. If something ever shipsfilewithoutdownloadUrl(say, demo mode, or a misconfigured server, or an older server version) the user gets an empty<h3>Download Links</h3>block and zero explanation.Either skip rendering the whole block if none of the entries has a
downloadUrl, or render a fallback row so the user isn't left staring at a lonely header wondering why their files disappeared.🩹 Small guard
function renderRecentUploadLinks(uploadedFiles) { - if (!uploadedFiles || uploadedFiles.length === 0) { + const linkable = (uploadedFiles || []).filter(f => f && f.downloadUrl); + if (linkable.length === 0) { recentUploadLinks.style.display = 'none'; recentUploadLinks.innerHTML = ''; return; } recentUploadLinks.style.display = 'block'; recentUploadLinks.innerHTML = ''; const title = document.createElement('h3'); title.textContent = 'Download Links'; recentUploadLinks.appendChild(title); - uploadedFiles.forEach(file => { - if (!file.downloadUrl) return; + linkable.forEach(file => {Also: you directly set
name.textContent = file.filename || file.path;— good,textContentis safe — butlink.textContent = file.downloadUrl;prints the full URL, including any?query=<script>...nonsense. Still safe because it'stextContent, notinnerHTML. Just double-checking you didn't plan to refactor it toinnerHTMLlater.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 739 - 772, In renderRecentUploadLinks, avoid silently dropping entries without downloadUrl: first filter uploadedFiles to collect only items with file.downloadUrl and if that filtered list is empty then either hide recentUploadLinks (set display='none' and clear innerHTML) or render a fallback row (e.g., a div with a message like "No download available for recent uploads"); otherwise iterate the filtered list to create rows as you do now. Also keep using textContent for name.textContent and link.textContent (do not switch to innerHTML) to avoid rendering any query strings as HTML.
🧹 Nitpick comments (9)
src/utils/cleanup.js (2)
251-254: Four signal listeners where one would do.Each module that does
process.on('SIGTERM', …)adds another listener. The server file already registers its own SIGTERM/SIGINT handlers; this file now adds four more (two intervals × two signals). DefaultEventEmitter.defaultMaxListenersis 10 — you're not over yet, but you're walking towardMaxListenersExceededWarningand the handlers are trivially mergeable.♻️ Proposed diff
- process.on('SIGTERM', () => clearInterval(metadataCleanupTimer)); - process.on('SIGINT', () => clearInterval(metadataCleanupTimer)); - process.on('SIGTERM', () => clearInterval(fileRetentionCleanupTimer)); - process.on('SIGINT', () => clearInterval(fileRetentionCleanupTimer)); + const stopTimers = () => { + clearInterval(metadataCleanupTimer); + clearInterval(fileRetentionCleanupTimer); + }; + process.once('SIGTERM', stopTimers); + process.once('SIGINT', stopTimers);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cleanup.js` around lines 251 - 254, Multiple duplicate process.on('SIGTERM'/'SIGINT') listeners are registered (for metadataCleanupTimer and fileRetentionCleanupTimer); consolidate them into a single handler per signal that clears both timers to avoid accumulating listeners and potential MaxListenersExceededWarning. Locate the places registering signals for metadataCleanupTimer and fileRetentionCleanupTimer and replace the four registrations with two: one process.on('SIGTERM', ...) and one process.on('SIGINT', ...) whose callbacks call clearInterval(metadataCleanupTimer) and clearInterval(fileRetentionCleanupTimer) (or extract a small helper like registerSignalCleanup that clears both) so both timers are cleaned with a single listener per signal.
257-312: Retention sweep keys offmtime— document this and watch out for partial uploads.
stats.mtime.getTime() <= cutoffdeletes by last-modified time. For a partial upload that's been receiving chunks, every chunk resetsmtime, so an in-flight upload won't be deleted (good). For a finished upload,mtime≈ creation time (also good). But anything that touches the file (e.g. an external sync tool, a backup utility that doesutimes) silently extends retention. Worth a one-line comment so the next person reading this doesn't think it's based on creation time.Additionally, the
entry === '.metadata'skip on line 273 fires at every directory level, not just the top. A user-created directory literally named.metadatainside their uploads will also be excluded forever. Unlikely, but worth either documenting or anchoring the skip to only the top-level scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cleanup.js` around lines 257 - 312, The cleanupExpiredFiles routine currently deletes files based on stats.mtime and skips any entry named '.metadata' at every directory level; add a one-line comment by the mtime check in cleanupExpiredFiles/walkAndCleanup noting that retention uses last-modified time (mtime) — not creation time — and that external tools touching utimes can extend retention, and change the '.metadata' skip so it only applies at the top-level scan (e.g., only skip when dirPath equals config.uploadDir or its resolved equivalent) rather than for every nested directory; keep cleanupEmptyFolders usage unchanged..env.example (1)
121-123: Trailing newline + key ordering nits flagged by dotenv-linter.dotenv-linter wants a final blank line and
DISABLE_FAILED_UPLOAD_CLEANUPplaced beforeDISABLE_SECURITY_CLEANUP(alphabetical). Trivial cleanup.♻️ Proposed diff
# Internal/testing toggles for cleanup schedulers (leave false in normal use) DISABLE_BATCH_CLEANUP=false +DISABLE_FAILED_UPLOAD_CLEANUP=false DISABLE_SECURITY_CLEANUP=false -DISABLE_FAILED_UPLOAD_CLEANUP=false +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 121 - 123, The .env.example file has two lint issues: missing a final newline and keys out of alphabetical order; reorder the three variables so DISABLE_FAILED_UPLOAD_CLEANUP appears before DISABLE_SECURITY_CLEANUP (alphabetical among DISABLE_BATCH_CLEANUP, DISABLE_FAILED_UPLOAD_CLEANUP, DISABLE_SECURITY_CLEANUP) and ensure the file ends with a trailing blank line; edit the block containing DISABLE_BATCH_CLEANUP, DISABLE_SECURITY_CLEANUP, and DISABLE_FAILED_UPLOAD_CLEANUP to reflect the new order and add the final newline.src/config/index.js (2)
95-109: DeadisNaNcheck.Line 103's
isNaN(amount)is unreachable — if the regex^(\d+)([dh])$matches,parsed[1]is purely digits andparseIntcannot returnNaN. Theamount <= 0check is the only useful part of that condition. Not a bug, just code that pretends to do something it can't.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.js` around lines 95 - 109, Remove the dead isNaN check in parseFileRetentionToMs: after matching the regex and computing amount = parseInt(parsed[1], 10), the parsed[1] is guaranteed to be digits so isNaN(amount) is unreachable—leave only the meaningful validation (amount <= 0) and its error throw; update the condition that currently reads if (isNaN(amount) || amount <= 0) to just check amount <= 0 in the parseFileRetentionToMs function to keep validation correct and clear.
76-93: Why are you readingterms_linkin lowercase?Line 77 falls back from
TERMS_LINKto lowercaseterms_link. Environment variables on Linux are case-sensitive and the convention is uppercase. Documenting one and silently accepting the other is a footgun: a user who typosterms_link=...will get a "working" config that silently breaks anywhere casing matters (e.g., a different deployment tool that uppercases keys). Pick one.♻️ Proposed diff
- const rawTermsLink = process.env.TERMS_LINK || process.env.terms_link; + const rawTermsLink = process.env.TERMS_LINK;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.js` around lines 76 - 93, The getTermsLink function is incorrectly falling back to a lowercase env key; remove the lowercase fallback so it only reads process.env.TERMS_LINK (not process.env.terms_link), update any related log messages or behavior in getTermsLink to reflect the single canonical uppercase variable, and adjust any tests or callers that relied on the lowercase key; locate this change in the getTermsLink function to ensure environment variable handling is consistent and case-sensitive across the codebase..github/workflows/docker-publish.yml (1)
22-42: Verify job pins Node 24 whileengines.nodeis>=20.
package.jsondeclares"node": ">=20.0.0", so consumers running Node 20 LTS are unverified. At minimum, run the matrix on the lowest supported version (20) — ideally both. Otherwise you'll discover a Node-20-only test failure in production, not in CI.♻️ Proposed diff (matrix)
verify: runs-on: ubuntu-latest + strategy: + matrix: + node-version: [20, 24] steps: - name: Checkout code uses: actions/checkout@v5 - name: Set up Node.js uses: actions/setup-node@v5 with: - node-version: 24 + node-version: ${{ matrix.node-version }} cache: npm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-publish.yml around lines 22 - 42, The verify job currently pins Node 24 while package.json allows >=20; update the verify job (job name "verify") to run against at least Node 20 by turning the job into a matrix over node versions (e.g., include 20 and 24) and change the actions/setup-node step (uses: actions/setup-node@v5) to use node-version: ${{ matrix.node-version }} so CI runs tests/lint/install on Node 20 as well as 24.public/styles.css (1)
86-118: Missing browser fallbacks and keyboard focus styling.Look, this is pretty basic stuff you overlooked:
color-mix(in srgb, …)is relatively new—Chrome 111, Safari 16.2, Firefox 113. Yes, most users have these versions by now, but if anyone on an older browser loads this, they'll see a plain, unstyled box instead of your nice red warning. Just add a simple fallbackbackgroundandborderdeclaration before thecolor-mixlines. It's not hard.That link hover effect on lines 116-118? It only works when people click or hover. Keyboard users tabbing through? They get nothing. No visual feedback. Add
:focus-visibleto the same rule so they can actually see the focus state.♻️ Proposed diff
.legal-warning { + background: var(--container-bg); + border: 1px solid var(--border-color); background: color-mix(in srgb, var(--danger-color) 10%, var(--container-bg)); border: 1px solid color-mix(in srgb, var(--danger-color) 35%, var(--border-color)); border-radius: 8px; padding: 10px 12px; text-align: left; font-size: 0.9rem; line-height: 1.45; } @@ -.legal-warning a:hover { +.legal-warning a:hover, +.legal-warning a:focus-visible { opacity: 0.9; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles.css` around lines 86 - 118, The .legal-warning rules use color-mix() with no fallbacks and the link hover lacks keyboard focus styling; add a simple solid background and border declarations (e.g., using var(--danger-color) alpha or a plain rgba) before the color-mix() background and border lines in the .legal-warning block so older browsers get a styled box, and add a matching .legal-warning a:focus-visible rule (mirroring .legal-warning a:hover) that provides visible focus feedback (underline/strong color and/or outline/box-shadow) so keyboard users can see focus.test/cleanup.test.js (1)
28-52: Add a positive assertion on the boundary, not just "old gets deleted".This test only verifies "31 days old → deleted" and "fresh → kept". The actual cutoff logic is
mtime <= cutoff(seesrc/utils/cleanup.js). A file aged exactly toconfig.fileRetentionMsis also deleted, while one aged slightly less is kept. One extra assertion at the boundary catches future off-by-one regressions when somebody "helpfully" flips<=to<.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cleanup.test.js` around lines 28 - 52, Add a boundary test for the retention cutoff: create one file whose mtime equals Date.now() - config.fileRetentionMs and assert cleanupExpiredFiles() deletes it, and create another file with mtime slightly newer than that (e.g., minus config.fileRetentionMs - 1 ms or +1 ms depending on sign) and assert it is kept; use the same helpers/variables in the test (oldFile/freshFile or new boundary files) and reference cleanupExpiredFiles and config.fileRetentionMs when setting utimes so the test will catch off-by-one changes to the mtime <= cutoff logic.src/routes/files.js (1)
159-159: Threadingreqthrough a recursive utility is awkward; prefer building the origin once.
getDirectoryContentsis now coupled to a live Express request just so it can callbuildDownloadUrl(req, …)deep inside recursion. That's an unnecessary entanglement — compute the base origin once at the route handler, pass a string down (or just the resolved origin), andbuildDownloadUrlbecomes a pure function of(origin, relativePath). Easier to test, easier to reuse from non-HTTP contexts (e.g. notifications).Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/files.js` at line 159, getDirectoryContents is passing the Express req through recursion just to call buildDownloadUrl(req, …); instead compute the request origin once in the route handler (e.g. derive origin/baseUrl from req) and pass that origin string into getDirectoryContents instead of req, update all recursive calls to accept the origin parameter, and change buildDownloadUrl to be a pure function that accepts (origin, relativePath) (or add a small wrapper that calls the existing function) so getDirectoryContents and its recursion no longer depend on the Express request object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fdfa5ee-e795-4da0-8a66-2406b85924b9
📒 Files selected for processing (24)
.env.example.github/workflows/docker-publish.ymlLICENSELOCAL_DEVELOPMENT.mdREADME.mddemo.mddocker-compose.ymldocs/BIND_MOUNT_FIX.mdpackage.jsonpublic/index.htmlpublic/login.htmlpublic/styles.csssrc/app.jssrc/config/index.jssrc/routes/files.jssrc/routes/upload.jssrc/scripts/pwa-manifest-generator.jssrc/utils/cleanup.jssrc/utils/fileUtils.jstest/auth.test.jstest/cleanup.test.jstest/files.test.jstest/security.test.jstest/upload.test.js
✅ Files skipped from review due to trivial changes (4)
- src/scripts/pwa-manifest-generator.js
- src/utils/fileUtils.js
- demo.md
- docs/BIND_MOUNT_FIX.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| - name: Log in to GitHub Container Registry | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_PASSWORD }} | ||
| registry: ghcr.io | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| # Step 3: Setup QEMU to enable multiarch builds | ||
| - name: Set up QEMU | ||
| uses: docker/setup-qemu-action@v3 | ||
|
|
||
| # Step 4: Set up docker buildx | ||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| # Step 5: Extract metadata and set versions | ||
| - name: Extract metadata (tags, labels) for Docker | ||
| id: meta | ||
| uses: docker/metadata-action@v5 | ||
| with: | ||
| # image name is dumbdrop, in the docker user's repo | ||
| # this allows the push to work in forks | ||
| images: | | ||
| name=dumbwareio/dumbdrop | ||
| ghcr.io/${{ github.repository_owner }}/dumbdrop |
There was a problem hiding this comment.
Quietly migrating the upstream image from Docker Hub to GHCR is a breaking change.
This PR is titled "Add persistent split logging with rotation and retention" and yet lines 53-58 and 71 silently switch the published image from dumbwareio/dumbdrop on Docker Hub to ghcr.io/${{ github.repository_owner }}/dumbdrop on GHCR. Every existing user with image: dumbwareio/dumbdrop in their compose file stops getting updates the moment this lands on main.
If the maintainers actually want this, it deserves its own PR with:
- A migration note in the README/release notes.
- Continued publishing to Docker Hub (multi-registry) for at least one release cycle.
- Org-level package permissions on GHCR (
packages: writeis set, but the package still needs to exist / be linked to the repo).
If they don't, drop it from this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docker-publish.yml around lines 53 - 71, The workflow
currently switches the published image to ghcr.io/${{ github.repository_owner
}}/dumbdrop (seen in the "Log in to GitHub Container Registry" and "Extract
metadata (tags, labels) for Docker" steps), which is a breaking change for users
of dumbwareio/dumbdrop; either revert the image reference back to the original
dockerhub name or explicitly add multi-registry publishing: keep the Docker Hub
image (dumbwareio/dumbdrop) in the docker/metadata-action input and also include
ghcr.io/${{ github.repository_owner }}/dumbdrop if you intend GHCR, ensure the
docker/login-action and workflow permissions include both registries and
GITHUB_TOKEN/packages write/organization package linking, and add a
README/release-note migration paragraph so the change is documented; if you do
not want the registry change, remove the GHCR lines from this PR.
| const TERMS_LINK = '{{TERMS_LINK}}'; | ||
| const termsNotice = document.getElementById('termsNotice'); | ||
| const termsLink = document.getElementById('termsLink'); | ||
| if (!TERMS_LINK || TERMS_LINK === '{{TERMS_LINK}}') { | ||
| termsNotice.style.display = 'none'; | ||
| } else { | ||
| termsLink.href = TERMS_LINK; | ||
| } |
There was a problem hiding this comment.
Stop dropping a server-side string straight into a JavaScript literal — that's how XSS happens.
Line 80 builds the JS string by raw text substitution: const TERMS_LINK = '{{TERMS_LINK}}';. The server-side render in src/app.js (html.replace(/{{TERMS_LINK}}/g, config.termsLink || '')) does zero escaping. Yes, config.termsLink is currently constrained to http(s)://..., but:
- A
'(apostrophe) in the URL string literally terminates the JS string. Page breaks. - A
\does the same via escape collision. - If the validation in
src/config/index.jsis ever relaxed or bypassed, this becomes a stored-XSS vector on an unauthenticated page. Lovely.
Pick one of: render via JSON.stringify server-side, or read the value from a data-* attribute on the anchor and skip the duplicate JS literal entirely. Option B is simpler since you already have the anchor.
🔒 Proposed fix (read from the DOM, drop the JS literal)
- const TERMS_LINK = '{{TERMS_LINK}}';
- const termsNotice = document.getElementById('termsNotice');
- const termsLink = document.getElementById('termsLink');
- if (!TERMS_LINK || TERMS_LINK === '{{TERMS_LINK}}') {
- termsNotice.style.display = 'none';
- } else {
- termsLink.href = TERMS_LINK;
- }
+ const termsNotice = document.getElementById('termsNotice');
+ const termsLink = document.getElementById('termsLink');
+ const rawHref = termsLink.getAttribute('href') || '';
+ if (!rawHref || rawHref === '{{TERMS_LINK}}') {
+ termsNotice.style.display = 'none';
+ }📝 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.
| const TERMS_LINK = '{{TERMS_LINK}}'; | |
| const termsNotice = document.getElementById('termsNotice'); | |
| const termsLink = document.getElementById('termsLink'); | |
| if (!TERMS_LINK || TERMS_LINK === '{{TERMS_LINK}}') { | |
| termsNotice.style.display = 'none'; | |
| } else { | |
| termsLink.href = TERMS_LINK; | |
| } | |
| const termsNotice = document.getElementById('termsNotice'); | |
| const termsLink = document.getElementById('termsLink'); | |
| const rawHref = termsLink.getAttribute('href') || ''; | |
| if (!rawHref || rawHref === '{{TERMS_LINK}}') { | |
| termsNotice.style.display = 'none'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/login.html` around lines 80 - 87, The code injects an unescaped
server-side string into a JS literal (const TERMS_LINK = '{{TERMS_LINK}}'),
creating an XSS and string-termination risk; instead remove that literal and
read the link from the DOM: set a safe data attribute on the anchor server-side
(e.g. data-terms-link) and in this script use
document.getElementById('termsLink').dataset.termsLink to get the value, then
apply the same logic that currently uses TERMS_LINK (hide termsNotice if empty,
otherwise set termsLink.href); update the server render to populate the anchor's
data-terms-link attribute (or use JSON.stringify server-side if you prefer that
alternative).
| function createSafeContentDisposition(filename) { | ||
| const basename = path.basename(filename); | ||
| // eslint-disable-next-line no-control-regex | ||
| const sanitized = basename.replace(/[\u0000-\u001F\u007F"\\]/g, '_'); | ||
|
|
||
| if (/^[\u0020-\u007E]*$/.test(sanitized)) { | ||
| const escaped = sanitized.replace(/["\\]/g, '\\$&'); | ||
| return `attachment; filename="${escaped}"`; | ||
| } | ||
|
|
||
| const encoded = encodeURIComponent(sanitized); | ||
| const asciiSafe = sanitized.replace(/[^\u0020-\u007E]/g, '_'); | ||
| return `attachment; filename="${asciiSafe}"; filename*=UTF-8''${encoded}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
createSafeContentDisposition is now defined twice — pick one home.
This is a verbatim copy of the same function in src/routes/files.js (lines 31-53). Two copies of the same security-sensitive code is exactly two places where someone has to remember to apply the next bug fix. Move it into src/utils/fileUtils.js (or src/utils/http.js) and require it from both sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.js` around lines 24 - 37, The function createSafeContentDisposition
is duplicated; extract it into a single utility module (e.g., fileUtils) and
export it, then remove the duplicate definitions and import/require that
exported createSafeContentDisposition in both the app module and the files route
module so both use the shared implementation; ensure the exported function name
remains createSafeContentDisposition and update callers to use the imported
symbol.
|
|
||
| let html = fs.readFileSync(path.join(__dirname, '../public', 'index.html'), 'utf8'); | ||
| html = html.replace(/{{SITE_TITLE}}/g, config.siteTitle); | ||
| html = html.replace(/{{TERMS_LINK}}/g, config.termsLink || ''); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Three copies of the same {{TERMS_LINK}} substitution — and none of them escape it.
The same line html = html.replace(/{{TERMS_LINK}}/g, config.termsLink || '') is repeated three times across the root handler, the login handler, and the static-HTML middleware. None of them HTML-escape config.termsLink before injecting it into an HTML attribute, and one of them (login.html) injects it into a JS string literal — see the dedicated comment on public/login.html.
Pull this into a single renderHtmlTemplate(filePath, req) helper that does all substitutions and applies proper context-aware escaping (escape-html for attributes, JSON.stringify for JS contexts). Today, with PR-level validation forcing http(s):// URLs, the risk is contained — tomorrow, when someone "improves" the validator, this becomes XSS at three sites instead of one.
Also applies to: 208-208, 223-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.js` at line 191, There are three repeated, unsafe substitutions of
config.termsLink across the root handler, login handler (public/login.html), and
static-HTML middleware; pull this into a single renderHtmlTemplate(filePath,
req) helper that loads the HTML, performs all template substitutions in one
place, and applies context-aware escaping: use the escape-html package (or
equivalent) when injecting into HTML attributes and use JSON.stringify when
injecting into JS string literals (e.g., login.html). Replace the repeated lines
that call html.replace(/{{TERMS_LINK}}/g, config.termsLink || '') in the root
handler, login handler, and static middleware with calls to
renderHtmlTemplate(filePath, req), and ensure renderHtmlTemplate references
config.termsLink, does a safe default (''), and is invoked everywhere the
template is served.
| fileRetentionMs: (() => { | ||
| const envValue = process.env.FILE_RETENTION; | ||
| const effectiveValue = envValue === undefined ? DEFAULT_FILE_RETENTION : envValue; | ||
|
|
||
| try { | ||
| const ms = parseFileRetentionToMs(effectiveValue); | ||
| logAndReturn('FILE_RETENTION', effectiveValue, envValue === undefined); | ||
| return ms; | ||
| } catch (err) { | ||
| if (envValue !== undefined) { | ||
| throw err; | ||
| } | ||
|
|
||
| logConfig(`Invalid default FILE_RETENTION value "${effectiveValue}". Falling back to ${DEFAULT_FILE_RETENTION}.`, 'warning'); | ||
| return parseFileRetentionToMs(DEFAULT_FILE_RETENTION); | ||
| } | ||
| })(), |
There was a problem hiding this comment.
Empty FILE_RETENTION= will crash the app at startup.
process.env.FILE_RETENTION is undefined when the variable is unset, but it's '' (empty string) when the user has FILE_RETENTION= in their .env (which is a perfectly common shape — and it's literally the shape of PUBLIC_DOMAIN= you ship in .env.example). With your current logic at line 238:
if (envValue !== undefined) {
throw err;
}…an empty string is !== undefined, so parseFileRetentionToMs('') throws, and the throw err re-throws it instead of falling back to 30d. Boom — startup crash on a config you ostensibly support.
🛡️ Proposed fix
fileRetentionMs: (() => {
const envValue = process.env.FILE_RETENTION;
- const effectiveValue = envValue === undefined ? DEFAULT_FILE_RETENTION : envValue;
+ const hasUserValue = envValue !== undefined && envValue.trim() !== '';
+ const effectiveValue = hasUserValue ? envValue : DEFAULT_FILE_RETENTION;
try {
const ms = parseFileRetentionToMs(effectiveValue);
- logAndReturn('FILE_RETENTION', effectiveValue, envValue === undefined);
+ logAndReturn('FILE_RETENTION', effectiveValue, !hasUserValue);
return ms;
} catch (err) {
- if (envValue !== undefined) {
+ if (hasUserValue) {
throw err;
}
logConfig(`Invalid default FILE_RETENTION value "${effectiveValue}". Falling back to ${DEFAULT_FILE_RETENTION}.`, 'warning');
return parseFileRetentionToMs(DEFAULT_FILE_RETENTION);
}
})(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/index.js` around lines 229 - 245, The current fileRetentionMs IIFE
treats an empty string in process.env.FILE_RETENTION as a provided value and
rethrows parse errors; update the check that decides whether to rethrow so that
an empty string is treated like "unset". Specifically, when catching errors from
parseFileRetentionToMs(effectiveValue) use a condition that only rethrows if
envValue is neither undefined nor empty (e.g., envValue !== undefined &&
envValue !== ''), and otherwise log the fallback via logConfig and return
parseFileRetentionToMs(DEFAULT_FILE_RETENTION); keep references to
fileRetentionMs, parseFileRetentionToMs, DEFAULT_FILE_RETENTION, logAndReturn,
logConfig and envValue to locate the code.
| function buildDownloadUrl(req, relativePath) { | ||
| const encodedPath = encodePathForUrl(relativePath); | ||
| const baseOrigin = config.publicDomain || `${req.protocol}://${req.get('host')}`; | ||
| return `${baseOrigin}/${encodedPath}`; | ||
| } |
There was a problem hiding this comment.
Host header injection: req.get('host') is whatever the client says it is.
When config.publicDomain is unset, buildDownloadUrl happily echoes back the Host header the requester supplied. That means an attacker who can hit /api/files (or trick a cached/proxied response) can have you generate downloadUrl: "https://evil.example.com/whatever" and serve it to other users via screenshots, share links, or any consumer that trusts the API. Classic host-header injection.
Two reasonable mitigations:
- Make
PUBLIC_DOMAINmandatory when this feature is on, and refuse to fall back to the request host. - If you must fall back, validate
req.get('host')againstconfig.baseUrl/ an allow-list before using it.
Doing neither leaves a footgun in the default config (which, per docker-compose.yml, ships with PUBLIC_DOMAIN: http://localhost:3000 — fine for production, but the dev/in-process fallback is the ugly path here).
🔒 Suggested hardening
function buildDownloadUrl(req, relativePath) {
const encodedPath = encodePathForUrl(relativePath);
- const baseOrigin = config.publicDomain || `${req.protocol}://${req.get('host')}`;
+ // Never trust the client-provided Host header for URLs we hand back to clients.
+ const baseOrigin = config.publicDomain
+ || (() => { try { return new URL(config.baseUrl).origin; } catch { return ''; } })();
+ if (!baseOrigin) {
+ return `/${encodedPath}`; // relative URL fallback — still works for the same origin
+ }
return `${baseOrigin}/${encodedPath}`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/files.js` around lines 19 - 23, buildDownloadUrl currently trusts
req.get('host') when config.publicDomain is unset, enabling host-header
injection; update buildDownloadUrl to stop using the raw request Host fallback:
either (A) require config.publicDomain and throw a clear error if unset (so
download URLs are always built from config.publicDomain), or (B) if you must
allow fallback, validate req.get('host') against a safe allow-list derived from
config.baseUrl or a new config.allowedHosts and only use it when it matches;
keep the rest of the logic (encodePathForUrl, returned path) intact and
reference the buildDownloadUrl function, config.publicDomain, req.get('host'),
and encodePathForUrl when making the change.
| let failedUploadCleanupInterval; | ||
| async function cleanupFailedUploads(referenceTime = Date.now()) { | ||
| let metadataFiles; | ||
| try { | ||
| metadataFiles = await fs.readdir(METADATA_DIR); | ||
| } catch (err) { | ||
| if (err.code !== 'ENOENT') { | ||
| logger.error(`Failed to list metadata directory for failed upload cleanup: ${err.message}`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| let cleanedCount = 0; | ||
| for (const file of metadataFiles) { | ||
| if (!file.endsWith('.meta')) continue; | ||
|
|
||
| const uploadId = file.slice(0, -5); | ||
| try { | ||
| const metadata = await readUploadMetadata(uploadId); | ||
| if (!metadata || !metadata.failedAt) continue; | ||
|
|
||
| if ((referenceTime - metadata.failedAt) < FAILED_UPLOAD_RETENTION_MS) continue; | ||
|
|
||
| if (metadata.partialFilePath && isPathWithinUploadDir(metadata.partialFilePath, config.uploadDir, false)) { | ||
| try { | ||
| await fs.unlink(metadata.partialFilePath); | ||
| } catch (unlinkErr) { | ||
| if (unlinkErr.code !== 'ENOENT') { | ||
| logger.warn(`Failed to remove partial file for failed upload ${uploadId}: ${unlinkErr.message}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await deleteUploadMetadata(uploadId); | ||
| cleanedCount++; | ||
| } catch (err) { | ||
| logger.warn(`Failed cleaning up failed upload metadata ${uploadId}: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (cleanedCount > 0) { | ||
| logger.info(`Cleaned up ${cleanedCount} failed uploads after retention period`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bless your heart — you wrote a cleanup that races with your own /chunk resume path.
Look carefully at what happens when cleanupFailedUploads runs at the same moment a slow client decides to POST another chunk for a previously-failed upload:
- Cleanup reads metadata (line 180), sees
failedAt, decides retention expired. - Meanwhile
/chunk/:uploadIdhandler reads the same metadata (line 442), deletesfailedAtin-memory (line 467), and startsfs.open(metadata.partialFilePath, 'a')at line 512. - Cleanup happily
fs.unlinksmetadata.partialFilePathat line 187. - Cleanup
deleteUploadMetadatas the file at line 195. /chunkhandler finishes the append into an unlinked inode, thenwriteUploadMetadataat line 532 re-creates the.metafile pointing at a file that no longer exists on disk (on Linux — on Windows the unlink would have failed while the handle was open).
Net result: the user thinks their resume succeeded (HTTP 200), but their bytes went to /dev/null, and from then on /chunk will keep appending to a newly-created empty file while metadata.bytesReceived drifts forever out of sync with the actual file size. Finalization renames an under-sized file as the "complete" upload.
This is exactly the race the "oh it's fine, we check failedAt again" comment tries to dodge, and it doesn't.
At minimum, re-read the metadata immediately before each fs.unlink / deleteUploadMetadata and bail out if failedAt was cleared. Better yet, use an atomic rename-out-of-the-way pattern (rename the .partial to something like .cleanup-<random> before unlinking, so a concurrent fs.open('a') either sees the new name or fails cleanly). Best of all, a per-upload lock.
🩹 Minimum viable tightening (re-check failedAt right before destroying anything)
const uploadId = file.slice(0, -5);
try {
const metadata = await readUploadMetadata(uploadId);
if (!metadata || !metadata.failedAt) continue;
if ((referenceTime - metadata.failedAt) < FAILED_UPLOAD_RETENTION_MS) continue;
+ // Re-read right before destructive ops to reduce the TOCTOU window
+ // against /chunk resuming the upload and clearing failedAt.
+ const recheck = await readUploadMetadata(uploadId);
+ if (!recheck || !recheck.failedAt) continue;
+ if ((referenceTime - recheck.failedAt) < FAILED_UPLOAD_RETENTION_MS) continue;
+
if (metadata.partialFilePath && isPathWithinUploadDir(metadata.partialFilePath, config.uploadDir, false)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/upload.js` around lines 162 - 205, The cleanupFailedUploads
function races with the /chunk resume path: before unlinking or deleting
metadata re-read the upload metadata (call readUploadMetadata(uploadId)) and
abort cleanup if metadata is missing or metadata.failedAt is no longer set (or
differs from the value that caused cleanup), and only then remove the partial
file and deleteUploadMetadata; alternatively (preferred) atomically rename the
partial file (e.g., metadata.partialFilePath -> metadata.partialFilePath +
`.cleanup-<random>`) before unlinking so concurrent fs.open(...,'a') either
fails or appends to the original name, and ensure isPathWithinUploadDir,
fs.unlink, and deleteUploadMetadata act on the renamed path; consider adding a
per-upload lock around
readUploadMetadata/writeUploadMetadata/deleteUploadMetadata and chunk-appends to
fully eliminate the race.
| if (!process.env.DISABLE_BATCH_CLEANUP) { | ||
| metadataCleanupTimer = setInterval(cleanupIncompleteMetadataUploads, METADATA_CLEANUP_INTERVAL); | ||
| metadataCleanupTimer.unref(); // Allow process to exit if this is the only timer | ||
|
|
||
| fileRetentionCleanupTimer = setInterval(cleanupExpiredFiles, FILE_RETENTION_CLEANUP_INTERVAL); | ||
| fileRetentionCleanupTimer.unref(); | ||
|
|
||
| process.on('SIGTERM', () => clearInterval(metadataCleanupTimer)); | ||
| process.on('SIGINT', () => clearInterval(metadataCleanupTimer)); | ||
| process.on('SIGTERM', () => clearInterval(fileRetentionCleanupTimer)); | ||
| process.on('SIGINT', () => clearInterval(fileRetentionCleanupTimer)); | ||
| } |
There was a problem hiding this comment.
No initial run — short-lived containers will never reap anything.
You only schedule cleanup on a 6-hour interval (line 248). If a container restarts more frequently than that — and a container handling uploads is exactly the kind of process that gets restarted often — the timer never fires and cleanupExpiredFiles never runs. Kick it off once on startup (after a small delay so it doesn't race with app initialize) in addition to the interval.
🛡️ Proposed fix
fileRetentionCleanupTimer = setInterval(cleanupExpiredFiles, FILE_RETENTION_CLEANUP_INTERVAL);
fileRetentionCleanupTimer.unref();
+
+ // Run once shortly after startup so short-lived containers still get a sweep.
+ setTimeout(() => {
+ cleanupExpiredFiles().catch((err) =>
+ logger.error(`Initial retention cleanup failed: ${err.message}`)
+ );
+ }, 30 * 1000).unref();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/cleanup.js` around lines 244 - 255, The interval-only scheduling
means cleanupExpiredFiles and cleanupIncompleteMetadataUploads may never run in
short-lived containers; call both cleanup functions once on startup (after a
small delay to avoid racing app initialization) in addition to setting
metadataCleanupTimer and fileRetentionCleanupTimer with
METADATA_CLEANUP_INTERVAL and FILE_RETENTION_CLEANUP_INTERVAL, respectively;
keep the existing timers and unref() behavior and still register the
SIGTERM/SIGINT handlers that clear metadataCleanupTimer and
fileRetentionCleanupTimer.
Allow same-host origin fallback when protocol differs behind reverse proxies, skip blocking requests with no Origin/Referer, and fix origin normalization comparison. Also update legal links in the upload page to the current TOS_Privacy URL.
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 (1)
public/index.html (1)
316-340:⚠️ Potential issue | 🟡 MinorYour retry loop forgets to clean up after itself when things go sideways.
On the happy path (response arrives),
clearTimeout(timeoutId)andremoveEventListener('abort', onAbort)both run at lines 336–339. Lovely. On the sad path — fetch rejects with a network error or non-abort failure — control jumps to thecatchat line 374 and neither runs.Consequences, in increasing order of "do I have to care":
timeoutIdstays armed and will eventually firecontroller.abort()on a controller whose request is already done. No-op, but noisy.onAbortwas registered with{ once: true }, so it self-removes only if the user actually hits cancel. If the abort never happens, every retry attempt stacks another listener onthis.abortController.signal. WithmaxRetries = 5, that's up to 6 listeners per chunk; if the user eventually cancels, you firecontroller.abort()6 times on the same (already-aborted) local controller. Harmless. Just sloppy.Stuff cleanup into a
finallyso both paths behave:♻️ Proposed cleanup
const controller = new AbortController(); const onAbort = () => controller.abort(); if (this.abortController) { this.abortController.signal.addEventListener('abort', onAbort, { once: true }); } const timeoutId = setTimeout(() => controller.abort(), 30000); // 30-second timeout per attempt - const response = await fetch(chunkApiUrl, { - method: 'POST', - headers: { - 'Content-Type': 'application/octet-stream', - 'X-Batch-ID': this.batchId - }, - body: chunk, - signal: controller.signal // Add abort signal - }); - - clearTimeout(timeoutId); // Clear timeout if fetch completes - if (this.abortController) { - this.abortController.signal.removeEventListener('abort', onAbort); - } + let response; + try { + response = await fetch(chunkApiUrl, { + method: 'POST', + headers: { + 'Content-Type': 'application/octet-stream', + 'X-Batch-ID': this.batchId + }, + body: chunk, + signal: controller.signal + }); + } finally { + clearTimeout(timeoutId); + if (this.abortController) { + this.abortController.signal.removeEventListener('abort', onAbort); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 316 - 340, The retry attempt leaks resources on failure: move the cleanup of the AbortController listener and timeout into a finally so they run on both success and error; specifically, after you create controller, onAbort and timeoutId (and before/around the fetch + retry logic in the same retry loop), ensure you wrap the fetch attempt in try/catch/finally and in the finally call clearTimeout(timeoutId) and, if this.abortController exists, removeEventListener('abort', onAbort) to guarantee removal even when fetch rejects or throws.
♻️ Duplicate comments (2)
public/index.html (2)
122-129:⚠️ Potential issue | 🔴 CriticalCongratulations, the page is still bricked. 🧱
Pop quiz, take two: where in this HTML are the elements with
id="termsNotice"andid="termsLink"? I'll save you the suspense — they still don't exist. The legal-warning<div>s at lines 32–38 and 76–81 haveclass="legal-warning ..."and zeroidattributes. This was flagged on the previous commit. The JS was not changed. The HTML was not changed. The bug is, miraculously, identical.So one more time, with feeling:
src/app.jsreplaces{{TERMS_LINK}}withconfig.termsLink || ''— i.e., empty string by default. That makes!TERMS_LINKtruthy, so we hittermsNotice.style.display = 'none'onnull→TypeError: Cannot read properties of null (reading 'style').- That error throws inside the top-level
<script defer>, halting everything below it. NoFileUploader. No cancel button (the very feature this PR is adding). NocancelUploadButton.addEventListenerat line 722. NofileListManager. No theme toggle. The entire page is dead on arrival.If you actually loaded this in a browser even once, you would have seen this. So... maybe do that next time?
The minimum fix is two parts — and yes, you need both:
🔧 Step 1: add the IDs the JS is desperately calling for
See the diff in the comment on lines 32–38.
id="termsNotice"on the<div>,id="termsLink"on the<a>, drop the hardcoded href.🛡️ Step 2: defensively guard the JS so a future you doesn't repeat history
const TERMS_LINK = '{{TERMS_LINK}}'; const termsNotice = document.getElementById('termsNotice'); const termsLink = document.getElementById('termsLink'); - if (!TERMS_LINK || TERMS_LINK === '{{TERMS_LINK}}') { - termsNotice.style.display = 'none'; - } else { - termsLink.href = TERMS_LINK; + if (termsNotice && termsLink) { + if (!TERMS_LINK || TERMS_LINK === '{{TERMS_LINK}}') { + termsNotice.style.display = 'none'; + } else { + termsLink.href = TERMS_LINK; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 122 - 129, Add id="termsNotice" to the legal-warning <div> and id="termsLink" to the corresponding <a> (remove the hardcoded href on that <a>) so the DOM elements referenced by the script exist, and defensively guard the top-level script that uses TERMS_LINK/termsNotice/termsLink by checking that termsNotice and termsLink are non-null before accessing properties (e.g., only call termsNotice.style or set termsLink.href when the queried elements exist and TERMS_LINK is truthy).
32-38:⚠️ Potential issue | 🟠 MajorYou changed the URL slug. That's it. That's the "fix". 🎉
The earlier review pointed out (politely!) that hardcoding a link to your personal domain into every deployment of someone else's OSS project is, you know, not great. The grand response here was to swap
/privacy-policy-terms-of-use-disclaimer-and-limitation-of-liability/for/TOS_Privacy. Same domain. Same problem. Every fork, every self-hoster, every Docker pull now points its users atwickedyoda.comfor "Full terms and disclaimer". They did not sign up for this. Neither did DumbWareio.The PR even advertises a
TERMS_LINKenv var for exactly this purpose — and the JS at lines 122–129 is trying (badly, see the other comment) to wire it up. So please actually use it: drop the hardcodedhref, addid="termsNotice"to the<div>andid="termsLink"to the<a>, and let the server inject the operator's URL. IfTERMS_LINKisn't set, hide the block entirely.Same exact treatment needed for the bottom block at lines 76–81 — including ripping the bare
wickedyoda.comURL out of the visible link text (line 79), which is even worse because it's literally printed on the page.Also, gentle reminder: this whole "legal notices" thing is well outside the PR's stated scope ("Add persistent split logging with rotation and retention"). It belongs in its own PR so it can be reviewed on its own (lack of) merits.
🧹 The fix that should have happened the first time
- <div class="legal-warning legal-warning-top" role="note" aria-label="Important upload warning"> + <div id="termsNotice" class="legal-warning legal-warning-top" role="note" aria-label="Important upload warning"> <p> <strong>Warning:</strong> Files uploaded to this service may be publicly accessible. Do not upload sensitive data. Files are temporary, not backed up, and may be deleted at any time. Use at your own risk. - <a href="https://www.wickedyoda.com/TOS_Privacy" target="_blank" rel="noopener noreferrer">Full terms and disclaimer</a> + <a id="termsLink" href="#" target="_blank" rel="noopener noreferrer">Full terms and disclaimer</a> </p> </div>And for the bottom block (lines 76–81), either give it the same
id-driven treatment via a second JS-injected variable, or just delete it — the top notice already says everything.Also applies to: 76-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 32 - 38, Replace the hardcoded wickedyoda URLs by making the top legal notice div id-driven and server/JS-injectable: add id="termsNotice" to the top <div> and id="termsLink" to the <a>, remove the hardcoded href and visible wickedyoda text, and have the server or the existing client-side wiring for the TERMS_LINK env var (the code around lines ~122–129 that reads TERMS_LINK) set the anchor's href and visible text; if TERMS_LINK is not provided, hide/remove the `#termsNotice` element entirely. Apply the same treatment to the bottom legal block (either add analogous ids and inject/hide via TERMS_LINK or delete the duplicate block) so no deployment ships with a hardcoded external personal domain.
🧹 Nitpick comments (3)
src/middleware/cors.js (3)
82-86:catchfalls through toundefined— returnfalseexplicitly.Callers treat the return as boolean (
if (isOriginValid)in the middleware). Implicitundefinedworks today by virtue of being falsy, but an explicitreturn false;makes the contract obvious and prevents future linters/refactors from regressing.♻️ Proposed tweak
catch (error) { console.error(error); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/cors.js` around lines 82 - 86, The catch block in the isOriginValid function currently only logs the error (console.error(error)) and falls through to undefined; change the catch to explicitly return false after logging so callers checking "if (isOriginValid)" receive a boolean. Locate isOriginValid in src/middleware/cors.js and update its catch(error) handler to log the error and then return false.
88-103: Middleware change looks fine; small redundancy on line 96.Bypassing validation when neither
OriginnorRefereris present is reasonable — top-level navigations and many same-origin requests legitimately omit both, and persrc/app.js:133-161/src/app.js:170-180downstreamrequirePin+ rate limiters serve as the second line of defense for non-public paths.Two notes:
- Line 96's
const origin = rawOrigin;is a no-op alias — you can drop it and passrawOrigindirectly tovalidateOrigin.- The fallback security model assumes
config.pinis set. If a deployment runs without a PIN and exposes write endpoints, this bypass + permissive same-host CORS would leave very little CSRF protection at the middleware layer. Worth a one-line note in the README's logging/CORS section, or a startup warning when PIN is unset.♻️ Minor cleanup
const rawOrigin = req.headers.origin || req.headers.referer; if (!rawOrigin) { return next(); } - const origin = rawOrigin; - const isOriginValid = validateOrigin(origin); + const isOriginValid = validateOrigin(rawOrigin); if (isOriginValid) { next(); } else { res.status(403).json({ error: 'Forbidden' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/cors.js` around lines 88 - 103, Remove the redundant alias in originValidationMiddleware by passing req.headers.origin || req.headers.referer directly into validateOrigin (drop the no-op const origin = rawOrigin), and add a short startup warning or README note about the fallback security model when config.pin is unset so operators know that bypassing Origin/Referer checks reduces CSRF protection for write endpoints; reference originValidationMiddleware, validateOrigin, req.headers.origin, req.headers.referer and config.pin when making these changes.
38-55: UseURL.hostnameinstead ofURL.hostto avoid port-mismatch failures on proxy rewrites.Look, here's the thing: your
hostcomparison breaks whenever there's a port discrepancy. When the allowlist hashttps://example.com:8443but the proxy rewrites it tohttps://example.com(default port 443 omitted), the comparison silently fails even though it's the same site. The WHATWG URL spec only omits default ports from the serializedhostvalue, so non-default ones hang around and ruin your matching logic.Switching to
hostnamesidesteps this entirely since it never includes the port. Just swap both occurrences to.hostname:♻️ Proposed refactor
try { - const incomingHost = new URL(origin).host; + const incomingHost = new URL(origin).hostname; return allowlist.some((allowedOrigin) => { try { - return new URL(allowedOrigin).host === incomingHost; + return new URL(allowedOrigin).hostname === incomingHost; } catch { return false; } });If you actually want port-strict matching, add a comment explaining why so the next person doesn't "fix" it. The error handling is fine, though.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/cors.js` around lines 38 - 55, The function hasMatchingHost currently compares URL.host (incomingHost and allowedOrigin host) which includes ports and causes false negatives when proxies strip default ports; update both usages to use URL.hostname instead (i.e., derive incomingHostname = new URL(origin).hostname and compare against new URL(allowedOrigin).hostname in the allowlist.some callback). If you need strict port-matching instead, add a clear comment in hasMatchingHost explaining that decision so it isn't changed later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/middleware/cors.js`:
- Around line 72-77: Update the branch that accepts origins by host-only
matching (the block that calls hasMatchingHost(origin, allowedOrigins) inside
validateOrigin) to clearly document the security trade-off by expanding the
inline comment to state that protocol (scheme) is intentionally not enforced
once the host matches (so http://host will be allowed for https://host entries)
and that this relaxes CORS policy behind TLS-terminating proxies; additionally,
add an optional environment-gated behavior using a flag like
CORS_ALLOW_PROTOCOL_MISMATCH (read in the same config used by validateOrigin) so
operators can opt into allowing protocol mismatches or require strict scheme
parity when the flag is false.
- Around line 32-33: The console.error in normalizeOrigin uses the stale message
"Error parsing referer URL:" while the thrown Error refers to "origin"; update
the log to consistently say "Error parsing origin URL:" (or include the origin
variable) so logs match the thrown Error; locate the normalizeOrigin helper and
change the console.error message there to align with the thrown Error and other
callers (Origin, Referer, and setupOrigins).
---
Outside diff comments:
In `@public/index.html`:
- Around line 316-340: The retry attempt leaks resources on failure: move the
cleanup of the AbortController listener and timeout into a finally so they run
on both success and error; specifically, after you create controller, onAbort
and timeoutId (and before/around the fetch + retry logic in the same retry
loop), ensure you wrap the fetch attempt in try/catch/finally and in the finally
call clearTimeout(timeoutId) and, if this.abortController exists,
removeEventListener('abort', onAbort) to guarantee removal even when fetch
rejects or throws.
---
Duplicate comments:
In `@public/index.html`:
- Around line 122-129: Add id="termsNotice" to the legal-warning <div> and
id="termsLink" to the corresponding <a> (remove the hardcoded href on that <a>)
so the DOM elements referenced by the script exist, and defensively guard the
top-level script that uses TERMS_LINK/termsNotice/termsLink by checking that
termsNotice and termsLink are non-null before accessing properties (e.g., only
call termsNotice.style or set termsLink.href when the queried elements exist and
TERMS_LINK is truthy).
- Around line 32-38: Replace the hardcoded wickedyoda URLs by making the top
legal notice div id-driven and server/JS-injectable: add id="termsNotice" to the
top <div> and id="termsLink" to the <a>, remove the hardcoded href and visible
wickedyoda text, and have the server or the existing client-side wiring for the
TERMS_LINK env var (the code around lines ~122–129 that reads TERMS_LINK) set
the anchor's href and visible text; if TERMS_LINK is not provided, hide/remove
the `#termsNotice` element entirely. Apply the same treatment to the bottom legal
block (either add analogous ids and inject/hide via TERMS_LINK or delete the
duplicate block) so no deployment ships with a hardcoded external personal
domain.
---
Nitpick comments:
In `@src/middleware/cors.js`:
- Around line 82-86: The catch block in the isOriginValid function currently
only logs the error (console.error(error)) and falls through to undefined;
change the catch to explicitly return false after logging so callers checking
"if (isOriginValid)" receive a boolean. Locate isOriginValid in
src/middleware/cors.js and update its catch(error) handler to log the error and
then return false.
- Around line 88-103: Remove the redundant alias in originValidationMiddleware
by passing req.headers.origin || req.headers.referer directly into
validateOrigin (drop the no-op const origin = rawOrigin), and add a short
startup warning or README note about the fallback security model when config.pin
is unset so operators know that bypassing Origin/Referer checks reduces CSRF
protection for write endpoints; reference originValidationMiddleware,
validateOrigin, req.headers.origin, req.headers.referer and config.pin when
making these changes.
- Around line 38-55: The function hasMatchingHost currently compares URL.host
(incomingHost and allowedOrigin host) which includes ports and causes false
negatives when proxies strip default ports; update both usages to use
URL.hostname instead (i.e., derive incomingHostname = new URL(origin).hostname
and compare against new URL(allowedOrigin).hostname in the allowlist.some
callback). If you need strict port-matching instead, add a clear comment in
hasMatchingHost explaining that decision so it isn't changed later.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67ef7387-b927-4c29-95ed-7ad91ecf4390
📒 Files selected for processing (2)
public/index.htmlsrc/middleware/cors.js
…ll-safety **CORS Middleware (src/middleware/cors.js):** - Add CORS_ALLOW_PROTOCOL_MISMATCH env flag to control protocol mismatch behavior - Fix console.error message consistency in normalizeOrigin - Update hasMatchingHost to use hostname instead of host for port-agnostic matching - Expand documentation for protocol mismatch handling with security trade-off notes - Fix validateOrigin catch block to explicitly return false instead of undefined - Remove redundant origin variable alias in originValidationMiddleware **Upload UI & Terms Link (public/index.html):** - Add id='termsNotice' and id='termsLink' to legal warning elements - Remove hardcoded wickedyoda.com URLs and duplicate legal block - Add defensive null checks to TERMS_LINK injection script - Fix resource leak in upload retry loop by wrapping fetch in try/catch/finally - Ensure AbortController listeners and timeouts are cleaned up on both success and error **Impact:** - Prevents TypeError from null DOM element access when TERMS_LINK not provided - Fixes page load hang/blank screen issue reported in latest Docker image - Improves CORS flexibility for proxied deployments (TLS termination) - Prevents resource leaks from uncleaned timers/event listeners on failed uploads - Clarifies security posture and provides env-gated configuration
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/index.html`:
- Around line 302-306: The retry backoff sleeps unconditionally after a failed
attempt so a Cancel click during that delay doesn't abort promptly; modify the
retry loop around the sleep/backoff (the loop using this.maxRetries and
this.cancelRequested) to use a cancellable wait instead of a plain sleep — add a
helper like sleepUntilOrCancel(delay) that periodically checks
this.cancelRequested (or attaches to an AbortSignal) and throws a
DOMException('Upload cancelled','AbortError') if cancelled, and replace the
unconditional sleep calls with awaits of that helper so backoff is interruptible
by cancelRequested.
- Around line 334-344: The client-side chunk upload handler is checking
data.completed which the server never returns; change the completion gating to
use the server-returned file object instead. In the response handling block (the
code around updateProgress and this.completedFile), use the presence of
data.file (truthy) to set this.completedFile and render the post-upload link,
and continue to pass data.progress into updateProgress so progress remains
accurate. Locate the response handling that calls updateProgress(...) and
assigns this.completedFile and replace the data.completed check with a data.file
check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21b3d089-4b90-4869-98e5-851501b6fce5
📒 Files selected for processing (2)
public/index.htmlsrc/middleware/cors.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/middleware/cors.js
| for (let attempt = 0; attempt <= this.maxRetries; attempt++) { | ||
| try { | ||
| if (this.cancelRequested) { | ||
| throw new DOMException('Upload cancelled', 'AbortError'); | ||
| } |
There was a problem hiding this comment.
Make the retry backoff cancellable too.
Lines 393-396 still sleep unconditionally after a failed attempt. If the user clicks Cancel during that backoff, the upload can sit there for up to 30 seconds before the loop notices cancelRequested, which makes the new cancel flow feel stuck.
Also applies to: 393-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/index.html` around lines 302 - 306, The retry backoff sleeps
unconditionally after a failed attempt so a Cancel click during that delay
doesn't abort promptly; modify the retry loop around the sleep/backoff (the loop
using this.maxRetries and this.cancelRequested) to use a cancellable wait
instead of a plain sleep — add a helper like sleepUntilOrCancel(delay) that
periodically checks this.cancelRequested (or attaches to an AbortSignal) and
throws a DOMException('Upload cancelled','AbortError') if cancelled, and replace
the unconditional sleep calls with awaits of that helper so backoff is
interruptible by cancelRequested.
| if (response.ok) { | ||
| const data = await response.json(); | ||
| if (attempt > 0) { | ||
| console.log(`Chunk upload successful on retry attempt ${attempt} for ${this.file.webkitRelativePath || this.file.name}`); | ||
| } | ||
| // Update progress based on server response | ||
| // this.position is updated by readChunk(), so progress reflects total uploaded | ||
| this.updateProgress(data.progress); | ||
| if (data.completed && data.file) { | ||
| this.completedFile = data.file; | ||
| } |
There was a problem hiding this comment.
Use the actual completion signal from the chunk API response.
Line 342 is gated on data.completed, but src/routes/upload.js:535-562 returns the terminal chunk as { bytesReceived, progress, file }. That means non-empty uploads never set this.completedFile, so the new post-upload link rendering never gets the uploaded file metadata.
🔧 Proposed fix
- if (data.completed && data.file) {
+ if (data.file) {
this.completedFile = data.file;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/index.html` around lines 334 - 344, The client-side chunk upload
handler is checking data.completed which the server never returns; change the
completion gating to use the server-returned file object instead. In the
response handling block (the code around updateProgress and this.completedFile),
use the presence of data.file (truthy) to set this.completedFile and render the
post-upload link, and continue to pass data.progress into updateProgress so
progress remains accurate. Locate the response handling that calls
updateProgress(...) and assigns this.completedFile and replace the
data.completed check with a data.file check.
Summary
This PR adds persistent, host-mountable application logging with category-based files and time-based lifecycle controls.
What changed
LOG_DIR(default/logsin production).container-*.logapp-*.logaccess-*.logerror-*.logdebug-*.logLOG_ROTATION_DAYS(default: 30).LOG_RETENTION_DAYS(default: 90).ACCESS_LOG_ENABLEDtoggle (default: true)./logscan be bind-mounted for local collection.Why
Operators needed persistent logs mounted from the container, with bounded growth and clearer separation between operational streams for troubleshooting.
Notes
LOG_TO_FILE=false.High-level PR Summary
This PR introduces persistent file-based logging with category-based log separation (
container,app,access,error,debug), time-based rotation windows, and automatic retention cleanup. Log files are written to a configurable directory (LOG_DIR) that can be mounted from the host, and include HTTP access logging middleware. The system supports rotation based onLOG_ROTATION_DAYS(default 30 days) and prunes old logs afterLOG_RETENTION_DAYS(default 90 days). Console logging remains unchanged, and file logging can be toggled via environment variables.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
README.mddocker-compose.ymlDockerfilesrc/utils/logger.jssrc/app.jsSummary by CodeRabbit
New Features
Bug Fixes
Documentation