Skip to content

Potential fix for code scanning alert no. 47: Log entries created from user input#202

Draft
jeffcumpsty-tpx wants to merge 1 commit intostagingfrom
alert-autofix-47
Draft

Potential fix for code scanning alert no. 47: Log entries created from user input#202
jeffcumpsty-tpx wants to merge 1 commit intostagingfrom
alert-autofix-47

Conversation

@jeffcumpsty-tpx
Copy link
Collaborator

Potential fix for https://github.com/OpenReferralUK/oruk-validator/security/code-scanning/47

In general, to fix log-forging problems, any user-controlled value written to logs should be sanitized before logging, typically by stripping or normalizing newline and other control characters so that they cannot break out of the intended log entry. For this codebase, we should continue the existing pattern of sanitizing values specifically for logging (as with SchemaResolverService.SanitizeUrlForLogging) and apply that to user-controlled header names and any other sensitive fields we log from DataSourceAuthentication.

The single best fix here, without changing external behavior, is:

  • Sanitize auth.BasicAuth.Username and header.Key before passing them into _logger.LogDebug.
  • Keep using the original unsanitized values for actual HTTP headers and authentication; only the logged representations are sanitized.
  • Implement a small private helper method in OpenApiValidationService (since we’re already in that class and must avoid assuming other helpers exist) to remove newline characters and other risky control characters from strings used in log messages.
  • Use this helper in the two logging calls:
    • _logger.LogDebug("Applied Basic authentication for user: {Username}", ...)
    • _logger.LogDebug("Applied custom header: {HeaderName}", ...)

Concretely, in OpenReferralApi.Core/Services/OpenApiValidationService.cs near ApplyAuthentication, we will:

  1. Add a private SanitizeForLogging(string? value) method that:
    • Returns an empty string for null.
    • Removes \r and \n (and optionally other standard control characters) using Replace/Where.
  2. Replace auth.BasicAuth.Username in the log call with SanitizeForLogging(auth.BasicAuth.Username).
  3. Replace header.Key in the log call with SanitizeForLogging(header.Key).

No changes are needed in OpenApiController.cs.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…m user input

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🚫 Merge blocked

Pull requests targeting main are only allowed from the staging branch.
Please merge your changes into staging first, then open a PR from stagingmain.

Current source branch: alert-autofix-47

@jeffcumpsty-tpx jeffcumpsty-tpx changed the base branch from main to staging March 5, 2026 00:08
@jeffcumpsty-tpx jeffcumpsty-tpx marked this pull request as ready for review March 5, 2026 00:09
@jeffcumpsty-tpx jeffcumpsty-tpx marked this pull request as draft March 5, 2026 00:09
@jeffcumpsty-tpx jeffcumpsty-tpx changed the base branch from staging to main March 5, 2026 00:09
@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🚫 Merge blocked

Pull requests targeting main are only allowed from the staging branch.
Please merge your changes into staging first, then open a PR from stagingmain.

Current source branch: alert-autofix-47

@jeffcumpsty-tpx jeffcumpsty-tpx changed the base branch from main to staging March 5, 2026 00:11
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.

1 participant