Skip to content

fix(security): sanitize user-controlled path in tenant validation logs#2

Merged
Pomdapis merged 2 commits intomainfrom
vk/70e8-pom-175-sanitize
Apr 24, 2026
Merged

fix(security): sanitize user-controlled path in tenant validation logs#2
Pomdapis merged 2 commits intomainfrom
vk/70e8-pom-175-sanitize

Conversation

@Pomdapis
Copy link
Copy Markdown
Contributor

Summary

  • Adds SanitizeForLog helper in TenantValidationMiddleware that strips CR/LF/tab from untrusted strings before they reach the logger.
  • Wraps context.Request.Path.Value at the excluded-path debug log and the tenant-validation-failed warning.
  • Also sanitizes tenantId in the two "tenant not found / not active" warnings, since that value originates from header / subdomain / JWT claim.

Fixes CodeQL cs/log-forging (CWE-117) alerts #2 and #3.

Refs POM-175.

Test plan

Wrap context.Request.Path.Value through SanitizeForLog helper before
logging in TenantValidationMiddleware to neutralize CRLF injection.

Refs POM-175, CodeQL alerts #2 and #3 (cs/log-forging).
Copilot AI review requested due to automatic review settings April 24, 2026 13:46
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses CodeQL cs/log-forging (CWE-117) findings by sanitizing user-controlled values before they are written to logs in the ASP.NET Core tenant validation middleware.

Changes:

  • Added a SanitizeForLog helper to strip certain control characters from untrusted strings before logging.
  • Applied sanitization to request path logging and tenant-id-related warning logs in TenantValidationMiddleware.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -65,7 +65,7 @@ public async Task InvokeAsync(
_logger.LogWarning(
"Tenant validation failed: {Error}. Path: {Path}",
validationResult.Error.Message,
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

validationResult.Error.Message can contain user-controlled tenant identifiers (e.g., mismatch errors include header/subdomain/JWT values) and is currently logged without sanitization. To fully mitigate CWE-117/log-forging here, sanitize the {Error} value as well (or log a stable error code and keep the raw message only for the response).

Suggested change
validationResult.Error.Message,
SanitizeForLog(validationResult.Error.Message),

Copilot uses AI. Check for mistakes.
{
if (string.IsNullOrEmpty(value))
return string.Empty;
return value.Replace("\r", "_").Replace("\n", "_").Replace("\t", "_");
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

SanitizeForLog only replaces \r, \n, and \t. Log-injection can also be done with other control/newline characters (e.g., other char.IsControl chars, Unicode line separators, or ANSI escape sequences). Consider normalizing/replacing all control characters (e.g., via char.IsControl/\p{Cc}) so the helper provides a more comprehensive guarantee.

Suggested change
return value.Replace("\r", "_").Replace("\n", "_").Replace("\t", "_");
var sanitized = new System.Text.StringBuilder(value.Length);
foreach (var ch in value)
{
sanitized.Append(char.IsControl(ch) || ch == '\u2028' || ch == '\u2029' ? '_' : ch);
}
return sanitized.ToString();

Copilot uses AI. Check for mistakes.
@Pomdapis Pomdapis merged commit e7ae80c into main Apr 24, 2026
5 checks passed
@Pomdapis Pomdapis deleted the vk/70e8-pom-175-sanitize branch April 24, 2026 13:53
Pomdapis added a commit that referenced this pull request Apr 26, 2026
## Summary

PR #23 incorrectly bumped CHANGELOG to `[1.0.0-preview.2] - 2026-04-26`
with the quality-sweep entries — but tag `v1.0.0-preview.2` was already
cut on **2026-04-25** from a different commit set (PRs #1-7) and
**already published to nuget.org** (`Compendium.Core 1.0.0-preview.2` is
live). Reusing that version was a mistake.

This PR reconciles the CHANGELOG with the published reality and rolls
today's work into a new **preview.3**:

### `[1.0.0-preview.2] - 2026-04-25` — rewritten retroactively

Now matches the auto-generated GitHub release notes for
`v1.0.0-preview.2`:
- **Added** — `Compendium.Adapters.Shared` (PII masking utilities,
introduced in #3).
- **Changed** — Dependabot bumps #4-7, OSS governance scaffolding.
- **Security** — workflow `permissions:` block (#1), tenant log
sanitization (#2), email removal from adapter logs / GDPR (#3).

### `[1.0.0-preview.3] - 2026-04-26` — new

Everything since `v1.0.0-preview.2`:
- **Added** — DocFX site (#17), 5 ADRs (#14), public ROADMAP (#15),
getting-started guide (#20), 4 concept pages (#21), 8 adapter how-to
guides (#22).
- **Changed** — CodeQL Default Setup → `extended` query suite.
- **Security** — `softprops/action-gh-release` pinned to commit SHA
(#16, alert #28 closed).

## Test plan

- [ ] CI green on this PR.
- [ ] After merge, tag `v1.0.0-preview.3` triggers Release workflow
successfully.
- [ ] `Compendium.* @ 1.0.0-preview.3` published on nuget.org.
- [ ] GitHub Release `v1.0.0-preview.3` created with auto-generated
notes.

VK: POM-186 (Code Quality sweep parent).

Co-authored-by: sacha <sacha@scojhconsult.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants