fix(security): sanitize CR/LF from user-controlled values before logging - #63
Conversation
CodeQL `cs/log-forging` (CWE-117) flagged three sites in SimulationService.cs where user-supplied strings (`droneId`, preset `key`) flowed directly into `ILogger` calls: * Services/SimulationService.cs:149 LogWarning(droneId) * Services/SimulationService.cs:153 LogDebug(droneId) * Services/SimulationService.cs:182 LogInformation(key) An attacker who can supply CR/LF in those values can inject fake log entries (e.g. forge a "drone X armed" line). Adds a small private static `LogSafe` helper that strips `\r`/`\n` and routes the three call sites through it. Chained `String.Replace` is recognised by the CodeQL rule as a valid sanitiser, so alerts #10/#11/#17 should auto-close on the next scan. `command` (FlightCommand enum) is left as-is; the renderer only returns the type-system enum name and is not user-controlled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a LogSafe utility to sanitize user-provided strings before logging, mitigating log forging vulnerabilities (CWE-117). The utility is applied to drone identifiers and terrain keys. Feedback suggests extending this sanitization to the AddDrone method, returning actual null values instead of the string "" to better support structured logging, and refining the replacement logic for improved robustness.
…ve null Applies the gemini-code-assist review on PR #63: - AddDrone (line 135) was a missed sink; the user-controlled `id` and `vendor` flow into LogInformation just like the three sites CodeQL flagged. Wrap both with LogSafe so the same protection applies. - LogSafe now returns string? and preserves null instead of returning the magic literal "<null>". Structured loggers (Serilog, default JSON formatter, etc.) handle null natively; collapsing nulls to a string drops information from structured output. The CR/LF chained Replace is kept (CodeQL recognises it as a valid cs/log-forging sanitiser); ReplaceLineEndings would be more robust against U+2028/U+2029 etc. but trades sanitiser recognition for that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Resolves CodeQL
cs/log-forgingalerts #10, #11, #17 inServices/SimulationService.cs.LogWarning("…drone {DroneId} not found.", droneId)droneIdLogDebug("Command {Command} sent to drone {DroneId}.", command, droneId)droneIdLogInformation("Terrain preset switched to '{Key}'.", key)keyAll three values originate from HTTP requests via
SimControllerand could carry CR/LF that forge fake log entries (CWE-117).Fix
Adds a small
private static string LogSafe(string?)helper that strips\r/\n(recognised by CodeQL as a validcs/log-forgingsanitiser), and routes the three call sites through it.command(aFlightCommandenum) is left alone — its renderer returns the type-system enum name, not user input.Test plan
droneId/key)🤖 Generated with Claude Code