Skip to content

[FIX][SECURITY]: Add log sanitizer utility to prevent log injection attacks (#3000)#3227

Merged
crivetimihai merged 3 commits intomainfrom
bugfix/log-injection-query-params
Mar 5, 2026
Merged

[FIX][SECURITY]: Add log sanitizer utility to prevent log injection attacks (#3000)#3227
crivetimihai merged 3 commits intomainfrom
bugfix/log-injection-query-params

Conversation

@smrutisahoo10
Copy link
Copy Markdown
Collaborator

@smrutisahoo10 smrutisahoo10 commented Feb 24, 2026

…ging

🔗 Related Issue

Closes #3000


📝 Summary

Added utility function to strip control characters from values before logging to prevent log injection attacks. This prevents malicious input from manipulating log output by injecting newlines, carriage returns, or other control characters that could be used to forge log entries or hide malicious activity.

Files Added:

  • mcpgateway/utils/log_sanitizer.py
  • tests/unit/mcpgateway/utils/test_log_sanitizer.py

Modified files:

  • mcpgateway/routers/oauth_router.py
  • mcpgateway/routers/sso.py
  • mcpgateway/routers/well_known.py

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

🧪 Verification

Check Command Status
Lint suite make lint Pass
Unit tests make test Pass
Coverage ≥ 80% make coverage Pass

✅ Checklist

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

📓 Notes (optional)

Screenshots, design decisions, or additional context.

@smrutisahoo10 smrutisahoo10 force-pushed the bugfix/log-injection-query-params branch from 3d35a9d to bcaef69 Compare February 24, 2026 16:22
@crivetimihai crivetimihai changed the title Added utility for strips and control character from values before logging [FIX][SECURITY]: Add log sanitizer utility to prevent log injection attacks (#3000) Feb 25, 2026
@crivetimihai crivetimihai added bug Something isn't working security Improves security SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels Feb 25, 2026
@crivetimihai crivetimihai added this to the Release 1.0.0-RC2 milestone Feb 25, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks for this @smrutisahoo10. Log injection prevention is an important security measure and the approach of centralizing sanitization in a utility is clean. A few items: (1) full CI hasn't triggered yet — only DCO passed, please rebase or push to trigger the full suite, (2) the verification table in the PR description shows blank status — please run make lint and make test and confirm they pass. Nice work addressing #3000.

@smrutisahoo10 smrutisahoo10 force-pushed the bugfix/log-injection-query-params branch from bcaef69 to c32bd4b Compare February 26, 2026 07:44
@smrutisahoo10
Copy link
Copy Markdown
Collaborator Author

Hi @crivetimihai, ran make test and make lint. Updated the verification table.

@crivetimihai crivetimihai added the release-fix Critical bugfix required for the release label Mar 5, 2026
@smrutisahoo10 smrutisahoo10 force-pushed the bugfix/log-injection-query-params branch from fd225d0 to fa135df Compare March 5, 2026 09:16
@smrutisahoo10 smrutisahoo10 requested a review from MohanLaksh March 5, 2026 09:29
MohanLaksh
MohanLaksh previously approved these changes Mar 5, 2026
Copy link
Copy Markdown
Collaborator

@MohanLaksh MohanLaksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The below test file PASSED all Log Injection Vulnerabilities Tests.

#!/usr/bin/env python3
# -*- coding: utf-8 -*-
"""Manual test script for log sanitizer to verify it works correctly."""

import sys
import logging
import io

# Set UTF-8 encoding for Windows console
if sys.platform == 'win32':
    import codecs
    sys.stdout = codecs.getwriter('utf-8')(sys.stdout.buffer, 'strict')

# Add mcpgateway to path
sys.path.insert(0, '.')

from mcpgateway.utils.log_sanitizer import sanitize_for_log

# Setup logging to capture output
handler = logging.StreamHandler(stream := io.StringIO())
handler.setFormatter(logging.Formatter('%(levelname)s:%(name)s:%(message)s'))
logger = logging.getLogger('test')
logger.addHandler(handler)
logger.setLevel(logging.DEBUG)

print("=" * 60)
print("Testing Log Injection Prevention")
print("=" * 60)

# Test 1: Normal text (should pass through unchanged)
print("\n1. Normal text test:")
normal_text = "normal error message"
logger.warning(f"OAuth error: {sanitize_for_log(normal_text)}")
output = stream.getvalue()
print(f"Input: {repr(normal_text)}")
print(f"Output: {output.strip()}")
assert "\n" not in output or output.count("\n") == 1, "Should be single line"
stream.truncate(0)
stream.seek(0)

# Test 2: Newline injection (the actual vulnerability)
print("\n2. Newline injection test (VULNERABILITY):")
malicious_input = "bad scope\nCRITICAL:root:SECURITY BREACH detected"
logger.warning(f"OAuth error: {sanitize_for_log(malicious_input)}")
output = stream.getvalue()
print(f"Input: {repr(malicious_input)}")
print(f"Output: {output.strip()}")
lines = output.strip().split('\n')
assert len(lines) == 1, f"Should be single line, got {len(lines)} lines"
assert "SECURITY BREACH" in output, "Content should be preserved"
print("✓ Attack prevented - single log line produced")
stream.truncate(0)
stream.seek(0)

# Test 3: CRLF injection
print("\n3. CRLF injection test:")
crlf_input = "error\r\nINFO:fake:Fake log entry"
logger.warning(f"OAuth error: {sanitize_for_log(crlf_input)}")
output = stream.getvalue()
print(f"Input: {repr(crlf_input)}")
print(f"Output: {output.strip()}")
lines = output.strip().split('\n')
assert len(lines) == 1, f"Should be single line, got {len(lines)} lines"
print("✓ CRLF attack prevented")
stream.truncate(0)
stream.seek(0)

# Test 4: Tab injection
print("\n4. Tab character test:")
tab_input = "col1\tcol2\tcol3"
logger.warning(f"Data: {sanitize_for_log(tab_input)}")
output = stream.getvalue()
print(f"Input: {repr(tab_input)}")
print(f"Output: {output.strip()}")
assert "\t" not in output, "Tabs should be removed"
print("✓ Tabs sanitized")
stream.truncate(0)
stream.seek(0)

# Test 5: Multiple control characters
print("\n5. Multiple control characters test:")
multi_input = "text\nwith\rmany\tcontrol\vchars"
logger.warning(f"Message: {sanitize_for_log(multi_input)}")
output = stream.getvalue()
print(f"Input: {repr(multi_input)}")
print(f"Output: {output.strip()}")
assert "\n" not in output or output.count("\n") == 1
assert "\r" not in output
assert "\t" not in output
assert "\v" not in output
print("✓ All control characters sanitized")
stream.truncate(0)
stream.seek(0)

# Test 6: Real-world OAuth scenario
print("\n6. Real-world OAuth callback scenario:")
error = "invalid_scope"
error_description = "Requested scope not available\nCRITICAL:security:BREACH DETECTED"
logger.warning(f"OAuth provider returned error callback: error={sanitize_for_log(error)}, description={sanitize_for_log(error_description)}")
output = stream.getvalue()
print(f"Error: {repr(error)}")
print(f"Description: {repr(error_description)}")
print(f"Output: {output.strip()}")
lines = output.strip().split('\n')
assert len(lines) == 1, f"Should be single line, got {len(lines)} lines"
print("✓ Real-world scenario protected")

print("\n" + "=" * 60)
print("✅ ALL TESTS PASSED - Log injection vulnerability fixed!")
print("=" * 60)


Smruti Sahoo and others added 2 commits March 5, 2026 18:54
…ging

Signed-off-by: Smruti Sahoo <smruti.sahoo1@ibm.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
crivetimihai
crivetimihai previously approved these changes Mar 5, 2026
Copy link
Copy Markdown
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Approved

Clean, focused security fix for CWE-117 (log injection via control characters in untrusted input).

What was reviewed

  • mcpgateway/utils/log_sanitizer.py — New utility with sanitize_for_log(), sanitize_dict_for_log(), and sanitize_optional(). Pre-compiled regex [\x00-\x1f\x7f-\x9f] correctly strips C0/C1 control characters while preserving printable space.
  • Router integration — Applied at 4 sites where untrusted user input (OAuth callback query params, SSO redirect_uri, RFC 9728 path segments) is logged. Server-side values correctly left unsanitized.
  • Test suite — 45 tests with 100% coverage on the new module, including real-world attack scenarios (CRLF injection, log forging, SIEM evasion).

Changes made during review

  • Rebased onto main (clean, no conflicts)
  • Removed unused import pytest from test file

Notes

  • Unicode line separators (U+2028/U+2029) are intentionally not filtered — Python's logging module doesn't treat them as newlines, so they're not an injection vector. This is correctly documented in the tests.
  • No security or performance issues introduced.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai crivetimihai self-assigned this Mar 5, 2026
@crivetimihai crivetimihai merged commit c6c2ad0 into main Mar 5, 2026
39 checks passed
@crivetimihai crivetimihai deleted the bugfix/log-injection-query-params branch March 5, 2026 19:12
MohanLaksh pushed a commit that referenced this pull request Mar 12, 2026
…ttacks (#3000) (#3227)

* Added utility for strips and control character from values before logging

Signed-off-by: Smruti Sahoo <smruti.sahoo1@ibm.com>

* fix(tests): remove unused pytest import from log sanitizer tests

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

* style(tests): remove trailing whitespace in log sanitizer tests

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

---------

Signed-off-by: Smruti Sahoo <smruti.sahoo1@ibm.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Smruti Sahoo <smruti.sahoo1@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Yosiefeyob pushed a commit that referenced this pull request Mar 13, 2026
…ttacks (#3000) (#3227)

* Added utility for strips and control character from values before logging

Signed-off-by: Smruti Sahoo <smruti.sahoo1@ibm.com>

* fix(tests): remove unused pytest import from log sanitizer tests

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

* style(tests): remove trailing whitespace in log sanitizer tests

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

---------

Signed-off-by: Smruti Sahoo <smruti.sahoo1@ibm.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Smruti Sahoo <smruti.sahoo1@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working release-fix Critical bugfix required for the release security Improves security SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][SECURITY]: Log injection via unsanitized control characters in unauthenticated query parameters

3 participants