mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-31 12:57:20 +02:00
🗝️ fix: Exempt Admin-Trusted Domains from MCP OAuth Validation (#12255)
* fix: exempt allowedDomains from MCP OAuth SSRF checks (#12254) The SSRF guard in validateOAuthUrl was context-blind — it blocked private/internal OAuth endpoints even for admin-trusted MCP servers listed in mcpSettings.allowedDomains. Add isHostnameAllowed() to domain.ts and skip SSRF checks in validateOAuthUrl when the OAuth endpoint hostname matches an allowed domain. * refactor: thread allowedDomains through MCP connection stack Pass allowedDomains from MCPServersRegistry through BasicConnectionOptions, MCPConnectionFactory, and into MCPOAuthHandler method calls so the OAuth layer can exempt admin-trusted domains from SSRF validation. * test: add allowedDomains bypass tests and fix registry mocks Add isHostnameAllowed unit tests (exact, wildcard, case-insensitive, private IPs). Add MCPOAuthSecurity tests covering the allowedDomains bypass for initiateOAuthFlow, refreshOAuthTokens, and revokeOAuthToken. Update registry mocks to include getAllowedDomains. * fix: enforce protocol/port constraints in OAuth allowedDomains bypass Replace isHostnameAllowed (hostname-only check) with isOAuthUrlAllowed which parses the full OAuth URL and matches against allowedDomains entries including protocol and explicit port constraints — mirroring isDomainAllowedCore's allowlist logic. Prevents a port-scoped entry like 'https://auth.internal:8443' from also exempting other ports. * test: cover auto-discovery and branch-3 refresh paths with allowedDomains Add three new integration tests using a real OAuth test server: - auto-discovered OAuth endpoints allowed when server IP is in allowedDomains - auto-discovered endpoints rejected when allowedDomains doesn't match - refreshOAuthTokens branch 3 (no clientInfo/config) with allowedDomains bypass Also rename describe block from ephemeral issue number to durable name. * docs: explain intentional absence of allowedDomains in completeOAuthFlow Prevents future contributors from assuming a missing parameter during security audits — URLs are pre-validated during initiateOAuthFlow. * test: update initiateOAuthFlow assertion for allowedDomains parameter * perf: avoid redundant URL parse for admin-trusted OAuth endpoints Move isOAuthUrlAllowed check before the hostname extraction so admin-trusted URLs short-circuit with a single URL parse instead of two. The hostname extraction (new URL) is now deferred to the SSRF-check path where it's actually needed.
This commit is contained in:
parent
8e8fb01d18
commit
acd07e8085
15 changed files with 432 additions and 18 deletions
|
|
@ -8,6 +8,7 @@ import {
|
|||
extractMCPServerDomain,
|
||||
isActionDomainAllowed,
|
||||
isEmailDomainAllowed,
|
||||
isOAuthUrlAllowed,
|
||||
isMCPDomainAllowed,
|
||||
isPrivateIP,
|
||||
isSSRFTarget,
|
||||
|
|
@ -1211,6 +1212,96 @@ describe('isMCPDomainAllowed', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('isOAuthUrlAllowed', () => {
|
||||
it('should return false when allowedDomains is null/undefined/empty', () => {
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', null)).toBe(false);
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', undefined)).toBe(false);
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', [])).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false for unparseable URLs', () => {
|
||||
expect(isOAuthUrlAllowed('not-a-url', ['example.com'])).toBe(false);
|
||||
});
|
||||
|
||||
it('should match exact hostnames', () => {
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', ['example.com'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://other.com/token', ['example.com'])).toBe(false);
|
||||
});
|
||||
|
||||
it('should match wildcard subdomains', () => {
|
||||
expect(isOAuthUrlAllowed('https://api.example.com/token', ['*.example.com'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://deep.nested.example.com/token', ['*.example.com'])).toBe(
|
||||
true,
|
||||
);
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', ['*.example.com'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://other.com/token', ['*.example.com'])).toBe(false);
|
||||
});
|
||||
|
||||
it('should be case-insensitive', () => {
|
||||
expect(isOAuthUrlAllowed('https://EXAMPLE.COM/token', ['example.com'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://example.com/token', ['EXAMPLE.COM'])).toBe(true);
|
||||
});
|
||||
|
||||
it('should match private/internal URLs when hostname is in allowedDomains', () => {
|
||||
expect(isOAuthUrlAllowed('http://localhost:8080/token', ['localhost'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('http://10.0.0.1/token', ['10.0.0.1'])).toBe(true);
|
||||
expect(
|
||||
isOAuthUrlAllowed('http://host.docker.internal:8044/token', ['host.docker.internal']),
|
||||
).toBe(true);
|
||||
expect(isOAuthUrlAllowed('http://myserver.local/token', ['*.local'])).toBe(true);
|
||||
});
|
||||
|
||||
it('should match internal URLs with wildcard patterns', () => {
|
||||
expect(isOAuthUrlAllowed('https://auth.company.internal/token', ['*.company.internal'])).toBe(
|
||||
true,
|
||||
);
|
||||
expect(isOAuthUrlAllowed('https://company.internal/token', ['*.company.internal'])).toBe(true);
|
||||
});
|
||||
|
||||
it('should not match when hostname is absent from allowedDomains', () => {
|
||||
expect(isOAuthUrlAllowed('http://10.0.0.1/token', ['192.168.1.1'])).toBe(false);
|
||||
expect(isOAuthUrlAllowed('http://localhost/token', ['host.docker.internal'])).toBe(false);
|
||||
});
|
||||
|
||||
describe('protocol and port constraint enforcement', () => {
|
||||
it('should enforce protocol when allowedDomains specifies one', () => {
|
||||
expect(isOAuthUrlAllowed('https://auth.internal/token', ['https://auth.internal'])).toBe(
|
||||
true,
|
||||
);
|
||||
expect(isOAuthUrlAllowed('http://auth.internal/token', ['https://auth.internal'])).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it('should allow any protocol when allowedDomains has bare hostname', () => {
|
||||
expect(isOAuthUrlAllowed('http://auth.internal/token', ['auth.internal'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://auth.internal/token', ['auth.internal'])).toBe(true);
|
||||
});
|
||||
|
||||
it('should enforce port when allowedDomains specifies one', () => {
|
||||
expect(
|
||||
isOAuthUrlAllowed('https://auth.internal:8443/token', ['https://auth.internal:8443']),
|
||||
).toBe(true);
|
||||
expect(
|
||||
isOAuthUrlAllowed('https://auth.internal:6379/token', ['https://auth.internal:8443']),
|
||||
).toBe(false);
|
||||
expect(isOAuthUrlAllowed('https://auth.internal/token', ['https://auth.internal:8443'])).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it('should allow any port when allowedDomains has no explicit port', () => {
|
||||
expect(isOAuthUrlAllowed('https://auth.internal:8443/token', ['auth.internal'])).toBe(true);
|
||||
expect(isOAuthUrlAllowed('https://auth.internal:22/token', ['auth.internal'])).toBe(true);
|
||||
});
|
||||
|
||||
it('should reject wrong port even when hostname matches (prevents port-scanning)', () => {
|
||||
expect(isOAuthUrlAllowed('http://10.0.0.1:6379/token', ['http://10.0.0.1:8080'])).toBe(false);
|
||||
expect(isOAuthUrlAllowed('http://10.0.0.1:25/token', ['http://10.0.0.1:8080'])).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('validateEndpointURL', () => {
|
||||
afterEach(() => {
|
||||
jest.clearAllMocks();
|
||||
|
|
|
|||
|
|
@ -500,6 +500,52 @@ export async function isMCPDomainAllowed(
|
|||
return isDomainAllowedCore(domain, allowedDomains, MCP_PROTOCOLS);
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks whether an OAuth URL matches any entry in the MCP allowedDomains list,
|
||||
* honoring protocol and port constraints when specified by the admin.
|
||||
*
|
||||
* Mirrors the allowlist-matching logic of {@link isDomainAllowedCore} (hostname,
|
||||
* protocol, and explicit-port checks) but is synchronous — no DNS resolution is
|
||||
* needed because the caller is deciding whether to *skip* the subsequent
|
||||
* SSRF/DNS checks, not replace them.
|
||||
*
|
||||
* @remarks `parseDomainSpec` normalizes `www.` prefixes, so both the input URL
|
||||
* and allowedDomains entries starting with `www.` are matched without that prefix.
|
||||
*/
|
||||
export function isOAuthUrlAllowed(url: string, allowedDomains?: string[] | null): boolean {
|
||||
if (!Array.isArray(allowedDomains) || allowedDomains.length === 0) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const inputSpec = parseDomainSpec(url);
|
||||
if (!inputSpec) {
|
||||
return false;
|
||||
}
|
||||
|
||||
for (const allowedDomain of allowedDomains) {
|
||||
const allowedSpec = parseDomainSpec(allowedDomain);
|
||||
if (!allowedSpec) {
|
||||
continue;
|
||||
}
|
||||
if (!hostnameMatches(inputSpec.hostname, allowedSpec)) {
|
||||
continue;
|
||||
}
|
||||
if (allowedSpec.protocol !== null) {
|
||||
if (inputSpec.protocol === null || inputSpec.protocol !== allowedSpec.protocol) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
if (allowedSpec.explicitPort) {
|
||||
if (!inputSpec.explicitPort || inputSpec.port !== allowedSpec.port) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/** Matches ErrorTypes.INVALID_BASE_URL — string literal avoids build-time dependency on data-provider */
|
||||
const INVALID_BASE_URL_TYPE = 'invalid_base_url';
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue