Fix ~238 code scanning security alerts: log injection, regex injection, XSS, container privilege escalation#424
Conversation
…s before logging Add SanitizeForLog helper to 30 C# files across all microservices to strip newline characters (\r and \n) from user-controlled string values before passing them to logging methods, preventing log injection/forgery attacks. Affected services: reference-data, claims, eligibility, appeals, tenant, provider, trading-partner, payment, enrollment-import, benefit-plan, attachment, authorization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…file non-root users Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| _logger.LogInformation( | ||
| "Searching providers: name={Name}, specialty={Specialty}, zip={Zip}, state={State}, plan={Plan}, lob={LOB}", | ||
| name, specialty, zipCode, state, planId, lineOfBusiness); | ||
| SanitizeForLog(name), SanitizeForLog(specialty), SanitizeForLog(zipCode), SanitizeForLog(state), SanitizeForLog(planId), lineOfBusiness); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
To fix the issue, we should stop writing the raw zipCode value (untrusted, potentially private data) directly to logs. Instead, log either: (a) a non-sensitive, coarser representation (such as whether a ZIP filter was provided, or the first few characters only), or (b) completely omit it from the log. This preserves logging for diagnostics while avoiding storing private information in an external location.
The minimal change, without altering existing behavior for other parameters, is:
- Introduce a new helper method that derives a non-sensitive representation of
zipCodefor logs, for example:- return
"none"whenzipCodeis null/empty, - otherwise return a fixed string such as
"provided", or a masked version like the first 3 characters plus"**"after passing throughSanitizeForLog.
- return
- Update the
LogInformationcall inSearchProvidersso that thezipplaceholder uses this helper instead ofSanitizeForLog(zipCode). - Keep
SanitizeForLogunchanged for other parameters (name, specialty, state, plan, etc.), because the CodeQL finding specifically targetszipCode.
Concretely, in src/services/provider-service/Controllers/ProvidersController.cs:
- Add a new private static method
GetZipCodeLogValue(string? zipCode)nearSanitizeForLog. - Change the logging line around line 59–61 so that the
zipargument isGetZipCodeLogValue(zipCode)and the format string no longer implies we’re logging the raw ZIP code value (e.g., changezip={Zip}tozipFilter={ZipFilter}or similar to reflect that it’s no longer the raw value).
No additional external dependencies are required; we only use standard C# string operations.
| @@ -57,8 +57,8 @@ | ||
| [FromQuery] int pageSize = 50) | ||
| { | ||
| _logger.LogInformation( | ||
| "Searching providers: name={Name}, specialty={Specialty}, zip={Zip}, state={State}, plan={Plan}, lob={LOB}", | ||
| SanitizeForLog(name), SanitizeForLog(specialty), SanitizeForLog(zipCode), SanitizeForLog(state), SanitizeForLog(planId), lineOfBusiness); | ||
| "Searching providers: name={Name}, specialty={Specialty}, zipFilter={ZipFilter}, state={State}, plan={Plan}, lob={LOB}", | ||
| SanitizeForLog(name), SanitizeForLog(specialty), GetZipCodeLogValue(zipCode), SanitizeForLog(state), SanitizeForLog(planId), lineOfBusiness); | ||
|
|
||
| var providers = await _providerRepository.SearchAsync( | ||
| name, specialty, zipCode, state, planId, lineOfBusiness, providerType, acceptingNewPatients, page, pageSize); | ||
| @@ -311,6 +311,23 @@ | ||
| return string.Empty; | ||
| return value.Replace("\r", string.Empty).Replace("\n", string.Empty); | ||
| } | ||
|
|
||
| private static string GetZipCodeLogValue(string? zipCode) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(zipCode)) | ||
| { | ||
| return "none"; | ||
| } | ||
|
|
||
| // Only log a coarse, non-sensitive representation of the ZIP code | ||
| var sanitized = SanitizeForLog(zipCode); | ||
| if (sanitized.Length <= 3) | ||
| { | ||
| return "provided"; | ||
| } | ||
|
|
||
| return sanitized.Substring(0, 3) + new string('*', sanitized.Length - 3); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> |
…ing consistency Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a large set of GitHub Advanced Security findings across the repo by hardening logging (CWE-117), regex construction (CWE-730), DOM manipulation (CWE-79), and container runtime privileges (CWE-250).
Changes:
- Sanitizes user-controlled values before logging in multiple C# services and two TypeScript audit loggers.
- Escapes user-provided strings before constructing
RegExpinstances in TypeScript utilities/redaction logic. - Replaces a DOM
innerHTMLinjection with safe DOM APIs; updates multiple Dockerfiles to run as non-root.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/site/js/auth.js | Replaces innerHTML sign-in link injection with safe DOM API construction. |
| src/site/Dockerfile | Switches nginx to non-root and moves to non-privileged port 8080. |
| src/services/trading-partner-service/Dockerfile | Runs .NET service as a non-root user. |
| src/services/trading-partner-service/Controllers/TradingPartnersController.cs | Sanitizes logged identifiers to prevent log injection. |
| src/services/tenant-service/Services/TenantRepository.cs | Sanitizes logged tenant identifiers/names. |
| src/services/tenant-service/Services/StripeService.cs | Sanitizes logged Stripe and tenant identifiers. |
| src/services/tenant-service/Dockerfile | Runs tenant-service as non-root (includes kubectl + scripts). |
| src/services/reference-data-service/Dockerfile | Runs reference-data-service as non-root. |
| src/services/reference-data-service/Controllers/ReferenceDataController.cs | Sanitizes logged query/path parameters. |
| src/services/provider-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/provider-service/Dockerfile | Runs provider-service as non-root. |
| src/services/provider-service/Controllers/ProvidersController.cs | Sanitizes logged identifiers in controller actions. |
| src/services/payment-service/Services/PaymentRunService.cs | Sanitizes logged identifiers in execution and error paths. |
| src/services/payment-service/Repositories/PaymentRunRepository.cs | Sanitizes logged identifiers in repository operations. |
| src/services/payment-service/Repositories/PaymentRepository.cs | Sanitizes logged identifiers (payment id/check number). |
| src/services/payment-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/payment-service/Controllers/PaymentsController.cs | Sanitizes logged identifiers from request payloads/params. |
| src/services/payment-service/Controllers/PaymentRunsController.cs | Sanitizes logged identifiers from request payloads/params. |
| src/services/enrollment-import-service/Services/EnrollmentRepository.cs | Sanitizes logged identifiers in repository operations. |
| src/services/enrollment-import-service/Services/EnrollmentImportService.cs | Sanitizes logged subscriber/maintenance type values. |
| src/services/enrollment-import-service/Dockerfile | Runs enrollment-import-service as non-root. |
| src/services/enrollment-import-service/Controllers/EnrollmentController.cs | Sanitizes logged filename/tenant id. |
| src/services/eligibility-service/Repositories/EligibilityRepository.cs | Sanitizes logged inquiry/response identifiers. |
| src/services/eligibility-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/eligibility-service/Controllers/EligibilityController.cs | Sanitizes logged subscriber id. |
| src/services/claims-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/claims-service/Dockerfile | Runs claims-service as non-root. |
| src/services/claims-service/Controllers/ClaimsController.cs | Sanitizes logged identifiers from payloads/params. |
| src/services/benefit-plan-service/Services/BenefitPlanService.cs | Sanitizes logged plan/category/CPT values. |
| src/services/benefit-plan-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/benefit-plan-service/Dockerfile | Runs benefit-plan-service as non-root. |
| src/services/benefit-plan-service/Controllers/BenefitPlansController.cs | Sanitizes logged tenant/payer/planType query values. |
| src/services/authorization-service/Middleware/TenantMiddleware.cs | Sanitizes logged tenant id and JWT subject. |
| src/services/authorization-service/Dockerfile | Runs authorization-service as non-root. |
| src/services/authorization-service/Controllers/AuthorizationsController.cs | Sanitizes logged identifiers from payloads/params. |
| src/services/attachment-service/Services/AcknowledgmentService.cs | Sanitizes logged payer/tenant identifiers. |
| src/services/attachment-service/Dockerfile | Runs attachment-service as non-root. |
| src/services/attachment-service/Controllers/AttachmentsController.cs | Sanitizes logged identifiers and error logs. |
| src/services/appeals-service/Repositories/AppealRepository.cs | Sanitizes logged appeal/claim identifiers. |
| src/services/appeals-service/Middleware/TenantMiddleware.cs | Sanitizes tenant id values written to logs. |
| src/services/appeals-service/Controllers/AppealsController.cs | Sanitizes logged identifiers from payloads/params. |
| src/fhir/provider-access-api.ts | Sanitizes audit log fields before console.log to prevent log injection. |
| src/fhir/patient-access-api.ts | Sanitizes audit log fields before console.log to prevent log injection. |
| src/ai/redaction.ts | Escapes PHI field names before constructing regexes. |
| scripts/utils/template-helpers.ts | Escapes replace helper search string before creating a regex. |
| containers/x12-834-parser/Dockerfile | Drops root privileges (runs as node) and fixes directory ownership. |
| containers/claims-publisher/Dockerfile | Drops root privileges (runs as non-root appuser). |
src/site/Dockerfile
Outdated
| RUN sed -i 's/listen\s*80;/listen 8080;/g' /etc/nginx/conf.d/default.conf \ | ||
| && chown -R nginx:nginx /usr/share/nginx/html \ | ||
| && chown -R nginx:nginx /var/cache/nginx \ | ||
| && chown -R nginx:nginx /var/log/nginx \ | ||
| && touch /var/run/nginx.pid \ | ||
| && chown nginx:nginx /var/run/nginx.pid |
There was a problem hiding this comment.
The sed expression uses \s*, which BusyBox sed (used in nginx:alpine) does not treat as whitespace. As a result, default.conf may remain unchanged (still listening on port 80), and nginx will fail to start under USER nginx due to binding a privileged port. Use a POSIX-compatible whitespace pattern (e.g., [[:space:]]*) and also update any listen [::]:80; line(s) so nginx binds only to 8080 when running non-root.
There was a problem hiding this comment.
Fixed in 5f6903e. Replaced FROM nginx:alpine with FROM nginxinc/nginx-unprivileged:alpine, which runs as non-root (UID 101) and listens on port 8080 by default — no sed needed at all.
src/site/Dockerfile
Outdated
| EXPOSE 8080 | ||
|
|
There was a problem hiding this comment.
This Dockerfile switches nginx to port 8080, but the repo’s Kubernetes manifest for the site (e.g., src/site/k8s/site-deployment.yaml) still targets/probes port 80. Unless those manifests are updated in this PR, the site pod will become unhealthy / unreachable after deployment. Either keep nginx listening on 80 via capabilities/unprivileged image, or update the deployment/service/probes to 8080 to match.
There was a problem hiding this comment.
Fixed in 5f6903e. Updated src/site/k8s/site-deployment.yaml: containerPort and both probe ports changed to 8080; Service targetPort changed to 8080 (Service still exposes port 80 externally so the Ingress is unaffected). Also switched to nginxinc/nginx-unprivileged:alpine which handles the non-root/port-8080 setup out of the box.
| # Create non-root user (kubectl and sftp scripts run as this user) | ||
| RUN groupadd -r appuser && useradd -r -g appuser appuser \ | ||
| && chown -R appuser:appuser /app | ||
| USER appuser |
There was a problem hiding this comment.
Creating appuser with useradd -r typically does not create a home directory or set HOME. Since this image installs and is intended to run tools like kubectl and provisioning scripts, lack of a writable home (e.g., for ~/.kube or tool caches) can cause runtime failures under the non-root user. Consider creating /home/appuser (or using useradd -m -d /home/appuser) and setting ENV HOME=/home/appuser before switching users.
There was a problem hiding this comment.
Fixed in 5f6903e. Changed useradd -r to useradd -r -m -d /home/appuser so the home directory is created, and added ENV HOME=/home/appuser so tools like kubectl resolve ~/.kube correctly.
|
@copilot implement code review comments |
… update k8s manifest ports, add home dir for appuser Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/site/Dockerfile:16
- The
sedexpression uses\s(listen\s*80;), which BusyBoxsedin Alpine typically does not support. This can result in the listen directive not being updated, leaving nginx attempting to bind to port 80 while running as the non-rootnginxuser (startup failure). Use a POSIX-compatible pattern (e.g.,[[:space:]]) and ensure all relevant listen directives (including common IPv6listen [::]:80;/ additional args) are updated, or ship an explicit nginx config that listens on 8080.
nginxinc/nginx-unprivileged:alpine(runs non-root on 8080, no sed needed); updatesrc/site/k8s/site-deployment.yamlcontainerPort/probes to 8080 and Service targetPort to 8080-m -d /home/appuserto tenant-serviceuseraddso kubectl can write to~/.kube; setENV HOME=/home/appuser🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.