Skip to content

Harden Loguru markup handling in test clients#8078

Draft
achamayou wants to merge 2 commits into
mainfrom
achamayou-harden-loguru-logging
Draft

Harden Loguru markup handling in test clients#8078
achamayou wants to merge 2 commits into
mainfrom
achamayou-harden-loguru-logging

Conversation

@achamayou

@achamayou achamayou commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

  • preserve arbitrary existing backslashes when escaping tag-like text for Loguru
  • safely embed dynamic request paths, headers, redirect locations, response bodies, and client descriptions in colored log messages
  • add a focused unit test that exercises the real flush_info(colors=True) path

Examples of previously unsafe input

Surface Example input Previous behavior
Escaped body text \<invalid> The old helper added one backslash, producing an even count before <invalid>, so Loguru parsed the unknown tag and raised ValueError.
Request path or redirect Location /records/<invalid> The value was interpolated directly into colored markup, so Loguru parsed <invalid> instead of logging it literally.
Request header or client description <red>spoofed</> Valid Loguru markup supplied by dynamic data was interpreted, changing the rendered log rather than preserving the input text.
Path or response body ending in a backslash trailing\ The final backslash could escape the generated </> closing tag; the response workaround avoided the parse issue by adding a visible trailing space.

The hardened paths render each example literally, including the original number of backslashes.

Context

This is defense-in-depth logging hardening split out from #8077. It intentionally does not add COSE, CBOR, or other binary media-type classification or response-body suppression; that minimal COSE-specific behavior remains in #8077.

The broader markup issue was revealed while investigating the failed GitHub Actions job: https://github.com/microsoft/CCF/actions/runs/29526537129/job/87716300085

Testing

  • tests/infra/client_logging_test.py
  • Black and Ruff on the changed Python files
  • Gersemi on CMakeLists.txt

Preserve arbitrary backslashes when escaping Loguru markup and safely embed dynamic request and response fields before generated closing tags.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7e07da9f-f450-44f5-93be-ed970fff11f1
@achamayou
achamayou requested a review from a team as a code owner July 17, 2026 07:43
Copilot AI review requested due to automatic review settings July 17, 2026 07:43
@achamayou
achamayou enabled auto-merge (squash) July 17, 2026 07:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 hardens the Python test client logging path against Loguru markup parsing errors by escaping tag-like substrings and handling tricky backslash interactions, so dynamically generated request/response data can be safely embedded in colored log messages.

Changes:

  • Update Loguru tag escaping to preserve arbitrary existing backslashes and prevent malformed markup sequences.
  • Escape dynamic request/response text (paths, headers, redirect locations, bodies) before embedding in colored log lines.
  • Add a focused Python unit test and register it with CTest to exercise flush_info(..., colors=True).

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/infra/clients.py Adds safer escaping helpers and applies them to dynamic text rendered inside Loguru colored messages.
tests/infra/client_logging_test.py Adds a unit test reproducing Loguru markup parsing scenarios via flush_info(colors=True).
CMakeLists.txt Registers the new Python unit test with CTest and sets PYTHONPATH for imports.

Comment thread tests/infra/clients.py Outdated
Convert descriptions to strings before Loguru escaping and cover trailing backslashes without duplicating their rendered output.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7e07da9f-f450-44f5-93be-ed970fff11f1
@achamayou

Copy link
Copy Markdown
Member Author

Having thought about this, tried other things, contemplated the logs, the loading times in the UI etc I think I want to do #8079 rather than this. I leave it to @eddyashton to make a decision, but I think that we need to do one or the other (or possibly something else, but something!), the current implementation is not sound.

@achamayou
achamayou marked this pull request as draft July 17, 2026 16:33
auto-merge was automatically disabled July 17, 2026 16:33

Pull request was converted to draft

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