Skip to content

fix(config): preserve MCP source through loadAppConfig unwrap - #1256

Open
89799969 wants to merge 1 commit into
Nano-Collective:mainfrom
89799969:fix/mcp-project-config-security-source
Open

fix(config): preserve MCP source through loadAppConfig unwrap#1256
89799969 wants to merge 1 commit into
Nano-Collective:mainfrom
89799969:fix/mcp-project-config-security-source

Conversation

@89799969

Copy link
Copy Markdown

Problem

validateProjectConfigSecurity is the app's defense-in-depth check for hardcoded credentials in project-level MCP configs. It filters on MCPServerConfig.source === 'project', but loadAppConfig unwrapped MCPServerWithSource to plain MCPServerConfig before validation:

const mcpServers = mcpServersWithSource.map(item => item.server);

The loader only tracks provenance on the wrapper, so every runtime object had source: undefined, the filter was always empty, and the scanner never ran. The unit test passed only because it hand-set .source on the inner type — a shape that never occurs in the real data flow.

Fix

When unwrapping, copy the wrapper's source onto the runtime object (MCPServerConfig.source is already an optional field):

const mcpServers = mcpServersWithSource.map(item => ({
  ...item.server,
  source: item.source,
}));

Both useAppInitialization and plain/initialize.ts then see real provenance.

Tests

  • Existing source/config/validation.spec.ts suite still passes
  • New regression test loader unwrap keeps source so project configs reach the validator models the real MCPServerWithSource[] shape and asserts:
    • the production-style unwrap keeps only project-server in the project filter
    • the old strip-wrapper path yields an empty filter (the bug)
    • validateProjectConfigSecurity accepts the preserved-source array
5 tests passed

tsc --noEmit passes.

Fixes #1248

validateProjectConfigSecurity filters on MCPServerConfig.source, but
loadAppConfig stripped the loader wrapper (which carries provenance)
before validation, so project-level hardcoded-credential checks never
ran in the shipped app.

Copy wrapper.source onto the runtime server objects and add a regression
test that models the real unwrap path.

Fixes Nano-Collective#1248

Signed-off-by: halaxy <63827956+89799969@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 2 nits

@89799969 — a few things worth a look, none blocking.

The change correctly fixes the integration break described in issue #1248 by copying MCPServerWithSource.source onto the runtime MCPServerConfig objects in loadAppConfig, which restores validateProjectConfigSecurity's filter and re-enables the hardcoded-credential scanner for project-level MCP configs. Both consumers (useAppInitialization and plain/initialize) share loadAppConfig and are covered by the single-site fix. The new regression test models the real wrapper shape and demonstrates both the bug and the fix; the existing test (which hand-set .source) is left in place. Changeset, package name and bump type are all correct.

⚪ nit · tests · source/config/validation.spec.ts

The new regression test demonstrates that the unwrap preserves .source and that validateProjectConfigSecurity does not throw on the resulting array, but it does not assert that a warning is actually emitted for a project-level server carrying a hardcoded credential. validateMCPConfigSecurity already has a logWarning capture pattern (see the env-server test at the top of this file). Extending the new test to also assert that validateProjectConfigSecurity(wrapped-with-preserved-source) triggers a warning would directly verify the security-relevant end-to-end behaviour the issue calls out — today the test would still pass even if the validator were silently a no-op for project sources. Not blocking, since the unwrap-preservation assertion is the load-bearing part of the regression test.

⚪ nit · completeness

Issue #1248 also flags source/config/mcp-config-loader.ts:47-63 (mapServerConfig never copies .source onto the returned MCPServerConfig) as part of the root cause. The PR's suggested fix at the consumer site in loadAppConfig is valid and is one of the two approaches the issue explicitly lists, so leaving mapServerConfig untouched is defensible — but worth noting that MCPServerConfig.source is still set only at the unwrap site, which means any other future caller of loadAllMCPConfigs() that forgets to copy .source would silently re-introduce the same bug. A defence-in-depth option would be to have loadAllMCPConfigs() return already-attributed MCPServerConfig[] directly (and retire the wrapper). Not blocking; the current fix does close the reported issue.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] validateProjectConfigSecurity never runs, hardcoded-credential scanner is dead code

1 participant