Open
Conversation
Add HtmlSanitizer to sanitize HTML generated by Markdig before rendering README and Release Notes in Razor pages. Previously, raw HTML from Markdown (including script, iframe, event handlers) was injected via HtmlString without any sanitization, allowing package authors to execute arbitrary JavaScript in browsers of anyone viewing the package page. Severity: CRITICAL
Replace string == and .Equals() with CryptographicOperations.FixedTimeEquals for all secret comparisons (API keys and Basic Auth passwords). String equality short-circuits on first differing byte, leaking information about the expected value through response timing. Severity: HIGH
Limit allowed CORS methods to GET, HEAD, and OPTIONS instead of AllowAnyMethod. This prevents cross-origin websites from performing package push (PUT), delete, or relist operations via the browser. AllowAnyHeader is also replaced with an explicit whitelist of safe headers. Severity: HIGH
Log a prominent warning at startup when neither ApiKey, Authentication:ApiKeys, nor Authentication:Credentials are configured. The default configuration ships with an empty ApiKey, meaning anyone can push, delete, and relist packages without credentials. Severity: HIGH
Reorder middleware pipeline: UseRouting -> UseCors -> UseAuthentication -> UseAuthorization (as per ASP.NET Core documentation). Add UseHsts() for production environments and UseHttpsRedirection() to ensure credentials (especially Basic Auth) are never transmitted in cleartext. Severity: MEDIUM
Call ValidatePackageEntriesAsync on uploaded packages (previously skipped with a TODO comment). This prevents zip-slip attacks and rejects malformed archive entries in uploaded NuGet packages. Severity: MEDIUM
Replace MaxRequestBodySize = null (unlimited) with an 8 GiB limit matching the MaxPackageSizeGiB default. Without a reverse proxy, the previous configuration allowed arbitrarily large uploads enabling denial-of-service. Severity: MEDIUM
… etc) storage configuration
…ions Add AllowedCorsOrigins and TrustedProxies properties to BaGetterOptions to support the configurable CORS and forwarded headers introduced in ConfigureBaGetterServer.
Defense-in-depth: validate that package IDs and versions do not contain path traversal sequences (.., /, \) before constructing storage paths. FileStorageService.GetFullPath() already guards against this at a lower layer, but validating early catches issues closer to the source.
Add security guidance on MirrorAuthenticationOptions recommending Docker secrets or environment variables for credentials. Clarify that MD5 in GoogleCloudStorageService is a GCS API constraint, not used for security purposes.
There was a problem hiding this comment.
Pull request overview
This PR strengthens BaGetter’s default deployment security posture by tightening HTTP pipeline behavior, reducing common web attack surface (XSS/zip-slip/path traversal), and adding safer defaults and configuration hooks for auth/proxy/CORS handling.
Changes:
- Add security protections: sanitize rendered Markdown HTML, validate package archive entries, and harden storage path segment handling.
- Improve security defaults and guidance: HSTS/HTTPS redirection, auth-missing warning, constant-time secret comparisons.
- Introduce configuration hooks for CORS origins and trusted proxies; update sample configuration and upload limits.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BaGetter/ValidateBaGetterOptions.cs | Logs a prominent warning when no authentication is configured. |
| src/BaGetter/Startup.cs | Adds HSTS/HTTPS redirection and adjusts auth middleware ordering. |
| src/BaGetter/Program.cs | Sets a Kestrel request body size limit (currently hard-coded). |
| src/BaGetter/ConfigureBaGetterServer.cs | Adds AllowedCorsOrigins/TrustedProxies handling and tightens CORS policy. |
| src/BaGetter/appsettings.json | Adds commented MinIO/S3 configuration example. |
| src/BaGetter.Web/Pages/Package.cshtml.cs | Sanitizes README/release notes HTML after Markdown rendering. |
| src/BaGetter.Web/BaGetter.Web.csproj | Adds HtmlSanitizer dependency. |
| src/BaGetter.Web/Authentication/NugetBasicAuthenticationHandler.cs | Uses fixed-time comparison for credential checks. |
| src/BaGetter.Gcp/GoogleCloudStorageService.cs | Clarifies MD5 usage rationale (metadata conflict detection). |
| src/BaGetter.Core/Storage/PackageStorageService.cs | Validates path segments before composing storage paths. |
| src/BaGetter.Core/Indexing/PackageIndexingService.cs | Validates package archive entries to mitigate zip-slip/malformed archives. |
| src/BaGetter.Core/Configuration/MirrorAuthenticationOptions.cs | Adds documentation warning against storing secrets in appsettings.json. |
| src/BaGetter.Core/Configuration/BaGetterOptions.cs | Adds AllowedCorsOrigins and TrustedProxies configuration properties. |
| src/BaGetter.Core/Authentication/ApiKeyAuthenticationService.cs | Uses fixed-time comparison for API key checks. |
| Directory.Packages.props | Adds centralized package version for HtmlSanitizer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
+45
| builder.WithOrigins(origins); | ||
| } | ||
| else | ||
| { | ||
| builder.AllowAnyOrigin(); | ||
| } | ||
|
|
||
| builder | ||
| .WithMethods("GET", "HEAD", "OPTIONS") | ||
| .WithHeaders("Accept", "Accept-Language", "Content-Language", "Content-Type"); |
| options.KnownProxies.Clear(); | ||
| foreach (var proxy in trustedProxies.Where(p => !string.IsNullOrWhiteSpace(p))) | ||
| { | ||
| options.KnownProxies.Add(IPAddress.Parse(proxy)); |
Comment on lines
+76
to
+78
| // Limit upload size to 8 GiB (matches MaxPackageSizeGiB default). | ||
| // Can be further restricted by a reverse proxy. | ||
| options.Limits.MaxRequestBodySize = 8L * 1024 * 1024 * 1024; |
Comment on lines
+17
to
+23
| // To use MinIO or any S3-compatible storage, uncomment and replace the Storage section above: | ||
| //"Storage": { | ||
| // "Type": "AwsS3", | ||
| // "Endpoint": "http://localhost:9000", | ||
| // "Bucket": "nuget-packages", | ||
| // "AccessKey": "minioadmin", | ||
| // "SecretKey": "minioadmin" |
Comment on lines
+41
to
+42
| var expectedBytes = Encoding.UTF8.GetBytes(expected); | ||
| var actualBytes = Encoding.UTF8.GetBytes(actual); |
| var expectedBytes = Encoding.UTF8.GetBytes(expected); | ||
| var actualBytes = Encoding.UTF8.GetBytes(actual); | ||
|
|
||
| return CryptographicOperations.FixedTimeEquals(expectedBytes, actualBytes); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Julien CHABLE <julien@chable.net>
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
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.
Dear team,
Several security or best practises fixes (santization, security enforcement). I don't know if some security choice is to minimize configuration for users (so if it's by deisgn) or not.
Best and thank you for you workand time.
Julien