feat/platform-v3 #1698
feat/platform-v3 #1698
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
emir-karabeg
force-pushed
the
feat/platform-v3
branch
from
862e9b2 to
a81ec74
Compare
emir-karabeg
force-pushed
the
feat/platform-v3
branch
from
3aa4155 to
42c71dc
Compare
emir-karabeg
force-pushed
the
feat/platform-v3
branch
from
934dce2 to
3ab1f21
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| export async function insertFileMetadata( | ||
| options: FileMetadataInsertOptions | ||
| ): Promise<FileMetadataRecord> { | ||
| const { key, userId, workspaceId, context, originalName, contentType, size, id } = options |
Check failure
Code scanning / CodeQL
Insecure randomness
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, replace the use of Math.random() when generating a random string component for storage keys with a cryptographically secure alternative. In Node.js, the best method is to use crypto.randomBytes to generate random bytes and encode them in base36 or hex for inclusion in file keys.
Specifically, update the generateWorkspaceFileKey function in apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts so that, instead of:
const random = Math.random().toString(36).substring(2, 9)you use:
const random = require('crypto').randomBytes(6).toString('base64url')or, if you prefer to avoid requiring non-standard encodings:
const random = require('crypto').randomBytes(5).toString('hex')This will make the random string cryptographically secure and suitable for security-sensitive identifiers.
You will need to import the crypto module (import * as crypto from 'crypto') at the top of the file, or use require('crypto') if the style is consistent. Only the region around generateWorkspaceFileKey needs to change.
| @@ -42,7 +42,8 @@ | ||
| */ | ||
| export function generateWorkspaceFileKey(workspaceId: string, fileName: string): string { | ||
| const timestamp = Date.now() | ||
| const random = Math.random().toString(36).substring(2, 9) | ||
| const crypto = require('crypto') | ||
| const random = crypto.randomBytes(5).toString('hex') | ||
| const safeFileName = fileName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9.-]/g, '_') | ||
| return `${workspaceId}/${timestamp}-${random}-${safeFileName}` | ||
| } |
| export async function insertFileMetadata( | ||
| options: FileMetadataInsertOptions | ||
| ): Promise<FileMetadataRecord> { | ||
| const { key, userId, workspaceId, context, originalName, contentType, size, id } = options |
Check failure
Code scanning / CodeQL
Insecure randomness
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To ensure predictable keys are not generated, replace Math.random() with a cryptographically secure alternative. For Node.js, the best choice is crypto.randomBytes. We'll generate enough randomness to preserve or improve the entropy previously present. Since Math.random().toString(36).substring(2, 9) generates a 7-character base36 string, we can generate a similar-length base36 string from random bytes.
Steps:
- In
apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts:- Import Node's
cryptomodule (from "crypto"). - In
generateWorkspaceFileKey, replace the assignment torandomwith one that usescrypto.randomBytes(n)(for sufficient entropy) and encodes using base36, slicing as needed to maintain the 7-character output.
- Import Node's
- No functional changes otherwise.
- No changes are needed elsewhere for this error.
| @@ -5,6 +5,7 @@ | ||
|
|
||
| import { db } from '@sim/db' | ||
| import { workspaceFiles } from '@sim/db/schema' | ||
| import * as crypto from 'crypto'; | ||
| import { and, eq } from 'drizzle-orm' | ||
| import { | ||
| checkStorageQuota, | ||
| @@ -42,7 +43,8 @@ | ||
| */ | ||
| export function generateWorkspaceFileKey(workspaceId: string, fileName: string): string { | ||
| const timestamp = Date.now() | ||
| const random = Math.random().toString(36).substring(2, 9) | ||
| // Use crypto to generate a 7 character base36 value (~5 bytes entropy) | ||
| const random = parseInt(crypto.randomBytes(5).toString('hex'), 16).toString(36).substring(0, 7) | ||
| const safeFileName = fileName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9.-]/g, '_') | ||
| return `${workspaceId}/${timestamp}-${random}-${safeFileName}` | ||
| } |
| export async function insertFileMetadata( | ||
| options: FileMetadataInsertOptions | ||
| ): Promise<FileMetadataRecord> { | ||
| const { key, userId, workspaceId, context, originalName, contentType, size, id } = options |
Check failure
Code scanning / CodeQL
Insecure randomness
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix this issue, we should replace the insecure ID generation using Math.random and Date.now() with the generation of a cryptographically secure unique identifier. The most standard and widely-adopted practice is to use a UUID v4, which is specifically designed for this use-case and is not susceptible to the prediction weaknesses of Math.random. This should be implemented where the IDs are generated—in this case, in apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts, specifically on line 74 (the definition of fileId). Instead of constructing the ID with Math.random, import a secure UUID generator (such as the uuid library's v4 function) and use it to generate a unique ID, optionally preserving the wf_ prefix if desired for namespacing.
What is needed:
- Add an import for the
uuidlibrary'sv4method inapps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts. - Change the assignment to
fileIdat line 74 so that it uses this secure ID (e.g.,fileId = 'wf_' + v4();). - Remove the use of
Math.randomin the construction offileId. - No changes are required in
apps/sim/lib/uploads/server/metadata.tsbecause the handling ofidthere is already secure, but the taint comes from thefileIdbeing generated insecurely upstream.
| @@ -20,6 +20,7 @@ | ||
| } from '@/lib/uploads/core/storage-service' | ||
| import { getFileMetadataByKey, insertFileMetadata } from '@/lib/uploads/server/metadata' | ||
| import type { UserFile } from '@/executor/types' | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| const logger = createLogger('WorkspaceFileStorage') | ||
|
|
||
| @@ -71,7 +72,7 @@ | ||
| } | ||
|
|
||
| const storageKey = generateWorkspaceFileKey(workspaceId, fileName) | ||
| let fileId = `wf_${Date.now()}_${Math.random().toString(36).substring(2, 9)}` | ||
| let fileId = `wf_${uuidv4()}` | ||
|
|
||
| try { | ||
| logger.info(`Generated storage key: ${storageKey}`) |
| const { isInternalFileUrl } = await import('./file-utils') | ||
| const { parseInternalFileUrl } = await import('./file-utils') | ||
| const controller = new AbortController() | ||
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs) |
Check failure
Code scanning / CodeQL
Resource exhaustion
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, enforce an upper bound for the timeoutMs parameter to downloadFileFromUrl.
- Add an upper limit (e.g., 3 minutes or a reasonable application-specific maximum, say 300,000ms = 5 minutes) to
timeoutMs. - If the provided value exceeds this maximum (or is not a number or is negative), set it to the default or clamp it within the legitimate range.
- This can be accomplished by inserting logic at the start of the function to coerce or clamp
timeoutMsto the accepted range (e.g., usingMath.min,Math.max). - These changes should be placed at the top of the
downloadFileFromUrlfunction, before the value is used insetTimeout. - No new dependencies are needed.
| @@ -27,6 +27,15 @@ | ||
| * For external URLs, uses HTTP fetch | ||
| */ | ||
| export async function downloadFileFromUrl(fileUrl: string, timeoutMs = 180000): Promise<Buffer> { | ||
| // Clamp timeoutMs to a reasonable range [1000ms, 300000ms] | ||
| const MAX_TIMEOUT_MS = 300_000 // 5 minutes | ||
| const MIN_TIMEOUT_MS = 1_000 // 1 second | ||
| if (typeof timeoutMs !== "number" || !isFinite(timeoutMs)) { | ||
| timeoutMs = 180_000 | ||
| } else { | ||
| timeoutMs = Math.min(Math.max(timeoutMs, MIN_TIMEOUT_MS), MAX_TIMEOUT_MS) | ||
| } | ||
|
|
||
| const { isInternalFileUrl } = await import('./file-utils') | ||
| const { parseInternalFileUrl } = await import('./file-utils') | ||
| const controller = new AbortController() |
| return buffer | ||
| } | ||
|
|
||
| const response = await fetch(fileUrl, { signal: controller.signal }) |
Check failure
Code scanning / CodeQL
Server-side request forgery
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To mitigate SSRF, we must validate that the fileUrl either matches an explicit allow-list of trusted domains or at least restricts protocols and hostname patterns before it can be fetched. The ideal fix will:
- Parse
fileUrland ensure it is an external URL (i.e., not internal: loopback, private, or link-local IPs, and not dangerous protocols). - Reject or throw an error if the URL fails validation.
- Optionally implement an allow-list of domains if business logic permits.
- Should not alter the behavior for valid, allowed, external URLs.
The proper place for the fix is just before using fetch(fileUrl, ...), specifically after internal URLs are handled and before any fetch for external URLs. We need to:
- Add a parsing and verification block between lines 43 and 44 to ensure
fileUrlis strictly an http(s) URL and not a local or private address. - Import (if permitted) a well-known package such as
is-ipornet, but since we can only add standard Node.js imports, use the built-inurlandnetlibraries for IP verification. - If host is an IP, verify it is not a loopback or private address (using
net.isIP,net.isIPv4, and a helper to check RFC1918). - If host is a name, optionally use a simple allow-list, or at minimum ensure it's not "localhost" or similar.
All code changes must be in apps/sim/lib/uploads/utils/file-utils.server.ts.
| @@ -4,7 +4,8 @@ | ||
| import type { StorageContext } from '@/lib/uploads' | ||
| import type { UserFile } from '@/executor/types' | ||
| import { inferContextFromKey } from './file-utils' | ||
|
|
||
| import { parse as parseUrl } from 'url'; | ||
| import * as net from 'net'; | ||
| /** | ||
| * Check if a file is from execution storage based on its key pattern | ||
| * Execution files have keys in format: workspaceId/workflowId/executionId/filename | ||
| @@ -41,6 +42,51 @@ | ||
| return buffer | ||
| } | ||
|
|
||
| // SSRF protection: Only allow external HTTP(S), prevent access to local/private addresses | ||
| let parsed; | ||
| try { | ||
| parsed = new URL(fileUrl); | ||
| } catch { | ||
| throw new Error('Invalid URL'); | ||
| } | ||
| const protocol = parsed.protocol; | ||
| if (protocol !== 'http:' && protocol !== 'https:') { | ||
| throw new Error('Only HTTP(S) protocols are allowed for file downloads.'); | ||
| } | ||
| const hostname = parsed.hostname.toLowerCase(); | ||
| // Block localhost hostnames | ||
| if ( | ||
| hostname === 'localhost' || | ||
| hostname === '127.0.0.1' || | ||
| hostname === '::1' | ||
| ) { | ||
| throw new Error('Refusing to fetch from localhost addresses'); | ||
| } | ||
| // If hostname is an IP, block private IPs (IPv4) | ||
| if (net.isIPv4(hostname)) { | ||
| const parts = hostname.split('.').map(Number); | ||
| // 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 | ||
| if ( | ||
| parts[0] === 10 || | ||
| (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) || | ||
| (parts[0] === 192 && parts[1] === 168) | ||
| ) { | ||
| throw new Error('Refusing to fetch from private network addresses'); | ||
| } | ||
| } | ||
| // If hostname is an IPv6, block link-local/unique/local/multicast/loopback | ||
| if (net.isIPv6(hostname)) { | ||
| if ( | ||
| hostname === '::1' || | ||
| hostname.startsWith('fe80:') || // link-local | ||
| hostname.startsWith('fc00:') || // unique local | ||
| hostname.startsWith('fd00:') // unique local | ||
| ) { | ||
| throw new Error('Refusing to fetch from IPv6 local addresses'); | ||
| } | ||
| } | ||
| // Further allow-listing of domains can be included here if desired | ||
|
|
||
| const response = await fetch(fileUrl, { signal: controller.signal }) | ||
| clearTimeout(timeoutId) | ||
|
|
…djusted button padding
…lt minus active path
…efactor sidebar components
emir-karabeg
force-pushed
the
feat/platform-v3
branch
from
999d4ab to
c866d89
Compare
Summary
Brief description of what this PR does and why.
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist
Screenshots/Videos