SonarCloud reports 10 open pythonsecurity:S5145 findings ("Change this code to not log user-controlled data"), spread across two files:
api/sockets.py lines 53, 73, 97, 102, 195, 198, 233, 240, 245
services/container_events.py line 40
These log values that arrive from WebSocket query parameters (unit names, container names) and from remote podman output.
Current state: mitigated, but by accident rather than by policy
Most of the flagged sites interpolate with !r, and that conversion is doing real work. repr() escapes control characters, so a crafted name containing a newline cannot forge a second log entry:
plain interpolation -> output spans 2 log lines (forged entry injected)
with !r -> output spans 1 log line
So the concrete log-forging attack is already blocked wherever !r is used. The problem is that this is load-bearing behavior that nothing documents, nothing tests, and nothing enforces. Anyone dropping the !r while tidying an f-string would silently reintroduce log injection, and no test would notice.
Worth doing
- Audit all 10 sites and confirm which actually interpolate untrusted data, and which of those use
!r. Not all 10 are necessarily the same shape; the container_events.py one takes remote command output rather than a query parameter.
- Decide on one deliberate approach rather than relying on an incidental
repr(). Options: keep !r but document why it is required and add a test asserting a newline-bearing name produces a single log line; or sanitize explicitly at the log call; or attach the value as a structured logging field instead of interpolating it.
- Whichever is chosen, add a regression test, since the current protection is invisible to a reader.
Context
Noticed while reviewing #311, which consolidated two of these sites (sockets.py:53 and :198) into a single shared _reject_invalid_name() helper, taking the count from 10 to 9. That PR deliberately did not attempt a fix, since it was a pure refactor and this needs its own decision.
SonarCloud reports 10 open
pythonsecurity:S5145findings ("Change this code to not log user-controlled data"), spread across two files:api/sockets.pylines 53, 73, 97, 102, 195, 198, 233, 240, 245services/container_events.pyline 40These log values that arrive from WebSocket query parameters (unit names, container names) and from remote podman output.
Current state: mitigated, but by accident rather than by policy
Most of the flagged sites interpolate with
!r, and that conversion is doing real work.repr()escapes control characters, so a crafted name containing a newline cannot forge a second log entry:So the concrete log-forging attack is already blocked wherever
!ris used. The problem is that this is load-bearing behavior that nothing documents, nothing tests, and nothing enforces. Anyone dropping the!rwhile tidying an f-string would silently reintroduce log injection, and no test would notice.Worth doing
!r. Not all 10 are necessarily the same shape; thecontainer_events.pyone takes remote command output rather than a query parameter.repr(). Options: keep!rbut document why it is required and add a test asserting a newline-bearing name produces a single log line; or sanitize explicitly at the log call; or attach the value as a structured logging field instead of interpolating it.Context
Noticed while reviewing #311, which consolidated two of these sites (
sockets.py:53and:198) into a single shared_reject_invalid_name()helper, taking the count from 10 to 9. That PR deliberately did not attempt a fix, since it was a pure refactor and this needs its own decision.