Conversation
Co-authored-by: Jennifer Wang <jennifer123wang@gmail.com> Co-authored-by: strawberry-raccoon <253751817+strawberry-raccoon@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/pages/url.tsx">
<violation number="1" location="src/pages/url.tsx:55">
P2: When a remote server supplies an all-dot or reserved filename, `getFilename` returns an empty name and the download fails. Fall back to a non-empty generated filename after sanitization, preserving the MIME extension when available.</violation>
<violation number="2" location="src/pages/url.tsx:56">
P1: A remote server can make this handler wait indefinitely and buffer an arbitrarily large body. Add an abort timeout and stream the response through an enforced maximum byte count instead of calling `res.blob()` without a limit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| throw new Error(`Failed to download URL, received ${res.status}`); | ||
| } | ||
| const filename = getFilename(body.url, res.headers); | ||
| const fileSizeBytes = await Bun.write(`${userUploadsDir}${filename}`, await res.blob()); |
There was a problem hiding this comment.
P1: A remote server can make this handler wait indefinitely and buffer an arbitrarily large body. Add an abort timeout and stream the response through an enforced maximum byte count instead of calling res.blob() without a limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pages/url.tsx, line 56:
<comment>A remote server can make this handler wait indefinitely and buffer an arbitrarily large body. Add an abort timeout and stream the response through an enforced maximum byte count instead of calling `res.blob()` without a limit.</comment>
<file context>
@@ -0,0 +1,65 @@
+ throw new Error(`Failed to download URL, received ${res.status}`);
+ }
+ const filename = getFilename(body.url, res.headers);
+ const fileSizeBytes = await Bun.write(`${userUploadsDir}${filename}`, await res.blob());
+
+ return {
</file context>
There was a problem hiding this comment.
7 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/validateUrl.test.ts">
<violation number="1" location="tests/validateUrl.test.ts:11">
P1: These expectations enshrine an SSRF-prone contract: an enabled URL upload can use the server to fetch `localhost` and private-network resources. Remove the private-host allow cases and reject resolved loopback, link-local, private, and other internal destinations before every fetch and redirect.</violation>
</file>
<file name="public/results.js">
<violation number="1" location="public/results.js:31">
P2: After the first attempt caches the file, this branch skips `isFetching = true`, so rapid clicks can start concurrent `navigator.share` calls. Set `isFetching` before the cache check and leave `finally` to clear it.</violation>
</file>
<file name="entrypoint.sh">
<violation number="1" location="entrypoint.sh:5">
P2: When `MAGICK_MAX_WIDTH` or `MAGICK_MAX_HEIGHT` is set to `0`, this check accepts it despite requiring a positive integer, then writes a zero ImageMagick dimension limit. Reject zero values so invalid configuration fails at startup rather than making normal image conversions exceed the limit.</violation>
</file>
<file name="src/pages/url.tsx">
<violation number="1" location="src/pages/url.tsx:15">
P2: When URL uploads are disabled, throwing a plain `Error` changes the response into an unhandled 500 instead of the 403 set immediately above. Return an error body after setting the status, or throw Elysia’s explicit 403 response.</violation>
<violation number="2" location="src/pages/url.tsx:59">
P2: When the origin sends headers and then stalls the response body, this clears the abort timer before `res.blob()` consumes that body, so the request can hang indefinitely. Keep the timer active through body consumption, or apply a separate timeout to the response stream.</violation>
</file>
<file name="src/helpers/getFilename.ts">
<violation number="1" location="src/helpers/getFilename.ts:12">
P2: When a response uses a valid RFC 5987 language tag, this regex ignores the supplied filename and falls back to the URL name. Allow the optional language token between `UTF-8'` and the encoded value.</violation>
<violation number="2" location="src/helpers/getFilename.ts:47">
P2: When the sanitized response filename is already 255 bytes and the MIME type supplies an extension, this appends the extension after truncation and makes the URL upload fail. Keep the final basename within 255 bytes, for example by falling back to a UUID when the suffix would exceed the limit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }); | ||
|
|
||
| test("allows local IP and LAN addresses for self-hosted usage", () => { | ||
| expect(() => validateSafeUrl("http://localhost:3000/test.jpg")).not.toThrow(); |
There was a problem hiding this comment.
P1: These expectations enshrine an SSRF-prone contract: an enabled URL upload can use the server to fetch localhost and private-network resources. Remove the private-host allow cases and reject resolved loopback, link-local, private, and other internal destinations before every fetch and redirect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/validateUrl.test.ts, line 11:
<comment>These expectations enshrine an SSRF-prone contract: an enabled URL upload can use the server to fetch `localhost` and private-network resources. Remove the private-host allow cases and reject resolved loopback, link-local, private, and other internal destinations before every fetch and redirect.</comment>
<file context>
@@ -0,0 +1,36 @@
+ });
+
+ test("allows local IP and LAN addresses for self-hosted usage", () => {
+ expect(() => validateSafeUrl("http://localhost:3000/test.jpg")).not.toThrow();
+ expect(() => validateSafeUrl("http://192.168.1.50/file.png")).not.toThrow();
+ expect(() => validateSafeUrl("http://10.0.0.5/doc.pdf")).not.toThrow();
</file context>
| if (!cachedFile) { | ||
| isFetching = true; |
There was a problem hiding this comment.
P2: After the first attempt caches the file, this branch skips isFetching = true, so rapid clicks can start concurrent navigator.share calls. Set isFetching before the cache check and leave finally to clear it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At public/results.js, line 31:
<comment>After the first attempt caches the file, this branch skips `isFetching = true`, so rapid clicks can start concurrent `navigator.share` calls. Set `isFetching` before the cache check and leave `finally` to clear it.</comment>
<file context>
@@ -21,17 +21,31 @@ const setupShareButtons = () => {
- const blob = await response.blob();
- const file = new File([blob], filename, { type: mimeType });
- await navigator.share({ files: [file] });
+ if (!cachedFile) {
+ isFetching = true;
+ const response = await fetch(fileUrl);
</file context>
| if (!cachedFile) { | |
| isFetching = true; | |
| isFetching = true; | |
| if (!cachedFile) { |
| set -e | ||
|
|
||
| if [ -n "$MAGICK_MAX_WIDTH" ]; then | ||
| if ! [[ "$MAGICK_MAX_WIDTH" =~ ^[0-9]+$ ]]; then |
There was a problem hiding this comment.
P2: When MAGICK_MAX_WIDTH or MAGICK_MAX_HEIGHT is set to 0, this check accepts it despite requiring a positive integer, then writes a zero ImageMagick dimension limit. Reject zero values so invalid configuration fails at startup rather than making normal image conversions exceed the limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At entrypoint.sh, line 5:
<comment>When `MAGICK_MAX_WIDTH` or `MAGICK_MAX_HEIGHT` is set to `0`, this check accepts it despite requiring a positive integer, then writes a zero ImageMagick dimension limit. Reject zero values so invalid configuration fails at startup rather than making normal image conversions exceed the limit.</comment>
<file context>
@@ -1,20 +1,44 @@
set -e
+if [ -n "$MAGICK_MAX_WIDTH" ]; then
+ if ! [[ "$MAGICK_MAX_WIDTH" =~ ^[0-9]+$ ]]; then
+ echo "Error: MAGICK_MAX_WIDTH must be a positive integer in pixels" >&2
+ exit 1
</file context>
| async ({ body, redirect, user, cookie: { jobId }, set }) => { | ||
| if (!ALLOW_URL_UPLOAD) { | ||
| set.status = 403; | ||
| throw new Error("URL upload is disabled"); |
There was a problem hiding this comment.
P2: When URL uploads are disabled, throwing a plain Error changes the response into an unhandled 500 instead of the 403 set immediately above. Return an error body after setting the status, or throw Elysia’s explicit 403 response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pages/url.tsx, line 15:
<comment>When URL uploads are disabled, throwing a plain `Error` changes the response into an unhandled 500 instead of the 403 set immediately above. Return an error body after setting the status, or throw Elysia’s explicit 403 response.</comment>
<file context>
@@ -1,39 +1,20 @@
+ async ({ body, redirect, user, cookie: { jobId }, set }) => {
+ if (!ALLOW_URL_UPLOAD) {
+ set.status = 403;
+ throw new Error("URL upload is disabled");
+ }
+
</file context>
| throw new Error("URL upload is disabled"); | |
| return { message: "URL upload is disabled" }; |
| break; | ||
| } | ||
| } finally { | ||
| clearTimeout(timeout); |
There was a problem hiding this comment.
P2: When the origin sends headers and then stalls the response body, this clears the abort timer before res.blob() consumes that body, so the request can hang indefinitely. Keep the timer active through body consumption, or apply a separate timeout to the response stream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pages/url.tsx, line 59:
<comment>When the origin sends headers and then stalls the response body, this clears the abort timer before `res.blob()` consumes that body, so the request can hang indefinitely. Keep the timer active through body consumption, or apply a separate timeout to the response stream.</comment>
<file context>
@@ -48,12 +29,47 @@ export const url = new Elysia().use(userService).post(
+ break;
+ }
+ } finally {
+ clearTimeout(timeout);
+ }
+
</file context>
| let candidate = ""; | ||
| const contentDisposition = headers.get("Content-Disposition"); | ||
| if (contentDisposition) { | ||
| const utf8Match = /filename\*=UTF-8''([^;\s]+)/i.exec(contentDisposition); |
There was a problem hiding this comment.
P2: When a response uses a valid RFC 5987 language tag, this regex ignores the supplied filename and falls back to the URL name. Allow the optional language token between UTF-8' and the encoded value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/helpers/getFilename.ts, line 12:
<comment>When a response uses a valid RFC 5987 language tag, this regex ignores the supplied filename and falls back to the URL name. Allow the optional language token between `UTF-8'` and the encoded value.</comment>
<file context>
@@ -0,0 +1,51 @@
+ let candidate = "";
+ const contentDisposition = headers.get("Content-Disposition");
+ if (contentDisposition) {
+ const utf8Match = /filename\*=UTF-8''([^;\s]+)/i.exec(contentDisposition);
+ if (utf8Match && utf8Match[1]) {
+ try {
</file context>
| } | ||
|
|
||
| if (!sanitized.includes(".") && extension) { | ||
| return `${sanitized}.${extension}`; |
There was a problem hiding this comment.
P2: When the sanitized response filename is already 255 bytes and the MIME type supplies an extension, this appends the extension after truncation and makes the URL upload fail. Keep the final basename within 255 bytes, for example by falling back to a UUID when the suffix would exceed the limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/helpers/getFilename.ts, line 47:
<comment>When the sanitized response filename is already 255 bytes and the MIME type supplies an extension, this appends the extension after truncation and makes the URL upload fail. Keep the final basename within 255 bytes, for example by falling back to a UUID when the suffix would exceed the limit.</comment>
<file context>
@@ -0,0 +1,51 @@
+ }
+
+ if (!sanitized.includes(".") && extension) {
+ return `${sanitized}.${extension}`;
+ }
+
</file context>
Summary by cubic
Adds opt-in URL imports, folder drag-and-drop, and file sharing on the results page. Filename sanitization now gives way to safe-path validation, preserving nested paths while rejecting traversal.
ALLOW_URL_UPLOADis off by default and controls both the URL field and POST/url./download/:userId/:jobId/*; clients that hardcode the old route must update."Unsafe filename"while allowing nested paths.entrypoint.sh, which validates ImageMagick dimension values and applies them to its policy.Written for commit 917c118. Summary will update on new commits.