CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-10-21 09:32:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

🧽 test: Restore the Admin Allowlist Override This Spec Leaks Into the Shard (#15543)

Browse source 
* 🧽 test: Restore the Admin Allowlist Override This Spec Leaks Into the Shard

The spec writes a per-user `mcpSettings.allowedDomains` override for the shared
primary user and never removes it. The list holds only its own fixture's
origin, and allowlist matching is protocol- and port-inclusive, so for the rest
of the shard every other MCP fixture is blocked: `e2e-oauth` fails inspection,
and an agent expecting its tools initializes into
AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE (503) instead of producing its OAuth
prompt.

Whether that bites depends only on which specs the shard runs afterwards, so it
surfaces as a deterministic failure in an unrelated spec — `mcp-oauth-resume`
failing an assertion that fires before any resume happens — which passes
whenever it runs first. A client-only PR whose path selection reordered the
shard hit it three times across two heads.

Delete the override in a `finally`, and confirm against the stored config
rather than by re-running `reinitialize`: the connection this test established
stays live, so reinitialize keeps succeeding once the allowlist is gone.

* 🩹 test: Read the Config Wrapper and Treat 404 as Cleared in the Override Teardown

`GET /api/admin/config/:principalType/:principalId` returns `{ config }`, and
deleting the last override removes the document, so the follow-up read is a
404 — the cleared state, not a read failure. The poll treated both as
unreadable and timed out.

* 🧭 fix: Await Config Cache Invalidation Before Answering Admin Config Writes

Every admin config mutation fired `invalidateConfigCaches` and answered without
waiting for it. The write was durable, but the effective configuration — app
config, per-principal overrides, cached tools, the MCP config-source cache —
could still describe the previous overrides when the response arrived, so a
caller that read right after writing could observe state it had just changed.
That is exactly how the allowlist-override e2e's teardown could report a clean
shard while the leaked allowlist was still in force.

`invalidateConfigCaches` already settles every clear and only logs failures,
so awaiting it cannot fail the request; it only orders the response after the
attempt. The e2e teardown now leans on that instead of polling the config
document, brackets the baseline assertion inside its cleanup scope so an
interrupted attempt can still un-poison the shard on retry, and treats a 404
from cleanup as a failure only when the override was actually installed, so it
can never mask the error that prevented installing it.

* 🧽 test: Confirm the Effective Allowlist Reverted Instead of Awaiting Invalidation

Reverts the awaited cache invalidation in the admin config handlers. Codex was
right on both counts: `invalidateCachedTools` can wait up to 30 seconds on the
Redis tool-cache lock, so awaiting it could hold a committed write until the
client's timeout; and APP_CONFIG is per-container by default, so an await only
ever proves this replica reverted while another keeps the previous overrides
for up to the override TTL. A test-hygiene PR should not ship a product
contract it cannot keep.

The teardown instead confirms the effective allowlist with the cache-backed
read the test already relies on. `reinitialize` disconnects the user
connection and re-resolves the merged allowlists per request, so the baseline
`success: false` returning is the definitive signal that the leaked override no
longer governs connections. The window exceeds the merged-config cache TTL, so
even a missed invalidation converges rather than leaking into the next spec.
The baseline stays inside the cleanup scope and a 404 from cleanup is a failure
only when the override was actually installed.

* 🧽 test: Verify the Override Is Gone, Not That a Connected Server Re-Blocks

The previous teardown polled `reinitialize` for the baseline `success: false`.
CI showed it never returns: a server that has already connected keeps
re-initializing successfully for more than 90 seconds after the override is
removed — past the 60-second merged-config TTL — because its allow decision
is not on the path that poisoned the shard. That path is agent tool loading,
which consults the merged app config per request; it is in-memory in these
shards and cleared by the mutation's asynchronous invalidation, and the
downstream victim, `mcp-oauth-resume`, passes with this cleanup in place.

Cleanup now always runs (a leaked override from an interrupted attempt is the
exact state it exists to remove), confirms the override document is gone with
only 200-without-the-override or 404 as definitive answers (anything else
retries), treats a 404 as a failure only when this attempt installed the
override, and describes the PUT's invalidation as what it is: asynchronous.
This commit is contained in:
Danny Avila 2026-09-03 12:42:10 -04:00 • committed by GitHub
parent 849e9bb40d
commit 5f187d3a6c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 79 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

97
e2e/specs/mock/mcp-allowlist-override.spec.ts
View file

@ -52,26 +52,87 @@ test.describe('MCP admin-panel allowlist override', () => {
expect(userId).toBeTruthy();
const headers = { Authorization: `Bearer ${token}` };
let installed = false;
// Baseline: the fixture's origin is not in the YAML allowlist, so reinit fails.
const before = await reinitialize(request, headers);
expect(before.status).toBe(200);
expect(before.success).toBe(false);
/**
* The override is per-USER and this is the shared primary user, so it must
* not outlive the test: the list holds only this fixture's origin and
* allowlist matching is port-inclusive, so every other MCP fixture would be
* blocked for the rest of the shard (`e2e-oauth` fails inspection and an
* agent expecting its tools 503s with AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE).
* The baseline assertion sits inside the cleanup scope on purpose: if an
* interrupted earlier attempt left the override behind, the baseline is
* what fails, and the `finally` is the only thing that can un-poison the
* shard for the retry and for every spec after it.
*/
try {
// Baseline: the fixture's origin is not in the YAML allowlist, so reinit fails.
const before = await reinitialize(request, headers);
expect(before.status).toBe(200);
expect(before.success).toBe(false);
// Admin-panel override: allow the fixture's origin for this user.
const put = await request.put(`/api/admin/config/user/${userId}`, {
headers,
data: { overrides: { mcpSettings: { allowedDomains: [FIXTURE_ORIGIN] } } },
});
expect(put.ok()).toBeTruthy();
// Admin-panel override: allow the fixture's origin for this user.
const put = await request.put(`/api/admin/config/user/${userId}`, {
headers,
data: { overrides: { mcpSettings: { allowedDomains: [FIXTURE_ORIGIN] } } },
});
expect(put.ok()).toBeTruthy();
installed = true;
// The override is honored on reinit: the server now connects. invalidateConfigCaches
// runs asynchronously after the PUT, so poll until the merged allowlist lands.
await expect
.poll(async () => (await reinitialize(request, headers)).success, {
timeout: 30000,
intervals: [1000, 2000, 3000],
})
.toBe(true);
// The override is honored on reinit: the server now connects. The handler
// invalidates config caches asynchronously after responding, so poll until
// the merged allowlist has actually landed.
await expect
.poll(async () => (await reinitialize(request, headers)).success, {
timeout: 30000,
intervals: [1000, 2000, 3000],
})
.toBe(true);
} finally {
/**
* Always run, whether or not this attempt installed anything: a leaked
* override from an earlier interrupted attempt is exactly the state the
* cleanup exists to remove. A 404 means there was nothing to delete, which
* is only a failure if this attempt had installed the override — and an
* assertion here must never mask the error that prevented installing it.
*/
const del = await request.delete(`/api/admin/config/user/${userId}`, { headers });
if (installed || del.status() !== 404) {
expect(del.ok()).toBeTruthy();
}
/**
* Confirm the override document is gone, retrying anything that is not a
* definitive answer (only 200-without-the-override and 404 are). The cache
* that gates the next spec — the merged app config that agent tool loading
* consults per request — is in-memory in these shards and is cleared by the
* mutation's (asynchronous) invalidation; the downstream victim,
* `mcp-oauth-resume`, passes with this cleanup in place. `reinitialize` is
* deliberately NOT used as the "reverted" signal: a server that has already
* connected keeps re-initializing successfully long after the override is
* removed (observed for more than 90 seconds in CI, past the 60-second
* merged-config TTL), because its allow decision is not on the path that
* poisoned the shard.
*/
await expect
.poll(
async () => {
const res = await request.get(`/api/admin/config/user/${userId}`, { headers });
if (res.status() === 404) {
return 'cleared';
}
if (res.status() !== 200) {
return `retry:${res.status()}`;
}
const body = (await res.json()) as {
config?: { overrides?: { mcpSettings?: { allowedDomains?: string[] } } };
};
return body.config?.overrides?.mcpSettings?.allowedDomains == null
? 'cleared'
: 'override still present';
},
{ timeout: 30000, intervals: [500, 1000, 2000] },
)
.toBe('cleared');
}
});
});