mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-19 22:26:33 +01:00
* 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.
287 lines
9.5 KiB
JavaScript
287 lines
9.5 KiB
JavaScript
const express = require('express');
|
|
const { nanoid } = require('nanoid');
|
|
const { logger } = require('@librechat/data-schemas');
|
|
const { generateCheckAccess, isActionDomainAllowed } = require('@librechat/api');
|
|
const {
|
|
Permissions,
|
|
ResourceType,
|
|
PermissionBits,
|
|
PermissionTypes,
|
|
actionDelimiter,
|
|
removeNullishValues,
|
|
validateActionDomain,
|
|
validateAndParseOpenAPISpec,
|
|
} = require('librechat-data-provider');
|
|
const {
|
|
legacyDomainEncode,
|
|
encryptMetadata,
|
|
domainParser,
|
|
} = require('~/server/services/ActionService');
|
|
const { findAccessibleResources } = require('~/server/services/PermissionService');
|
|
const { getAgent, updateAgent, getListAgentsByAccess } = require('~/models/Agent');
|
|
const { updateAction, getActions, deleteAction } = require('~/models/Action');
|
|
const { canAccessAgentResource } = require('~/server/middleware');
|
|
const { getRoleByName } = require('~/models/Role');
|
|
|
|
const router = express.Router();
|
|
|
|
const checkAgentCreate = generateCheckAccess({
|
|
permissionType: PermissionTypes.AGENTS,
|
|
permissions: [Permissions.USE, Permissions.CREATE],
|
|
getRoleByName,
|
|
});
|
|
|
|
/**
|
|
* Retrieves all user's actions
|
|
* @route GET /actions/
|
|
* @param {string} req.params.id - Assistant identifier.
|
|
* @returns {Action[]} 200 - success response - application/json
|
|
*/
|
|
router.get('/', async (req, res) => {
|
|
try {
|
|
const userId = req.user.id;
|
|
const editableAgentObjectIds = await findAccessibleResources({
|
|
userId,
|
|
role: req.user.role,
|
|
resourceType: ResourceType.AGENT,
|
|
requiredPermissions: PermissionBits.EDIT,
|
|
});
|
|
|
|
const agentsResponse = await getListAgentsByAccess({
|
|
accessibleIds: editableAgentObjectIds,
|
|
});
|
|
|
|
const editableAgentIds = agentsResponse.data.map((agent) => agent.id);
|
|
const actions =
|
|
editableAgentIds.length > 0 ? await getActions({ agent_id: { $in: editableAgentIds } }) : [];
|
|
|
|
res.json(actions);
|
|
} catch (error) {
|
|
res.status(500).json({ error: error.message });
|
|
}
|
|
});
|
|
|
|
/**
|
|
* Adds or updates actions for a specific agent.
|
|
* @route POST /actions/:agent_id
|
|
* @param {string} req.params.agent_id - The ID of the agent.
|
|
* @param {FunctionTool[]} req.body.functions - The functions to be added or updated.
|
|
* @param {string} [req.body.action_id] - Optional ID for the action.
|
|
* @param {ActionMetadata} req.body.metadata - Metadata for the action.
|
|
* @returns {Object} 200 - success response - application/json
|
|
*/
|
|
router.post(
|
|
'/:agent_id',
|
|
canAccessAgentResource({
|
|
requiredPermission: PermissionBits.EDIT,
|
|
resourceIdParam: 'agent_id',
|
|
}),
|
|
checkAgentCreate,
|
|
async (req, res) => {
|
|
try {
|
|
const { agent_id } = req.params;
|
|
|
|
/** @type {{ functions: FunctionTool[], action_id: string, metadata: ActionMetadata }} */
|
|
const { functions, action_id: _action_id, metadata: _metadata } = req.body;
|
|
if (!functions.length) {
|
|
return res.status(400).json({ message: 'No functions provided' });
|
|
}
|
|
|
|
let metadata = await encryptMetadata(removeNullishValues(_metadata, true));
|
|
const appConfig = req.config;
|
|
|
|
// SECURITY: Validate the OpenAPI spec and extract the server URL
|
|
if (metadata.raw_spec) {
|
|
const validationResult = validateAndParseOpenAPISpec(metadata.raw_spec);
|
|
if (!validationResult.status || !validationResult.serverUrl) {
|
|
return res.status(400).json({
|
|
message: validationResult.message || 'Invalid OpenAPI specification',
|
|
});
|
|
}
|
|
|
|
// SECURITY: Validate the client-provided domain matches the spec's server URL domain
|
|
// This prevents SSRF attacks where an attacker provides a whitelisted domain
|
|
// but uses a different (potentially internal) URL in the raw_spec
|
|
const domainValidation = validateActionDomain(metadata.domain, validationResult.serverUrl);
|
|
if (!domainValidation.isValid) {
|
|
logger.warn(`Domain mismatch detected: ${domainValidation.message}`, {
|
|
userId: req.user.id,
|
|
agent_id,
|
|
});
|
|
return res.status(400).json({
|
|
message:
|
|
'Domain mismatch: The domain in the OpenAPI spec does not match the provided domain',
|
|
});
|
|
}
|
|
}
|
|
|
|
const isDomainAllowed = await isActionDomainAllowed(
|
|
metadata.domain,
|
|
appConfig?.actions?.allowedDomains,
|
|
);
|
|
if (!isDomainAllowed) {
|
|
return res.status(400).json({ message: 'Domain not allowed' });
|
|
}
|
|
|
|
const encodedDomain = await domainParser(metadata.domain, true);
|
|
|
|
if (!encodedDomain) {
|
|
return res.status(400).json({ message: 'No domain provided' });
|
|
}
|
|
|
|
const legacyDomain = legacyDomainEncode(metadata.domain);
|
|
|
|
const action_id = _action_id ?? nanoid();
|
|
const initialPromises = [];
|
|
|
|
// Permissions already validated by middleware - load agent directly
|
|
initialPromises.push(getAgent({ id: agent_id }));
|
|
if (_action_id) {
|
|
initialPromises.push(getActions({ action_id }, true));
|
|
}
|
|
|
|
/** @type {[Agent, [Action|undefined]]} */
|
|
const [agent, actions_result] = await Promise.all(initialPromises);
|
|
if (!agent) {
|
|
return res.status(404).json({ message: 'Agent not found for adding action' });
|
|
}
|
|
|
|
if (actions_result && actions_result.length) {
|
|
const action = actions_result[0];
|
|
if (action.agent_id !== agent_id) {
|
|
return res.status(403).json({ message: 'Action does not belong to this agent' });
|
|
}
|
|
metadata = { ...action.metadata, ...metadata };
|
|
}
|
|
|
|
const { actions: _actions = [], author: agent_author } = agent ?? {};
|
|
const actions = [];
|
|
for (const action of _actions) {
|
|
const [_action_domain, current_action_id] = action.split(actionDelimiter);
|
|
if (current_action_id === action_id) {
|
|
continue;
|
|
}
|
|
|
|
actions.push(action);
|
|
}
|
|
|
|
actions.push(`${encodedDomain}${actionDelimiter}${action_id}`);
|
|
|
|
/** @type {string[]}} */
|
|
const { tools: _tools = [] } = agent;
|
|
|
|
const shouldRemoveAgentTool = (tool) => {
|
|
if (!tool) {
|
|
return false;
|
|
}
|
|
return (
|
|
tool.includes(encodedDomain) || tool.includes(legacyDomain) || tool.includes(action_id)
|
|
);
|
|
};
|
|
|
|
const tools = _tools
|
|
.filter((tool) => !shouldRemoveAgentTool(tool))
|
|
.concat(functions.map((tool) => `${tool.function.name}${actionDelimiter}${encodedDomain}`));
|
|
|
|
// Force version update since actions are changing
|
|
const updatedAgent = await updateAgent(
|
|
{ id: agent_id },
|
|
{ tools, actions },
|
|
{
|
|
updatingUserId: req.user.id,
|
|
forceVersion: true,
|
|
},
|
|
);
|
|
|
|
// Only update user field for new actions
|
|
const actionUpdateData = { metadata, agent_id };
|
|
if (!actions_result || !actions_result.length) {
|
|
// For new actions, use the agent owner's user ID
|
|
actionUpdateData.user = agent_author || req.user.id;
|
|
}
|
|
|
|
/** @type {[Action]} */
|
|
const updatedAction = await updateAction({ action_id, agent_id }, actionUpdateData);
|
|
|
|
const sensitiveFields = ['api_key', 'oauth_client_id', 'oauth_client_secret'];
|
|
for (let field of sensitiveFields) {
|
|
if (updatedAction.metadata[field]) {
|
|
delete updatedAction.metadata[field];
|
|
}
|
|
}
|
|
|
|
res.json([updatedAgent, updatedAction]);
|
|
} catch (error) {
|
|
const message = 'Trouble updating the Agent Action';
|
|
logger.error(message, error);
|
|
res.status(500).json({ message });
|
|
}
|
|
},
|
|
);
|
|
|
|
/**
|
|
* Deletes an action for a specific agent.
|
|
* @route DELETE /actions/:agent_id/:action_id
|
|
* @param {string} req.params.agent_id - The ID of the agent.
|
|
* @param {string} req.params.action_id - The ID of the action to delete.
|
|
* @returns {Object} 200 - success response - application/json
|
|
*/
|
|
router.delete(
|
|
'/:agent_id/:action_id',
|
|
canAccessAgentResource({
|
|
requiredPermission: PermissionBits.EDIT,
|
|
resourceIdParam: 'agent_id',
|
|
}),
|
|
checkAgentCreate,
|
|
async (req, res) => {
|
|
try {
|
|
const { agent_id, action_id } = req.params;
|
|
|
|
// Permissions already validated by middleware - load agent directly
|
|
const agent = await getAgent({ id: agent_id });
|
|
if (!agent) {
|
|
return res.status(404).json({ message: 'Agent not found for deleting action' });
|
|
}
|
|
|
|
const { tools = [], actions = [] } = agent;
|
|
|
|
let storedDomain = '';
|
|
const updatedActions = actions.filter((action) => {
|
|
if (action.includes(action_id)) {
|
|
[storedDomain] = action.split(actionDelimiter);
|
|
return false;
|
|
}
|
|
return true;
|
|
});
|
|
|
|
if (!storedDomain) {
|
|
return res.status(400).json({ message: 'No domain provided' });
|
|
}
|
|
|
|
const updatedTools = tools.filter(
|
|
(tool) => !(tool && (tool.includes(storedDomain) || tool.includes(action_id))),
|
|
);
|
|
|
|
// Force version update since actions are being removed
|
|
await updateAgent(
|
|
{ id: agent_id },
|
|
{ tools: updatedTools, actions: updatedActions },
|
|
{ updatingUserId: req.user.id, forceVersion: true },
|
|
);
|
|
const deleted = await deleteAction({ action_id, agent_id });
|
|
if (!deleted) {
|
|
logger.warn('[Agent Action Delete] No matching action document found', {
|
|
action_id,
|
|
agent_id,
|
|
});
|
|
}
|
|
res.status(200).json({ message: 'Action deleted successfully' });
|
|
} catch (error) {
|
|
const message = 'Trouble deleting the Agent Action';
|
|
logger.error(message, error);
|
|
res.status(500).json({ message });
|
|
}
|
|
},
|
|
);
|
|
|
|
module.exports = router;
|