fix: Handle Empty String in iFrame Embedding Config options by prakhar-singh1928 · Pull Request #2958 · IBM/mcp-context-forge

prakhar-singh1928 · 2026-02-15T21:09:13Z

🔗 Related Issue

Closes #2492

📝 Summary

This PR fixes X_FRAME_OPTIONS related middleware behavior not properly handling empty string values.

Key changes:

Fixed middleware bug where empty string "" was falling through to the default case and being treated like DENY (adding frame-ancestors 'none'), when it should allow all embedding (no frame-ancestors directive)
Updated test cases in test_security_middleware_comprehensive.py to use None instead of "" when testing disabled iframe restrictions, matching the config validator's behavior that converts empty strings to None
Added test_x_frame_options_empty_string_returns_none

The root cause was:

The middleware was incorrectly treating empty string "" as an unknown value, defaulting to DENY behavior(adding frame-ancestors 'none') (blocking embedding)

All X_FRAME_OPTIONS now exhibit expected behaviour

🏷️ Type of Change

🧪 Verification

Check	Command	Status
Lint suite	make lint	✅ Passed (Pylint score: 10/10)
Unit tests	make test	✅ Passed (all X_FRAME_OPTIONS tests)
Coverage ≥ 80%	make coverage	✅ Passed (100% for modified files, 99%+ overal

✅ Checklist

Code formatted (make black isort pre-commit)
Tests added/updated for changes
Documentation updated (if applicable)
No secrets or credentials committed

📓 Notes (optional)

Targeted local verification executed:

pytest tests/unit/mcpgateway/test_config.py -v ✅
pytest tests/security/test_security_middleware_comprehensive.py ✅

ja8zyjits · 2026-02-17T16:29:21Z

Looks good to me

Verified by following steps:

Update .env file and update this X_FRAME_OPTIONS= with value " "/null
Create this file in your local machine

`index.html`

<!DOCTYPE html>
<html>
<head>
    <title>Test X-Frame-Options</title>
</head>
<body>
    <h1>Testing X-Frame-Options</h1>
    <p>The iframe below should may or may not be blocked depending on config by the browser:</p>
    <iframe src="http://localhost:8000/" width="800" height="600"></iframe>
    <p>Check browser console (F12) for errors.</p>
</body>
</html>

Run Mcp gateway and try to open index.html in a different tab as a file (no need to update gateway code). The embedded mcp-gateway must be visible.

Regards,
Jitesh

crivetimihai · 2026-02-18T01:36:28Z

Review Notes

Rebased onto current main (1902707) and squashed 9 commits into a single clean commit preserving original author attribution.

Changes made during rebase

Squashed commits: 9 incremental commits → 1 clean conventional commit
Style fix: Added missing blank lines between new top-level test functions in test_config.py (project standard: 2 blank lines)

Review findings

Bug confirmed: The root cause was correctly identified. When X_FRAME_OPTIONS="", the old validator passed the empty string through unchanged. The middleware then correctly omitted the X-Frame-Options header (empty string is falsy), but the CSP frame-ancestors block still executed ("" is not None is True), and "".upper() matched no branch, falling to the else default → frame-ancestors 'none' — which still blocked embedding via CSP.

Fix is correct: Normalizing "" / whitespace-only to None at the config validator level is the right approach. The middleware already handles None properly (skips both X-Frame-Options and frame-ancestors). No middleware changes needed.

Security: No regression — default remains DENY. Empty string → None is an intentional opt-in.

Test coverage: 100% on the middleware module, 99% on config (3 misses are unrelated). All validator branches covered. The ALLOW-ALL expected value fix ("*" → "* file: http: https:") corrects a pre-existing test bug.

All 167 related tests pass.

The config validator now converts empty/whitespace-only X_FRAME_OPTIONS values to None, matching the middleware's expectation that None means "allow all embedding". Previously, an empty string fell through to the default DENY behavior in the middleware, blocking embedding unexpectedly. Closes #2492 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai

Approved

Rebased onto main, squashed 9 commits into 1, and applied additional hardening based on review.

Changes applied during review

Docs fix (P2): Updated .env.example — empty string docs incorrectly claimed "" yields frame-ancestors * file: http: https:. It now correctly documents that "" / null / none all remove iframe restrictions (no headers sent).
Middleware hardening (P3): Added defense-in-depth normalization in the middleware itself — if x_frame_options is ever set to "" or whitespace programmatically (bypassing the config validator), the middleware now treats it as None instead of falling through to frame-ancestors 'none'.
Stale comments (P4): Fixed misleading comments in middleware (line 258) and test file (line 665) that still referenced the old empty-string semantics.
Test coverage: Added 2 new middleware-level unit tests (test_x_frame_options_raw_empty_string, test_x_frame_options_raw_whitespace_string) to exercise the hardened bypass path. Middleware coverage remains at 100%.

Manual verification

Tested end-to-end with Playwright across all four config scenarios:

Config	X-Frame-Options	CSP frame-ancestors	iframe Result
Default (DENY)	`DENY`	`'none'`	Blocked
`X_FRAME_OPTIONS=""`	absent	absent	Loads successfully
`X_FRAME_OPTIONS=SAMEORIGIN`	`SAMEORIGIN`	`'self'`	Blocked (cross-origin)
`X_FRAME_OPTIONS=" "`	absent	absent	Loads successfully

Test results

223 related tests pass (config + security + middleware)
Full unit suite: 11,932 passed
Full security suite: 238 passed
Differential coverage: security_headers.py 100%, config.py 99%

…ing (#2958) The config validator now converts empty/whitespace-only X_FRAME_OPTIONS values to None, matching the middleware's expectation that None means "allow all embedding". Previously, an empty string fell through to the default DENY behavior in the middleware, blocking embedding unexpectedly. Closes #2492 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>

…ing (IBM#2958) The config validator now converts empty/whitespace-only X_FRAME_OPTIONS values to None, matching the middleware's expectation that None means "allow all embedding". Previously, an empty string fell through to the default DENY behavior in the middleware, blocking embedding unexpectedly. Closes IBM#2492 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

…ing (#2958) The config validator now converts empty/whitespace-only X_FRAME_OPTIONS values to None, matching the middleware's expectation that None means "allow all embedding". Previously, an empty string fell through to the default DENY behavior in the middleware, blocking embedding unexpectedly. Closes #2492 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

…ing (#2958) The config validator now converts empty/whitespace-only X_FRAME_OPTIONS values to None, matching the middleware's expectation that None means "allow all embedding". Previously, an empty string fell through to the default DENY behavior in the middleware, blocking embedding unexpectedly. Closes #2492 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

prakhar-singh1928 requested review from crivetimihai, kevalmahajan and madhav165 as code owners February 15, 2026 21:09

crivetimihai added this to the Release 1.0.0-RC1 milestone Feb 16, 2026

ja8zyjits self-requested a review February 16, 2026 14:55

prakhar-singh1928 force-pushed the iframe-embedding-fix-2492 branch 2 times, most recently from 69caaeb to 40e89a3 Compare February 17, 2026 11:13

ja8zyjits previously approved these changes Feb 17, 2026

View reviewed changes

crivetimihai self-assigned this Feb 17, 2026

crivetimihai dismissed ja8zyjits’s stale review via e147eaa February 18, 2026 01:36

crivetimihai force-pushed the iframe-embedding-fix-2492 branch from e394c61 to e147eaa Compare February 18, 2026 01:36

crivetimihai force-pushed the iframe-embedding-fix-2492 branch from e147eaa to e93f7c8 Compare February 18, 2026 01:58

crivetimihai approved these changes Feb 18, 2026

View reviewed changes

crivetimihai merged commit 8610dc9 into main Feb 18, 2026
54 checks passed

crivetimihai deleted the iframe-embedding-fix-2492 branch February 18, 2026 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Handle Empty String in iFrame Embedding Config options#2958

fix: Handle Empty String in iFrame Embedding Config options#2958
crivetimihai merged 1 commit intomainfrom
iframe-embedding-fix-2492

prakhar-singh1928 commented Feb 15, 2026 •

edited by crivetimihai

Loading

Uh oh!

ja8zyjits commented Feb 17, 2026 •

edited

Loading

Uh oh!

crivetimihai commented Feb 18, 2026

Uh oh!

crivetimihai left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

prakhar-singh1928 commented Feb 15, 2026 • edited by crivetimihai Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

📝 Summary

All X_FRAME_OPTIONS now exhibit expected behaviour

🏷️ Type of Change

🧪 Verification

✅ Checklist

📓 Notes (optional)

Uh oh!

ja8zyjits commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verified by following steps:

Uh oh!

crivetimihai commented Feb 18, 2026

Review Notes

Changes made during rebase

Review findings

Uh oh!

crivetimihai left a comment

Choose a reason for hiding this comment

Approved

Changes applied during review

Manual verification

Test results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prakhar-singh1928 commented Feb 15, 2026 •

edited by crivetimihai

Loading

ja8zyjits commented Feb 17, 2026 •

edited

Loading