Migrate security fixes to Test - #43
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Forwarded-header trust configuration and download cancellation have unresolved moderate issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Migrates security hardening and dependency updates to the Test deployment, including proxy-header handling, admin authorization, external schedule downloads, container updates, tests, and documentation.
Changes:
- Restricts forwarded-header processing and validates admin requesters.
- Adds a schedule-download timeout and refreshes dependencies and tests.
- Pins container images and updates security and architecture documentation.
File summaries
| File | Summary |
|---|---|
src/RfcBuddy.Web/RfcBuddy.Web.csproj |
Updates web dependencies and versioning. |
src/RfcBuddy.Web/Program.cs |
Configures trusted forwarded headers; proxy trust validation remains incomplete. |
src/RfcBuddy.Web/packages.lock.json |
Refreshes web dependency locks. |
src/RfcBuddy.Web/appsettings.json |
Adds forwarded-header settings, including an empty proxy list. |
src/RfcBuddy.Web.Tests/RfcBuddy.Web.Tests.csproj |
Updates web test dependencies. |
src/RfcBuddy.Web.Tests/packages.lock.json |
Refreshes web test locks. |
src/RfcBuddy.Web.Tests/Controllers/ApiTokensControllerTests.cs |
Updates sequence assertions. |
src/RfcBuddy.Web.Tests/Controllers/AdminControllerTests.cs |
Updates sequence assertions. |
src/RfcBuddy.App/Services/UserRegistryService.cs |
Validates admin requesters. |
src/RfcBuddy.App/Services/ExcelService.cs |
Adds download timeout; response copying remains uncancellable and non-atomic. |
src/RfcBuddy.App/RfcBuddy.App.csproj |
Updates application dependencies. |
src/RfcBuddy.App/packages.lock.json |
Refreshes application dependency locks. |
src/RfcBuddy.App.Tests/Services/UserServiceTests.cs |
Updates collection and sequence assertions. |
src/RfcBuddy.App.Tests/Services/UserRegistryServiceTests.cs |
Tests unauthorized admin changes. |
src/RfcBuddy.App.Tests/RfcBuddy.App.Tests.csproj |
Updates application test dependencies. |
src/RfcBuddy.App.Tests/packages.lock.json |
Refreshes application test locks. |
packages.lock.json |
Removes obsolete root lock data. |
docs/security-review.md |
Updates security findings and remediation documentation. |
docs/architecture.md |
Updates architecture evidence; forwarded-header documentation does not match enforcement. |
Dockerfile |
Pins base images and configures UTF-8 globalization. |
Review details
Suppressed comments (3)
docs/architecture.md:312
- This checklist marks forwarded-header handling as failing closed when proxy addresses are absent, but
appsettings.jsondefines an empty list andProgram.csdoes not reject it; the middleware can therefore trust forwarded headers from an arbitrary peer. Update the documentation after enforcing a non-empty trusted-proxy configuration (or document the actual development fallback).
- [x] **Safe Degradation and Evidence (conditional):** Forwarded-header trust fails closed without configured proxy addresses. `[Confidence: Verified]`
src/RfcBuddy.Web/Program.cs:84
- The default
ForwardedHeadersOptions.KnownIPNetworksstill trusts loopback addresses, because this block only adds configured entries toKnownProxies. A request arriving from loopback can therefore still supply trustedX-Forwarded-*values, which contradicts the explicit-proxy trust boundary and can affect generated HTTPS URLs. ClearKnownIPNetworks(andKnownProxiesbefore adding the parsed values) so only the configured proxy addresses are trusted.
foreach (IPAddress knownProxyAddress in knownProxyAddresses)
{
options.KnownProxies.Add(knownProxyAddress);
}
src/RfcBuddy.Web/Program.cs:76
- Because the checked-in configuration supplies an empty
KnownProxieslist and this code accepts it,KnownProxies/KnownNetworksremain empty. ASP.NET Core then performs no trusted-proxy check;ForwardLimit = 1limits only the number of hops, so a deployment that omits the environment override will trust attacker-suppliedX-Forwarded-Proto/X-Forwarded-Forheaders. Fail closed when no proxy is configured (for example, reject an empty list outside Development or use an explicit local-only fallback).
int forwardedHeaderLimit = builder.Configuration.GetValue<int?>("ForwardedHeaders:ForwardLimit") ?? 1;
if (forwardedHeaderLimit < 1)
{
throw new InvalidOperationException("ForwardedHeaders:ForwardLimit must be greater than zero.");
}
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| handler.Credentials = new NetworkCredential(_appSettings.SourceUser, _appSettings.SourcePassword); | ||
| } | ||
| using HttpClient client = new(handler); | ||
| client.Timeout = sourceRequestTimeout; |
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.
No description provided.