Skip to content

fix(security): prevent log forging and add workflow permissions #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Erlend Ellefsen (erlendellefsen) merged 1 commit into from
Jan 24, 2026
Merged

fix(security): prevent log forging and add workflow permissions #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions .config/dotnet-tools.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,6 @@
"dotnet-csharpier"
],
"rollForward": false
},
"dotnet-ef": {
"version": "9.0.2",
"commands": [
"dotnet-ef"
],
"rollForward": false
}
Comment on lines 8 to 11
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The removal of the dotnet-ef tool from dotnet-tools.json appears unrelated to the stated purpose of this PR (preventing log forging and adding workflow permissions). While the project may be using EF Core (as evidenced by references in documentation), removing the Entity Framework tooling doesn't address any security issues. This change should either be reverted or explained in the PR description as a separate cleanup task.

Copilot uses AI. Check for mistakes.
}
}
5 changes: 5 additions & 0 deletions .github/workflows/ci-cd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ concurrency:
jobs:
build-and-test:
runs-on: ubuntu-latest
permissions:
contents: read

steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -52,6 +54,9 @@ jobs:
needs: build-and-test
if: github.event_name == 'release'
runs-on: ubuntu-latest
permissions:
contents: read
packages: write

steps:
- uses: actions/checkout@v4
Expand Down
33 changes: 29 additions & 4 deletions JsonApiToolkit/Extensions/Querying/Filtering/NestedPropertyNavigator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,37 @@
using System.Linq.Expressions;
using System.Reflection;
using System.Text.RegularExpressions;
using JsonApiToolkit.Models.Querying.Filtering;
using Microsoft.Extensions.Logging;

namespace JsonApiToolkit.Extensions.Querying;

internal static class NestedPropertyNavigator
internal static partial class NestedPropertyNavigator
{
private const int MaxLogValueLength = 100;

/// <summary>
/// Sanitizes user input for safe logging by removing control characters
/// and truncating long values to prevent log forging attacks.
/// </summary>
private static string SanitizeForLog(string? value)
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

Consider making the SanitizeForLog method accessible to other classes in the codebase (change from private to internal) so it can be reused in FilterExpressionBuilder.cs and FilteredIncludeBuilder.cs, where filter.Field and include paths are also logged without sanitization. This would provide a consistent approach to log sanitization across the entire codebase and prevent code duplication.

Suggested change
private static string SanitizeForLog(string? value)
internal static string SanitizeForLog(string? value)

Copilot uses AI. Check for mistakes.
{
if (string.IsNullOrEmpty(value))
return "(empty)";

// Remove control characters (newlines, tabs, etc.) that could forge log entries
string sanitized = ControlCharRegex().Replace(value, " ");

// Truncate long values
if (sanitized.Length > MaxLogValueLength)
return string.Concat(sanitized.AsSpan(0, MaxLogValueLength), "...(truncated)");

return sanitized;
}
Comment on lines +13 to +30
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The user-provided field name filter.Field is being logged without sanitization in FilterExpressionBuilder.cs at lines 54, 68, and 152. While this PR adds sanitization for filter.Value in NestedPropertyNavigator.cs, it should also sanitize filter.Field wherever it's logged to fully address the log forging vulnerability. Consider adding the SanitizeForLog helper to FilterExpressionBuilder and applying it to these log statements as well.

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +30
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The SanitizeForLog method lacks comprehensive test coverage. Given that this is a critical security function preventing log forging attacks, it should have dedicated unit tests covering edge cases such as: empty strings, strings with only control characters, strings at the truncation boundary, strings with mixed control and normal characters, and null values. Consider adding tests in the JsonApiToolkit.Tests project to verify this function behaves correctly in all scenarios.

Copilot uses AI. Check for mistakes.

[GeneratedRegex(@"[\x00-\x1F\x7F]")]
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The regex pattern [\x00-\x1F\x7F] removes ASCII control characters (0x00-0x1F and DEL), but doesn't handle Unicode control characters (e.g., U+0085 NEL, U+2028 Line Separator, U+2029 Paragraph Separator) that could also be used for log forging. Consider expanding the regex to [\x00-\x1F\x7F\u0080-\u009F\u2028\u2029] to cover additional control characters, or use [\p{Cc}] to match all Unicode control characters.

Suggested change
[GeneratedRegex(@"[\x00-\x1F\x7F]")]
[GeneratedRegex(@"[\x00-\x1F\x7F\u0080-\u009F\u2028\u2029]")]

Copilot uses AI. Check for mistakes.
private static partial Regex ControlCharRegex();

internal static Expression? BuildSafeNestedFilterExpression(
ParameterExpression parameter,
FilterParameter filter,
Expand Down Expand Up @@ -268,7 +293,7 @@ internal static class NestedPropertyNavigator
{
logger?.LogWarning(
"Failed to convert '{Value}' to {ElementType} for collection filter",
filter.Value,
SanitizeForLog(filter.Value),
elementType.Name
);
return null;
Expand All @@ -295,7 +320,7 @@ internal static class NestedPropertyNavigator
{
logger?.LogWarning(
"Failed to convert '{Value}' to {ElementType} for collection filter",
filter.Value,
SanitizeForLog(filter.Value),
elementType.Name
);
return null;
Expand Down Expand Up @@ -412,7 +437,7 @@ internal static class NestedPropertyNavigator
{
logger?.LogWarning(
"Failed to convert '{Value}' to {PropertyType}",
filter.Value,
SanitizeForLog(filter.Value),
targetType.Name
);
return null;
Expand Down
Loading