Repository navigation
fix(mcp): keep a credential typed into the MCP component's Headers box - #15761
Merged
Merged
Conversation
The MCP Tools component has a Headers input beside its server field, and its own
help text says those headers override the ones configured on the server. So it
is a reasonable place to put an Authorization header, and people do.
Packaging nulled it. The value is secret-named, so the scrubber removed it --
correctly, because a package is a file people store and share -- but nothing
declared a variable to put it back. The manifest named nothing, the deploy check
reported ready, and the deployed flow called the server with no header at all.
The failure surfaced as a 401 from someone else's service, with nothing anywhere
connecting it to a credential the deploy had removed.
Handled the same way as a credential in the server's own config, because the box
is the same shape as a config's `headers` map and the existing scrubber already
knows how to name one. Saving the flow moves the literal into a global variable
and leaves the generated name behind; packaging keeps that name and declares it;
the deploy refuses until the target has it.
Both storage shapes. The input is `DictInput(is_list=True)`, so the canvas stores
it as a mapping or as `{"key", "value"}` rows and the component reads either at
run time. Handling one and not the other would have missed half the flows.
Scoped to this component. The same nulling affects API Request, URL, Cloudflare
and Confluent Kafka Producer, and fixing those needs a decision this change
cannot make: an MCP server has a name to key the variable on and they do not.
Tracked separately.
What is deliberately still nulled: a literal that was never rewritten, a name
that is not the one this server and field generate, every value when the reader
has no variable manifest (a public share), and anything the ordinary rules
catch -- a credential inside a URL under an innocent header name, or one nested
deeper in a value.
Four defects in earlier drafts of this change were found by open-code-review and
each reproduced before being fixed:
- Skipping the generic pass for the whole field switched off scrubbing it was
doing. A URL carrying credentials under a non-secret header name survived.
References are now lifted out, the ordinary scrub runs, and they go back --
the shape `_strip_table_rows_in_place` already uses.
- The row form of the box was skipped entirely at save time.
- The box and the server config generated the same variable name, so whichever
was written last won. In practice the config, which silently discarded the
override -- the one thing the box exists to do. Both sides now derive from
`mcp_header_scope`.
- Save time rewrites every header outside `NON_SECRET_HEADERS`; packaging only
kept the ones `_is_secret_name` matched, so `X-Tenant` got a generated name
that nothing declared and nothing would resolve.
Found by QA against MCP Tools. Not a regression: this path has behaved this way
independently of recent deployment work.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change extracts credentials from the MCP component’s separate Headers input into server-scoped variables. Packaging preserves recognized generated references and scrubs other header values for mapping and key/value-row formats. ChangesMCP Header Secret Handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant MCPFlowExtraction
participant MCPServerFieldIterator
participant ComponentHeadersScrubber
participant SecretVariables
MCPFlowExtraction->>MCPServerFieldIterator: iterate server fields and templates
MCPServerFieldIterator-->>MCPFlowExtraction: return field and template
MCPFlowExtraction->>ComponentHeadersScrubber: scrub Headers using server name
ComponentHeadersScrubber->>SecretVariables: add scoped header credentials
Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 79.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Test Coverage For New Implementations The PR adds regression coverage in the backend-named file src/backend/tests/unit/api/utils/mcp/test_flow_secrets.py. The tests cover mapping and row header storage, save-time extraction, packaging r…Test Quality And Coverage The added pytest coverage tests the main MCP Headers behavior. It covers mapping and key/value-row storage, save-time variable extraction, packaging reference retention, unknown and missing-manifest s… Test File Naming And Structure The PR adds tests in src/backend/tests/unit/api/utils/mcp/test_flow_secrets.py, which matches the backendtest_*.pypattern. The file uses pytest imports,pytest.mark.parametrize, discoverable `…Excessive Mock Usage Warning The pull request adds 14 tests without adding mocks, patches, or mock objects. The new tests exercise real secret extraction and packaging behavior with plain dictionaries and small test doubles. Exis… Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving credentials entered in the MCP component's Headers box.
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.13.0 #15761 +/- ##
==================================================
- Coverage 70.44% 69.93% -0.52%
==================================================
Files 2786 2783 -3
Lines 301831 302049 +218
Branches 41012 43906 +2894
==================================================
- Hits 212638 211239 -1399
- Misses 86696 88315 +1619
+ Partials 2497 2495 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
added
bug
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
bug
erichare
approved these changes
Oct 15, 2026
erichare
left a comment
erichare
left a comment
Member
There was a problem hiding this comment.
@Jkavia I pushed 5d6e0af to fix both review findings. Header overrides now have stable flow/component scopes, including nested flows, and missing custom-header bindings fail strict snapshot validation. Regressions cover separate components and flows, copied components, repeated literal saves, and both header storage shapes.
Validation: 383 affected-route tests passed, 1 skipped. The deployment/persistence test run also passed all 331 tests. Pre-commit checks passed. Approving this updated head. CI has been queued for the new commit.
github-actions
Bot
added
lgtm
Jkavia
enabled auto-merge
October 15, 2026 14:06
github-actions
Bot
added
bug
added 2 commits
October 15, 2026 16:26
CI caught this: `test_public_flow_read_strips_secrets` asserts a non-secret
header survives a public read, and it stopped surviving.
X-Tenant: acme
before this commit -> MCP_TENANT_MCP_HEADER_OVERRIDE_X_TENANT_9EFC07A6
after -> acme
Authorization: Bearer ghp_real
unchanged -> MCP_GITHUB_HEADER_OVERRIDE_AUTHORIZATION_4B0ECCD7, declared
The two halves of the Headers-box handling disagreed about which headers are
credentials, and I resolved it the wrong way round. Save time was widened to the
rule a *server config* follows -- everything outside `NON_SECRET_HEADERS` is a
secret -- which is right there, because a config is a connection's own
credentials. Applied to the component's Headers box it swept up ordinary routing
headers, so a tenant id became a variable somebody had to provision on the
serving plane for a value that is not a secret.
Narrowed instead: both halves now ask `is_secret_header_name`, one predicate so
they cannot drift apart again. A header that does not read as a credential is
left alone at save and at packaging, which is what that test has always said
should happen.
The test I added asserting the opposite is replaced by one asserting this.
Found by CI in a directory my own sweep skipped. The test reaches this code
through `langflow.api.utils.core`, so it names neither the function I edited nor
its module, and no grep for either would have found it. Grepping for the public
names the module exports does; that is how the 14 files run for this commit were
chosen, 600 passing.
Takes the per-node scoping and the explicit null for a rejected reference, and holds both sides to `_is_secret_name` instead of `NON_SECRET_HEADERS`. Treating every header outside `NON_SECRET_HEADERS` as a credential is the right rule for a server *config*, where the whole map is operator-supplied auth. In the component's own Headers box it swept up routing data: `X-Tenant: acme` became a variable someone had to provision on the serving plane for a value that is not a secret. `test_public_flow_read_strips_secrets` already asserted the narrower rule, and was red on the branch. The two tests that used `X-Tenant` to stand for a custom credential header now use `X-Api-Key`, which is custom and reads as a credential, so what they check is intact.
github-actions
Bot
added
bug
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
QA found that a credential typed into the MCP Tools component's Headers box is silently dropped at deploy.
That box overrides the headers set on the server, so it is a reasonable place to put an
Authorizationheader. Packaging removed the value, which is right, but nothing declared a variable to put it back. The manifest named nothing, the deploy check said ready, and the deployed flow called the server with no header at all. You found out when someone else's API returned a 401.Fix
DictInput(is_list=True), so the canvas stores it as a mapping or as{"key", "value"}rows and the component reads either one.Still removed, deliberately
A literal that was never rewritten, a name that is not the one this server and field generate, every value when the reader has no variable manifest (a public share), and anything the ordinary rules already catch, such as a credential inside a URL under an innocent header name.
Scope
MCP Tools only. The same nulling affects API Request, URL, Cloudflare and Confluent Kafka Producer, and fixing those needs a decision this change cannot make: an MCP server has a name to key the variable on and they do not. Tracked separately.
Testing
unit/api,unit/utilsandunit/services/deployment_artifacts.Summary by CodeRabbit