fix(security): SSRF protection always-on + path traversal guard on uploads#177
Open
juliendoclot wants to merge 1 commit intodocdyhr:mainfrom
Open
fix(security): SSRF protection always-on + path traversal guard on uploads#177juliendoclot wants to merge 1 commit intodocdyhr:mainfrom
juliendoclot wants to merge 1 commit intodocdyhr:mainfrom
Conversation
…guard on uploads SSRF protection (V3): - Private/localhost URL blocking now active in all environments, not just production - Developers can opt-in with ALLOW_PRIVATE_URLS=true for local WordPress instances - Previously, omitting NODE_ENV=production left the server vulnerable to SSRF Path traversal protection (V1): - Add validateFilePath() call before filesystem access in handleUploadMedia - Uses the existing security utility (src/utils/validation/security.ts) that was defined but never wired up for media uploads - MCP_UPLOAD_BASE_DIR env var restricts uploads to a specific directory (recommended: set to /tmp in Docker deployments) All 72 existing tests pass with zero modifications. Ref: docdyhr#176
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnables SSRF protection against private/localhost URLs in all environments by default and wires in a centralized path traversal validator for media uploads, introducing env‑flag opt-outs/config without changing existing tests. Sequence diagram for validated media upload path traversal protectionsequenceDiagram
actor MCPClient
participant MediaTools
participant SecurityValidation
participant FS as FSPromises
participant WordPressClient
MCPClient->>MediaTools: handleUploadMedia(client, params)
MediaTools->>MediaTools: uploadParams = toolParams(params)
MediaTools->>MediaTools: allowedBasePath = MCP_UPLOAD_BASE_DIR or "/"
MediaTools->>SecurityValidation: validateFilePath(uploadParams.file_path, allowedBasePath)
SecurityValidation-->>MediaTools: safePath
MediaTools->>MediaTools: uploadParams.file_path = safePath
MediaTools->>FSPromises: access(safePath)
FSPromises-->>MediaTools: success or error
alt file not accessible
MediaTools-->>MCPClient: Error("File not found at path: " + safePath)
else file accessible
MediaTools->>WordPressClient: uploadMedia(uploadParams)
WordPressClient-->>MediaTools: media
MediaTools-->>MCPClient: media
end
Sequence diagram for SSRF protection on WordPressClient requestssequenceDiagram
participant Caller
participant WordPressClient
participant URLParser
Caller->>WordPressClient: request(url)
WordPressClient->>URLParser: parse(url)
URLParser-->>WordPressClient: parsed
WordPressClient->>WordPressClient: ensure protocol is http or https
alt invalid protocol
WordPressClient-->>Caller: Error("Only HTTP and HTTPS protocols are allowed")
else valid protocol
WordPressClient->>WordPressClient: check ALLOW_PRIVATE_URLS env var
alt ALLOW_PRIVATE_URLS !== "true"
WordPressClient->>WordPressClient: hostname = parsed.hostname.toLowerCase()
WordPressClient->>WordPressClient: evaluate private ip and localhost patterns
alt hostname is private or localhost
WordPressClient-->>Caller: Error("Private/localhost URLs not allowed. Set ALLOW_PRIVATE_URLS=true for local development.")
else hostname is public
WordPressClient->>WordPressClient: proceed with HTTP request
WordPressClient-->>Caller: response
end
else ALLOW_PRIVATE_URLS === "true"
WordPressClient->>WordPressClient: skip private hostname checks
WordPressClient->>WordPressClient: proceed with HTTP request
WordPressClient-->>Caller: response
end
end
Class diagram for MediaTools and WordPressClient security changesclassDiagram
class MediaTools {
+handleUploadMedia(client, params) Promise
}
class WordPressClient {
+uploadMedia(uploadParams) Promise
+request(url) Promise
}
class SecurityValidation {
+validateFilePath(filePath, allowedBasePath) string
}
class FSModule {
+access(path) Promise
}
class Environment {
+MCP_UPLOAD_BASE_DIR string
+ALLOW_PRIVATE_URLS string
}
MediaTools --> WordPressClient : uses
MediaTools --> SecurityValidation : calls validateFilePath
MediaTools --> FSModule : calls access
MediaTools --> Environment : reads MCP_UPLOAD_BASE_DIR
WordPressClient --> Environment : reads ALLOW_PRIVATE_URLS
WordPressClient --> WordPressClient : validates protocol and hostname
SecurityValidation <.. WordPressClient : complements SSRF protection conceptually
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
handleUploadMedia, consider wrapping thevalidateFilePathcall in a try/catch and translating its error into a clearer, tool-level error message so callers don’t see low-level validation details directly. - The
ALLOW_PRIVATE_URLStoggle currently disables all private IP blocking; consider a more granular option (e.g., allowing onlylocalhost/loopback while still blocking RFC1918 and metadata ranges) to better support local dev without fully dropping SSRF protections.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handleUploadMedia`, consider wrapping the `validateFilePath` call in a try/catch and translating its error into a clearer, tool-level error message so callers don’t see low-level validation details directly.
- The `ALLOW_PRIVATE_URLS` toggle currently disables all private IP blocking; consider a more granular option (e.g., allowing only `localhost`/loopback while still blocking RFC1918 and metadata ranges) to better support local dev without fully dropping SSRF protections.
## Individual Comments
### Comment 1
<location path="src/client/api.ts" line_range="263-264" />
<code_context>
+ // Set ALLOW_PRIVATE_URLS=true for local development with a local WordPress
+ if (process.env.ALLOW_PRIVATE_URLS !== "true") {
const hostname = parsed.hostname.toLowerCase();
if (
hostname === "localhost" ||
</code_context>
<issue_to_address>
**🚨 suggestion (security):** IPv6 localhost and other private ranges are not covered by the current hostname checks.
Current checks only cover IPv4 localhost/RFC1918, not IPv6 (`::1`, link-local, or other private IPv6 ranges). Consider extending the logic to also treat `::1`, `localhost6`, and relevant IPv6 private/link-local patterns as private so IPv6 local-only endpoints aren’t inadvertently allowed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
263
to
264
| if ( | ||
| hostname === "localhost" || |
There was a problem hiding this comment.
🚨 suggestion (security): IPv6 localhost and other private ranges are not covered by the current hostname checks.
Current checks only cover IPv4 localhost/RFC1918, not IPv6 (::1, link-local, or other private IPv6 ranges). Consider extending the logic to also treat ::1, localhost6, and relevant IPv6 private/link-local patterns as private so IPv6 local-only endpoints aren’t inadvertently allowed.
Author
|
Thanks for the review @sourcery-ai! Re: wrapping Re: granular ALLOW_PRIVATE_URLS — Agreed this would be a nice improvement (e.g., allow loopback but block RFC1918 + cloud metadata Re: IPv6 coverage — |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two targeted security fixes that wire up existing security code. Zero test modifications — all 72 unit tests + 110 security tests pass unchanged.
Changes
V3 — SSRF protection enabled by default (
src/client/api.ts)Before: Private/localhost URL blocking only active when
NODE_ENV=productionAfter: Active by default in all environments. Opt-out with
ALLOW_PRIVATE_URLS=truefor local WordPress instances.Why: A MCP server in dev/staging is often exposed the same way as in production (Docker, tunnels). SSRF to
169.254.169.254(cloud metadata) orlocalhost:6379(Redis) works regardless ofNODE_ENV.V1 — Path traversal guard on media uploads (
src/tools/media.ts)Before:
handleUploadMediapasses user-suppliedfile_pathdirectly tofs.promises.access()without validationAfter: Calls
validateFilePath()fromsrc/utils/validation/security.ts(already in the codebase, just not wired up for uploads)Why: Without validation, a compromised MCP client could read arbitrary files (
/etc/shadow,~/.ssh/id_rsa) and upload them to WordPress. ThevalidateFilePath()function normalizes the path and ensures it stays within the allowed directory.MCP_UPLOAD_BASE_DIRdefaults to/(no restriction beyond traversal protection) to maintain backwards compatibility. In Docker deployments, setting it to/tmpis recommended.Test plan
npm run build— compiles cleanlynpm test— 72/72 tests pass (zero modifications to existing tests)npm run test:security— 110/110 security tests passRef: #176
Summary by Sourcery
Tighten security around outbound requests and media uploads by enabling SSRF protections by default and validating upload file paths to prevent traversal attacks.
Bug Fixes: