🛡️ Sentinel: [CRITICAL] Fix SQL injection in agent import#84
🛡️ Sentinel: [CRITICAL] Fix SQL injection in agent import#84Dexploarer wants to merge 1 commit intodevelopfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Summary of ChangesHello @Dexploarer, 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 delivers a critical security patch to prevent SQL injection during the agent import process. By implementing strict validation for memory table names, it safeguards the system from potential data exfiltration, deletion, or arbitrary SQL command execution via crafted agent files. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| ); | ||
| }); | ||
|
|
||
| it("GET /api/cloud/agents lists agents", async () => { |
There was a problem hiding this comment.
The test for GET /api/cloud/login/status sets environment variables (process.env.ELIZAOS_CLOUD_API_KEY and process.env.ELIZAOS_CLOUD_ENABLED) but does not clean them up after the test. This can lead to test pollution and unpredictable results in subsequent tests that may depend on these variables.
Recommended solution:
Add cleanup logic in the afterEach block to unset or restore these environment variables:
const originalEnv = { ...process.env };
beforeEach(() => { /* ... */ });
afterEach(() => { process.env = { ...originalEnv }; });Or explicitly delete the variables set during the test.
| it("POST /api/database/query rejects mutation keywords in read-only mode", async () => { | ||
| const { req, res } = createMocks("POST", "/api/database/query", { | ||
| sql: "DELETE FROM users", | ||
| }); | ||
|
|
||
| await handleDatabaseRoute(req, res, { adapter: {} } as any, "/api/database/query"); | ||
| await handleDatabaseRoute( | ||
| req, | ||
| res, | ||
| { adapter: {} } as any, | ||
| "/api/database/query", | ||
| ); | ||
|
|
||
| expect(res.statusCode).toBe(400); | ||
| expect(res.end).toHaveBeenCalledWith(expect.stringContaining("mutation keyword")); | ||
| expect(res.end).toHaveBeenCalledWith( | ||
| expect.stringContaining("mutation keyword"), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Mutation Keyword Detection May Be Too Simplistic
The test for mutation keyword rejection in read-only mode (lines 148-164) only checks for the presence of 'DELETE' in the SQL string. This approach may not catch all mutation attempts, such as case variations (delete), comments, or obfuscated SQL. Consider enhancing the mutation keyword detection logic to use a case-insensitive search and to parse SQL more robustly, possibly using a SQL parser library.
Example improvement:
const mutationRegex = /\b(INSERT|UPDATE|DELETE|DROP|ALTER|TRUNCATE)\b/i;
if (mutationRegex.test(sql)) {
// reject
}This would improve security and reliability of the read-only enforcement.
| if (call?.tableName === maliciousType) { | ||
| throw new Error( | ||
| `VULNERABILITY DETECTED: createMemory called with malicious table name: "${call.tableName}"`, | ||
| ); |
There was a problem hiding this comment.
Security Issue: Test expects vulnerability to exist, but does not explicitly fail if the malicious table name is used.
Currently, the test throws an error if the malicious table name is passed to createMemory, but this is not an explicit test failure. It would be clearer and more robust to use expect(call?.tableName).not.toBe(maliciousType) to ensure the test fails in a standard way if the vulnerability is present.
Recommended solution:
Replace the manual error throw with a standard assertion:
expect(call?.tableName).not.toBe(maliciousType);This will make the test result clearer and easier to interpret.
| return "messages"; | ||
| } |
There was a problem hiding this comment.
Potential Data Misclassification:
The function defaults to returning 'messages' when the memory type is unrecognized or missing. This silent fallback may lead to misclassification of memory records, especially if new types are introduced or if malformed records are present. Consider logging a warning or throwing an error when an unknown type is encountered to improve robustness and maintain data integrity.
Recommended Solution:
if (typeof memType === "string" && memType.length > 0) {
if ((MEMORY_TABLES as readonly string[]).includes(memType)) {
return memType;
} else {
logger.warn(`Unknown memory type: ${memType}. Defaulting to 'messages'.`);
}
}
return "messages";Alternatively, throw an error if strictness is desired.
![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 addresses a critical SQL injection vulnerability in the agent import functionality. The fix correctly validates the memory table name against a whitelist, preventing malicious table names from being used in database queries. A new security regression test has been added to verify this fix. The changes look good and effectively mitigate the vulnerability. I have one suggestion to improve the clarity and assertions in the new security test.
| // We expect this to either fail (if we fix it) or succeed (vulnerable) | ||
| // Currently vulnerable: | ||
| try { | ||
| await importAgent(targetRuntime, fileBuffer, password); | ||
| } catch (e) { | ||
| // If it throws "Invalid table name", we are good. | ||
| if (e.message.includes("Invalid memory table name")) { | ||
| return; // Pass | ||
| } | ||
| throw e; | ||
| } | ||
|
|
||
| // If we reach here, check what table name was passed to createMemory | ||
| const call = targetDb.createMemoryCalls.find( | ||
| (c) => c.memory.content.text === "evil", | ||
| ); | ||
| expect(call).toBeDefined(); | ||
|
|
||
| // Assert that the table name is NOT the malicious one | ||
| // If it IS the malicious one, the test fails (demonstrating vulnerability) | ||
| if (call?.tableName === maliciousType) { | ||
| throw new Error( | ||
| `VULNERABILITY DETECTED: createMemory called with malicious table name: "${call.tableName}"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test logic can be simplified and made more explicit. The current implementation of the fix in resolveMemoryTableName falls back to a default table name ("messages") instead of throwing an error. The try...catch block is therefore not necessary and can be confusing.
Additionally, the assertion at the end can be more specific by checking that the table name is indeed the expected fallback value, rather than just checking that it's not the malicious one. Using expect from vitest will also provide better test failure messages.
| // We expect this to either fail (if we fix it) or succeed (vulnerable) | |
| // Currently vulnerable: | |
| try { | |
| await importAgent(targetRuntime, fileBuffer, password); | |
| } catch (e) { | |
| // If it throws "Invalid table name", we are good. | |
| if (e.message.includes("Invalid memory table name")) { | |
| return; // Pass | |
| } | |
| throw e; | |
| } | |
| // If we reach here, check what table name was passed to createMemory | |
| const call = targetDb.createMemoryCalls.find( | |
| (c) => c.memory.content.text === "evil", | |
| ); | |
| expect(call).toBeDefined(); | |
| // Assert that the table name is NOT the malicious one | |
| // If it IS the malicious one, the test fails (demonstrating vulnerability) | |
| if (call?.tableName === maliciousType) { | |
| throw new Error( | |
| `VULNERABILITY DETECTED: createMemory called with malicious table name: "${call.tableName}"`, | |
| ); | |
| } | |
| // We expect the import to succeed, but the malicious table name should be sanitized. | |
| await importAgent(targetRuntime, fileBuffer, password); | |
| // Check what table name was passed to createMemory | |
| const call = targetDb.createMemoryCalls.find( | |
| (c) => c.memory.content.text === "evil", | |
| ); | |
| expect(call).toBeDefined(); | |
| // Assert that the table name is the safe fallback ("messages") and not the malicious one. | |
| expect(call?.tableName).toBe("messages"); | |
| expect(call?.tableName).not.toBe(maliciousType); |
1 participant
🛡️ Sentinel: [CRITICAL] Fix SQL Injection in Agent Import
🚨 Severity: CRITICAL
💡 Vulnerability: The
importAgentfunction insrc/services/agent-export.tswas vulnerable to SQL injection. It allowed arbitrary strings from thetypefield of imported memory objects to be used as table names in database queries (db.createMemory). An attacker could craft a malicious.eliza-agentfile with a SQL payload in thetypefield (e.g.,"messages; DROP TABLE users; --") which would be executed by the database adapter.🎯 Impact: This could allow an attacker to delete tables, exfiltrate data, or execute arbitrary SQL commands if they can trick a user into importing a malicious agent file.
🔧 Fix: Modified
resolveMemoryTableNameinsrc/services/agent-export.tsto strictly validate the resolved table name against a whitelist of known tables (MEMORY_TABLES). If the type is not in the whitelist, it falls back to"messages".✅ Verification: Added a new security regression test
src/services/agent-export-security.test.tsthat attempts to import a memory with a malicious table name. The test confirms that the malicious name is not passed to the database adapter. Existing tests insrc/services/agent-export.test.tswere also run to ensure no regressions.PR created automatically by Jules for task 4327353746641705221 started by @Dexploarer