mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-28 20:54:59 +00:00
fix(mcp): bound query expansions, resolve the head injection point safely, scope caches by connection
Compile form-style query expansions as a bounded ordered sequence in which each non-exploded
declared variable contributes at most once, so db://items{?id} no longer authorizes
db://items?id=public&id=admin, which no conforming expansion produces and which a server using
first or last value semantics would resolve. Prefix bounds compose as a per-key value limit, an
explode modifier still permits its own key to repeat, and query expansions now share the same
declared-variable ceiling as the other ordered compiler.
Resolve the sandbox bootstrap injection point by scanning past comments, CDATA and raw-text spans
instead of matching the first head-like text. App HTML containing a commented head tag previously
received the attestation bootstrap inside the comment, so it never executed and every bridge message
queued until the view timed out. Anything unresolvable falls back to wrapping the document, which
keeps the marker check and the bootstrap ahead of all untrusted markup.
Key app tool and resource authorization metadata by connection identity rather than by user when the
connection is the shared app-level instance. Every user of a global server previously added a
permanent entry to six maps, and the per-user cleanup path never runs for app-level connections.
User-scoped connections keep separate entries, since their tool sets and visibility can differ.
Also drop three unrelated files that a previous merge committed by accident: two draft patch
artifacts and a local docker compose override.
This commit is contained in:
Dustin Healy
parent
7ac9fd31eb
commit
cd80c8d68e
8 changed files with 366 additions and 733 deletions
|
|
@ -1,62 +0,0 @@
|
|||
# fix: filter admin config reads by section-scoped read capability
|
||||
|
||||
**Branch:** `fix/admin-config-section-scoped-read-filtering` (base: `main`)
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
`listConfigs`, `getBaseConfig`, and `getConfig` (the admin config read handlers) only ever checked the broad `read:configs` capability, so a caller holding nothing but `read:configs:<section>` grants got a blanket 403 on all three instead of a response filtered to the sections they actually hold. Any deployment using section-scoped config grants hits this.
|
||||
|
||||
Adds `hasAnyConfigReadAccess` as a cheap pre-flight check (true if the caller holds the broad `read:configs`/`manage:configs` capability or any section-scoped `read:configs:<section>`/`manage:configs:<section>` grant), so a zero-access caller still 403s before a DB fetch, while a section-scoped caller passes through and gets the response filtered to exactly what they hold instead of being denied outright. `manage:configs`/`manage:configs:<section>` are included because manage already implies read; a caller who can write a section must be able to read it too.
|
||||
|
||||
The actual section resolution (`getReadableConfigSections`) resolves every section for a request in one batched `getHeldCapabilities` query instead of one `hasConfigCapability` round trip per section, so `/base` no longer fans out roughly one query per `AppConfig` section. The same manage-implies-read rule applies here: fixed at its root in `getParentCapabilities` (`packages/data-schemas/src/methods/systemGrant.ts`) so a `manage:configs:<section>`-only caller correctly sees the section they manage, rather than passing the pre-flight and then having that exact section stripped by the resolution query.
|
||||
|
||||
Includes the `AppConfig` field-renaming normalization (`interfaceConfig`/`turnstileConfig`/`mcpConfig`), needed so the filter checks the canonical section name rather than the renamed response field; without it, a caller holding `read:configs:interface` would have `interfaceConfig` incorrectly stripped from the response.
|
||||
|
||||
`STRUCTURAL_APP_CONFIG_KEYS` (the small set of top-level response keys exempt from the generic per-key check) no longer includes `fileStrategy`. Unlike `paths` (a server-computed constant with no corresponding `TCustomConfig` section at all) and the nested `config` container (exempted only so its own contents get filtered individually instead of the whole object being dropped), `fileStrategy` is a genuine, grantable section. Exempting it meant it was always returned regardless of what the caller actually holds.
|
||||
|
||||
`availableTools` stays in that exemption set for the same structural reason `config` does (there is no `read:configs:availableTools` grant type to check), but it is no longer unconditionally returned. It is derived from the `filteredTools`/`includedTools` sections plus a filesystem scan, so it is now gated on the caller holding read access to either of those two source sections instead of being shown to everyone regardless of grants.
|
||||
|
||||
## Change Type
|
||||
|
||||
- [x] Bug fix (non-breaking change which fixes an issue)
|
||||
|
||||
## Testing
|
||||
|
||||
Added `describe('read handlers: section-scoped-only caller (no broad read:configs)', ...)` to `config.handler.spec.ts` covering `getConfig`, `listConfigs`, and `getBaseConfig` for a caller holding only a single section-scoped read grant, plus cases for the field-renaming normalization and the `availableTools` gating (present when the caller holds `filteredTools` or `includedTools`, stripped otherwise). Existing 403 tests for a caller with no capability at all were updated to also mock `hasAnyConfigReadAccess: false`.
|
||||
|
||||
Added regression coverage in `systemGrant.spec.ts` and `capabilities.integration.spec.ts` for a broad `manage:configs` holder, a section-scoped `manage:configs:<section>` holder, and confirming a different section's manage grant does not leak read access into an unrelated section. Added coverage in `capabilities.integration.spec.ts` asserting `getReadableConfigSections` resolves an entire request's sections via a single `getHeldCapabilities` call regardless of section count.
|
||||
|
||||
Also verified live against a real backend and real MongoDB, comparing `main` and this branch with the same role, holding only `access:admin` and `read:configs:memory` (no broad `read:configs`):
|
||||
|
||||
**Before** (`main`):
|
||||
```
|
||||
GET /api/admin/config/base
|
||||
-> 403 {"error":"Insufficient permissions"}
|
||||
```
|
||||
|
||||
**After** (this branch):
|
||||
```
|
||||
GET /api/admin/config/base
|
||||
-> 200 {"config": {
|
||||
"paths": { "uploads": "<server upload dir>", "structuredTools": "<server tools dir>", ... },
|
||||
"config": { "memory": { "disabled": false, "tokenLimit": 3000, ... } },
|
||||
"memory": { "disabled": false, "tokenLimit": 3000, ... }
|
||||
}}
|
||||
```
|
||||
|
||||
Only `memory` (the one section this role actually holds) plus the always-present structural keys (`paths`/`config`) come through; every other section (`endpoints`, `interface`, `mcpServers`, `availableTools`, etc.) is correctly stripped instead of the whole request being denied.
|
||||
|
||||
### **Test Configuration**:
|
||||
- `packages/api`: `config.handler.spec.ts`, `capabilities.spec.ts`, `capabilities.integration.spec.ts` (167 tests) all pass
|
||||
- `packages/data-schemas`: `systemGrant.spec.ts` (118 tests) passes
|
||||
- `tsc --noEmit` clean
|
||||
|
||||
## Checklist
|
||||
|
||||
- [x] My code adheres to this project's style guidelines
|
||||
- [x] I have performed a self-review of my own code
|
||||
- [x] I have written tests demonstrating that my changes are effective or that my feature works
|
||||
- [x] Local unit tests pass with my changes
|
||||
- [x] My changes do not introduce new warnings
|
||||
|
|
@ -1,627 +0,0 @@
|
|||
diff --git a/api/server/middleware/roles/capabilities.js b/api/server/middleware/roles/capabilities.js
|
||||
index 6f2aa43e9..1867dfc16 100644
|
||||
--- a/api/server/middleware/roles/capabilities.js
|
||||
+++ b/api/server/middleware/roles/capabilities.js
|
||||
@@ -1,8 +1,18 @@
|
||||
const { generateCapabilityCheck, capabilityContextMiddleware } = require('@librechat/api');
|
||||
-const { getUserPrincipals, hasCapabilityForPrincipals } = require('~/models');
|
||||
+const {
|
||||
+ getUserPrincipals,
|
||||
+ hasAnyConfigReadAccess,
|
||||
+ hasCapabilityForPrincipals,
|
||||
+} = require('~/models');
|
||||
|
||||
-const { hasCapability, requireCapability, hasConfigCapability } = generateCapabilityCheck({
|
||||
+const {
|
||||
+ hasCapability,
|
||||
+ requireCapability,
|
||||
+ hasConfigCapability,
|
||||
+ hasAnyConfigReadAccess: checkAnyConfigReadAccess,
|
||||
+} = generateCapabilityCheck({
|
||||
getUserPrincipals,
|
||||
+ hasAnyConfigReadAccess,
|
||||
hasCapabilityForPrincipals,
|
||||
});
|
||||
|
||||
@@ -11,4 +21,5 @@ module.exports = {
|
||||
requireCapability,
|
||||
hasConfigCapability,
|
||||
capabilityContextMiddleware,
|
||||
+ hasAnyConfigReadAccess: checkAnyConfigReadAccess,
|
||||
};
|
||||
diff --git a/api/server/routes/admin/config.js b/api/server/routes/admin/config.js
|
||||
index ab7aa01a2..4f0eed2de 100644
|
||||
--- a/api/server/routes/admin/config.js
|
||||
+++ b/api/server/routes/admin/config.js
|
||||
@@ -3,8 +3,9 @@ const { createAdminConfigHandlers } = require('@librechat/api');
|
||||
const { SystemCapabilities } = require('@librechat/data-schemas');
|
||||
const {
|
||||
hasCapability,
|
||||
- hasConfigCapability,
|
||||
requireCapability,
|
||||
+ hasConfigCapability,
|
||||
+ hasAnyConfigReadAccess,
|
||||
} = require('~/server/middleware/roles/capabilities');
|
||||
const { getAppConfig, invalidateConfigCaches } = require('~/server/services/Config');
|
||||
const { requireJwtAuth } = require('~/server/middleware');
|
||||
@@ -23,6 +24,7 @@ const handlers = createAdminConfigHandlers({
|
||||
unsetConfigField: db.unsetConfigField,
|
||||
deleteConfig: db.deleteConfig,
|
||||
toggleConfigActive: db.toggleConfigActive,
|
||||
+ hasAnyConfigReadAccess,
|
||||
hasConfigCapability,
|
||||
hasCapability,
|
||||
getAppConfig,
|
||||
diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts
|
||||
index d802c31f4..08963677f 100644
|
||||
--- a/packages/api/src/admin/config.handler.spec.ts
|
||||
+++ b/packages/api/src/admin/config.handler.spec.ts
|
||||
@@ -70,6 +70,7 @@ function createHandlers(overrides = {}) {
|
||||
deleteConfig: jest.fn().mockResolvedValue({ _id: 'c1' }),
|
||||
toggleConfigActive: jest.fn().mockResolvedValue({ _id: 'c1', isActive: false }),
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(true),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(true),
|
||||
hasCapability: jest.fn().mockResolvedValue(true),
|
||||
|
||||
getAppConfig: jest.fn().mockResolvedValue({ interface: { modelSelect: true } }),
|
||||
@@ -118,6 +119,7 @@ describe('createAdminConfigHandlers', () => {
|
||||
it('returns 403 before DB lookup when user lacks READ_CONFIGS', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(false),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(false),
|
||||
});
|
||||
const req = mockReq({ params: { principalType: 'role', principalId: 'admin' } });
|
||||
const res = mockRes();
|
||||
@@ -178,6 +180,149 @@ describe('createAdminConfigHandlers', () => {
|
||||
});
|
||||
});
|
||||
|
||||
+ describe('read handlers: section-scoped-only caller (no broad read:configs)', () => {
|
||||
+ function sectionOnlyDeps(section: string, overrides: Record<string, unknown> = {}) {
|
||||
+ return {
|
||||
+ hasConfigCapability: jest.fn(
|
||||
+ async (_user: unknown, s: string | null, verb = 'manage') =>
|
||||
+ verb === 'read' && s === section,
|
||||
+ ),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(true),
|
||||
+ ...overrides,
|
||||
+ };
|
||||
+ }
|
||||
+
|
||||
+ it('getConfig: returns 200 with only the held section, other sections stripped', async () => {
|
||||
+ const config = {
|
||||
+ _id: 'c1',
|
||||
+ principalType: 'role',
|
||||
+ principalId: 'admin',
|
||||
+ overrides: { memory: { charLimit: 500 }, endpoints: { allowedAddresses: ['10.0.0.1'] } },
|
||||
+ tombstones: ['memory.tokenLimit', 'endpoints.allowedAddresses'],
|
||||
+ };
|
||||
+ const { handlers } = createHandlers(
|
||||
+ sectionOnlyDeps('memory', { findConfigByPrincipal: jest.fn().mockResolvedValue(config) }),
|
||||
+ );
|
||||
+ const req = mockReq({ params: { principalType: 'role', principalId: 'admin' } });
|
||||
+ const res = mockRes();
|
||||
+
|
||||
+ await handlers.getConfig(req, res);
|
||||
+
|
||||
+ expect(res.statusCode).toBe(200);
|
||||
+ const body = res.body!.config as { overrides: Record<string, unknown>; tombstones: string[] };
|
||||
+ expect(body.overrides.memory).toEqual({ charLimit: 500 });
|
||||
+ expect(body.overrides.endpoints).toBeUndefined();
|
||||
+ expect(body.tombstones).toEqual(['memory.tokenLimit']);
|
||||
+ });
|
||||
+
|
||||
+ it('listConfigs: strips non-held sections from every listed config', async () => {
|
||||
+ const configs = [
|
||||
+ { _id: 'c1', principalType: 'role', principalId: 'admin', overrides: { memory: {} } },
|
||||
+ {
|
||||
+ _id: 'c2',
|
||||
+ principalType: 'user',
|
||||
+ principalId: 'u1',
|
||||
+ overrides: { endpoints: {}, memory: { charLimit: 10 } },
|
||||
+ },
|
||||
+ ];
|
||||
+ const { handlers } = createHandlers(
|
||||
+ sectionOnlyDeps('memory', { listAllConfigs: jest.fn().mockResolvedValue(configs) }),
|
||||
+ );
|
||||
+ const req = mockReq();
|
||||
+ const res = mockRes();
|
||||
+
|
||||
+ await handlers.listConfigs(req, res);
|
||||
+
|
||||
+ expect(res.statusCode).toBe(200);
|
||||
+ const body = res.body!.configs as Array<{ overrides: Record<string, unknown> }>;
|
||||
+ expect(body[0].overrides).toEqual({ memory: {} });
|
||||
+ expect(body[1].overrides).toEqual({ memory: { charLimit: 10 } });
|
||||
+ });
|
||||
+
|
||||
+ it('getBaseConfig: strips top-level sections and the nested config field to only the held section', async () => {
|
||||
+ const appConfig = {
|
||||
+ memory: { charLimit: 500 },
|
||||
+ endpoints: { allowedAddresses: ['10.0.0.1'] },
|
||||
+ fileStrategy: 's3',
|
||||
+ config: { memory: { charLimit: 500 }, endpoints: { allowedAddresses: ['10.0.0.1'] } },
|
||||
+ paths: { uploads: '/tmp' },
|
||||
+ availableTools: { foo: {} },
|
||||
+ };
|
||||
+ const { handlers } = createHandlers(
|
||||
+ sectionOnlyDeps('memory', { getAppConfig: jest.fn().mockResolvedValue(appConfig) }),
|
||||
+ );
|
||||
+ const req = mockReq();
|
||||
+ const res = mockRes();
|
||||
+
|
||||
+ await handlers.getBaseConfig(req, res);
|
||||
+
|
||||
+ expect(res.statusCode).toBe(200);
|
||||
+ const body = res.body!.config as Record<string, unknown>;
|
||||
+ expect(body.memory).toEqual({ charLimit: 500 });
|
||||
+ expect(body.endpoints).toBeUndefined();
|
||||
+ expect(body.fileStrategy).toBeUndefined();
|
||||
+ expect((body.config as Record<string, unknown>).memory).toEqual({ charLimit: 500 });
|
||||
+ expect((body.config as Record<string, unknown>).endpoints).toBeUndefined();
|
||||
+ expect(body.paths).toEqual({ uploads: '/tmp' });
|
||||
+ expect(body.availableTools).toEqual({ foo: {} });
|
||||
+ });
|
||||
+
|
||||
+ it('getBaseConfig: returns fileStrategy only to a caller holding read:configs:fileStrategy', async () => {
|
||||
+ const appConfig = {
|
||||
+ fileStrategy: 's3',
|
||||
+ memory: { charLimit: 500 },
|
||||
+ paths: { uploads: '/tmp' },
|
||||
+ availableTools: {},
|
||||
+ };
|
||||
+ const { handlers } = createHandlers(
|
||||
+ sectionOnlyDeps('fileStrategy', { getAppConfig: jest.fn().mockResolvedValue(appConfig) }),
|
||||
+ );
|
||||
+ const req = mockReq();
|
||||
+ const res = mockRes();
|
||||
+
|
||||
+ await handlers.getBaseConfig(req, res);
|
||||
+
|
||||
+ expect(res.statusCode).toBe(200);
|
||||
+ const body = res.body!.config as Record<string, unknown>;
|
||||
+ expect(body.fileStrategy).toBe('s3');
|
||||
+ expect(body.memory).toBeUndefined();
|
||||
+ });
|
||||
+
|
||||
+ it('getBaseConfig: normalizes renamed top-level fields to their canonical section before checking read access', async () => {
|
||||
+ // getAppConfig renames interface -> interfaceConfig, turnstile -> turnstileConfig,
|
||||
+ // and mcpServers -> mcpConfig in the resolved payload. A caller holding
|
||||
+ // read:configs:interface and read:configs:turnstile (but not mcpServers) must
|
||||
+ // still see interfaceConfig/turnstileConfig, since checking the raw field name
|
||||
+ // against a nonexistent "interfaceConfig"/"turnstileConfig" section would wrongly
|
||||
+ // strip them.
|
||||
+ const appConfig = {
|
||||
+ interfaceConfig: { modelSelect: true },
|
||||
+ turnstileConfig: { siteKey: 'abc' },
|
||||
+ mcpConfig: { docs: {} },
|
||||
+ paths: { uploads: '/tmp' },
|
||||
+ availableTools: {},
|
||||
+ };
|
||||
+ const { handlers } = createHandlers({
|
||||
+ hasConfigCapability: jest.fn(
|
||||
+ async (_user: unknown, s: string | null, verb = 'manage') =>
|
||||
+ verb === 'read' && (s === 'interface' || s === 'turnstile'),
|
||||
+ ),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(true),
|
||||
+ getAppConfig: jest.fn().mockResolvedValue(appConfig),
|
||||
+ });
|
||||
+ const req = mockReq();
|
||||
+ const res = mockRes();
|
||||
+
|
||||
+ await handlers.getBaseConfig(req, res);
|
||||
+
|
||||
+ expect(res.statusCode).toBe(200);
|
||||
+ const body = res.body!.config as Record<string, unknown>;
|
||||
+ expect(body.interfaceConfig).toEqual({ modelSelect: true });
|
||||
+ expect(body.turnstileConfig).toEqual({ siteKey: 'abc' });
|
||||
+ expect(body.mcpConfig).toBeUndefined();
|
||||
+ });
|
||||
+ });
|
||||
+
|
||||
describe('upsertConfigOverrides', () => {
|
||||
it('returns 201 when creating a new config (configVersion === 1)', async () => {
|
||||
const { handlers } = createHandlers({
|
||||
@@ -2031,6 +2176,7 @@ describe('createAdminConfigHandlers', () => {
|
||||
it(`${name} returns 403 when user lacks capability`, async () => {
|
||||
const { handlers } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(false),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(false),
|
||||
});
|
||||
const req = mockReq(reqOverrides);
|
||||
const res = mockRes();
|
||||
@@ -2075,6 +2221,7 @@ describe('createAdminConfigHandlers', () => {
|
||||
it('returns 403 when user lacks READ_CONFIGS', async () => {
|
||||
const { handlers } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(false),
|
||||
+ hasAnyConfigReadAccess: jest.fn().mockResolvedValue(false),
|
||||
});
|
||||
const req = mockReq();
|
||||
const res = mockRes();
|
||||
diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts
|
||||
index f1e3558f0..859268be6 100644
|
||||
--- a/packages/api/src/admin/config.ts
|
||||
+++ b/packages/api/src/admin/config.ts
|
||||
@@ -131,6 +131,8 @@ export interface AdminConfigDeps {
|
||||
section: ConfigSection | null,
|
||||
verb?: 'manage' | 'read',
|
||||
) => Promise<boolean>;
|
||||
+ /** Pre-flight-only: whether the caller holds any config-read capability at all (broad or any section), so a zero-access caller 403s before a DB fetch. */
|
||||
+ hasAnyConfigReadAccess?: (user: CapabilityUser) => Promise<boolean>;
|
||||
hasCapability?: (user: CapabilityUser, capability: SystemCapability) => Promise<boolean>;
|
||||
getAppConfig?: (options?: {
|
||||
role?: string;
|
||||
@@ -183,6 +185,125 @@ function getCapabilityUser(req: ServerRequest): CapabilityUser | null {
|
||||
};
|
||||
}
|
||||
|
||||
+/**
|
||||
+ * `AppConfig` keys exempt from per-section read filtering, for two distinct
|
||||
+ * reasons:
|
||||
+ * - `paths` and `availableTools` are server-computed constants (resolved at
|
||||
+ * module load / from a filesystem scan), identical for every caller. They
|
||||
+ * are not `TCustomConfig` sections, so no `read:configs:<section>` grant
|
||||
+ * exists that could apply to them.
|
||||
+ * - `config` is the nested container whose contents are filtered separately
|
||||
+ * below; checking the outer key against a nonexistent `read:configs:config`
|
||||
+ * grant would always fail and strip the whole object, including sections
|
||||
+ * the caller legitimately holds.
|
||||
+ * Real `TCustomConfig` sections (e.g. `fileStrategy`) must never be added
|
||||
+ * here: exempting one would return it to every caller regardless of grants.
|
||||
+ */
|
||||
+const STRUCTURAL_APP_CONFIG_KEYS = new Set(['paths', 'availableTools', 'config']);
|
||||
+
|
||||
+/**
|
||||
+ * Top-level `AppConfig` response field → canonical `ConfigSection` name.
|
||||
+ * `getAppConfig` renames a few sections in the resolved payload
|
||||
+ * (`interface` → `interfaceConfig`, `turnstile` → `turnstileConfig`,
|
||||
+ * `mcpServers` → `mcpConfig`). The read-grant capability is keyed by the
|
||||
+ * canonical section name, so the top-level filter must normalize through
|
||||
+ * this map before calling `canRead`. Otherwise a caller holding
|
||||
+ * `read:configs:interface` gets `interfaceConfig` incorrectly stripped
|
||||
+ * because no section named "interfaceConfig" exists to grant.
|
||||
+ */
|
||||
+const APP_CONFIG_FIELD_TO_SECTION: Readonly<Record<string, string>> = {
|
||||
+ interfaceConfig: 'interface',
|
||||
+ turnstileConfig: 'turnstile',
|
||||
+ mcpConfig: 'mcpServers',
|
||||
+};
|
||||
+
|
||||
+/** Memoizes `hasConfigCapability` per section for one request's filtering pass. */
|
||||
+function createSectionReadChecker(
|
||||
+ hasConfigCapability: AdminConfigDeps['hasConfigCapability'],
|
||||
+ user: CapabilityUser,
|
||||
+): (section: string) => Promise<boolean> {
|
||||
+ let broad: Promise<boolean> | undefined;
|
||||
+ const perSection = new Map<string, Promise<boolean>>();
|
||||
+
|
||||
+ return async function canRead(section: string): Promise<boolean> {
|
||||
+ if (await (broad ??= hasConfigCapability(user, null, 'read'))) {
|
||||
+ return true;
|
||||
+ }
|
||||
+ let result = perSection.get(section);
|
||||
+ if (!result) {
|
||||
+ result = hasConfigCapability(user, section as ConfigSection, 'read');
|
||||
+ perSection.set(section, result);
|
||||
+ }
|
||||
+ return result;
|
||||
+ };
|
||||
+}
|
||||
+
|
||||
+/** Strips every top-level key not in `preserveKeys` that `canRead` rejects. */
|
||||
+async function filterSectionsByReadAccess<T extends Record<string, unknown>>(
|
||||
+ obj: T,
|
||||
+ canRead: (section: string) => Promise<boolean>,
|
||||
+ preserveKeys: Set<string> = new Set(),
|
||||
+): Promise<T> {
|
||||
+ const keys = Object.keys(obj).filter((key) => !preserveKeys.has(key));
|
||||
+ const allowed = await Promise.all(keys.map((key) => canRead(key)));
|
||||
+ const result: Record<string, unknown> = { ...obj };
|
||||
+ keys.forEach((key, i) => {
|
||||
+ if (!allowed[i]) {
|
||||
+ delete result[key];
|
||||
+ }
|
||||
+ });
|
||||
+ return result as T;
|
||||
+}
|
||||
+
|
||||
+async function filterConfigDocForReadAccess(
|
||||
+ config: IConfig,
|
||||
+ canRead: (section: string) => Promise<boolean>,
|
||||
+): Promise<IConfig> {
|
||||
+ const filteredOverrides = await filterSectionsByReadAccess(
|
||||
+ (config.overrides ?? {}) as Record<string, unknown>,
|
||||
+ canRead,
|
||||
+ );
|
||||
+
|
||||
+ let filteredTombstones = config.tombstones;
|
||||
+ if (config.tombstones?.length) {
|
||||
+ const sections = config.tombstones.map((path) => getTopLevelSection(path));
|
||||
+ const uniqueSections = [...new Set(sections)];
|
||||
+ const readableEntries = await Promise.all(
|
||||
+ uniqueSections.map(async (section) => [section, await canRead(section)] as const),
|
||||
+ );
|
||||
+ const readable = new Map(readableEntries);
|
||||
+ filteredTombstones = config.tombstones.filter((_, i) => readable.get(sections[i]));
|
||||
+ }
|
||||
+
|
||||
+ return {
|
||||
+ ...config,
|
||||
+ overrides: filteredOverrides as Partial<TCustomConfig>,
|
||||
+ tombstones: filteredTombstones,
|
||||
+ } as IConfig;
|
||||
+}
|
||||
+
|
||||
+async function filterAppConfigForReadAccess(
|
||||
+ appConfig: AppConfig,
|
||||
+ canRead: (section: string) => Promise<boolean>,
|
||||
+): Promise<AppConfig> {
|
||||
+ const canReadTopLevelField = (field: string): Promise<boolean> =>
|
||||
+ canRead(APP_CONFIG_FIELD_TO_SECTION[field] ?? field);
|
||||
+
|
||||
+ const filtered = await filterSectionsByReadAccess(
|
||||
+ appConfig as unknown as Record<string, unknown>,
|
||||
+ canReadTopLevelField,
|
||||
+ STRUCTURAL_APP_CONFIG_KEYS,
|
||||
+ );
|
||||
+ const nestedConfig = (filtered as { config?: Record<string, unknown> }).config;
|
||||
+ if (nestedConfig != null && typeof nestedConfig === 'object') {
|
||||
+ (filtered as { config?: unknown }).config = await filterSectionsByReadAccess(
|
||||
+ nestedConfig,
|
||||
+ canRead,
|
||||
+ );
|
||||
+ }
|
||||
+ return filtered as unknown as AppConfig;
|
||||
+}
|
||||
+
|
||||
function redactConfigForResponse(config: IConfig): IConfig {
|
||||
const safeConfig = JSON.parse(JSON.stringify(config)) as IConfig;
|
||||
if (safeConfig.overrides) {
|
||||
@@ -245,6 +366,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
||||
deleteConfig,
|
||||
toggleConfigActive,
|
||||
hasConfigCapability,
|
||||
+ hasAnyConfigReadAccess = async () => false,
|
||||
hasCapability = async () => false,
|
||||
getAppConfig,
|
||||
invalidateConfigCaches,
|
||||
@@ -260,12 +382,17 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
- if (!(await hasConfigCapability(user, null, 'read'))) {
|
||||
+ if (!(await hasAnyConfigReadAccess(user))) {
|
||||
return res.status(403).json({ error: 'Insufficient permissions' });
|
||||
}
|
||||
|
||||
+ const canRead = createSectionReadChecker(hasConfigCapability, user);
|
||||
const configs = await listAllConfigs();
|
||||
- const safeConfigs = configs.map(redactConfigForResponse);
|
||||
+ const filtered = await Promise.all(
|
||||
+ configs.map((config) => filterConfigDocForReadAccess(config, canRead)),
|
||||
+ );
|
||||
+
|
||||
+ const safeConfigs = filtered.map(redactConfigForResponse);
|
||||
return res.status(200).json({ configs: safeConfigs });
|
||||
} catch (error) {
|
||||
logger.error('[adminConfig] listConfigs error:', error);
|
||||
@@ -284,20 +411,23 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
- if (!(await hasConfigCapability(user, null, 'read'))) {
|
||||
- return res.status(403).json({ error: 'Insufficient permissions' });
|
||||
- }
|
||||
-
|
||||
if (!getAppConfig) {
|
||||
return res.status(501).json({ error: 'Base config endpoint not configured' });
|
||||
}
|
||||
|
||||
+ if (!(await hasAnyConfigReadAccess(user))) {
|
||||
+ return res.status(403).json({ error: 'Insufficient permissions' });
|
||||
+ }
|
||||
+
|
||||
+ const canRead = createSectionReadChecker(hasConfigCapability, user);
|
||||
const baseOnly = (req.query as Record<string, unknown>).baseOnly === 'true';
|
||||
const appConfig = await getAppConfig({
|
||||
tenantId: user.tenantId,
|
||||
baseOnly,
|
||||
});
|
||||
- return res.status(200).json({ config: redactAppConfigForResponse(appConfig) });
|
||||
+ const filteredAppConfig = await filterAppConfigForReadAccess(appConfig, canRead);
|
||||
+
|
||||
+ return res.status(200).json({ config: redactAppConfigForResponse(filteredAppConfig) });
|
||||
} catch (error) {
|
||||
logger.error('[adminConfig] getBaseConfig error:', error);
|
||||
return res.status(500).json({ error: 'Failed to get base config' });
|
||||
@@ -323,7 +453,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
- if (!(await hasConfigCapability(user, null, 'read'))) {
|
||||
+ if (!(await hasAnyConfigReadAccess(user))) {
|
||||
return res.status(403).json({ error: 'Insufficient permissions' });
|
||||
}
|
||||
|
||||
@@ -334,7 +464,10 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
||||
return res.status(404).json({ error: 'Config not found' });
|
||||
}
|
||||
|
||||
- return res.status(200).json({ config: redactConfigForResponse(config) });
|
||||
+ const canRead = createSectionReadChecker(hasConfigCapability, user);
|
||||
+ const filteredConfig = await filterConfigDocForReadAccess(config, canRead);
|
||||
+
|
||||
+ return res.status(200).json({ config: redactConfigForResponse(filteredConfig) });
|
||||
} catch (error) {
|
||||
logger.error('[adminConfig] getConfig error:', error);
|
||||
return res.status(500).json({ error: 'Failed to get config' });
|
||||
diff --git a/packages/api/src/middleware/capabilities.ts b/packages/api/src/middleware/capabilities.ts
|
||||
index 0135f2c77..ef392be34 100644
|
||||
--- a/packages/api/src/middleware/capabilities.ts
|
||||
+++ b/packages/api/src/middleware/capabilities.ts
|
||||
@@ -26,6 +26,10 @@ interface CapabilityDeps {
|
||||
capability: SystemCapability;
|
||||
tenantId?: string;
|
||||
}) => Promise<boolean>;
|
||||
+ hasAnyConfigReadAccess?: (params: {
|
||||
+ principals: ResolvedPrincipal[];
|
||||
+ tenantId?: string;
|
||||
+ }) => Promise<boolean>;
|
||||
}
|
||||
|
||||
export interface CapabilityUser {
|
||||
@@ -127,11 +131,38 @@ export function generateCapabilityCheck(deps: CapabilityDeps): {
|
||||
hasCapability: HasCapabilityFn;
|
||||
requireCapability: RequireCapabilityFn;
|
||||
hasConfigCapability: HasConfigCapabilityFn;
|
||||
+ hasAnyConfigReadAccess: (user: CapabilityUser) => Promise<boolean>;
|
||||
} {
|
||||
- const { getUserPrincipals, hasCapabilityForPrincipals } = deps;
|
||||
+ const {
|
||||
+ getUserPrincipals,
|
||||
+ hasCapabilityForPrincipals,
|
||||
+ hasAnyConfigReadAccess: checkAny = async () => false,
|
||||
+ } = deps;
|
||||
|
||||
let workerWarned = false;
|
||||
|
||||
+ async function resolvePrincipals(user: CapabilityUser): Promise<ResolvedPrincipal[]> {
|
||||
+ const store = capabilityStore.getStore();
|
||||
+ const principalKey = `${user.id}:${user.role}:${user.tenantId ?? ''}`;
|
||||
+ const cached = store?.principals.get(principalKey);
|
||||
+ if (cached) {
|
||||
+ return cached;
|
||||
+ }
|
||||
+ const principals = await getUserPrincipals({
|
||||
+ userId: user.id,
|
||||
+ role: user.role,
|
||||
+ idOnTheSource: user.idOnTheSource,
|
||||
+ });
|
||||
+ store?.principals.set(principalKey, principals);
|
||||
+ return principals;
|
||||
+ }
|
||||
+
|
||||
+ /** Whether the user holds any config-read capability at all, broad or section-scoped. */
|
||||
+ async function hasAnyConfigReadAccess(user: CapabilityUser): Promise<boolean> {
|
||||
+ const principals = await resolvePrincipals(user);
|
||||
+ return checkAny({ principals, tenantId: user.tenantId });
|
||||
+ }
|
||||
+
|
||||
async function hasCapability(
|
||||
user: CapabilityUser,
|
||||
capability: SystemCapability,
|
||||
@@ -153,20 +184,7 @@ export function generateCapabilityCheck(deps: CapabilityDeps): {
|
||||
return cached;
|
||||
}
|
||||
|
||||
- const principalKey = `${user.id}:${user.role}:${user.tenantId ?? ''}`;
|
||||
- let principals: ResolvedPrincipal[];
|
||||
- const cachedPrincipals = store?.principals.get(principalKey);
|
||||
- if (cachedPrincipals) {
|
||||
- principals = cachedPrincipals;
|
||||
- } else {
|
||||
- principals = await getUserPrincipals({
|
||||
- userId: user.id,
|
||||
- role: user.role,
|
||||
- idOnTheSource: user.idOnTheSource,
|
||||
- });
|
||||
- store?.principals.set(principalKey, principals);
|
||||
- }
|
||||
-
|
||||
+ const principals = await resolvePrincipals(user);
|
||||
const result = await hasCapabilityForPrincipals({
|
||||
principals,
|
||||
capability,
|
||||
@@ -237,5 +255,5 @@ export function generateCapabilityCheck(deps: CapabilityDeps): {
|
||||
};
|
||||
}
|
||||
|
||||
- return { hasCapability, requireCapability, hasConfigCapability };
|
||||
+ return { hasCapability, requireCapability, hasConfigCapability, hasAnyConfigReadAccess };
|
||||
}
|
||||
diff --git a/packages/data-schemas/src/methods/systemGrant.ts b/packages/data-schemas/src/methods/systemGrant.ts
|
||||
index 6d82e9cd5..13b3f15ef 100644
|
||||
--- a/packages/data-schemas/src/methods/systemGrant.ts
|
||||
+++ b/packages/data-schemas/src/methods/systemGrant.ts
|
||||
@@ -87,6 +87,13 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): {
|
||||
capability: SystemCapability;
|
||||
tenantId?: string;
|
||||
}) => Promise<boolean>;
|
||||
+ hasAnyConfigReadAccess: ({
|
||||
+ principals,
|
||||
+ tenantId,
|
||||
+ }: {
|
||||
+ principals: Array<{ principalType: PrincipalType; principalId?: string | Types.ObjectId }>;
|
||||
+ tenantId?: string;
|
||||
+ }) => Promise<boolean>;
|
||||
getHeldCapabilities: ({
|
||||
principals,
|
||||
capabilities,
|
||||
@@ -134,6 +141,59 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): {
|
||||
: { tenantId: { $exists: false } };
|
||||
}
|
||||
|
||||
+ const READ_CONFIGS_SECTION_PATTERN = /^read:configs:\w+$/;
|
||||
+
|
||||
+ /**
|
||||
+ * Whether any of the given principals holds *some* config-read
|
||||
+ * capability, the broad `read:configs` or any `read:configs:<section>`,
|
||||
+ * without needing to know which sections in advance. Used to gate a
|
||||
+ * cheap pre-flight 403 before fetching a config document, so a caller
|
||||
+ * with zero read access never triggers a DB lookup, while a
|
||||
+ * section-scoped-only caller still passes through to have the response
|
||||
+ * filtered to what they actually hold.
|
||||
+ *
|
||||
+ * @param principals - Resolved principal list from getUserPrincipals
|
||||
+ * @param tenantId - If present, checks tenant-scoped grant; if absent, checks platform-level
|
||||
+ */
|
||||
+ async function hasAnyConfigReadAccess({
|
||||
+ principals,
|
||||
+ tenantId,
|
||||
+ }: {
|
||||
+ principals: Array<{ principalType: PrincipalType; principalId?: string | Types.ObjectId }>;
|
||||
+ tenantId?: string;
|
||||
+ }): Promise<boolean> {
|
||||
+ const SystemGrant = mongoose.models.SystemGrant as Model<ISystemGrant>;
|
||||
+ const principalsQuery = principals
|
||||
+ .filter(
|
||||
+ (p): p is typeof p & { principalId: string | Types.ObjectId } =>
|
||||
+ p.principalType !== PrincipalType.PUBLIC && p.principalId != null,
|
||||
+ )
|
||||
+ .map((p) => ({
|
||||
+ principalType: p.principalType,
|
||||
+ principalId: normalizePrincipalId(p.principalId, p.principalType),
|
||||
+ }));
|
||||
+
|
||||
+ if (!principalsQuery.length) {
|
||||
+ return false;
|
||||
+ }
|
||||
+
|
||||
+ const query: FilterQuery<ISystemGrant> = {
|
||||
+ $and: [
|
||||
+ { $or: principalsQuery },
|
||||
+ {
|
||||
+ $or: [
|
||||
+ { capability: SystemCapabilities.READ_CONFIGS },
|
||||
+ { capability: READ_CONFIGS_SECTION_PATTERN },
|
||||
+ ],
|
||||
+ },
|
||||
+ tenantCondition(tenantId),
|
||||
+ ],
|
||||
+ };
|
||||
+
|
||||
+ const doc = await SystemGrant.exists(query);
|
||||
+ return doc != null;
|
||||
+ }
|
||||
+
|
||||
/**
|
||||
* Check if any of the given principals holds a specific capability.
|
||||
* Follows the same principal-resolution pattern as AclEntry:
|
||||
@@ -534,6 +594,7 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): {
|
||||
seedSystemGrants,
|
||||
revokeCapability,
|
||||
hasCapabilityForPrincipals,
|
||||
+ hasAnyConfigReadAccess,
|
||||
getHeldCapabilities,
|
||||
listGrants,
|
||||
countGrants,
|
||||
|
|
@ -324,13 +324,101 @@
|
|||
}
|
||||
});
|
||||
|
||||
const RAW_TEXT_TAGS = ['script', 'style', 'title', 'textarea'];
|
||||
|
||||
function findTagEnd(html, at) {
|
||||
let quote = '';
|
||||
for (let i = at + 1; i < html.length; i += 1) {
|
||||
const char = html[i];
|
||||
if (quote) {
|
||||
if (char === quote) {
|
||||
quote = '';
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (char === '"' || char === "'") {
|
||||
quote = char;
|
||||
continue;
|
||||
}
|
||||
if (char === '>') {
|
||||
return i;
|
||||
}
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
function isTagAt(lower, at, name) {
|
||||
if (!lower.startsWith('<' + name, at)) {
|
||||
return false;
|
||||
}
|
||||
const after = lower[at + name.length + 1];
|
||||
return after === undefined || after === '>' || after === '/' || /\s/.test(after);
|
||||
}
|
||||
|
||||
/**
|
||||
* A literal `<head>` inside a comment, a CDATA section or a raw-text element is never parsed as
|
||||
* a tag, so those spans are walked past instead of matched. Injecting into one would leave the
|
||||
* bootstrap commented out, the view unattested, and every App Bridge message queued until it
|
||||
* times out. Anything the scan cannot resolve confidently returns -1 so the caller prefixes the
|
||||
* whole document, which keeps the bootstrap ahead of the app's markup either way.
|
||||
*/
|
||||
function findHeadInjectionPoint(html) {
|
||||
const lower = html.toLowerCase();
|
||||
let at = 0;
|
||||
while (at < lower.length) {
|
||||
const next = lower.indexOf('<', at);
|
||||
if (next === -1) {
|
||||
return -1;
|
||||
}
|
||||
if (lower.startsWith('<!--', next)) {
|
||||
const close = lower.indexOf('-->', next + 4);
|
||||
// An unterminated comment or raw-text span runs to the end of the document, so no head
|
||||
// can follow it.
|
||||
if (close === -1) {
|
||||
return -1;
|
||||
}
|
||||
at = close + 3;
|
||||
continue;
|
||||
}
|
||||
if (lower.startsWith('<![cdata[', next)) {
|
||||
const close = lower.indexOf(']]>', next + 9);
|
||||
if (close === -1) {
|
||||
return -1;
|
||||
}
|
||||
at = close + 3;
|
||||
continue;
|
||||
}
|
||||
const kind = lower[next + 1];
|
||||
if (kind === undefined || !/[a-z!/?]/.test(kind)) {
|
||||
at = next + 1;
|
||||
continue;
|
||||
}
|
||||
if (isTagAt(lower, next, 'head')) {
|
||||
const end = findTagEnd(lower, next);
|
||||
return end === -1 ? -1 : end + 1;
|
||||
}
|
||||
const rawText = RAW_TEXT_TAGS.find((name) => isTagAt(lower, next, name));
|
||||
if (rawText) {
|
||||
const open = findTagEnd(lower, next);
|
||||
const close = open === -1 ? -1 : lower.indexOf('</' + rawText, open + 1);
|
||||
if (close === -1) {
|
||||
return -1;
|
||||
}
|
||||
at = close + rawText.length + 2;
|
||||
continue;
|
||||
}
|
||||
const end = findTagEnd(lower, next);
|
||||
at = end === -1 ? lower.length : end + 1;
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
function injectIntoHead(html, injection) {
|
||||
// The bootstrap must run before any app script, and the app's bytes must reach the blob
|
||||
// verbatim: inject, never rewrap. Prefixing a document that opens with a doctype would push
|
||||
// the doctype down and flip the app into quirks mode.
|
||||
const headMatch = html.match(/<head[^>]*>/i);
|
||||
if (headMatch) {
|
||||
const at = headMatch.index + headMatch[0].length;
|
||||
const at = findHeadInjectionPoint(html);
|
||||
if (at !== -1) {
|
||||
return html.slice(0, at) + injection + html.slice(at);
|
||||
}
|
||||
const doctypeMatch = html.match(/^\s*<!doctype[^>]*>/i);
|
||||
|
|
|
|||
|
|
@ -330,6 +330,35 @@ describe('mcp-sandbox proxy', () => {
|
|||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
[
|
||||
'<!-- template includes <head> --><html><head></head><body>app</body></html>',
|
||||
'<html><head>',
|
||||
],
|
||||
['<!doctype html><!-- <head> --><body>app</body>', '<!doctype html>'],
|
||||
['<!doctype html><!-- <head>', '<!doctype html>'],
|
||||
[
|
||||
'<!doctype html><html><head></head><body><script>var s="<head>";</script></body></html>',
|
||||
'<head>',
|
||||
],
|
||||
[
|
||||
'<!doctype html><html><body><script>var s="<head>";</script></body></html>',
|
||||
'<!doctype html>',
|
||||
],
|
||||
['<!doctype html><HTML><HEAD></HEAD><body>a</body></HTML>', '<HEAD>'],
|
||||
['<!doctype html><html><head data-x="1"></head><body>a</body></html>', '<head data-x="1">'],
|
||||
['<!doctype html><html><body><div title="<head>"></div></body></html>', '<!doctype html>'],
|
||||
['<p>bare</p>', ''],
|
||||
])('injects the bootstrap at the first parsed head (%s)', (html, marker) => {
|
||||
const sandbox = loadSandbox();
|
||||
sandbox.deliverResource({ html });
|
||||
const blob = sandbox.blobs[0];
|
||||
const bootstrap = blob.match(/<script>\(function\(\)\{var N=[\s\S]*?<\/script>/)?.[0] ?? '';
|
||||
expect(bootstrap).not.toBe('');
|
||||
const at = marker === '' ? 0 : html.indexOf(marker) + marker.length;
|
||||
expect(blob).toBe(html.slice(0, at) + bootstrap + html.slice(at));
|
||||
});
|
||||
|
||||
it('mints a distinct nonce per document', () => {
|
||||
const first = loadSandbox();
|
||||
first.deliverResource();
|
||||
|
|
|
|||
|
|
@ -1,4 +0,0 @@
|
|||
services:
|
||||
langfuse-fanout-collector:
|
||||
ports:
|
||||
- '4318:4318'
|
||||
|
|
@ -71,6 +71,11 @@ export class ConnectionsRepository {
|
|||
return canConnect;
|
||||
}
|
||||
|
||||
/** The connection currently pooled for a server, without loading, validating or creating one. */
|
||||
public getPooledConnection(serverName: string): MCPConnection | undefined {
|
||||
return this.connections.get(serverName);
|
||||
}
|
||||
|
||||
/** Gets or creates a connection for the specified server with lazy loading */
|
||||
async get(
|
||||
serverName: string,
|
||||
|
|
|
|||
|
|
@ -97,7 +97,8 @@ export class MCPManager extends UserConnectionManager {
|
|||
private static readonly RESERVED_TEMPLATE_OPERATOR = /^[=,!@|]/;
|
||||
/** RE2 rejects a repeat count above this, so a larger RFC 6570 prefix cannot be compiled as one. */
|
||||
private static readonly MAX_REPEAT_COUNT = 1000;
|
||||
private static readonly MAX_PREFIXED_TEMPLATE_VARS = 8;
|
||||
/** Declared-variable ceiling for the expansions compiled as an ordered chain of optional units. */
|
||||
private static readonly MAX_ORDERED_TEMPLATE_VARS = 8;
|
||||
|
||||
/** Creates and initializes the singleton MCPManager instance */
|
||||
public static async createInstance(configs: t.MCPServers): Promise<MCPManager> {
|
||||
|
|
@ -515,6 +516,21 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
return `${connection.createdAt}:${connection.resourceListVersion}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Scope for the tool-metadata and resource-authorization caches. An app-level connection is shared
|
||||
* by every user, and nothing clears its entries (`removeUserConnection` only runs for user-scoped
|
||||
* connections), so keying it per user would retain one entry set per user for the process lifetime.
|
||||
* User-scoped connections (OAuth/OBO/customUserVars/runtime placeholders) are distinct connections
|
||||
* that can expose different tools and different visibility per user, so those keep their per-user
|
||||
* key. Decided by connection identity rather than by re-deriving the config's connection scope.
|
||||
*/
|
||||
private cacheScope(serverName: string, connection: MCPConnection, userId?: string): string {
|
||||
if (this.appConnections?.getPooledConnection(serverName) === connection) {
|
||||
return `${serverName}:`;
|
||||
}
|
||||
return `${serverName}:${userId ?? ''}`;
|
||||
}
|
||||
|
||||
private isToolCacheFresh(cacheKey: string, connection: MCPConnection): boolean {
|
||||
return (
|
||||
this.knownToolNamesCache.has(cacheKey) &&
|
||||
|
|
@ -598,7 +614,7 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
const { serverMap } = await this.buildToolCaches(connection);
|
||||
return serverMap.get(toolName);
|
||||
}
|
||||
const cacheKey = `${serverName}:${userId ?? ''}`;
|
||||
const cacheKey = this.cacheScope(serverName, connection, userId);
|
||||
if (!this.isToolCacheFresh(cacheKey, connection)) {
|
||||
await this.populateToolCaches(connection, cacheKey);
|
||||
}
|
||||
|
|
@ -1019,7 +1035,12 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
);
|
||||
}
|
||||
|
||||
await this.assertResourceReadable(connection, `${serverName}:${userId}`, uri, logPrefix);
|
||||
await this.assertResourceReadable(
|
||||
connection,
|
||||
this.cacheScope(serverName, connection, userId),
|
||||
uri,
|
||||
logPrefix,
|
||||
);
|
||||
|
||||
return connection.client.readResource({ uri }, { timeout: connection.timeout });
|
||||
}
|
||||
|
|
@ -1332,6 +1353,18 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
if (!keys) {
|
||||
return null;
|
||||
}
|
||||
// Query expansions are bounded per declared variable rather than as an open run of declared
|
||||
// keys, so they route here ahead of the prefix branch: a `:max-length` on a query variable
|
||||
// is only a tighter value bound within the same bounded sequence.
|
||||
if (op === '?' || op === '&') {
|
||||
const queryExpansion = MCPManager.compileQueryExpansion(op, varSpecs);
|
||||
if (queryExpansion == null) {
|
||||
return null;
|
||||
}
|
||||
pattern += queryExpansion;
|
||||
i = end + 1;
|
||||
continue;
|
||||
}
|
||||
if (varSpecs.some((spec) => spec.includes(':'))) {
|
||||
const prefixed = MCPManager.compilePrefixedExpansion(op, varSpecs);
|
||||
if (prefixed == null) {
|
||||
|
|
@ -1369,12 +1402,6 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
// with a value class excluding `;` so one value cannot swallow `;admin=true`.
|
||||
pattern += bounded(`;(?:${keys})(?:=[^/?#;&]*)?`);
|
||||
break;
|
||||
case '?': // query: only the declared parameter names, in any order
|
||||
pattern += `\\?(?:${keys})=[^#&]*(?:&(?:${keys})=[^#&]*)*`;
|
||||
break;
|
||||
case '&': // query continuation: only the declared parameter names
|
||||
pattern += `(?:&(?:${keys})=[^#&]*)+`;
|
||||
break;
|
||||
default: // simple expansion: a single value. RFC 6570 percent-encodes reserved chars,
|
||||
// so a real value never contains a raw `&` or `=`; excluding them stops a query value
|
||||
// like `q={q}` from matching `q=foo&admin=true` and authorizing an undeclared param.
|
||||
|
|
@ -1413,6 +1440,79 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
return { name, prefix: Number(maxLength), explode };
|
||||
}
|
||||
|
||||
/**
|
||||
* Parses every varspec of one expansion, rejecting the whole expression when any is invalid. The
|
||||
* count is capped because both compiled forms chain one optional unit per variable, which is
|
||||
* quadratic in the declared count: a pathological varspec list authorizes nothing instead of being
|
||||
* handed to RE2 as a compile-time cost on every read.
|
||||
*/
|
||||
private static parseVarSpecs(varSpecs: string[]): UriTemplateVarSpec[] | null {
|
||||
if (varSpecs.length > MCPManager.MAX_ORDERED_TEMPLATE_VARS) {
|
||||
return null;
|
||||
}
|
||||
const specs: UriTemplateVarSpec[] = [];
|
||||
for (const varSpec of varSpecs) {
|
||||
const parsed = MCPManager.parseVarSpec(varSpec);
|
||||
if (parsed == null) {
|
||||
return null;
|
||||
}
|
||||
specs.push(parsed);
|
||||
}
|
||||
return specs;
|
||||
}
|
||||
|
||||
/**
|
||||
* RFC 6570 §2.4.1 truncates a prefixed value to `:max-length` characters, and templates are matched
|
||||
* against the fully percent-decoded URI, so the limit is a plain character bound on the matched
|
||||
* text. A prefix RE2 cannot express as a repeat count leaves the variable unbounded (its own class
|
||||
* still applies), which cannot deny a legitimate expansion.
|
||||
*/
|
||||
private static boundedClass(spec: UriTemplateVarSpec, cls: string, min: number): string {
|
||||
if (spec.prefix == null || spec.prefix > MCPManager.MAX_REPEAT_COUNT) {
|
||||
return `${cls}${min === 0 ? '*' : '+'}`;
|
||||
}
|
||||
return `${cls}{${min},${spec.prefix}}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Compiles a form-style query expansion (`{?a,b}`) or continuation (`{&a,b}`) as a bounded
|
||||
* sequence. RFC 6570 §3.2.8/§3.2.9: variables expand in declared order, an undefined one is
|
||||
* skipped entirely, a `?` expression prefixes its first present component with `?` and the rest
|
||||
* with `&` (a `&` expression prefixes every component with `&`), and a non-exploded variable
|
||||
* appends its name, `=`, and one value. So the expansion of `{?id}` is a single `?id=<value>` pair,
|
||||
* never `?id=public&id=admin`, which the previous open run of declared keys accepted and forwarded
|
||||
* to a server whose first/last-value semantics could resolve a resource no expansion produces. The
|
||||
* bound is per varspec, so a literal pair the template already carries next to an expansion of the
|
||||
* same name still matches.
|
||||
*
|
||||
* Declared order is enforced rather than any permutation: expansion is order-preserving, so a
|
||||
* reordered query string is not something the advertised template can emit.
|
||||
*
|
||||
* An exploded variable over a list repeats its own key (`{?list*}` produces
|
||||
* `?list=red&list=green`), so its component allows that repeat. An exploded associative array
|
||||
* expands to keys the template never names; those stay unmatchable, as they already were, because
|
||||
* nothing in the template makes them knowable.
|
||||
*/
|
||||
private static compileQueryExpansion(op: string, varSpecs: string[]): string | null {
|
||||
const specs = MCPManager.parseVarSpecs(varSpecs);
|
||||
if (specs == null) {
|
||||
return null;
|
||||
}
|
||||
const units = specs.map((spec) => {
|
||||
const name = spec.name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
const unit = `${name}=${MCPManager.boundedClass(spec, '[^#&]', 0)}`;
|
||||
return spec.explode ? `${unit}(?:&${unit})*` : unit;
|
||||
});
|
||||
const lead = op === '?' ? '\\?' : '&';
|
||||
// One branch per possible first present variable, each followed by the optional later ones in
|
||||
// declared order. At least one component is required, keeping the existing denial for a URI that
|
||||
// omits the whole expansion.
|
||||
const branches = units.map((unit, index) =>
|
||||
units.slice(index + 1).reduce((branch, rest) => `${branch}(?:&${rest})?`, `${lead}${unit}`),
|
||||
);
|
||||
return branches.length === 1 ? branches[0] : `(?:${branches.join('|')})`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Compiles an expansion in which at least one variable carries a `:max-length` prefix. RFC 6570
|
||||
* §2.4.1 truncates a prefixed string value to that many characters, and templates are matched
|
||||
|
|
@ -1425,31 +1525,13 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
* shared quantifier: a shared quantifier would either apply the tightest bound to every position
|
||||
* or, as before, none to any. The chain still requires at least one component, keeping the
|
||||
* existing denial for a URI that omits the whole expansion.
|
||||
*
|
||||
* A prefix RE2 cannot express as a repeat count leaves that variable unbounded (its own class
|
||||
* still applies), which is the pre-existing behavior and cannot deny a legitimate expansion.
|
||||
*/
|
||||
private static compilePrefixedExpansion(op: string, varSpecs: string[]): string | null {
|
||||
// The ordered chain is quadratic in the declared variable count, so a pathological varspec list
|
||||
// authorizes nothing instead of being handed to RE2 as a compile-time cost on every read.
|
||||
if (varSpecs.length > MCPManager.MAX_PREFIXED_TEMPLATE_VARS) {
|
||||
const specs = MCPManager.parseVarSpecs(varSpecs);
|
||||
if (specs == null) {
|
||||
return null;
|
||||
}
|
||||
const specs: UriTemplateVarSpec[] = [];
|
||||
for (const varSpec of varSpecs) {
|
||||
const parsed = MCPManager.parseVarSpec(varSpec);
|
||||
if (parsed == null) {
|
||||
return null;
|
||||
}
|
||||
specs.push(parsed);
|
||||
}
|
||||
|
||||
const bound = (spec: UriTemplateVarSpec, cls: string, min: number): string => {
|
||||
if (spec.prefix == null || spec.prefix > MCPManager.MAX_REPEAT_COUNT) {
|
||||
return `${cls}${min === 0 ? '*' : '+'}`;
|
||||
}
|
||||
return `${cls}{${min},${spec.prefix}}`;
|
||||
};
|
||||
const bound = MCPManager.boundedClass;
|
||||
const component = (spec: UriTemplateVarSpec, delimiter: string, cls: string): string => {
|
||||
const unit = `${delimiter}${bound(spec, cls, 1)}`;
|
||||
return spec.explode ? `(?:${unit})+` : unit;
|
||||
|
|
@ -1476,8 +1558,6 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
};
|
||||
const escaped = (spec: UriTemplateVarSpec): string =>
|
||||
spec.name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
const query = (): string =>
|
||||
specs.map((spec) => `${escaped(spec)}=${bound(spec, '[^#&]', 0)}`).join('|');
|
||||
|
||||
switch (op) {
|
||||
case '+':
|
||||
|
|
@ -1497,10 +1577,6 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
return spec.explode ? `(?:${unit})+` : unit;
|
||||
}),
|
||||
);
|
||||
case '?':
|
||||
return `\\?(?:${query()})(?:&(?:${query()}))*`;
|
||||
case '&':
|
||||
return `(?:&(?:${query()}))+`;
|
||||
default:
|
||||
return joined('[^/?#&=]', 1);
|
||||
}
|
||||
|
|
@ -1640,7 +1716,7 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
);
|
||||
}
|
||||
|
||||
const cacheKey = `${serverName}:${userId ?? ''}`;
|
||||
const cacheKey = this.cacheScope(serverName, connection, userId);
|
||||
if (!this.isToolCacheFresh(cacheKey, connection)) {
|
||||
await this.populateToolCaches(connection, cacheKey);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -166,6 +166,7 @@ describe('MCPManager', () => {
|
|||
const mock = {
|
||||
has: jest.fn().mockResolvedValue(false),
|
||||
get: jest.fn().mockResolvedValue({} as unknown as MCPConnection),
|
||||
getPooledConnection: jest.fn().mockReturnValue(undefined),
|
||||
getAll: jest.fn().mockResolvedValue(new Map()),
|
||||
getMany: jest.fn().mockResolvedValue(new Map()),
|
||||
...appConnectionsConfig,
|
||||
|
|
@ -2153,6 +2154,39 @@ describe('MCPManager', () => {
|
|||
{ uriTemplate: 'api://x{;a,b}', uri: 'api://x;a=1;b=2;c=3', allowed: false },
|
||||
{ uriTemplate: 'api://x{;p*}', uri: 'api://x;p=1;p=2', allowed: true },
|
||||
{ uriTemplate: 'db://items{;id,identity}', uri: 'db://items;identity=x', allowed: true },
|
||||
// RFC 6570 3.2.8/3.2.9: a non-exploded query variable contributes one `key=value` pair, in
|
||||
// declared order, so duplicates, reordering and undeclared keys are all unproducible.
|
||||
{ uriTemplate: 'db://items{?id}', uri: 'db://items?id=42', allowed: true },
|
||||
{ uriTemplate: 'db://items{?id}', uri: 'db://items?id=public&id=admin', allowed: false },
|
||||
{ uriTemplate: 'db://items{?id}', uri: 'db://items?id=1&admin=true', allowed: false },
|
||||
{ uriTemplate: 'db://items{?id}', uri: 'db://items', allowed: false },
|
||||
{ uriTemplate: 'db://items{?id*}', uri: 'db://items?id=1&id=2', allowed: true },
|
||||
{ uriTemplate: 'db://items{?id*}', uri: 'db://items?id=1&other=2', allowed: false },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?a=1&b=2', allowed: true },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?a=1', allowed: true },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?b=2', allowed: true },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?b=2&a=1', allowed: false },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?a=1&a=2', allowed: false },
|
||||
{ uriTemplate: 'api://x{?a,b}', uri: 'api://x?a=1&b=2&c=3', allowed: false },
|
||||
{ uriTemplate: 'api://x?f=1{&a,b}', uri: 'api://x?f=1&a=1&b=2', allowed: true },
|
||||
{ uriTemplate: 'api://x?f=1{&a,b}', uri: 'api://x?f=1&b=2', allowed: true },
|
||||
{ uriTemplate: 'api://x?f=1{&a,b}', uri: 'api://x?f=1&b=2&a=1', allowed: false },
|
||||
{ uriTemplate: 'api://x?f=1{&a}', uri: 'api://x?f=1&a=1&a=2', allowed: false },
|
||||
// The bound is per varspec, not per key name: a literal pair the template already carries plus
|
||||
// one expansion of the same name is a URI the template does produce.
|
||||
{ uriTemplate: 'api://x?a=1{&a}', uri: 'api://x?a=1&a=2', allowed: true },
|
||||
// A `:max-length` prefix bounds each key's own value inside that bounded sequence.
|
||||
{ uriTemplate: 'api://x{?a:2,b}', uri: 'api://x?a=ab&b=anything', allowed: true },
|
||||
{ uriTemplate: 'api://x{?a:2,b}', uri: 'api://x?a=abc&b=anything', allowed: false },
|
||||
{ uriTemplate: 'api://x{?a:2,b}', uri: 'api://x?b=anything', allowed: true },
|
||||
{ uriTemplate: 'api://x{?a:2,b}', uri: 'api://x?a=ab&a=cd', allowed: false },
|
||||
{ uriTemplate: 'api://x{?a:3*}', uri: 'api://x?a=foo', allowed: false },
|
||||
// Same conservative ceiling as the other ordered chains.
|
||||
{
|
||||
uriTemplate: `api://x{?${Array.from({ length: 9 }, (_, i) => `v${i}`).join(',')}}`,
|
||||
uri: 'api://x?v0=1',
|
||||
allowed: false,
|
||||
},
|
||||
// Malformed expressions authorize nothing rather than falling back to an open query string.
|
||||
{ uriTemplate: 'db://x{?}', uri: 'db://x?a=1', allowed: false },
|
||||
{ uriTemplate: 'db://x{&}', uri: 'db://x&a=1', allowed: false },
|
||||
|
|
@ -2292,6 +2326,100 @@ describe('MCPManager', () => {
|
|||
expect(new UriTemplate('files://root{+path}').match('files://root/x?y=z#f')).not.toBeNull();
|
||||
});
|
||||
|
||||
const cacheKeys = (manager: MCPManager): string[][] => {
|
||||
const caches = manager as unknown as {
|
||||
resourceUriCache: Map<string, unknown>;
|
||||
appHiddenToolCache: Map<string, unknown>;
|
||||
knownToolNamesCache: Map<string, unknown>;
|
||||
toolCacheConnStamp: Map<string, unknown>;
|
||||
advertisedResourceCache: Map<string, unknown>;
|
||||
advertisedResourceConnStamp: Map<string, unknown>;
|
||||
};
|
||||
return [
|
||||
[...caches.resourceUriCache.keys()],
|
||||
[...caches.appHiddenToolCache.keys()],
|
||||
[...caches.knownToolNamesCache.keys()],
|
||||
[...caches.toolCacheConnStamp.keys()],
|
||||
[...caches.advertisedResourceCache.keys()],
|
||||
[...caches.advertisedResourceConnStamp.keys()],
|
||||
];
|
||||
};
|
||||
|
||||
const readBoth = async (manager: MCPManager, userId: string) => {
|
||||
for (const uri of ['ui://app/main', 'db://items/42']) {
|
||||
await manager.readResource({
|
||||
userId,
|
||||
serverName: 'srv',
|
||||
uri,
|
||||
user: { id: userId } as IUser,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
it('does not grow the metadata caches per user on a shared app-level connection', async () => {
|
||||
const request = templateOnlyRequest('db://items/{id}');
|
||||
const snapshot = jest
|
||||
.fn()
|
||||
.mockResolvedValue({ tools: [uiTool('t', 'ui://app/main')], complete: true });
|
||||
const shared = buildConnection(request, snapshot);
|
||||
mockAppConnections({ getPooledConnection: jest.fn().mockReturnValue(shared) });
|
||||
const manager = await MCPManager.createInstance(newMCPServersConfig());
|
||||
jest.spyOn(manager, 'getConnection').mockResolvedValue(shared);
|
||||
|
||||
for (const userId of ['u1', 'u2', 'u3', 'u4', 'u5']) {
|
||||
await readBoth(manager, userId);
|
||||
}
|
||||
|
||||
for (const keys of cacheKeys(manager)) {
|
||||
expect(keys).toEqual(['srv:']);
|
||||
}
|
||||
expect(snapshot).toHaveBeenCalledTimes(1);
|
||||
expect(
|
||||
request.mock.calls.filter((c) => (c[0] as { method: string }).method === 'resources/list'),
|
||||
).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('scopes by connection identity when one user falls back to its own connection', async () => {
|
||||
// getConnection hands an app-level server's connection to most users but falls back to a
|
||||
// user connection when the app one is unavailable, so the scope has to follow the instance.
|
||||
const snapshot = () =>
|
||||
jest.fn().mockResolvedValue({ tools: [uiTool('t', 'ui://app/main')], complete: true });
|
||||
const shared = buildConnection(templateOnlyRequest('db://items/{id}'), snapshot());
|
||||
const ownConnection = buildConnection(templateOnlyRequest('db://items/{id}'), snapshot());
|
||||
mockAppConnections({ getPooledConnection: jest.fn().mockReturnValue(shared) });
|
||||
const manager = await MCPManager.createInstance(newMCPServersConfig());
|
||||
jest
|
||||
.spyOn(manager, 'getConnection')
|
||||
.mockImplementation(async ({ user }) => (user?.id === 'u2' ? ownConnection : shared));
|
||||
|
||||
await readBoth(manager, 'u1');
|
||||
await readBoth(manager, 'u2');
|
||||
await readBoth(manager, 'u3');
|
||||
|
||||
for (const keys of cacheKeys(manager)) {
|
||||
expect(keys).toEqual(['srv:', 'srv:u2']);
|
||||
}
|
||||
});
|
||||
|
||||
it('keeps per-user cache entries for user-scoped connections', async () => {
|
||||
const snapshot = () =>
|
||||
jest.fn().mockResolvedValue({ tools: [uiTool('t', 'ui://app/main')], complete: true });
|
||||
const first = buildConnection(templateOnlyRequest('db://items/{id}'), snapshot());
|
||||
const second = buildConnection(templateOnlyRequest('db://items/{id}'), snapshot());
|
||||
mockAppConnections({ getPooledConnection: jest.fn().mockReturnValue(undefined) });
|
||||
const manager = await MCPManager.createInstance(newMCPServersConfig());
|
||||
jest
|
||||
.spyOn(manager, 'getConnection')
|
||||
.mockImplementation(async ({ user }) => (user?.id === 'u1' ? first : second));
|
||||
|
||||
await readBoth(manager, 'u1');
|
||||
await readBoth(manager, 'u2');
|
||||
|
||||
for (const keys of cacheKeys(manager)) {
|
||||
expect(keys).toEqual(['srv:u1', 'srv:u2']);
|
||||
}
|
||||
});
|
||||
|
||||
const pagedListRequest = (pages: number, pageSize = 1, nextCursor: () => string = () => 'c') =>
|
||||
jest.fn().mockImplementation((req: { method: string; params?: { cursor?: string } }) => {
|
||||
if (req.method === 'resources/list') {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue