fix: harden MCP and supply chain security - #6
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds MCP-first behavior and Wavespeed-focused security: debug logging, model-path and request-id encoding/validation, API base URL hostname and network checks, robust image I/O (secure downloads, local-file support, per-index save results), MCP tool wiring/output root options, tests, docs, and CI/workflow pinning. ChangesWavespeed MCP Platform with Security Hardening
Possibly Related PRs
🚥 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5696cc1928
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| filePath: string, | ||
| options: LocalImageReadOptions = {}, | ||
| ): Promise<string> { | ||
| const info = await lstat(filePath); |
There was a problem hiding this comment.
When WAVESPEED_MCP_INPUT_DIR is set and an MCP client supplies a relative image path such as foo.png, this lstat(filePath) still checks the server's current working directory before the later root containment check. As a result, files that exist under the configured input root are reported as missing unless the server happens to be started from that same directory, which breaks the advertised “allow local image reads only under that directory” mode for relative paths. Resolve relative paths against options.rootDir before stat/realpath validation.
Useful? React with 👍 / 👎.
![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 (2)
tests/utils/images.test.ts (2)
123-131: ⚡ Quick winTighten mock-server route matching to avoid false positives.
Line 123 uses substring checks, so
/invalid.pngwould be treated as valid and could hide regressions in failure-path tests.Proposed diff
- server = createServer(async (req, res) => { - if (req.url?.includes("valid") || req.url?.includes("image")) { + server = createServer(async (req, res) => { + const pathname = new URL(req.url ?? "/", "http://127.0.0.1").pathname; + if (pathname === "/valid.png" || pathname === "/image1.png" || pathname === "/image2.png") { const pngData = await readFile(testImagePath); res.writeHead(200, { "content-type": "image/png" }); res.end(pngData); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/images.test.ts` around lines 123 - 131, The mock HTTP handler in tests/utils/images.test.ts is matching routes with substring checks (req.url?.includes("valid") || req.url?.includes("image")), which causes false positives like "/invalid.png"; change the route matching to strict checks (e.g., exact path comparison or anchored regex) against the known test route(s) that should return the image (reference the request handler using req.url and the testImagePath variable) so only the intended URL(s) trigger the 200 PNG response and all other URLs fall through to the 404 response.
211-213: ⚡ Quick winUse an explicit absolute outside path for output-root rejection.
Line 212 currently depends on
"../outside"resolution relative to process CWD. Making it explicitly absolute keeps this test deterministic across runners.Proposed diff
it("should reject output directories outside a configured root", async () => { const base64 = await convertFileToBase64(testImagePath); + const outsideDir = path.resolve(outputDir, "..", "outside"); await expect( - saveImagesFromOutputs([base64], "../outside", "task", { outputRoot: outputDir }), + saveImagesFromOutputs([base64], outsideDir, "task", { outputRoot: outputDir }), ).rejects.toThrow("Output directory must stay within configured output root"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/images.test.ts` around lines 211 - 213, Update the test that calls saveImagesFromOutputs to pass an explicit absolute "outside" path instead of the relative "../outside" so the assertion is deterministic across runners; construct the outside path with Node's path.resolve using the test's outputDir (e.g. path.resolve(outputDir, '..', 'outside')) and pass that resolved absolute path to saveImagesFromOutputs while keeping the same expectation about rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 23-24: Add persist-credentials: false to both actions/checkout
usages to prevent the GITHUB_TOKEN from being automatically persisted to the
workspace; specifically update the checkout step in the test job (the
actions/checkout@... step referenced as the "Checkout code" step) and the
checkout step in the release job (the actions/checkout@... step that runs before
steps requiring write/id-token permissions) by adding the persist-credentials:
false input so the token with contents: write / id-token: write is not left in
the working directory.
In @.github/workflows/test.yml:
- Around line 16-17: The checkout steps using actions/checkout (e.g., the step
with "uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5" and the
second checkout invocation later in the workflow) must include "with:
persist-credentials: false" so the runner does not persist GITHUB_TOKEN into git
config; update both checkout steps to add a "with" block containing
persist-credentials: false.
In `@src/config/models.ts`:
- Around line 126-136: The hostname check in isLocalOrPrivateHost doesn't
canonicalize trailing dots nor checks resolved A/AAAA records; update
isLocalOrPrivateHost to strip and normalize trailing dots (e.g., remove a final
"." after toLowerCase()), then perform a DNS lookup (resolve4/resolve6 or
resolve) for the hostname and run net.isIP + isPrivateIPv4/isPrivateIPv6 against
each resolved address; retain the existing literal checks for "localhost" and
".localhost" but ensure the function returns true if any resolved IP is
private/loopback. Use the same logic where similar checks appear (lines
referenced around 163-168) so both literal and resolved addresses are validated
before allowing the host.
In `@src/utils/images.ts`:
- Around line 186-193: The save helpers (e.g., saveBase64Image) currently force
a .png filename but the decoded image bytes include the real MIME type; update
saveBase64Image to derive the file extension from the detected MIME (use the
MIME returned by decodeBase64Image or otherwise detect it) — map
image/jpeg→.jpg, image/png→.png, image/gif→.gif, image/webp→.webp,
image/bmp→.bmp — and pass a dest filename with that extension into
writeUniqueFile instead of hardcoding .png; apply the same change to the other
save helpers referenced (the save functions around the other blocks noted) so
filenames match the actual image MIME.
- Around line 516-523: The code that checks normalizedContentType (using
getHeader and redactUrl) throws an Error when the response is not an image but
does not consume or destroy res.body, which can leak sockets; before throwing
the Error for non-image content types, ensure you tear down the response body
the same way the non-2xx branch does (e.g., call
res.body.cancel()/destroy()/read and discard the stream) so the socket is
released, then throw the Error including redactUrl(url).
In `@src/utils/logging.ts`:
- Around line 21-41: The fallback path in redactUrl currently only strips query
strings and can leak credentials for malformed URLs; update the catch block of
redactUrl to also detect and redact credentials in the authority portion before
returning (e.g., remove or replace any "username:password@" segment), then
proceed to redact the query string as now; target the redactUrl function to
perform a regex-based or string-based replacement of credentials (look for the
pattern "//...@") prior to slicing/returning so malformed URLs cannot expose
user/password in logs.
In `@src/utils/validation.ts`:
- Around line 49-78: The guard that decides to run the data-URI splitting uses a
lowercase-only substring check (if (s.includes("data:image/"))), so uppercase or
mixed-case data URIs are missed; change that guard to be case-insensitive (e.g.,
use a case-insensitive regex like /data:image\//i.test(s) or compare
s.toLowerCase().includes("data:image/")) so the existing case-insensitive
dataUriPattern and the parsing logic around s.matchAll, items, and
isDataUriImage will correctly detect and split DATA URIs regardless of case.
---
Nitpick comments:
In `@tests/utils/images.test.ts`:
- Around line 123-131: The mock HTTP handler in tests/utils/images.test.ts is
matching routes with substring checks (req.url?.includes("valid") ||
req.url?.includes("image")), which causes false positives like "/invalid.png";
change the route matching to strict checks (e.g., exact path comparison or
anchored regex) against the known test route(s) that should return the image
(reference the request handler using req.url and the testImagePath variable) so
only the intended URL(s) trigger the 200 PNG response and all other URLs fall
through to the 404 response.
- Around line 211-213: Update the test that calls saveImagesFromOutputs to pass
an explicit absolute "outside" path instead of the relative "../outside" so the
assertion is deterministic across runners; construct the outside path with
Node's path.resolve using the test's outputDir (e.g. path.resolve(outputDir,
'..', 'outside')) and pass that resolved absolute path to saveImagesFromOutputs
while keeping the same expectation about rejection.
🪄 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: 4cc94912-e5b1-4485-b3ac-1ff37528eac9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.github/dependabot.yml.github/workflows/junie.yml.github/workflows/release.yml.github/workflows/test.yml.gitignore.releaserc.jsonCHANGELOG.mdREADME.mdWARP.mdpackage.jsonskills/wavespeed-image-generation/SKILL.mdsrc/api/client.tssrc/api/types.tssrc/commands/mcp.tssrc/commands/models.tssrc/config/models.tssrc/core/operations.tssrc/core/output-formatter.tssrc/mcp/tools.tssrc/utils/images.tssrc/utils/logging.tssrc/utils/polling.tssrc/utils/validation.tstests/api/client.test.tstests/commands/cli.test.tstests/config/models.test.tstests/core/output-formatter.test.tstests/utils/images.test.tstests/utils/validation.test.ts
💤 Files with no reviewable changes (1)
- WARP.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/images.ts (2)
657-665:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftValidate
outputRootbefore creating directories.
ensureOutputDir(resolvedOutputDir)on Line 658 can already create directories through a symlinked intermediate component before therealpathcheck on Lines 659-665 runs. With a pre-existing symlink under the configured root, this mutates the filesystem outsideoutputRootand only fails afterwards.Please move the confinement validation ahead of
mkdirby resolving/checking the nearest existing ancestor first, then keep the post-createrealpathcheck as a defense-in-depth revalidation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/images.ts` around lines 657 - 665, The current flow calls ensureOutputDir(resolvedOutputDir) before verifying confinement, which can create directories through symlinks; change the order in the block that uses resolveOutputDir/resolvedOutputDir so you first validate that the nearest existing ancestor of resolvedOutputDir is within options.outputRoot by calling realpath on path.resolve(options.outputRoot) and realpath on the nearest existing parent (walk up from resolvedOutputDir until fs.existsSync or similar), use isSubpath(outputRealAncestor, rootRealPath) to enforce the confinement, and only then call ensureOutputDir(resolvedOutputDir); keep the existing post-create realpath/isSubpath check as a defense-in-depth revalidation.
566-583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-root inputs before
lstat.Line 570 touches
candidatePathbefore therootDirconfinement check on Lines 584-589. That means absolute paths or..escapes can still probe file existence/type/size outside the allowed root via different errors, even though the read is later rejected.Suggested fix
async function resolveValidatedLocalImagePath( filePath: string, options: LocalImageReadOptions = {}, ): Promise<string> { + const root = options.rootDir ? await realpath(options.rootDir) : undefined; const candidatePath = options.rootDir && !path.isAbsolute(filePath) ? path.resolve(options.rootDir, filePath) : filePath; + + if (root && !isSubpath(path.resolve(candidatePath), root)) { + throw new Error(`Image file must stay within configured input root: ${root}`); + } + const info = await lstat(candidatePath); if (!info.isFile()) { throw new Error(`Image path is not a regular file: ${filePath}`); } @@ const resolvedPath = await realpath(candidatePath); - if (options.rootDir) { - const root = await realpath(options.rootDir); + if (root) { if (!isSubpath(resolvedPath, root)) { throw new Error(`Image file must stay within configured input root: ${root}`); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/images.ts` around lines 566 - 583, The code currently calls lstat on candidatePath before enforcing the rootDir confinement; move or add an early check so any input outside options.rootDir is rejected before filesystem probing. Specifically, when options.rootDir is set, compute the absolute candidatePath (using path.resolve) and then use path.relative(options.rootDir, candidatePath) (or equivalent) to detect escapes (reject when the relative path starts with '..' or is absolute) and throw an error if outside the root; only after that safe containment check call lstat, realpath, etc. Reference symbols: candidatePath, options.rootDir, path.resolve, path.relative, lstat, realpath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/images.ts`:
- Around line 657-665: The current flow calls ensureOutputDir(resolvedOutputDir)
before verifying confinement, which can create directories through symlinks;
change the order in the block that uses resolveOutputDir/resolvedOutputDir so
you first validate that the nearest existing ancestor of resolvedOutputDir is
within options.outputRoot by calling realpath on
path.resolve(options.outputRoot) and realpath on the nearest existing parent
(walk up from resolvedOutputDir until fs.existsSync or similar), use
isSubpath(outputRealAncestor, rootRealPath) to enforce the confinement, and only
then call ensureOutputDir(resolvedOutputDir); keep the existing post-create
realpath/isSubpath check as a defense-in-depth revalidation.
- Around line 566-583: The code currently calls lstat on candidatePath before
enforcing the rootDir confinement; move or add an early check so any input
outside options.rootDir is rejected before filesystem probing. Specifically,
when options.rootDir is set, compute the absolute candidatePath (using
path.resolve) and then use path.relative(options.rootDir, candidatePath) (or
equivalent) to detect escapes (reject when the relative path starts with '..' or
is absolute) and throw an error if outside the root; only after that safe
containment check call lstat, realpath, etc. Reference symbols: candidatePath,
options.rootDir, path.resolve, path.relative, lstat, realpath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a66055da-664b-4955-b331-b1d328540072
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/release.yml.github/workflows/test.ymlpackage.jsonsrc/config/models.tssrc/utils/images.tssrc/utils/logging.tssrc/utils/validation.tstests/config/models.test.tstests/utils/images.test.tstests/utils/validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/utils/validation.test.ts
- tests/utils/images.test.ts
- package.json
- tests/config/models.test.ts
- src/utils/logging.ts
- .github/workflows/test.yml
- src/utils/validation.ts
- src/config/models.ts
- .github/workflows/release.yml
1 participant
Summary
bun.lock, add Dependabot, pin CI/release toolchains/actions, use frozen installs, and enable npm provenanceValidation
bun run lintbun test— 102 pass, 4 skippedbun run buildbun audit --json— 0 advisoriesnpm pack --dry-run --json— no suspicious env/token/config filesgit diff --checkNotes
mainto block force-pushes and deletions.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests