mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-23 08:06:34 +01:00
🪢 fix: Action Domain Encoding Collision for HTTPS URLs (#12271)
* fix: strip protocol from domain before encoding in `domainParser`
All https:// (and http://) domains produced the same 10-char base64
prefix due to ENCODED_DOMAIN_LENGTH truncation, causing tool name
collisions for agents with multiple actions.
Strip the protocol before encoding so the base64 key is derived from
the hostname. Add `legacyDomainEncode` to preserve the old encoding
logic for backward-compatible matching of existing stored actions.
* fix: backward-compatible tool matching in ToolService
Update `getActionToolDefinitions` to match stored tools against both
new and legacy domain encodings. Update `loadActionToolsForExecution`
to resolve model-called tool names via a `normalizedToDomain` map
that includes both encoding variants, with legacy fallback for
request builder lookup.
* fix: action route save/delete domain encoding issues
Save routes now remove old tools matching either new or legacy domain
encoding, preventing stale entries when an action's encoding changes
on update.
Delete routes no longer re-encode the already-encoded domain extracted
from the stored actions array, which was producing incorrect keys and
leaving orphaned tools.
* test: comprehensive coverage for action domain encoding
Rewrite ActionService tests to cover real matching patterns used by
ToolService and action routes. Tests verify encode/decode round-trips,
protocol stripping, backward-compatible tool name matching at both
definition and execution phases, save-route cleanup of old/new
encodings, delete-route domain extraction, and the collision fix for
multi-action agents.
* fix: add legacy domain compat to all execution paths, make legacyDomainEncode sync
CRITICAL: processRequiredActions (assistants path) was not updated with
legacy domain matching — existing assistants with https:// domain actions
would silently fail post-deployment because domainMap only had new encoding.
MAJOR: loadAgentTools definitionsOnly=false path had the same issue.
Both now use a normalizedToDomain map with legacy+new entries and extract
function names via the matched key (not the canonical domain).
Also: make legacyDomainEncode synchronous (no async operations), store
legacyNormalized in processedActionSets to eliminate recomputation in
the per-tool fallback, and hoist domainSeparatorRegex to module level.
* refactor: clarify domain variable naming and tool-filter helpers in action routes
Rename shadowed 'domain' to 'encodedDomain' to separate raw URL from
encoded key in both agent and assistant save routes.
Rename shouldRemoveTool to shouldRemoveAgentTool / shouldRemoveAssistantTool
to make the distinct data-shape guards explicit.
Remove await on now-synchronous legacyDomainEncode.
* test: expand coverage for all review findings
- Add validateAndUpdateTool tests (protocol-stripping match logic)
- Restore unicode domain encode/decode/round-trip tests
- Add processRequiredActions matching pattern tests (assistants path)
- Add legacy guard skip test for short bare hostnames
- Add pre-normalized Set test for definition-phase optimization
- Fix corrupt-cache test to assert typeof instead of toBeDefined
- Verify legacyDomainEncode is synchronous (not a Promise)
- Remove all await on legacyDomainEncode (now sync)
58 tests, up from 44.
* fix: address follow-up review findings A-E
A: Fix stale JSDoc @returns {Promise<string>} on now-synchronous
legacyDomainEncode — changed to @returns {string}.
B: Rename normalizedToDomain to domainLookupMap in processRequiredActions
and loadAgentTools where keys are raw encoded domains (not normalized),
avoiding confusion with loadActionToolsForExecution where keys ARE
normalized.
C: Pre-normalize actionToolNames into a Set<string> in
getActionToolDefinitions, replacing O(signatures × tools) per-check
.some() + .replace() with O(1) Set.has() lookups.
D: Remove stripProtocol from ActionService exports — it is a one-line
internal helper. Spec tests for it removed; behavior is fully covered
by domainParser protocol-stripping tests.
E: Fix pre-existing bug where processRequiredActions re-loaded action
sets on every missing-tool iteration. The guard !actionSets.length
always re-triggered because actionSets was reassigned to a plain
object (whose .length is undefined). Replaced with a null-check
on a dedicated actionSetsData variable.
* fix: strip path and query from domain URLs in stripProtocol
URLs like 'https://api.example.com/v1/endpoint?foo=bar' previously
retained the path after protocol stripping, contaminating the encoded
domain key. Now strips everything after the first '/' following the
host, using string indexing instead of URL parsing to avoid punycode
normalization of unicode hostnames.
Closes Copilot review comments 1, 2, and 5.
This commit is contained in:
Danny Avila
GitHub
parent
c68066a636
commit
9a64791e3e
5 changed files with 693 additions and 227 deletions
|
|
@ -42,6 +42,7 @@ const {
|
|||
} = require('librechat-data-provider');
|
||||
const {
|
||||
createActionTool,
|
||||
legacyDomainEncode,
|
||||
decryptMetadata,
|
||||
loadActionSets,
|
||||
domainParser,
|
||||
|
|
@ -65,6 +66,8 @@ const { findPluginAuthsByKeys } = require('~/models');
|
|||
const { getFlowStateManager } = require('~/config');
|
||||
const { getLogStores } = require('~/cache');
|
||||
|
||||
const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g');
|
||||
|
||||
/**
|
||||
* Resolves the set of enabled agent capabilities from endpoints config,
|
||||
* falling back to app-level or default capabilities for ephemeral agents.
|
||||
|
|
@ -172,8 +175,7 @@ async function processRequiredActions(client, requiredActions) {
|
|||
|
||||
const promises = [];
|
||||
|
||||
/** @type {Action[]} */
|
||||
let actionSets = [];
|
||||
let actionSetsData = null;
|
||||
let isActionTool = false;
|
||||
const ActionToolMap = {};
|
||||
const ActionBuildersMap = {};
|
||||
|
|
@ -259,9 +261,9 @@ async function processRequiredActions(client, requiredActions) {
|
|||
if (!tool) {
|
||||
// throw new Error(`Tool ${currentAction.tool} not found.`);
|
||||
|
||||
// Load all action sets once if not already loaded
|
||||
if (!actionSets.length) {
|
||||
actionSets =
|
||||
if (!actionSetsData) {
|
||||
/** @type {Action[]} */
|
||||
const actionSets =
|
||||
(await loadActionSets({
|
||||
assistant_id: client.req.body.assistant_id,
|
||||
})) ?? [];
|
||||
|
|
@ -269,11 +271,16 @@ async function processRequiredActions(client, requiredActions) {
|
|||
// Process all action sets once
|
||||
// Map domains to their processed action sets
|
||||
const processedDomains = new Map();
|
||||
const domainMap = new Map();
|
||||
const domainLookupMap = new Map();
|
||||
|
||||
for (const action of actionSets) {
|
||||
const domain = await domainParser(action.metadata.domain, true);
|
||||
domainMap.set(domain, action);
|
||||
domainLookupMap.set(domain, domain);
|
||||
|
||||
const legacyDomain = legacyDomainEncode(action.metadata.domain);
|
||||
if (legacyDomain !== domain) {
|
||||
domainLookupMap.set(legacyDomain, domain);
|
||||
}
|
||||
|
||||
const isDomainAllowed = await isActionDomainAllowed(
|
||||
action.metadata.domain,
|
||||
|
|
@ -328,27 +335,26 @@ async function processRequiredActions(client, requiredActions) {
|
|||
ActionBuildersMap[action.metadata.domain] = requestBuilders;
|
||||
}
|
||||
|
||||
// Update actionSets reference to use the domain map
|
||||
actionSets = { domainMap, processedDomains };
|
||||
actionSetsData = { domainLookupMap, processedDomains };
|
||||
}
|
||||
|
||||
// Find the matching domain for this tool
|
||||
let currentDomain = '';
|
||||
for (const domain of actionSets.domainMap.keys()) {
|
||||
if (currentAction.tool.includes(domain)) {
|
||||
currentDomain = domain;
|
||||
let matchedKey = '';
|
||||
for (const [key, canonical] of actionSetsData.domainLookupMap.entries()) {
|
||||
if (currentAction.tool.includes(key)) {
|
||||
currentDomain = canonical;
|
||||
matchedKey = key;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
if (!currentDomain || !actionSets.processedDomains.has(currentDomain)) {
|
||||
// TODO: try `function` if no action set is found
|
||||
// throw new Error(`Tool ${currentAction.tool} not found.`);
|
||||
if (!currentDomain || !actionSetsData.processedDomains.has(currentDomain)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
const { action, requestBuilders, encrypted } = actionSets.processedDomains.get(currentDomain);
|
||||
const functionName = currentAction.tool.replace(`${actionDelimiter}${currentDomain}`, '');
|
||||
const { action, requestBuilders, encrypted } =
|
||||
actionSetsData.processedDomains.get(currentDomain);
|
||||
const functionName = currentAction.tool.replace(`${actionDelimiter}${matchedKey}`, '');
|
||||
const requestBuilder = requestBuilders[functionName];
|
||||
|
||||
if (!requestBuilder) {
|
||||
|
|
@ -586,12 +592,17 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to
|
|||
|
||||
const definitions = [];
|
||||
const allowedDomains = appConfig?.actions?.allowedDomains;
|
||||
const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g');
|
||||
const normalizedToolNames = new Set(
|
||||
actionToolNames.map((n) => n.replace(domainSeparatorRegex, '_')),
|
||||
);
|
||||
|
||||
for (const action of actionSets) {
|
||||
const domain = await domainParser(action.metadata.domain, true);
|
||||
const normalizedDomain = domain.replace(domainSeparatorRegex, '_');
|
||||
|
||||
const legacyDomain = legacyDomainEncode(action.metadata.domain);
|
||||
const legacyNormalized = legacyDomain.replace(domainSeparatorRegex, '_');
|
||||
|
||||
const isDomainAllowed = await isActionDomainAllowed(action.metadata.domain, allowedDomains);
|
||||
if (!isDomainAllowed) {
|
||||
logger.warn(
|
||||
|
|
@ -611,7 +622,8 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to
|
|||
|
||||
for (const sig of functionSignatures) {
|
||||
const toolName = `${sig.name}${actionDelimiter}${normalizedDomain}`;
|
||||
if (!actionToolNames.some((name) => name.replace(domainSeparatorRegex, '_') === toolName)) {
|
||||
const legacyToolName = `${sig.name}${actionDelimiter}${legacyNormalized}`;
|
||||
if (!normalizedToolNames.has(toolName) && !normalizedToolNames.has(legacyToolName)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
|
|
@ -990,15 +1002,17 @@ async function loadAgentTools({
|
|||
};
|
||||
}
|
||||
|
||||
// Process each action set once (validate spec, decrypt metadata)
|
||||
const processedActionSets = new Map();
|
||||
const domainMap = new Map();
|
||||
const domainLookupMap = new Map();
|
||||
|
||||
for (const action of actionSets) {
|
||||
const domain = await domainParser(action.metadata.domain, true);
|
||||
domainMap.set(domain, action);
|
||||
domainLookupMap.set(domain, domain);
|
||||
|
||||
// Check if domain is allowed (do this once per action set)
|
||||
const legacyDomain = legacyDomainEncode(action.metadata.domain);
|
||||
if (legacyDomain !== domain) {
|
||||
domainLookupMap.set(legacyDomain, domain);
|
||||
}
|
||||
const isDomainAllowed = await isActionDomainAllowed(
|
||||
action.metadata.domain,
|
||||
appConfig?.actions?.allowedDomains,
|
||||
|
|
@ -1060,11 +1074,12 @@ async function loadAgentTools({
|
|||
continue;
|
||||
}
|
||||
|
||||
// Find the matching domain for this tool
|
||||
let currentDomain = '';
|
||||
for (const domain of domainMap.keys()) {
|
||||
if (toolName.includes(domain)) {
|
||||
currentDomain = domain;
|
||||
let matchedKey = '';
|
||||
for (const [key, canonical] of domainLookupMap.entries()) {
|
||||
if (toolName.includes(key)) {
|
||||
currentDomain = canonical;
|
||||
matchedKey = key;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
|
@ -1075,7 +1090,7 @@ async function loadAgentTools({
|
|||
|
||||
const { action, encrypted, zodSchemas, requestBuilders, functionSignatures } =
|
||||
processedActionSets.get(currentDomain);
|
||||
const functionName = toolName.replace(`${actionDelimiter}${currentDomain}`, '');
|
||||
const functionName = toolName.replace(`${actionDelimiter}${matchedKey}`, '');
|
||||
const functionSig = functionSignatures.find((sig) => sig.name === functionName);
|
||||
const requestBuilder = requestBuilders[functionName];
|
||||
const zodSchema = zodSchemas[functionName];
|
||||
|
|
@ -1310,12 +1325,20 @@ async function loadActionToolsForExecution({
|
|||
}
|
||||
|
||||
const processedActionSets = new Map();
|
||||
const domainMap = new Map();
|
||||
/** Maps both new and legacy normalized domains to their canonical (new) domain key */
|
||||
const normalizedToDomain = new Map();
|
||||
const allowedDomains = appConfig?.actions?.allowedDomains;
|
||||
|
||||
for (const action of actionSets) {
|
||||
const domain = await domainParser(action.metadata.domain, true);
|
||||
domainMap.set(domain, action);
|
||||
const normalizedDomain = domain.replace(domainSeparatorRegex, '_');
|
||||
normalizedToDomain.set(normalizedDomain, domain);
|
||||
|
||||
const legacyDomain = legacyDomainEncode(action.metadata.domain);
|
||||
const legacyNormalized = legacyDomain.replace(domainSeparatorRegex, '_');
|
||||
if (legacyNormalized !== normalizedDomain) {
|
||||
normalizedToDomain.set(legacyNormalized, domain);
|
||||
}
|
||||
|
||||
const isDomainAllowed = await isActionDomainAllowed(action.metadata.domain, allowedDomains);
|
||||
if (!isDomainAllowed) {
|
||||
|
|
@ -1364,16 +1387,15 @@ async function loadActionToolsForExecution({
|
|||
functionSignatures,
|
||||
zodSchemas,
|
||||
encrypted,
|
||||
legacyNormalized,
|
||||
});
|
||||
}
|
||||
|
||||
const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g');
|
||||
for (const toolName of actionToolNames) {
|
||||
let currentDomain = '';
|
||||
for (const domain of domainMap.keys()) {
|
||||
const normalizedDomain = domain.replace(domainSeparatorRegex, '_');
|
||||
for (const [normalizedDomain, canonicalDomain] of normalizedToDomain.entries()) {
|
||||
if (toolName.includes(normalizedDomain)) {
|
||||
currentDomain = domain;
|
||||
currentDomain = canonicalDomain;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
|
@ -1382,7 +1404,7 @@ async function loadActionToolsForExecution({
|
|||
continue;
|
||||
}
|
||||
|
||||
const { action, encrypted, zodSchemas, requestBuilders, functionSignatures } =
|
||||
const { action, encrypted, zodSchemas, requestBuilders, functionSignatures, legacyNormalized } =
|
||||
processedActionSets.get(currentDomain);
|
||||
const normalizedDomain = currentDomain.replace(domainSeparatorRegex, '_');
|
||||
const functionName = toolName.replace(`${actionDelimiter}${normalizedDomain}`, '');
|
||||
|
|
@ -1391,6 +1413,25 @@ async function loadActionToolsForExecution({
|
|||
const zodSchema = zodSchemas[functionName];
|
||||
|
||||
if (!requestBuilder) {
|
||||
const legacyFnName = toolName.replace(`${actionDelimiter}${legacyNormalized}`, '');
|
||||
if (legacyFnName !== toolName && requestBuilders[legacyFnName]) {
|
||||
const legacyTool = await createActionTool({
|
||||
userId: req.user.id,
|
||||
res,
|
||||
action,
|
||||
streamId,
|
||||
encrypted,
|
||||
requestBuilder: requestBuilders[legacyFnName],
|
||||
zodSchema: zodSchemas[legacyFnName],
|
||||
name: toolName,
|
||||
description:
|
||||
functionSignatures.find((sig) => sig.name === legacyFnName)?.description ?? '',
|
||||
useSSRFProtection: !Array.isArray(allowedDomains) || allowedDomains.length === 0,
|
||||
});
|
||||
if (legacyTool) {
|
||||
loadedActionTools.push(legacyTool);
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue