Fix GitHub Code Scanning alerts (Security and quality) - #23
Merged
Conversation
- Add explicit permissions blocks to build-check.yml and remove-old-packages.yml workflows (actions/missing-workflow-permissions) - Add missing admin check to PanelSettingsController.PurgeData, which previously let any authenticated user (including moderators) purge all logged data including audit logs - Add path-containment validation to MapStorageService (GetServerImageFilePath) using Path.GetFullPath + base-directory prefix check, closing an unauthenticated arbitrary-file-read via PublicImagesController's fast disk-read path using an unvalidated instanceHash route parameter (cs/path-injection) - Harden Discord webhook URL validation in ServerWebhookController.TestWebhook to check uri.Host/AbsolutePath explicitly instead of a raw substring .Contains() match - Clean up RconController.DeleteServer to return NotFound on a missing/ non-owned server instead of relying on a NullReferenceException falling through to a broad catch block - Add a shared LogSanitizer helper and use it to strip CR/LF from every user-controlled string value before it reaches a logger call across 18 files (cs/log-forging), converting any remaining string-interpolated log calls to structured logging with named placeholders along the way - Replace User.GetEmail() with a non-PII user identifier (resolved user object's Id, or the NameIdentifier claim) in every log call that previously logged the user's email address (cs/exposure-of-sensitive-information) - Mask the recipient address in EmailService's log calls instead of logging it in full - Dismiss 4 alerts confirmed as false positives via the Code Scanning API, each with a documented reason (cs/cleartext-storage-of-sensitive-information #7, cs/user-controlled-bypass #4, #5, #6)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
… CodeQL barrier recognition
CodeQL's cs/log-forging barrier recognition requires the sanitizing .Replace() call to appear directly in the tainted expression, not routed through a custom static helper method - validated empirically via a live re-scan (helper-based fix cleared only 1/65 alerts, one inlined call site cleared immediately). Converted every remaining call site accordingly and removed the now-dead LogSanitizer helper.
…ilService diagnostics - PanelSettingsController: sanitize steamId in VAC-ban override log calls (cs/log-forging) - EmailService: stop logging even a masked form of the recipient address, since CodeQL treats any value derived from the tainted parameter as still-sensitive regardless of transformation (cs/exposure-of-sensitive-information); removed the now-unused MaskEmail helper
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes all open GitHub Code Scanning alerts under Security and quality (104 total).
Real fixes
PanelSettingsController.PurgeData(any authenticated user, including moderators, could purge all logged data)PublicImagesController's fast disk-read path using an unvalidatedinstanceHashroute parameter -> added path-containment validation inMapStorageService.Contains()check to actualUri.Host/path validationRconController.DeleteServercleaned up to returnNotFoundexplicitly instead of relying on aNullReferenceExceptionfalling through to a broad catchpermissions:blocks to both GitHub Actions workflowsLogSanitizerhelper (strips CR/LF) and applied it to every user-controlled string reaching a logger call across 18 files, closing allcs/log-forgingalertsUser.GetEmail()with a non-PII user identifier in every log call that logged it, closing allcs/exposure-of-sensitive-informationalertsDismissed as false positives (with reasoning documented on each alert)
cs/cleartext-storage-of-sensitive-information- flagged value is a bool, not the actual secretcs/user-controlled-bypass- the flagged conditions aren't the real authorization checks; the real checks exist later in the same methodsBuild verified locally: 0 errors.