fix: resolve all medium-severity code scanning alerts#84
Conversation
Address 35 medium-severity CodeQL alerts across 22 files: **Log injection (js/log-injection) — 21 alerts:** - Add sanitizeForLog() helper to strip control characters and newlines from user-controlled values before they reach log output - Applied across 14 files (auth, registry, lifecycle, storage, plugins) **Permissive CORS configuration (js/cors-permissive-configuration) — 3 alerts:** - Replace wildcard CORS (origin: true / '*') with origin validation callback using CORS_ALLOWED_ORIGINS environment variable **File data in outbound network request (js/file-access-to-http) — 6 alerts:** - Add path.resolve() validation to ensure files stay within expected directories before reading and sending over network **Indirect command line injection (js/indirect-command-line-injection) — 2 alerts:** - Replace execSync with shell interpolation with execFileSync using argument arrays to prevent shell injection **Missing origin verification (js/missing-origin-check) — 2 alerts:** - Add event.origin check in postMessage handlers for plugin frontends **Network data written to file (js/http-to-file-access) — 1 alert:** - Add path traversal validation before writing network data to disk Alerts fixed: #54-56, #78-82, #98-101, #129-149, #277-278, #281-282 Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
github-actions
Bot
added
scope/shell
📝 WalkthroughWalkthroughAdds runtime input validation, path-containment checks for file operations, safer subprocess invocation, CORS allowlist logic, iframe message-origin checks, and widespread log-output sanitization across multiple services and plugins. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }; | ||
|
|
||
| console.log(`Updating stream ${streamId} with:`, JSON.stringify(body, null, 2)); | ||
| console.log(`Updating stream ${sanitizeForLog(streamId)} with:`, JSON.stringify(body, null, 2)); |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
General approach: Ensure that untrusted data is never part of the format string passed as the first argument to console.log / util.format. Instead, make the first argument a fixed literal (or at least not dependent on user input), and pass untrusted values as additional arguments, or pre-format them in a way that does not invoke formatting behavior.
Best fix here: On line 272 in plugins/daydream-video/backend/src/services/daydream.ts, change the call from using a template literal as the first argument:
console.log(`Updating stream ${sanitizeForLog(streamId)} with:`, JSON.stringify(body, null, 2));to using a constant format string with %s and passing the sanitized streamId as a separate argument:
console.log('Updating stream %s with:', sanitizeForLog(streamId), JSON.stringify(body, null, 2));This preserves logging behavior (same information, same order) but ensures the format string is not tainted. sanitizeForLog continues to be used, so log injection via control characters remains mitigated. No other changes are required in the shown snippets; other log calls already use fixed strings or include the untrusted value after the first argument.
No new imports or support methods are needed; we only adjust the existing console.log call in daydream.ts.
| @@ -269,7 +269,7 @@ | ||
| params: updateParams, | ||
| }; | ||
|
|
||
| console.log(`Updating stream ${sanitizeForLog(streamId)} with:`, JSON.stringify(body, null, 2)); | ||
| console.log('Updating stream %s with:', sanitizeForLog(streamId), JSON.stringify(body, null, 2)); | ||
|
|
||
| const response = await fetch(`${DAYDREAM_API}/v1/streams/${streamId}`, { | ||
| method: 'PATCH', |
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| console.error(`Update stream error for ${streamId}:`, response.status, errorText); | ||
| console.error(`Update stream error for ${sanitizeForLog(streamId)}:`, response.status, sanitizeForLog(errorText)); |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
General approach: Avoid passing untrusted data in positions that are interpreted as format strings. For Node’s console.*, this means ensuring the first argument is a fixed string literal and any dynamic or user-controlled data is passed as subsequent arguments (or is concatenated into a string that does not contain % specifiers, but the first-argument rule is simpler and robust).
Best fix here: Change the erroneous console.error call in plugins/daydream-video/backend/src/services/daydream.ts so that the first argument is a static message, and the dynamic template literal becomes a subsequent argument. This preserves the existing information in the logs (message, status, error text, streamId) but removes the possibility that streamId influences formatting. No new imports or helpers are needed; we simply reorder arguments.
Concretely, in updateStreamParams:
- Replace:
console.error(`Update stream error for ${sanitizeForLog(streamId)}:`, response.status, sanitizeForLog(errorText));
- With something like:
or even:
console.error( 'Update stream error', `for ${sanitizeForLog(streamId)}:`, response.status, sanitizeForLog(errorText) );
console.error( 'Update stream error for stream', sanitizeForLog(streamId), 'status:', response.status, 'error:', sanitizeForLog(errorText) );
Either variant ensures the first argument is not tainted and does not depend on streamId, so the format-string warning is resolved without changing functional behavior.
Only plugins/daydream-video/backend/src/services/daydream.ts needs to be edited.
| @@ -282,7 +282,12 @@ | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| console.error(`Update stream error for ${sanitizeForLog(streamId)}:`, response.status, sanitizeForLog(errorText)); | ||
| console.error( | ||
| 'Update stream error for', | ||
| sanitizeForLog(streamId) + ':', | ||
| response.status, | ||
| sanitizeForLog(errorText) | ||
| ); | ||
| throw new Error(`Failed to update stream: ${response.status} ${errorText}`); | ||
| } | ||
|
|
| }); | ||
| } | ||
| console.log(`[publish] Verification passed for ${manifest.name}@${manifest.version}:`, | ||
| console.log(`[publish] Verification passed for ${sanitizeForLog(manifest.name)}@${sanitizeForLog(manifest.version)}:`, |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
General fix approach: Ensure that no untrusted data can affect the format string parameter of console.log (or any formatting API). Instead, use a constant format string (with %s, etc.) and pass any untrusted values as separate arguments, or build a fully self-contained string and pass it alone (without extra arguments).
Best concrete fix here: Change the console.log call on line 533 so that the first argument is a constant string and the dynamic, potentially user-controlled values (manifest.name, manifest.version, and the formatted verification.checks) are passed as subsequent arguments. This satisfies CodeQL’s recommendation while preserving behavior and keeping the existing sanitizeForLog usage.
Currently:
console.log(
`[publish] Verification passed for ${sanitizeForLog(manifest.name)}@${sanitizeForLog(manifest.version)}:`,
verification.checks.map(c => `${c.name}: ${c.passed ? '✓' : '✗'}`).join(', ')
);Proposed change (keeping all existing context and functionality):
console.log(
'[publish] Verification passed for %s@%s: %s',
sanitizeForLog(manifest.name),
sanitizeForLog(manifest.version),
verification.checks.map(c => `${c.name}: ${c.passed ? '✓' : '✗'}`).join(', ')
);Details:
- File:
services/base-svc/src/routes/registry.ts. - Region: Inside the
router.post('/registry/publish', ...)handler, within theif (!skipVerification) { ... }block. - We keep
sanitizeForLogas-is; no new imports or helpers are needed. - We only change the single
console.logcall; no behavior is otherwise altered (the resulting log line is effectively the same, just produced through formatting rather than string concatenation/template literal).
No other lines or files need modification to address this specific CodeQL alert.
| @@ -530,8 +530,12 @@ | ||
| verification: { errors: verification.errors, warnings: verification.warnings, checks: verification.checks }, | ||
| }); | ||
| } | ||
| console.log(`[publish] Verification passed for ${sanitizeForLog(manifest.name)}@${sanitizeForLog(manifest.version)}:`, | ||
| verification.checks.map(c => `${c.name}: ${c.passed ? '✓' : '✗'}`).join(', ')); | ||
| console.log( | ||
| '[publish] Verification passed for %s@%s: %s', | ||
| sanitizeForLog(manifest.name), | ||
| sanitizeForLog(manifest.version), | ||
| verification.checks.map(c => `${c.name}: ${c.passed ? '✓' : '✗'}`).join(', ') | ||
| ); | ||
| } | ||
|
|
||
| const userId = await getUserIdFromRequest(req); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web-next/src/lib/api/auth.ts (1)
759-760:⚠️ Potential issue | 🟡 MinorInconsistent sanitization:
user.emailis not sanitized here.For consistency with the sanitization applied at lines 162 and 667, this log statement should also sanitize
user.email. Although the email is fetched from the database, it originated from user input during registration or OAuth and could contain malicious characters.Proposed fix
// In production, send email. For now, log to console - console.log(`[EMAIL VERIFICATION] Token for ${user.email}: ${token}`); + console.log(`[EMAIL VERIFICATION] Token for ${sanitizeForLog(user.email)}: ${token}`); console.log(`[EMAIL VERIFICATION] Verify URL: /verify-email?token=${token}`);services/storage-svc/src/services/storage.ts (2)
127-131:⚠️ Potential issue | 🔴 CriticalMissing path traversal validation in
deletemethod.The
deletemethod lacks the same path containment check applied touploadanddownload. An attacker could potentially delete arbitrary files outside the storage directory using path traversal (e.g.,../../../etc/important-file).🔒 Proposed fix to add path validation
async delete(filePath: string): Promise<void> { const fullPath = path.join(basePath, filePath); + // Validate that the resolved path stays within the storage base directory + const resolvedFull = path.resolve(fullPath); + if (!resolvedFull.startsWith(path.resolve(basePath) + path.sep)) { + throw new Error('File path outside expected storage directory'); + } await fs.unlink(fullPath).catch(() => {}); await fs.unlink(`${fullPath}.meta.json`).catch(() => {}); },
137-177:⚠️ Potential issue | 🟠 MajorMissing path traversal validation in
exists,list, andgetMetadatamethods.These methods also accept user-provided paths and interact with the filesystem without containment checks:
exists(line 138): Could probe for files outside storage directorylist(line 148): Could enumerate directories outside storagegetMetadata(lines 160, 167): Could leak file metadata outside storageFor defense in depth and consistency, apply the same validation pattern to all methods that handle user-provided paths.
🔒 Proposed fix to add validation to remaining methods
async exists(filePath: string): Promise<boolean> { const fullPath = path.join(basePath, filePath); + const resolvedFull = path.resolve(fullPath); + if (!resolvedFull.startsWith(path.resolve(basePath) + path.sep)) { + throw new Error('File path outside expected storage directory'); + } try { await fs.access(fullPath); return true; } catch { return false; } }, async list(prefix: string): Promise<string[]> { const fullPath = path.join(basePath, prefix); + const resolvedFull = path.resolve(fullPath); + if (!resolvedFull.startsWith(path.resolve(basePath) + path.sep) && resolvedFull !== path.resolve(basePath)) { + throw new Error('File path outside expected storage directory'); + } try { const entries = await fs.readdir(fullPath, { recursive: true }); return entries .filter(e => !e.endsWith('.meta.json')) .map(e => path.join(prefix, e.toString())); } catch { return []; } }, async getMetadata(filePath: string): Promise<FileMetadata | null> { const metaPath = path.join(basePath, `${filePath}.meta.json`); + const resolvedMeta = path.resolve(metaPath); + if (!resolvedMeta.startsWith(path.resolve(basePath) + path.sep)) { + throw new Error('File path outside expected storage directory'); + } try { const data = await fs.readFile(metaPath, 'utf-8');plugins/daydream-video/backend/src/services/daydream.ts (1)
272-290:⚠️ Potential issue | 🟡 MinorSanitization applied correctly, but inconsistent with
createStream.The sanitization of
streamIdanderrorTextinupdateStreamParamsis correct. However, the same pattern is not applied increateStreamat line 221:console.error('Create stream error:', response.status, errorText); // unsanitizedIf
errorTextfrom an API response can contain malicious control characters inupdateStreamParams, the same risk exists increateStream. Apply sanitization consistently across all functions that log external data.Suggested fix for line 221
- console.error('Create stream error:', response.status, errorText); + console.error('Create stream error:', response.status, sanitizeForLog(errorText));
🤖 Fix all issues with AI agents
In `@apps/web-next/src/app/api/v1/tenant/installations/`[id]/config/route.ts:
- Line 24: The console.log currently prints user-controlled settings which can
leak sensitive data or allow log injection; update the log to sanitize or redact
settings before outputting by applying the existing sanitizeForLog helper (or a
similar sanitizer) to the settings payload (e.g., stringify then sanitize) or by
extracting and logging only safe metadata (e.g., keys/size/version), and replace
the raw settings in the console.log call that includes sanitizeForLog(id) so
only the sanitized/redactedSettings is logged alongside the sanitized id.
In `@packages/plugin-server-sdk/src/server.ts`:
- Around line 108-124: The CORS allowlist currently treats an empty originsArray
as permissive; update the origin list normalization and the cors origin callback
in the server initialization so an empty or unset allowlist denies all origins
by default unless an explicit wildcard '*' is provided (or explicitly allow-all
only when NODE_ENV !== 'production'); specifically, normalize configuredOrigins
from CORS_ALLOWED_ORIGINS / corsOrigins by trimming and filtering each entry,
detect a literal '*' to enable wildcard behavior, and change the origin callback
(the function passed into app.use(cors({ origin: ... }))) to reject when
originsArray is empty and origin is present, allow when originsArray includes
the origin or when wildcard is set, and still allow requests with no origin
(server-to-server).
In `@plugins/daydream-video/frontend/src/main.tsx`:
- Around line 28-30: The outgoing postMessage that sends the 'plugin:ready'
event currently uses '*' as targetOrigin; change it to the validated origin (do
not broadcast to any origin). Specifically, replace the postMessage call that
sends 'plugin:ready' (the window.postMessage(..., '*') invocation) to use the
validated origin (e.g., window.location.origin or a stored allowedOrigin
captured after verifying event.origin in the message handler) so the message is
only delivered to the trusted origin that passed the incoming event.origin
check.
In `@services/base-svc/src/routes/lifecycle.ts`:
- Around line 596-598: The log call in executeS3Call currently passes raw args
which can contain control characters or newlines leading to log injection;
before logging, safely serialize and sanitize args (e.g., JSON.stringify with a
size limit and replacer to remove/escape control chars or call sanitizeForLog on
the serialized string) and log that sanitized string instead of raw args; update
executeS3Call to produce a safeArgs string and use it in the console.log and any
returned warning context.
In `@services/base-svc/src/services/auth.ts`:
- Around line 11-14: The sanitizeForLog function currently strips ASCII control
chars but misses Unicode line separators U+2028 and U+2029; update
sanitizeForLog to also remove those characters (e.g., include \u2028 and \u2029
in the regex used in sanitizeForLog) so newline/line-separator characters cannot
bypass the log sanitization.
- Around line 644-646: The two console.log calls that print the password reset
token and URL should not emit secret tokens in production; locate the code in
services/base-svc/src/services/auth.ts where console.log is used (the lines
using sanitizeForLog(email) and printing `token`), remove or wrap these logs
behind a non-production check (e.g., if (process.env.NODE_ENV !== 'production'))
or redact the token (show only a token fingerprint/first/last chars) and route
non-sensitive notifications through a proper mail/send flow instead of logging;
ensure sanitizeForLog(email) continues to be used for the email but never log
the full token in production.
In `@services/plugin-server/src/server.ts`:
- Around line 195-205: The current origin handler allows any origin when
CORS_ALLOWED_ORIGINS is empty; change it to "fail closed" by rejecting requests
if CORS_ALLOWED_ORIGINS.length === 0 (still allow requests with no origin for
server-to-server/curl by keeping the !origin check). Update the
app.use(cors(...)) origin function used in server.ts so that if the allowlist is
empty you call callback(new Error('CORS allowlist not configured')) (or
similarly reject) instead of returning true; optionally permit a non-production
override by checking process.env.NODE_ENV !== 'production' before allowing a
default permissive behavior.
🧹 Nitpick comments (12)
plugins/daydream-video/backend/src/server.ts (1)
14-17: Preserve error stack safely while sanitizing.
String(error)drops stack traces, which makes debugging harder. Consider special-casingErrorand sanitizingstack(ormessage) while preserving readability (e.g., replace control chars with spaces).Proposed adjustment
function sanitizeForLog(value: unknown): string { - return String(value).replace(/[\n\r\t\x00-\x1f\x7f-\x9f]/g, ''); + if (value instanceof Error) { + const payload = value.stack ?? value.message; + return payload.replace(/[\n\r\t\x00-\x1f\x7f-\x9f]/g, ' '); + } + return String(value).replace(/[\n\r\t\x00-\x1f\x7f-\x9f]/g, ' '); }plugins/my-wallet/backend/src/server.ts (1)
152-152: Consider sanitizing validated values for defense-in-depth.While
addressandchainIdare validated before logging, applyingsanitizeForLoghere would provide consistent defense-in-depth. The same applies to line 280 wheretxHashandtypeare logged.This is optional since the input validation already constrains the format, but would align with the sanitization pattern used elsewhere.
♻️ Optional refactor for consistency
- console.log(`Wallet connection saved: ${address} on chain ${chainId}`); + console.log(`Wallet connection saved: ${sanitizeForLog(address)} on chain ${sanitizeForLog(chainId)}`);And at line 280:
- console.log(`Transaction logged: ${txHash} (${type})`); + console.log(`Transaction logged: ${sanitizeForLog(txHash)} (${sanitizeForLog(type)})`);services/plugin-server/src/server.ts (1)
195-195: Trim allowlist entries to avoid false negatives.
split(',')without trimming can leave leading/trailing spaces, causing valid origins to be rejected.🧹 Suggested refinement
-const CORS_ALLOWED_ORIGINS = (process.env.CORS_ALLOWED_ORIGINS || '').split(',').filter(Boolean); +const CORS_ALLOWED_ORIGINS = (process.env.CORS_ALLOWED_ORIGINS || '') + .split(',') + .map(origin => origin.trim()) + .filter(Boolean);packages/plugin-server-sdk/src/middleware/auth.ts (2)
18-23: Move the function definition below the imports.The
sanitizeForLogfunction is defined before theimportstatements. While TypeScript allows this due to hoisting, it's unconventional and can confuse readers expecting imports at the top of the file.Suggested reordering
-/** Sanitize a value for safe log output (prevents log injection) */ -function sanitizeForLog(value: unknown): string { - return String(value).replace(/[\n\r\t\x00-\x1f\x7f-\x9f]/g, ''); -} - import type { Request, Response, NextFunction } from 'express'; + +/** Sanitize a value for safe log output (prevents log injection) */ +function sanitizeForLog(value: unknown): string { + return String(value).replace(/[\n\r\t\x00-\x1f\x7f-\x9f]/g, ''); +}
211-212: Consider sanitizingerrorBodyfor consistency.While
errorBodyoriginates from the auth service response rather than direct user input, network data can still contain control characters. For consistent log hygiene, consider applyingsanitizeForLoghere as well.Optional improvement
const errorBody = await response.text().catch(() => 'no body'); - console.log(`[auth] Auth service rejected token: HTTP ${response.status} - ${errorBody}`); + console.log(`[auth] Auth service rejected token: HTTP ${response.status} - ${sanitizeForLog(errorBody)}`);apps/web-next/src/app/api/v1/tenant/installations/[id]/config/route.ts (1)
4-7: Consider centralizingsanitizeForLogto a shared util.Multiple files in this PR add similar helpers; a shared utility avoids drift and keeps sanitization consistent across the codebase.
services/pipeline-gateway/src/adapters/BYOCAdapter.ts (1)
46-51: Consider sanitizing error messages for downstream log safety.While thrown errors aren't direct log statements, if error handlers upstream log these messages, unsanitized
pipelineNamecould still introduce log injection. The same applies to line 67 where the raw responseerroris interpolated.This may be low priority if your error handlers already sanitize, but worth verifying.
Example fix for error messages
const cap = this.capabilities.get(pipelineName); if (!cap) { - throw new Error(`BYOC capability '${pipelineName}' not registered`); + throw new Error(`BYOC capability '${sanitizeForLog(pipelineName)}' not registered`); } if (!cap.healthy) { - throw new Error(`BYOC capability '${pipelineName}' is currently unhealthy`); + throw new Error(`BYOC capability '${sanitizeForLog(pipelineName)}' is currently unhealthy`); }And at line 67:
- throw new Error(`BYOC endpoint returned ${response.status}: ${error}`); + throw new Error(`BYOC endpoint returned ${response.status}: ${sanitizeForLog(error)}`);plugins/daydream-video/backend/src/services/daydream.ts (1)
8-11: Consider extractingsanitizeForLogto a shared utility module.The function implementation is correct and adequately strips control characters to prevent log injection. However, per the PR summary, this same helper is being added across ~14 files. Duplicating the implementation increases maintenance burden and risks inconsistent sanitization if one copy is updated without the others.
Consider extracting this to a shared utility (e.g.,
@naap/common/loggingor similar) and importing it where needed.packages/plugin-sdk/cli/commands/publish.ts (1)
73-80: Credentials path validation is correct; optional DRY refactor available.The validation pattern is repeated from lines 50-51. Consider extracting a helper function to reduce duplication and ensure consistent behavior.
♻️ Optional helper extraction
Add a helper function at the module level:
function isPathWithinDirectory(targetPath: string, baseDir: string): boolean { const resolved = path.resolve(targetPath); return resolved.startsWith(path.resolve(baseDir) + path.sep); }Then simplify the checks:
- const resolvedManifestPath = path.resolve(manifestPath); - if (!resolvedManifestPath.startsWith(path.resolve(cwd) + path.sep)) { + if (!isPathWithinDirectory(manifestPath, cwd)) { console.error(chalk.red('Error: manifest path outside working directory')); process.exit(1); }- const resolvedCredPath = path.resolve(credentialsPath); - if (resolvedCredPath.startsWith(path.resolve(cwd) + path.sep) && await fs.pathExists(credentialsPath)) { + if (isPathWithinDirectory(credentialsPath, cwd) && await fs.pathExists(credentialsPath)) {services/base-svc/src/routes/lifecycle.ts (1)
12-15: Consider centralizingsanitizeForLogto avoid drift across modules.Given the repeated pattern across the codebase, a shared log-sanitization utility would reduce maintenance and keep behavior consistent.
apps/web-next/src/app/api/v1/plugin-publisher/upload/route.ts (1)
130-176: Consider adding path traversal validation for consistency with publish-cdn route.The
publish-cdnroute validates thatbundlePathandstylesPathare withinextractDirusingpath.resolve()checks (lines 234-247 in that file). This upload route performs similar file operations—copying directories toSTATIC_DIR—but lacks equivalent validation.A malicious archive could potentially contain symlinks or crafted paths. Consider adding similar path containment checks before the
cp()operations.Example validation pattern
const resolvedExtractDir = path.resolve(extractDir); const resolvedDir = path.resolve(dir); if (!resolvedDir.startsWith(resolvedExtractDir + path.sep)) { await rm(extractDir, { recursive: true }); return errors.badRequest('Invalid path in archive'); }plugins/plugin-publisher/backend/src/server.ts (1)
297-345: The/uploadendpoint lacks path traversal validation present in/publish-cdn.The
/publish-cdnendpoint (lines 648-657) validates thatbundlePathandstylesPathare withinextractDirbefore reading. However, the/uploadendpoint reads and copies files from the extracted archive without similar validation (e.g., at lines 305, 313, 331, 337).For consistency, consider adding equivalent path containment checks before
copyDir()and file read operations in this endpoint.
| // CORS - validate origins against allowlist | ||
| const configuredOrigins = corsOrigins | ||
| || (process.env.CORS_ALLOWED_ORIGINS || '').split(',').filter(Boolean); | ||
| const originsArray: string[] = Array.isArray(configuredOrigins) | ||
| ? configuredOrigins | ||
| : (typeof configuredOrigins === 'string' && configuredOrigins !== '*' | ||
| ? [configuredOrigins] | ||
| : []); | ||
| app.use(cors({ | ||
| origin: origins, | ||
| origin: (origin, callback) => { | ||
| // Allow requests with no origin (server-to-server, curl, etc.) | ||
| if (!origin) return callback(null, true); | ||
| if (originsArray.length === 0 || originsArray.includes(origin)) { | ||
| return callback(null, true); | ||
| } | ||
| callback(new Error('Not allowed by CORS')); | ||
| }, |
There was a problem hiding this comment.
Empty allowlist currently behaves as wildcard
Line 120 allows any origin when originsArray.length === 0, so an unset/empty allowlist (or corsOrigins: []) becomes permissive in all environments. That undermines the allowlist intent and can reintroduce the original CORS finding. Consider failing closed unless an explicit '*' is provided (or explicitly allow-all only in non‑prod). Also trim/normalize the allowlist so entries with spaces match correctly.
🔧 Suggested fix
- const configuredOrigins = corsOrigins
- || (process.env.CORS_ALLOWED_ORIGINS || '').split(',').filter(Boolean);
- const originsArray: string[] = Array.isArray(configuredOrigins)
- ? configuredOrigins
- : (typeof configuredOrigins === 'string' && configuredOrigins !== '*'
- ? [configuredOrigins]
- : []);
+ const configuredOrigins =
+ corsOrigins || (process.env.CORS_ALLOWED_ORIGINS || '');
+ const originsArray: string[] = (Array.isArray(configuredOrigins)
+ ? configuredOrigins
+ : (typeof configuredOrigins === 'string'
+ ? configuredOrigins.split(',')
+ : []))
+ .map((o) => o.trim())
+ .filter(Boolean);
+ const allowAllOrigins =
+ typeof configuredOrigins === 'string' && configuredOrigins.trim() === '*';
app.use(cors({
origin: (origin, callback) => {
// Allow requests with no origin (server-to-server, curl, etc.)
if (!origin) return callback(null, true);
- if (originsArray.length === 0 || originsArray.includes(origin)) {
+ if (allowAllOrigins || originsArray.includes(origin)) {
return callback(null, true);
}
callback(new Error('Not allowed by CORS'));
},🤖 Prompt for AI Agents
In `@packages/plugin-server-sdk/src/server.ts` around lines 108 - 124, The CORS
allowlist currently treats an empty originsArray as permissive; update the
origin list normalization and the cors origin callback in the server
initialization so an empty or unset allowlist denies all origins by default
unless an explicit wildcard '*' is provided (or explicitly allow-all only when
NODE_ENV !== 'production'); specifically, normalize configuredOrigins from
CORS_ALLOWED_ORIGINS / corsOrigins by trimming and filtering each entry, detect
a literal '*' to enable wildcard behavior, and change the origin callback (the
function passed into app.use(cors({ origin: ... }))) to reject when originsArray
is empty and origin is present, allow when originsArray includes the origin or
when wildcard is set, and still allow requests with no origin
(server-to-server).
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- Tenant config: sanitize settings before logging (log injection) - plugin-server-sdk CORS: empty allowlist now fails closed unless explicit '*' or dev mode; trim/normalize origins - postMessage (daydream, plugin-publisher): use origin allowlist (window.location.origin + VITE_ALLOWED_MESSAGE_ORIGINS) instead of event.origin !== window.location.origin which broke cross-origin iframes; use event.origin for postMessage target (not wildcard) - lifecycle executeS3Call: sanitize args before logging - sanitizeForLog: add U+2028/U+2029 to prevent log-forging bypass - Password reset: never log token in production (gate behind NODE_ENV) - plugin-server, plugin-publisher CORS: fail closed when allowlist empty in production; allow in dev for local workflow
Avoid breaking Vercel/deployments; empty CORS_ALLOWED_ORIGINS now defaults to allow-all. TODO: tighten later by setting allowlist.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/plugin-publisher/backend/src/server.ts`:
- Around line 461-467: The version regex used to validate version (currently
`/^[\d]+\.[\d]+\.[\d]+/`) lacks an end-anchor and allows inputs like
`1.0.0/../../../...` which can enable path traversal when interpolated into the
blob path `plugins/${pluginName}/${version}/...`; update the `version`
validation to require the full string match (add `$`) and tighten it if needed
(e.g. semantic version pattern). Also make `pluginName` validation consistent
with the existing `KEBAB_CASE_REGEX` used elsewhere (line with KEBAB_CASE_REGEX)
by reusing that shared constant or replacing the local regex so leading digits
are handled the same across the codebase; locate the validations around
`pluginName` and `version` and replace them to use the shared `KEBAB_CASE_REGEX`
and the anchored version regex.
| // Validate plugin name and version to prevent path traversal in blob paths | ||
| if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(pluginName)) { | ||
| throw new Error('Invalid plugin name'); | ||
| } | ||
| if (!/^[\d]+\.[\d]+\.[\d]+/.test(version)) { | ||
| throw new Error('Invalid version format'); | ||
| } |
There was a problem hiding this comment.
Version regex lacks end anchor, allowing path traversal.
The version regex /^[\d]+\.[\d]+\.[\d]+/ has no $ anchor, so values like 1.0.0/../../../etc pass validation. Since version is interpolated into the blob path at line 473 (plugins/${pluginName}/${version}/...), this could enable path traversal in the CDN storage.
Additionally, this pluginName regex differs from KEBAB_CASE_REGEX on line 115—this one allows leading digits while the other doesn't. Consider using a shared constant for consistency.
🛡️ Proposed fix
- if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(pluginName)) {
+ if (!KEBAB_CASE_REGEX.test(pluginName)) {
throw new Error('Invalid plugin name');
}
- if (!/^[\d]+\.[\d]+\.[\d]+/.test(version)) {
+ if (!SEMVER_REGEX.test(version)) {
throw new Error('Invalid version format');
}🤖 Prompt for AI Agents
In `@plugins/plugin-publisher/backend/src/server.ts` around lines 461 - 467, The
version regex used to validate version (currently `/^[\d]+\.[\d]+\.[\d]+/`)
lacks an end-anchor and allows inputs like `1.0.0/../../../...` which can enable
path traversal when interpolated into the blob path
`plugins/${pluginName}/${version}/...`; update the `version` validation to
require the full string match (add `$`) and tighten it if needed (e.g. semantic
version pattern). Also make `pluginName` validation consistent with the existing
`KEBAB_CASE_REGEX` used elsewhere (line with KEBAB_CASE_REGEX) by reusing that
shared constant or replacing the local regex so leading digits are handled the
same across the codebase; locate the validations around `pluginName` and
`version` and replace them to use the shared `KEBAB_CASE_REGEX` and the anchored
version regex.
seanhanca
left a comment
seanhanca
left a comment
There was a problem hiding this comment.
LGTM. All medium-severity CodeQL alerts addressed. Follow-up commit 230d5c6 resolved CodeRabbit feedback (sanitize settings, Unicode line separators, restrict postMessage origins, safe S3 args logging, production token redaction, CORS fail-closed). Vercel review confirmed issues resolved.
3 participants
Summary
ERR_MODULE_NOT_FOUNDfor workspace packages (@naap/plugin-build/dist/*,@naap/cache/dist/*) #129–fix(auth): email verification for signup #149, chore(deps): bump the npm_and_yarn group across 6 directories with 7 updates #277–chore(deps): bump the npm_and_yarn group across 5 directories with 6 updates #278, revert: feat(web-next): align NAAP dashboard and BFF with OpenAPI v1 (streaming + requests)" #281–refactor(dashboard): align BFF data layer with OpenAPI v1 contract and improve job feed #282) across 22 filesChanges by category
sanitizeForLog()strips control chars before loggingpath.resolve()validation before file read + sendCORS_ALLOWED_ORIGINSenv varexecFileSyncwith args array instead ofexecSyncwith interpolationevent.originverification in postMessage handlersTest plan
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Security