fix/3811-pipe-validation-issue by Nayana-R-Gowda · Pull Request #3814 · IBM/mcp-context-forge

Nayana-R-Gowda · 2026-03-24T06:44:58Z

Signed-off-by: NAYANA.R nayana.r7813@gmail.com

💡 Fix Description

Fixes false-positive validation that rejected tool descriptions containing the pipe (|) character.

Problem

The validation logic blocked "|" as an unsafe character, causing legitimate tool descriptions (e.g., LogQL queries like {app="foo"} |= "error") to fail.

This prevented critical tools such as query_loki_logs from being registered.

Solution

Removed "|" from the forbidden patterns list, while keeping "||" to block shell OR injection.

Changes

Updated forbidden_patterns in ToolCreate.validate_description()
Added comments explaining why "|" is allowed

Testing

✅ Tool creation with LogQL (|=) succeeds
❌ Unsafe patterns like && are still blocked
Verified via API using curl

Impact

Fixes Grafana Loki MCP backend registration
Prevents false positives for LogQL, PromQL, regex, and Markdown tables

🧪 Verification

Check	Command	Status
Lint suite	`make lint`	Pass
Unit tests	`make test`	pass

📐 MCP Compliance (if relevant)

Matches current MCP spec
No breaking change to MCP clients

✅ Checklist

Code formatted (make black isort pre-commit)
No secrets/credentials committed

crivetimihai · 2026-03-27T11:38:29Z

Review Summary

Rebased onto main and substantially reworked this PR to align with the current codebase architecture.

Changes from the original PR

The original PR removed "|" from a hardcoded forbidden_patterns list in schemas.py. However, main has since moved ToolCreate.validate_description to use configurable patterns from settings.tool_description_forbidden_patterns. The original approach would have caused a merge conflict and wouldn't have addressed the ToolUpdate path.

What this PR now does (2 commits)

Commit 1 — fix(validation): remove single pipe from forbidden description patterns

mcpgateway/config.py: Removes "|" from the default tool_description_forbidden_patterns list. "||" (shell OR) remains blocked.
mcpgateway/schemas.py: Aligns ToolUpdate.validate_description with ToolCreate — replaces the hardcoded pattern list with the config-driven settings.tool_description_forbidden_patterns + settings.tool_description_forbidden_patterns_enabled. This was a pre-existing inconsistency: ToolCreate already used the config, ToolUpdate did not.
Tests: Removes "|" from strict-mode rejection loops, adds test_single_pipe_allowed_in_strict_mode for both ToolCreate and ToolUpdate, adds test_disabled_allows_any_description and test_empty_pattern_skipped for ToolUpdate's new config-driven paths. 100% differential coverage verified.
Docs/config: Updates all 5 references to the default pattern list (docker-compose.yml, charts/mcp-stack/values.yaml, charts/mcp-stack/README.md, docs/docs/best-practices/input-validation.md, docs/docs/manage/configuration.md).

Commit 2 — fix(env): align .env.example JWT secret with docker-compose default

Fixes .env.example to use my-test-key-but-now-longer-than-32-bytes, matching docker-compose.yml and mcp_test_helpers.py (updated in chore: remove redundant linters, fix coverage output, update roadmap #3716 but .env.example was missed). See [BUG]: JWT secret default mismatch after #3716 — E2E tests, docker-compose variants, and scripts still use old key #3889 for remaining files.

Verification

168 unit tests pass (90 validator + 78 schema tests)
22/22 make test-mcp-cli E2E tests pass
Full E2E testing against localhost:8080 docker-compose deployment:
- Single-pipe descriptions accepted via curl (POST create, PUT update) and Playwright admin UI
- ||, &&, ;, $(, > , < still correctly rejected (422)
- Zero unexpected errors in docker logs

Closes #3811

crivetimihai

Reviewed, rebased, reworked, and verified. LGTM.

lucarlig

Thanks for tackling this. I found three issues:

Default-secret detection is now inconsistent with the new published JWT secret. This PR changes examples/defaults to my-test-key-but-now-longer-than-32-bytes, but the runtime still only treats my-test-key as the insecure default, so users can follow the new docs and bypass the existing warning/guard paths. See .env.example:22, mcpgateway/config.py:1087, and mcpgateway/main.py:1955.
The helper/doc defaults now disagree with the actual app default, which can break standalone local flows. The application still defaults JWT_SECRET_KEY to my-test-key, but updated helpers/docs mint tokens with the new long secret, so no-env make dev style setups can generate invalid tokens. See mcpgateway/config.py:285 and smoketest.py:236.
The | fix may not cover the full request path. The schema validators now allow single pipe characters in tool descriptions, but the broader dangerous-pattern middleware still appears to reject | in JSON bodies, so LogQL-style descriptions may still fail when validation middleware is enabled. See mcpgateway/config.py:360, mcpgateway/config.py:368, and mcpgateway/middleware/validation_middleware.py:143.

crivetimihai · 2026-03-27T14:21:29Z

Follow-up: Review feedback addressed + JWT secret alignment

Feedback response

Point 1 (guards inconsistent with new default) — Fixed in 0c6f44e99.
config.py:1088, main.py:1955, and main.py:2040 now check in ("my-test-key", "my-test-key-but-now-longer-than-32-bytes") so both defaults trigger the secure_secrets: false flag and critical-issues warning.

Point 2 (helper/doc defaults disagree with app default) — By design.
config.py:286 intentionally defaults to the short key for standalone make dev. Docker-compose overrides to the long key. Helpers use os.getenv("JWT_SECRET_KEY", "long-key") which resolves correctly in both contexts. This two-tier default is the intended design from #3716.

Point 3 (validation middleware dangerous_patterns still blocks |) — Pre-existing, not this PR.
The dangerous_patterns regex [;&|...] is part of EXPERIMENTAL_VALIDATE_IO which defaults to False and is warn-only in dev/staging. It operates on all JSON body values broadly — a different layer from tool_description_forbidden_patterns. When opt-in in production, | in any JSON value is flagged — but that's the intended behavior of that experimental feature. The pipe fix correctly targets the tool-description-specific validation path.

Additional changes since last review

fix(security): detect both JWT default secrets in guard paths — addresses feedback point 1
fix(env): align all JWT secret defaults with docker-compose ([BUG]: JWT secret default mismatch after #3716 — E2E tests, docker-compose variants, and scripts still use old key #3889) — updates 70 files (docker-compose variants, E2E/load tests, scripts, Helm charts, docs) and adds the long key to validate_env.py weak secrets list
chore: restore .secrets.baseline from main — file was dropped during rebase
chore: apply linter and formatter fixes

Verification

14,968 unit tests pass, 0 failed
22/22 make test-mcp-cli E2E tests pass
8/8 previously-failing Playwright tests now pass (1 skipped, timeouts were flaky)
Pipe validation E2E confirmed (pipe → 200, || → 422)
Docker logs clean

Remove single pipe character ("|") from the default TOOL_DESCRIPTION_FORBIDDEN_PATTERNS list. The pipe is a valid character in LogQL (|=, |~), PromQL, regex patterns, and Markdown tables. The dangerous shell OR operator "||" remains blocked. Also aligns ToolUpdate.validate_description with ToolCreate by using the configurable settings.tool_description_forbidden_patterns instead of a hardcoded list, ensuring consistent behavior when the pattern list is customized via environment variables. Closes #3811 Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

.env.example still used the old short `my-test-key` while docker-compose.yml and the E2E test helpers were updated to `my-test-key-but-now-longer-than-32-bytes` in #3716. Users who copied .env.example to .env got a secret mismatch that caused `make test-mcp-cli` to hang. See #3889 for the remaining files that need the same update. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

PR #3716 updated docker-compose.yml and mcp_test_helpers.py to use `my-test-key-but-now-longer-than-32-bytes` (meeting the 32-byte minimum for HS256 per RFC 7518 §3.2) but missed ~70 other files that still hardcoded the old `my-test-key`. This caused test failures (make test-mcp-cli hangs, load tests fail auth) when .env is derived from .env.example. Updated: - All docker-compose variant files (debug, embedded, performance, verbose) - All E2E and load test defaults - All scripts and smoketests - Helm chart values, schema, and docs - Makefile targets - All documentation examples - Added long key to validate_env.py weak_jwt list Not changed (intentionally): - mcpgateway/config.py:286 — Python config default (source of truth for standalone `make dev`) - mcpgateway/config.py:888 — already lists both keys in weak_secrets - mcpgateway/config.py:1088, main.py:1955,2040 — guards checking for the default value - tests/unit/test_main_helpers_extra.py:55 — test that mocks the default Closes #3889 Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

The secure_secrets flag and critical-issues check only matched the short `my-test-key` default. Users running with the docker-compose default `my-test-key-but-now-longer-than-32-bytes` would bypass the security warning and the `secure_secrets: false` status flag. - config.py get_security_status(): `!=` → `not in (short, long)` - main.py validate_security_configuration(): same - main.py security recommendations log: same - validate_env.py already had both (updated in prior commit) - config.py:888 weak_secrets already had both Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Pre-commit detect-secrets hook requires line numbers to match. The linter/formatter commit shifted lines in db_util.py. All entries remain is_secret=false (confirmed false positives). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

The longer JWT default triggers IBM detect-secrets "Secret Keyword" detection on 7 lines that are all test/dev defaults. Added `pragma: allowlist secret` inline comments. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

dima-zakharov

The default key optimization is possible (extract key from .env) but should work as is as well.

Pre-commit detect-secrets hook removed entries that are now covered by inline `pragma: allowlist secret` comments, and adjusted line numbers. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

brian-hussey

LGTM

brian-hussey · 2026-03-27T16:22:50Z

Merged with admin override for this issue.
Approvals were obtained, risk for linting issues is low, there is an external issue with some of checks execution.

Nayana-R-Gowda requested review from crivetimihai, kevalmahajan and madhav165 as code owners March 24, 2026 06:44

Nayana-R-Gowda added the bug Something isn't working label Mar 24, 2026

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch from 0c187fd to e4e8781 Compare March 27, 2026 11:38

crivetimihai approved these changes Mar 27, 2026

View reviewed changes

crivetimihai previously approved these changes Mar 27, 2026

View reviewed changes

crivetimihai dismissed their stale review via d4307fc March 27, 2026 12:49

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch from 43e5140 to d4307fc Compare March 27, 2026 12:49

crivetimihai requested review from araujof, dima-zakharov, jonpspri, lucarlig and terylt as code owners March 27, 2026 12:49

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch from 079f939 to aed20e2 Compare March 27, 2026 13:03

lucarlig requested changes Mar 27, 2026

View reviewed changes

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch 2 times, most recently from 16b153e to 0c6f44e Compare March 27, 2026 14:07

Nayana-R-Gowda and others added 5 commits March 27, 2026 14:22

chore: apply linter and formatter fixes

aab6f25

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch from 0c6f44e to d5582d2 Compare March 27, 2026 14:23

crivetimihai added 2 commits March 27, 2026 14:58

chore: allowlist test JWT secrets for detect-secrets hook

d7b783c

The longer JWT default triggers IBM detect-secrets "Secret Keyword" detection on 7 lines that are all test/dev defaults. Added `pragma: allowlist secret` inline comments. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

dima-zakharov previously approved these changes Mar 27, 2026

View reviewed changes

crivetimihai dismissed dima-zakharov’s stale review via 890d3c2 March 27, 2026 15:33

chore: update .secrets.baseline after allowlist pragmas

d9ab152

Pre-commit detect-secrets hook removed entries that are now covered by inline `pragma: allowlist secret` comments, and adjusted line numbers. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai force-pushed the fix/3811-pipe-validation-issue branch from 890d3c2 to d9ab152 Compare March 27, 2026 15:50

baseline

afc0d2d

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

brian-hussey approved these changes Mar 27, 2026

View reviewed changes

brian-hussey self-assigned this Mar 27, 2026

brian-hussey merged commit f85c5ae into main Mar 27, 2026
39 of 43 checks passed

brian-hussey deleted the fix/3811-pipe-validation-issue branch March 27, 2026 16:22

araujof mentioned this pull request Apr 8, 2026

[TRACKING]: Port new capabilities from CF's in-tree plugin framework contextforge-org/contextforge-plugins-framework#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/3811-pipe-validation-issue#3814

fix/3811-pipe-validation-issue#3814
brian-hussey merged 9 commits intomainfrom
fix/3811-pipe-validation-issue

Nayana-R-Gowda commented Mar 24, 2026 •

edited

Loading

Uh oh!

crivetimihai commented Mar 27, 2026

Uh oh!

crivetimihai left a comment

Uh oh!

lucarlig left a comment •

edited

Loading

Uh oh!

crivetimihai commented Mar 27, 2026

Uh oh!

dima-zakharov left a comment

Uh oh!

brian-hussey left a comment

Uh oh!

Uh oh!

brian-hussey commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Nayana-R-Gowda commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💡 Fix Description

Problem

Solution

Changes

Testing

Impact

🧪 Verification

📐 MCP Compliance (if relevant)

✅ Checklist

Uh oh!

crivetimihai commented Mar 27, 2026

Review Summary

Changes from the original PR

What this PR now does (2 commits)

Verification

Uh oh!

crivetimihai left a comment

Choose a reason for hiding this comment

Uh oh!

lucarlig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crivetimihai commented Mar 27, 2026

Follow-up: Review feedback addressed + JWT secret alignment

Feedback response

Additional changes since last review

Verification

Uh oh!

dima-zakharov left a comment

Choose a reason for hiding this comment

Uh oh!

brian-hussey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brian-hussey commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Nayana-R-Gowda commented Mar 24, 2026 •

edited

Loading

lucarlig left a comment •

edited

Loading