mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-17 13:16:34 +01:00
🧯 fix: Prevent Env-Variable Exfil. via Placeholder Injection (#12260)
* 🔒 fix: Resolve env vars before body placeholder expansion to prevent secret exfiltration Body placeholders ({{LIBRECHAT_BODY_*}}) were substituted before extractEnvVariable ran, allowing user-controlled body fields containing ${SECRET} patterns to be expanded into real environment values in outbound headers. Reorder so env vars resolve first, preventing untrusted input from triggering env expansion. * 🛡️ fix: Block sensitive infrastructure env vars from placeholder resolution Add isSensitiveEnvVar blocklist to extractEnvVariable so that internal infrastructure secrets (JWT_SECRET, JWT_REFRESH_SECRET, CREDS_KEY, CREDS_IV, MEILI_MASTER_KEY, MONGO_URI, REDIS_URI, REDIS_PASSWORD) can never be resolved via ${VAR} expansion — even if an attacker manages to inject a placeholder pattern. Uses exact-match set (not substring patterns) to avoid breaking legitimate operator config that references OAuth/API secrets in MCP and custom endpoint configurations. * 🧹 test: Rename ANOTHER_SECRET test fixture to ANOTHER_VALUE Avoid using SECRET-containing names for non-sensitive test fixtures to prevent confusion with the new isSensitiveEnvVar blocklist. * 🔒 fix: Resolve env vars before all user-controlled substitutions in processSingleValue Move extractEnvVariable to run on the raw admin-authored template BEFORE customUserVars, user fields, OIDC tokens, and body placeholders. Previously env resolution ran after customUserVars, so a user setting a custom MCP variable to "${SECRET}" could still trigger env expansion. Now env vars are resolved strictly on operator config, and all subsequent user-controlled substitutions cannot introduce ${VAR} patterns that would be expanded. Gated by !dbSourced so DB-stored servers continue to skip env resolution. Adds a security-invariant comment documenting the ordering requirement. * 🧪 test: Comprehensive security regression tests for placeholder injection - Cover all three body fields (conversationId, parentMessageId, messageId) - Add user-field injection test (user.name containing ${VAR}) - Add customUserVars injection test (MY_TOKEN = "${VAR}") - Add processMCPEnv injection tests for body and customUserVars paths - Remove redundant process.env setup/teardown already handled by beforeEach/afterEach * 🧹 chore: Add REDIS_PASSWORD to blocklist integration test; document customUserVars gate
This commit is contained in:
Danny Avila
GitHub
parent
85e24e4c61
commit
951d261f5c
4 changed files with 200 additions and 24 deletions
|
|
@ -111,12 +111,12 @@ describe('encodeHeaderValue', () => {
|
|||
describe('resolveHeaders', () => {
|
||||
beforeEach(() => {
|
||||
process.env.TEST_API_KEY = 'test-api-key-value';
|
||||
process.env.ANOTHER_SECRET = 'another-secret-value';
|
||||
process.env.ANOTHER_VALUE = 'another-test-value';
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete process.env.TEST_API_KEY;
|
||||
delete process.env.ANOTHER_SECRET;
|
||||
delete process.env.ANOTHER_VALUE;
|
||||
});
|
||||
|
||||
it('should return empty object when headers is undefined', () => {
|
||||
|
|
@ -139,7 +139,7 @@ describe('resolveHeaders', () => {
|
|||
it('should process environment variables in headers', () => {
|
||||
const headers = {
|
||||
Authorization: '${TEST_API_KEY}',
|
||||
'X-Secret': '${ANOTHER_SECRET}',
|
||||
'X-Secret': '${ANOTHER_VALUE}',
|
||||
'Content-Type': 'application/json',
|
||||
};
|
||||
|
||||
|
|
@ -147,7 +147,7 @@ describe('resolveHeaders', () => {
|
|||
|
||||
expect(result).toEqual({
|
||||
Authorization: 'test-api-key-value',
|
||||
'X-Secret': 'another-secret-value',
|
||||
'X-Secret': 'another-test-value',
|
||||
'Content-Type': 'application/json',
|
||||
});
|
||||
});
|
||||
|
|
@ -526,6 +526,40 @@ describe('resolveHeaders', () => {
|
|||
expect(result['X-Conversation']).toBe('conv-123');
|
||||
});
|
||||
|
||||
it('should not resolve env vars introduced via LIBRECHAT_BODY placeholders', () => {
|
||||
const body = {
|
||||
conversationId: '${TEST_API_KEY}',
|
||||
parentMessageId: '${TEST_API_KEY}',
|
||||
messageId: '${TEST_API_KEY}',
|
||||
};
|
||||
const headers = {
|
||||
'X-Conv': '{{LIBRECHAT_BODY_CONVERSATIONID}}',
|
||||
'X-Parent': '{{LIBRECHAT_BODY_PARENTMESSAGEID}}',
|
||||
'X-Msg': '{{LIBRECHAT_BODY_MESSAGEID}}',
|
||||
};
|
||||
const result = resolveHeaders({ headers, body });
|
||||
|
||||
expect(result['X-Conv']).toBe('${TEST_API_KEY}');
|
||||
expect(result['X-Parent']).toBe('${TEST_API_KEY}');
|
||||
expect(result['X-Msg']).toBe('${TEST_API_KEY}');
|
||||
});
|
||||
|
||||
it('should not resolve env vars introduced via LIBRECHAT_USER placeholders', () => {
|
||||
const user = createTestUser({ name: '${TEST_API_KEY}' });
|
||||
const headers = { 'X-Name': '{{LIBRECHAT_USER_NAME}}' };
|
||||
const result = resolveHeaders({ headers, user });
|
||||
|
||||
expect(result['X-Name']).toBe('${TEST_API_KEY}');
|
||||
});
|
||||
|
||||
it('should not resolve env vars introduced via customUserVars', () => {
|
||||
const customUserVars = { MY_TOKEN: '${TEST_API_KEY}' };
|
||||
const headers = { Authorization: 'Bearer {{MY_TOKEN}}' };
|
||||
const result = resolveHeaders({ headers, customUserVars });
|
||||
|
||||
expect(result.Authorization).toBe('Bearer ${TEST_API_KEY}');
|
||||
});
|
||||
|
||||
describe('non-string header values (type guard tests)', () => {
|
||||
it('should handle numeric header values without crashing', () => {
|
||||
const headers = {
|
||||
|
|
@ -657,12 +691,12 @@ describe('resolveHeaders', () => {
|
|||
describe('resolveNestedObject', () => {
|
||||
beforeEach(() => {
|
||||
process.env.TEST_API_KEY = 'test-api-key-value';
|
||||
process.env.ANOTHER_SECRET = 'another-secret-value';
|
||||
process.env.ANOTHER_VALUE = 'another-test-value';
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete process.env.TEST_API_KEY;
|
||||
delete process.env.ANOTHER_SECRET;
|
||||
delete process.env.ANOTHER_VALUE;
|
||||
});
|
||||
|
||||
it('should preserve nested object structure', () => {
|
||||
|
|
@ -952,7 +986,7 @@ describe('resolveNestedObject', () => {
|
|||
describe('processMCPEnv', () => {
|
||||
beforeEach(() => {
|
||||
process.env.TEST_API_KEY = 'test-api-key-value';
|
||||
process.env.ANOTHER_SECRET = 'another-secret-value';
|
||||
process.env.ANOTHER_VALUE = 'another-test-value';
|
||||
process.env.OAUTH_CLIENT_ID = 'oauth-client-id-value';
|
||||
process.env.OAUTH_CLIENT_SECRET = 'oauth-client-secret-value';
|
||||
process.env.MCP_SERVER_URL = 'https://mcp.example.com';
|
||||
|
|
@ -960,7 +994,7 @@ describe('processMCPEnv', () => {
|
|||
|
||||
afterEach(() => {
|
||||
delete process.env.TEST_API_KEY;
|
||||
delete process.env.ANOTHER_SECRET;
|
||||
delete process.env.ANOTHER_VALUE;
|
||||
delete process.env.OAUTH_CLIENT_ID;
|
||||
delete process.env.OAUTH_CLIENT_SECRET;
|
||||
delete process.env.MCP_SERVER_URL;
|
||||
|
|
@ -977,7 +1011,7 @@ describe('processMCPEnv', () => {
|
|||
command: 'mcp-server',
|
||||
env: {
|
||||
API_KEY: '${TEST_API_KEY}',
|
||||
SECRET: '${ANOTHER_SECRET}',
|
||||
SECRET: '${ANOTHER_VALUE}',
|
||||
PLAIN_VALUE: 'plain-text',
|
||||
},
|
||||
args: ['--key', '${TEST_API_KEY}', '--url', '${MCP_SERVER_URL}'],
|
||||
|
|
@ -990,7 +1024,7 @@ describe('processMCPEnv', () => {
|
|||
command: 'mcp-server',
|
||||
env: {
|
||||
API_KEY: 'test-api-key-value',
|
||||
SECRET: 'another-secret-value',
|
||||
SECRET: 'another-test-value',
|
||||
PLAIN_VALUE: 'plain-text',
|
||||
},
|
||||
args: ['--key', 'test-api-key-value', '--url', 'https://mcp.example.com'],
|
||||
|
|
@ -1137,6 +1171,49 @@ describe('processMCPEnv', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('should not resolve env vars introduced via body placeholders in MCP headers', () => {
|
||||
const body = {
|
||||
conversationId: '${TEST_API_KEY}',
|
||||
parentMessageId: '${TEST_API_KEY}',
|
||||
messageId: '${TEST_API_KEY}',
|
||||
};
|
||||
|
||||
const options: MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://api.example.com',
|
||||
headers: {
|
||||
'X-Conv': '{{LIBRECHAT_BODY_CONVERSATIONID}}',
|
||||
'X-Parent': '{{LIBRECHAT_BODY_PARENTMESSAGEID}}',
|
||||
},
|
||||
};
|
||||
|
||||
const result = processMCPEnv({ options, body });
|
||||
|
||||
if (!isStreamableHTTPOptions(result)) {
|
||||
throw new Error('Expected streamable-http options');
|
||||
}
|
||||
expect(result.headers?.['X-Conv']).toBe('${TEST_API_KEY}');
|
||||
expect(result.headers?.['X-Parent']).toBe('${TEST_API_KEY}');
|
||||
});
|
||||
|
||||
it('should not resolve env vars introduced via customUserVars in MCP headers', () => {
|
||||
const customUserVars = { MY_TOKEN: '${TEST_API_KEY}' };
|
||||
const options: MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://api.example.com',
|
||||
headers: {
|
||||
Authorization: 'Bearer {{MY_TOKEN}}',
|
||||
},
|
||||
};
|
||||
|
||||
const result = processMCPEnv({ options, customUserVars });
|
||||
|
||||
if (!isStreamableHTTPOptions(result)) {
|
||||
throw new Error('Expected streamable-http options');
|
||||
}
|
||||
expect(result.headers?.Authorization).toBe('Bearer ${TEST_API_KEY}');
|
||||
});
|
||||
|
||||
it('should handle mixed placeholders in OAuth configuration', () => {
|
||||
const user = createTestUser({
|
||||
id: 'user-123',
|
||||
|
|
|
|||
|
|
@ -226,9 +226,20 @@ function processSingleValue({
|
|||
|
||||
let value = originalValue;
|
||||
|
||||
/**
|
||||
* SECURITY INVARIANT — ordering matters:
|
||||
* Resolve env vars on the admin-authored template BEFORE any user-controlled
|
||||
* data is substituted (customUserVars, user fields, OIDC tokens, body placeholders).
|
||||
* This prevents second-order injection where user values containing ${VAR}
|
||||
* patterns would otherwise be expanded against process.env.
|
||||
*/
|
||||
if (!dbSourced) {
|
||||
value = extractEnvVariable(value);
|
||||
}
|
||||
|
||||
/** Runs for both dbSourced and non-dbSourced — it is the only resolution DB-stored servers get */
|
||||
if (customUserVars) {
|
||||
for (const [varName, varVal] of Object.entries(customUserVars)) {
|
||||
/** Escaped varName for use in regex to avoid issues with special characters */
|
||||
const escapedVarName = varName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
const placeholderRegex = new RegExp(`\\{\\{${escapedVarName}\\}\\}`, 'g');
|
||||
value = value.replace(placeholderRegex, varVal);
|
||||
|
|
@ -250,8 +261,6 @@ function processSingleValue({
|
|||
value = processBodyPlaceholders(value, body);
|
||||
}
|
||||
|
||||
value = extractEnvVariable(value);
|
||||
|
||||
return value;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { extractEnvVariable } from '../src/utils';
|
||||
import { extractEnvVariable, isSensitiveEnvVar } from '../src/utils';
|
||||
|
||||
describe('Environment Variable Extraction', () => {
|
||||
const originalEnv = process.env;
|
||||
|
|
@ -7,7 +7,7 @@ describe('Environment Variable Extraction', () => {
|
|||
process.env = {
|
||||
...originalEnv,
|
||||
TEST_API_KEY: 'test-api-key-value',
|
||||
ANOTHER_SECRET: 'another-secret-value',
|
||||
ANOTHER_VALUE: 'another-value',
|
||||
};
|
||||
});
|
||||
|
||||
|
|
@ -55,7 +55,7 @@ describe('Environment Variable Extraction', () => {
|
|||
describe('extractEnvVariable function', () => {
|
||||
it('should extract environment variables from exact matches', () => {
|
||||
expect(extractEnvVariable('${TEST_API_KEY}')).toBe('test-api-key-value');
|
||||
expect(extractEnvVariable('${ANOTHER_SECRET}')).toBe('another-secret-value');
|
||||
expect(extractEnvVariable('${ANOTHER_VALUE}')).toBe('another-value');
|
||||
});
|
||||
|
||||
it('should extract environment variables from strings with prefixes', () => {
|
||||
|
|
@ -82,7 +82,7 @@ describe('Environment Variable Extraction', () => {
|
|||
describe('extractEnvVariable', () => {
|
||||
it('should extract environment variable values', () => {
|
||||
expect(extractEnvVariable('${TEST_API_KEY}')).toBe('test-api-key-value');
|
||||
expect(extractEnvVariable('${ANOTHER_SECRET}')).toBe('another-secret-value');
|
||||
expect(extractEnvVariable('${ANOTHER_VALUE}')).toBe('another-value');
|
||||
});
|
||||
|
||||
it('should return the original string if environment variable is not found', () => {
|
||||
|
|
@ -126,4 +126,71 @@ describe('Environment Variable Extraction', () => {
|
|||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isSensitiveEnvVar', () => {
|
||||
it('should flag infrastructure secrets', () => {
|
||||
expect(isSensitiveEnvVar('JWT_SECRET')).toBe(true);
|
||||
expect(isSensitiveEnvVar('JWT_REFRESH_SECRET')).toBe(true);
|
||||
expect(isSensitiveEnvVar('CREDS_KEY')).toBe(true);
|
||||
expect(isSensitiveEnvVar('CREDS_IV')).toBe(true);
|
||||
expect(isSensitiveEnvVar('MEILI_MASTER_KEY')).toBe(true);
|
||||
expect(isSensitiveEnvVar('MONGO_URI')).toBe(true);
|
||||
expect(isSensitiveEnvVar('REDIS_URI')).toBe(true);
|
||||
expect(isSensitiveEnvVar('REDIS_PASSWORD')).toBe(true);
|
||||
});
|
||||
|
||||
it('should allow non-infrastructure vars through (including operator-configured secrets)', () => {
|
||||
expect(isSensitiveEnvVar('OPENAI_API_KEY')).toBe(false);
|
||||
expect(isSensitiveEnvVar('ANTHROPIC_API_KEY')).toBe(false);
|
||||
expect(isSensitiveEnvVar('GOOGLE_KEY')).toBe(false);
|
||||
expect(isSensitiveEnvVar('PROXY')).toBe(false);
|
||||
expect(isSensitiveEnvVar('DEBUG_LOGGING')).toBe(false);
|
||||
expect(isSensitiveEnvVar('DOMAIN_CLIENT')).toBe(false);
|
||||
expect(isSensitiveEnvVar('APP_TITLE')).toBe(false);
|
||||
expect(isSensitiveEnvVar('OPENID_CLIENT_SECRET')).toBe(false);
|
||||
expect(isSensitiveEnvVar('DISCORD_CLIENT_SECRET')).toBe(false);
|
||||
expect(isSensitiveEnvVar('MY_CUSTOM_SECRET')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractEnvVariable sensitive var blocklist', () => {
|
||||
beforeEach(() => {
|
||||
process.env.JWT_SECRET = 'super-secret-jwt';
|
||||
process.env.JWT_REFRESH_SECRET = 'super-secret-refresh';
|
||||
process.env.CREDS_KEY = 'encryption-key';
|
||||
process.env.CREDS_IV = 'encryption-iv';
|
||||
process.env.MEILI_MASTER_KEY = 'meili-key';
|
||||
process.env.MONGO_URI = 'mongodb://user:pass@host/db';
|
||||
process.env.REDIS_URI = 'redis://:pass@host:6379';
|
||||
process.env.REDIS_PASSWORD = 'redis-pass';
|
||||
process.env.OPENAI_API_KEY = 'sk-legit-key';
|
||||
});
|
||||
|
||||
it('should refuse to resolve sensitive vars (single-match path)', () => {
|
||||
expect(extractEnvVariable('${JWT_SECRET}')).toBe('${JWT_SECRET}');
|
||||
expect(extractEnvVariable('${JWT_REFRESH_SECRET}')).toBe('${JWT_REFRESH_SECRET}');
|
||||
expect(extractEnvVariable('${CREDS_KEY}')).toBe('${CREDS_KEY}');
|
||||
expect(extractEnvVariable('${CREDS_IV}')).toBe('${CREDS_IV}');
|
||||
expect(extractEnvVariable('${MEILI_MASTER_KEY}')).toBe('${MEILI_MASTER_KEY}');
|
||||
expect(extractEnvVariable('${MONGO_URI}')).toBe('${MONGO_URI}');
|
||||
expect(extractEnvVariable('${REDIS_URI}')).toBe('${REDIS_URI}');
|
||||
expect(extractEnvVariable('${REDIS_PASSWORD}')).toBe('${REDIS_PASSWORD}');
|
||||
});
|
||||
|
||||
it('should refuse to resolve sensitive vars in composite strings (multi-match path)', () => {
|
||||
expect(extractEnvVariable('key=${JWT_SECRET}&more')).toBe('key=${JWT_SECRET}&more');
|
||||
expect(extractEnvVariable('db=${MONGO_URI}/extra')).toBe('db=${MONGO_URI}/extra');
|
||||
});
|
||||
|
||||
it('should still resolve non-sensitive vars normally', () => {
|
||||
expect(extractEnvVariable('${OPENAI_API_KEY}')).toBe('sk-legit-key');
|
||||
expect(extractEnvVariable('Bearer ${OPENAI_API_KEY}')).toBe('Bearer sk-legit-key');
|
||||
});
|
||||
|
||||
it('should resolve non-sensitive vars while blocking sensitive ones in the same string', () => {
|
||||
expect(extractEnvVariable('key=${OPENAI_API_KEY}&secret=${JWT_SECRET}')).toBe(
|
||||
'key=sk-legit-key&secret=${JWT_SECRET}',
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,29 @@
|
|||
export const envVarRegex = /^\${(.+)}$/;
|
||||
|
||||
/**
|
||||
* Infrastructure env vars that must never be resolved via placeholder expansion.
|
||||
* These are internal secrets whose exposure would compromise the system —
|
||||
* they have no legitimate reason to appear in outbound headers, MCP env/args, or OAuth config.
|
||||
*
|
||||
* Intentionally excludes API keys (operators reference them in config) and
|
||||
* OAuth/session secrets (referenced in MCP OAuth config via processMCPEnv).
|
||||
*/
|
||||
const SENSITIVE_ENV_VARS = new Set([
|
||||
'JWT_SECRET',
|
||||
'JWT_REFRESH_SECRET',
|
||||
'CREDS_KEY',
|
||||
'CREDS_IV',
|
||||
'MEILI_MASTER_KEY',
|
||||
'MONGO_URI',
|
||||
'REDIS_URI',
|
||||
'REDIS_PASSWORD',
|
||||
]);
|
||||
|
||||
/** Returns true when `varName` refers to an infrastructure secret that must not leak. */
|
||||
export function isSensitiveEnvVar(varName: string): boolean {
|
||||
return SENSITIVE_ENV_VARS.has(varName);
|
||||
}
|
||||
|
||||
/** Extracts the environment variable name from a template literal string */
|
||||
export function extractVariableName(value: string): string | null {
|
||||
if (!value) {
|
||||
|
|
@ -16,21 +40,20 @@ export function extractEnvVariable(value: string) {
|
|||
return value;
|
||||
}
|
||||
|
||||
// Trim the input
|
||||
const trimmed = value.trim();
|
||||
|
||||
// Special case: if it's just a single environment variable
|
||||
const singleMatch = trimmed.match(envVarRegex);
|
||||
if (singleMatch) {
|
||||
const varName = singleMatch[1];
|
||||
if (isSensitiveEnvVar(varName)) {
|
||||
return trimmed;
|
||||
}
|
||||
return process.env[varName] || trimmed;
|
||||
}
|
||||
|
||||
// For multiple variables, process them using a regex loop
|
||||
const regex = /\${([^}]+)}/g;
|
||||
let result = trimmed;
|
||||
|
||||
// First collect all matches and their positions
|
||||
const matches = [];
|
||||
let match;
|
||||
while ((match = regex.exec(trimmed)) !== null) {
|
||||
|
|
@ -41,12 +64,12 @@ export function extractEnvVariable(value: string) {
|
|||
});
|
||||
}
|
||||
|
||||
// Process matches in reverse order to avoid position shifts
|
||||
for (let i = matches.length - 1; i >= 0; i--) {
|
||||
const { fullMatch, varName, index } = matches[i];
|
||||
if (isSensitiveEnvVar(varName)) {
|
||||
continue;
|
||||
}
|
||||
const envValue = process.env[varName] || fullMatch;
|
||||
|
||||
// Replace at exact position
|
||||
result = result.substring(0, index) + envValue + result.substring(index + fullMatch.length);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue