fix(security): prevent log forging and add workflow permissions#51
Conversation
Erlend Ellefsen (erlendellefsen)
commented
Jan 24, 2026
Erlend Ellefsen (erlendellefsen)
commented
- Sanitize user input before logging to prevent log forging attacks (3 high-severity fixes)
- Add explicit least-privilege permissions to CI/CD workflow jobs (2 medium-severity fixes)
- Closes CodeQL security alerts
- Sanitize user input before logging to prevent log forging attacks (3 high-severity fixes) - Add explicit least-privilege permissions to CI/CD workflow jobs (2 medium-severity fixes) - Closes CodeQL security alerts
Erlend Ellefsen (erlendellefsen)
deleted the
fix/security-log-forging-and-workflow-permissions
branch
There was a problem hiding this comment.
Pull request overview
This PR addresses log forging vulnerabilities and improves CI/CD security by sanitizing user input before logging and adding least-privilege permissions to workflow jobs. However, the fix is incomplete and misses several additional log forging vectors.
Changes:
- Added
SanitizeForLog()method to remove control characters and truncate long values in log messages - Sanitized
filter.Valuein three logging locations within NestedPropertyNavigator.cs - Added explicit read-only permissions to the build-and-test workflow job and read/write permissions to the publish job
- Removed dotnet-ef tool (unrelated to security fixes)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| JsonApiToolkit/Extensions/Querying/Filtering/NestedPropertyNavigator.cs | Added SanitizeForLog helper method and applied it to filter.Value logging statements to prevent log forging attacks |
| .github/workflows/ci-cd.yml | Added explicit least-privilege permissions to CI/CD workflow jobs (contents:read for build-and-test, contents:read + packages:write for publish) |
| .config/dotnet-tools.json | Removed dotnet-ef tool entry (unrelated to the stated security fix purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return sanitized; | ||
| } | ||
|
|
||
| [GeneratedRegex(@"[\x00-\x1F\x7F]")] |
There was a problem hiding this comment.
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.
| [GeneratedRegex(@"[\x00-\x1F\x7F]")] | |
| [GeneratedRegex(@"[\x00-\x1F\x7F\u0080-\u009F\u2028\u2029]")] |
| /// <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) | ||
| { | ||
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| "dotnet-csharpier" | ||
| ], | ||
| "rollForward": false | ||
| }, | ||
| "dotnet-ef": { | ||
| "version": "9.0.2", | ||
| "commands": [ | ||
| "dotnet-ef" | ||
| ], | ||
| "rollForward": false | ||
| } |
There was a problem hiding this comment.
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.
| private static string SanitizeForLog(string? value) | ||
| { | ||
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| /// 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) |
There was a problem hiding this comment.
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.
| private static string SanitizeForLog(string? value) | |
| internal static string SanitizeForLog(string? value) |
🤖 I have created a release *beep* *boop* --- ## [1.3.0](Intility.JsonApiToolkit-v1.2.5...Intility.JsonApiToolkit-v1.3.0) (2026-01-24) ### Features * ✨ `AllowedIncludesAttribute` to whitelist allowed include paths ([6a26c29](6a26c29)) * ✨ `JsonApiOkAsync` ([d22466d](d22466d)) * ✨ `JsonApiOkAsync` ([bc26940](bc26940)) * ✨ add filtering support for included resources ([4e81c99](4e81c99)) * ✨ add support for filtering in primary resource with included r… ([0b0bb87](0b0bb87)) * ✨ add support for filtering in primary resource with included relationships ([5c90d41](5c90d41)) * ✨ add too many reqyests exeption ([94a810d](94a810d)) * ✨ Allow collections and json columns to be mapped ([6a096bc](6a096bc)) * ✨ Code cleanup and standardization of error handling ([fec75f5](fec75f5)) * ✨ Enhance QueryHelpers with enum support and additional types ([4949c26](4949c26)) * ✨ general-purpose exception class ([0cdf9a5](0cdf9a5)) * ✨ general-purpose exception class ([e222bb4](e222bb4)) * ✨ Overall project cleanup ([c8c10f4](c8c10f4)) * ✨ Remove IncludeAsAttribute and related logic ([a1593be](a1593be)) * ✨ Support complex JsonCols ([b744b8e](b744b8e)) * 📚 add comprehensive debugging guide and enhance logging for better troubleshooting ([0ecc0cf](0ecc0cf)) * 🚀 add ApplyFiltersOnly method for pre-aggregation filtering and add documentation on statistics and aggregations ([b9c6546](b9c6546)) * 🚀 enhance query processing with AsSingleQuery for pagination and add detailed logging for inclusion processing ([7824da9](7824da9)) * **errors:** add JsonApiErrorCodes and JsonApiErrors factory methods ([#60](#60)) ([8531ad3](8531ad3)) * **errors:** complete refactor Phase 1 with exception filter tests a… ([#61](#61)) ([e5b50be](e5b50be)) ### Bug Fixes * 🐛 single included resources are no longer ignored ([1e2e4b6](1e2e4b6)) * 🚑️ `[JsonIgnore]` not being respected ([af12b0b](af12b0b)) * 🚑️ adds support for filtering on included collection fields ([ee2eb19](ee2eb19)) * 🚑️ adds support for filtering on included collection fields ([1194fd6](1194fd6)) * 🚑️ adjust query processing order for filtered and regular includes to enhance EF Core compatibility ([8d21509](8d21509)) * 🚑️ bracket nested filtering without the nessesary includes breaking main filtering ([e1e5785](e1e5785)) * 🚑️ correct version number in project file to match release version ([a0a51dd](a0a51dd)) * 🚑️ error responses for forbidden includes did not include meta information ([9610878](9610878)) * 🚑️ filtering on includes not working on 2-level ([c658327](c658327)) * 🚑️ Fixed the filtering issue for included resources. ([86cab81](86cab81)) * 🚑️ improve error messages for forbidden includes to clarify not found status ([07ea15e](07ea15e)) * 🚑️ Initial working fix. Needs further testing and validation. ([0fa5628](0fa5628)) * 🚑️ JsonApiOk and JsonApiCreated methods not adding includes ([903eda3](903eda3)) * 🚑️ refactor querying files and fix single resource relationship issues ([962d4d4](962d4d4)) * 🚑️ reorder query processing to apply sorting before includes for better EF Core compatibility ([20bf0d9](20bf0d9)) * 🚑️ three level nested values and collection include filters ([7f9a336](7f9a336)) * 🚑️ three level nested values and collection include filters ([044aaf0](044aaf0)) * 🚑️ use single query mode to prevent EF Core split query correlation issues with filtered includes ([ff48615](ff48615)) * add defensive reflection checks with ReflectionMethodCache ([#57](#57)) ([75eb978](75eb978)) * **mapping:** remove dead AddIncludedResourcesRecursive method ([#55](#55)) ([bbc8c17](bbc8c17)) * **pagination:** guard against division by zero when Size is 0 ([#59](#59)) ([0863dee](0863dee)) * **parsing:** guard unsafe string parsing in filter parsers ([#58](#58)) ([9fb463d](9fb463d)) * **security:** prevent log forging and add workflow permissions ([#51](#51)) ([5fbbaba](5fbbaba)) * **security:** prevent log forging and update tooling ([#52](#52)) ([52d73ce](52d73ce)) * support JsonPropertyName attribute and fix many-to-many collecti… ([634abff](634abff)) * support JsonPropertyName attribute and fix many-to-many collection filtering ([6f1d961](6f1d961)) ### Refactoring * 🔨 follow ts-package renaming ([4cd1e7e](4cd1e7e)) * 🔨 optimize logging and add XML documentation ([8c14bc0](8c14bc0)) * 🔨 remove Microsoft.Identity.Abstractions package reference ([55933b7](55933b7)) * 🔨 remove the OR max count ([65107d5](65107d5)) * 🔨 remove the OR max count ([5a3aa87](5a3aa87)) * 🔨 Update JsonApiOk function and docs to align with what it actually does ([bfe7635](bfe7635)) ### Documentation * 📝 update stats docs ([549743c](549743c)) * 📜 add too many request exeption to docs ([872ae2a](872ae2a)) * 📜 Clarify that filtering is only on main entity ([5ee3568](5ee3568)) * 📜 Update Claude.md ([88502bb](88502bb)) * 📜 update error message for forbidden includes to clarify not found status ([95ab6ce](95ab6ce)) ### Dependencies * **actions:** bump actions/checkout from 4 to 6 ([#47](#47)) ([a16ab53](a16ab53)) * **actions:** bump actions/setup-dotnet from 4 to 5 ([#45](#45)) ([db8c0d1](db8c0d1)) * **actions:** bump actions/upload-pages-artifact from 3 to 4 ([#44](#44)) ([c5e35fb](c5e35fb)) * **actions:** bump github/codeql-action from 3 to 4 ([#46](#46)) ([4bad70c](4bad70c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: intility-release-bot[bot] <175299729+intility-release-bot[bot]@users.noreply.github.com>
