security: fix SSRF, API key exposure, pattern bypass, symlink escapes#618
security: fix SSRF, API key exposure, pattern bypass, symlink escapes#618MarvelNwachukwu merged 3 commits intomainfrom
Conversation
…apes S1/S2: Add SSRF protection via validateUrlForFetch utility (blocks private IPs, localhost, non-HTTP protocols). Revalidate on redirects in http-request-tool (redirect: manual) and web-fetch-tool (beforeRedirect hook). S3: Move Gemini API key from URL query param to x-goog-api-key header to prevent logging/history/proxy exposure. S4: Strengthen BashTool dangerous patterns to cover interpreter execution (python, node, perl, ruby, php) and alternative netcat/ncat/netcat binaries. S5: Fix FileOperationsTool symlink escape via fs.realpath validation with boundary-checked startsWith, use fs.lstat in directory listing. Also includes: IPv6-mapped address detection (::ffff:127.0.0.1), full 0.0.0.0/8 blocking, path prefix confusion mitigation, and comprehensive tests (19 URL validation, 7 file ops, 15 bash patterns).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 642778b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of LLM-controlled inputs within ADK-TS tools by addressing five critical and high-severity vulnerabilities. The changes focus on preventing Server-Side Request Forgery (SSRF), securing API key handling, improving detection of dangerous bash commands, and mitigating symlink escape attacks in file operations. These updates are complemented by extensive new test coverage to ensure robustness and prevent common attack vectors. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important security fixes, including preventing SSRF, avoiding API key exposure, strengthening dangerous command patterns, and fixing symlink escape vulnerabilities. The changes are generally well-implemented and come with good test coverage. However, I've identified a few areas for improvement. The new path validation logic in FileOperationsTool is complex and has a potential security weakness in its fallback mechanism. Additionally, the new URL validation for SSRF prevention is susceptible to bypass via alternative IP address formats. I've also left some minor comments on markdown file formatting for better readability.
| function isPrivateIP(hostname: string): boolean { | ||
| const match = hostname.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); | ||
| if (!match) return false; | ||
|
|
||
| const [, a, b] = match; | ||
| const first = Number(a); | ||
| const second = Number(b); | ||
|
|
||
| // 0.0.0.0/8 (entire block is reserved) | ||
| if (first === 0) return true; | ||
| // 127.0.0.0/8 | ||
| if (first === 127) return true; | ||
| // 10.0.0.0/8 | ||
| if (first === 10) return true; | ||
| // 172.16.0.0/12 | ||
| if (first === 172 && second >= 16 && second <= 31) return true; | ||
| // 192.168.0.0/16 | ||
| if (first === 192 && second === 168) return true; | ||
| // 169.254.0.0/16 (link-local / cloud metadata) | ||
| if (first === 169 && second === 254) return true; | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The isPrivateIP function is vulnerable to IP address format variations, which can lead to a bypass of the SSRF protection. The current implementation uses a regex that only matches the dotted-decimal format. However, hostnames can be specified in other formats like dword (2130706433), octal (0177.0.0.1), or hex (0x7f.0.0.1), which may be resolved to a private IP address by the underlying fetch implementation. Node's URL parser does not normalize all of these formats (e.g., dword), so they would bypass your regex check. This is a common and critical SSRF bypass vector. To fix this, you should use a robust IP address parsing method that can handle these alternative formats before checking if the IP is private.
There was a problem hiding this comment.
Verified this is not exploitable in Node.js. The URL constructor normalizes all alternative IP formats (dword, octal, hex) to standard dotted-decimal before we see the hostname. Added test cases for dword, octal, and hex formats to document this guarantee.
| private async validatePath(filepath: string): Promise<void> { | ||
| const realBasePath = await fs.realpath(this.basePath); | ||
|
|
||
| const isWithinBase = (p: string, base: string) => | ||
| p === base || p.startsWith(base + path.sep); | ||
|
|
||
| try { | ||
| const realPath = await fs.realpath(filepath); | ||
| if (!isWithinBase(realPath, realBasePath)) { | ||
| throw new Error( | ||
| "Access denied: Can't access paths outside the base directory", | ||
| ); | ||
| } | ||
| } catch (error: unknown) { | ||
| if ( | ||
| error instanceof Error && | ||
| "code" in error && | ||
| (error as NodeJS.ErrnoException).code === "ENOENT" | ||
| ) { | ||
| // File doesn't exist yet (write/mkdir) — validate the parent directory | ||
| const parentDir = path.dirname(filepath); | ||
| try { | ||
| const realParent = await fs.realpath(parentDir); | ||
| if (!isWithinBase(realParent, realBasePath)) { | ||
| throw new Error( | ||
| "Access denied: Can't access paths outside the base directory", | ||
| ); | ||
| } | ||
| } catch (parentError: unknown) { | ||
| if ( | ||
| parentError instanceof Error && | ||
| "code" in parentError && | ||
| (parentError as NodeJS.ErrnoException).code === "ENOENT" | ||
| ) { | ||
| // Parent also doesn't exist — fall back to normalize check | ||
| const normalizedPath = path.normalize(filepath); | ||
| const normalizedBasePath = path.normalize(this.basePath); | ||
| if (!isWithinBase(normalizedPath, normalizedBasePath)) { | ||
| throw new Error( | ||
| "Access denied: Can't access paths outside the base directory", | ||
| ); | ||
| } | ||
| } else { | ||
| throw parentError; | ||
| } | ||
| } | ||
| } else { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new implementation of validatePath is quite complex with nested try-catch blocks, which harms readability and maintainability. More importantly, the fallback logic for when a file and its parent directory do not exist re-introduces a security risk. It falls back to a path.normalize() check, which does not resolve symbolic links that may exist in the non-existent parts of the path. This could lead to a Time-of-Check to Time-of-Use (TOCTOU) vulnerability if an attacker can create symlinks in the path after validation but before the file operation. For a security-critical function, it would be safer to disallow operations on paths where the parent directory's real path cannot be resolved. Consider refactoring for clarity and adopting a stricter validation approach that avoids this fallback.
There was a problem hiding this comment.
Agreed. Removed the normalize fallback entirely — if the parent directory can't be resolved via realpath, we now reject with Access denied. This eliminates the TOCTOU window and simplifies the logic significantly. See 0c189c7.
|
|
||
| ### 2. Second Step | ||
| Content for step 2. | ||
| ### 1. First Step Content for step 1. ### 2. Second Step Content for step 2. |
There was a problem hiding this comment.
This change consolidates the Steps component's content onto a single line, which significantly reduces the readability and maintainability of the markdown source. For better clarity, please restore the original multi-line formatting.
| ### 1. First Step Content for step 1. ### 2. Second Step Content for step 2. | |
| ### 1. First Step | |
| Content for step 1. | |
| ### 2. Second Step | |
| Content for step 2. |
There was a problem hiding this comment.
This is caused by the prettier pre-commit hook collapsing markdown inside code fence examples. The original commit already had this formatting before our changes — it's a pre-existing linter behavior, not something we introduced. Restoring the multi-line format gets re-collapsed on commit.
| <Tab value="pnpm">```bash pnpm add @iqai/adk ```</Tab> | ||
| <Tab value="npm">```bash npm install @iqai/adk ```</Tab> |
There was a problem hiding this comment.
Similar to the previous comment, consolidating the Tab content onto single lines makes the code harder to read and edit. Please revert to the multi-line format for better maintainability.
| <Tab value="pnpm">```bash pnpm add @iqai/adk ```</Tab> | |
| <Tab value="npm">```bash npm install @iqai/adk ```</Tab> | |
| <Tab value="pnpm"> | |
| ```bash | |
| pnpm add @iqai/adk | |
| ``` | |
| </Tab> | |
| <Tab value="npm"> | |
| ```bash | |
| npm install @iqai/adk | |
| ``` | |
| </Tab> |
There was a problem hiding this comment.
Same as above — prettier pre-commit hook collapses this. Not introduced by our changes.
- Remove unsafe normalize fallback in validatePath — reject if parent directory can't be resolved via realpath (stricter, eliminates TOCTOU) - Add changeset for patch version bump - Restore multi-line formatting in SKILL.md (mangled by linter) - Add tests for dword/octal/hex IP formats (Node URL normalizes them)
|
@claude review |
|
Claude finished @MarvelNwachukwu's task —— View job Security Review Complete ✅I've conducted a comprehensive security review of this PR's vulnerability fixes. Here's my analysis: Review Checklist:
✅ Overall Assessment: LGTM with Strong Security ImprovementsThis PR successfully addresses five critical security vulnerabilities with well-implemented fixes and comprehensive test coverage. The author has also effectively addressed previous review feedback. 🛡️ Security Fixes AnalysisS1/S2 SSRF Prevention -
|
|
Claude finished @MarvelNwachukwu's task —— View job Security Review Complete ✅I've conducted a comprehensive security review of this PR's vulnerability fixes. Here's my analysis: Review Checklist:
✅ Overall Assessment: LGTM with Strong Security ImprovementsThis PR successfully addresses five critical security vulnerabilities with well-implemented fixes and comprehensive test coverage. The author has also effectively addressed previous review feedback. 🛡️ Security Fixes AnalysisS1/S2 SSRF Prevention -
|
- python3.11, python3.12 etc bypass python[23]? pattern — now matches python[23]?(\.\d+)? - node --eval is long form of -e — now matches (-e|--eval) - Added 3 new test cases for these bypass vectors
2 participants
Description
Motivation
A security audit identified five vulnerabilities in ADK-TS tools where LLM-controlled inputs could be exploited: two SSRF vectors allowing access to cloud metadata and internal services, an API key leaked in URL query parameters, a command blocklist trivially bypassed via scripting interpreters, and a symlink-based filesystem sandbox escape. These affect any deployment where agents use built-in HTTP, bash, or file tools with untrusted LLM outputs.
Changes
S1/S2 SSRF: Added
validateUrlForFetch()utility blocking private IPs (0/8, 10/8, 127/8, 169.254/16, 172.16-31/12, 192.168/16), localhost, IPv6-mapped addresses, and non-HTTP protocols. Revalidates on redirects usingredirect: "manual"(http-request-tool) and axios'sbeforeRedirecthook (web-fetch-tool).S3 API key exposure: Moved Gemini API key from URL query param (
?key=...) tox-goog-api-keyheader, matching OpenAI/Anthropic patterns and preventing logging/history/proxy exposure.S4 Pattern bypass: Strengthened BashTool dangerous pattern regex to cover interpreter execution (python, node, perl, ruby, php) and alternative netcat variants (nc/ncat/netcat).
S5 Symlink escape: Fixed FileOperationsTool path validation using
fs.realpathwith boundary-checkedstartsWith(prevents/appfrom matching/app-evil), and switched listDirectory tofs.lstatto avoid following symlinks.Additional hardening: IPv6-mapped address extraction (::ffff:127.0.0.1), full 0.0.0.0/8 blocking, strict parent-directory validation (no normalize fallback), and comprehensive test coverage (41 new tests).
Type of Change
How Has This Been Tested?
All 514 existing tests pass, plus 41 new security tests: 22 URL validation (protocol, IP ranges, IPv6 mapping, dword/octal/hex formats), 7 file operations (path traversal, symlinks, prefix confusion), 15 bash patterns (interpreters, downloads, netcat). Code formatted with Biome, builds without errors.
Checklist