Implement security hardening with CSP and localized assets#88
Implement security hardening with CSP and localized assets#88Carl Sixsmith (carlsixsmith-moj) wants to merge 1 commit intomainfrom
Conversation
Implements a Content Security Policy (CSP) and localizes the vis-network dependency to ensure compliance. Additionally, this adds anti-forgery token validation to page models and forces the HTTPS scheme in middleware.
There was a problem hiding this comment.
Pull request overview
This PR aims to harden the Visualiser ASP.NET Core app by adding CSRF protections, enforcing HTTPS, adding a CSP header, and switching the vis-network script from a CDN to a local static asset.
Changes:
- Added anti-forgery validation attributes to Razor PageModels.
- Added middleware to force HTTPS and to add a
Content-Security-Policyresponse header. - Updated
Index.cshtmlto loadvis-network.min.jsfromwwwroot/jsinstead of a CDN.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Visualiser/Program.cs |
Adds middleware intended to enforce HTTPS and attach a CSP header to responses. |
src/Visualiser/Pages/ProcessedFiles.cshtml.cs |
Adds [ValidateAntiForgeryToken] to the page model. |
src/Visualiser/Pages/Index.cshtml.cs |
Adds [ValidateAntiForgeryToken] to the page model. |
src/Visualiser/Pages/Index.cshtml |
Switches vis-network script reference from CDN to a local file under ~/js/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string csp = app.Configuration["Content-Security-Policy"] | ||
| ?? "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; object-src 'self' data:; frame-src 'self' data:;"; | ||
| context.Response.Headers.Append("Content-Security-Policy", csp); | ||
| } |
There was a problem hiding this comment.
Using Response.Headers.Append("Content-Security-Policy", ...) can result in multiple CSP headers if something upstream/downstream also sets CSP, and browsers will apply the intersection which can be confusing to debug. Prefer setting the header value (e.g., Headers["Content-Security-Policy"] = ...) or only add it when it's not already present.
|
|
||
| @section Scripts { | ||
| <script type="text/javascript" src="https://unpkg.com/vis-network/standalone/umd/vis-network.min.js"></script> | ||
| <script type="text/javascript" src="~/js/vis-network.min.js"></script> |
There was a problem hiding this comment.
This script is now served from a local static file but doesn't use cache-busting (asp-append-version) like the other local assets in this project. Consider adding asp-append-version="true" to avoid clients being stuck with an old vis-network.min.js after deployments.
| <script type="text/javascript" src="~/js/vis-network.min.js"></script> | |
| <script type="text/javascript" src="~/js/vis-network.min.js" asp-append-version="true"></script> |
| app.Use((context, next) => | ||
| { | ||
| context.Request.Scheme = "https"; | ||
| return next(); | ||
| }); |
There was a problem hiding this comment.
Setting context.Request.Scheme = "https" does not enforce HTTPS (it doesn't redirect) and can also override the actual scheme determined by UseForwardedHeaders/UseHttpsRedirection, potentially leading to incorrect URL generation and masking HTTP traffic. Remove this middleware and rely on app.UseHttpsRedirection() (already present) plus correctly configured forwarded headers / HSTS for enforcement behind a proxy.
| if(context.Response.Headers.IsReadOnly == false) | ||
| { | ||
| string csp = app.Configuration["Content-Security-Policy"] | ||
| ?? "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; object-src 'self' data:; frame-src 'self' data:;"; |
There was a problem hiding this comment.
The default CSP includes 'unsafe-inline' for script-src and style-src, which substantially weakens XSS protection. Prefer removing 'unsafe-inline' and using nonces/hashes (or moving inline code to external files) so the CSP actually blocks injected inline script/style.
| ?? "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; object-src 'self' data:; frame-src 'self' data:;"; | |
| ?? "default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; font-src 'self'; object-src 'self' data:; frame-src 'self' data:;"; |
This pull request focuses on improving security and resource management in the Visualiser application by introducing anti-forgery protections, enforcing HTTPS, and adding a Content Security Policy (CSP) header. Additionally, it updates the way JavaScript dependencies are loaded. The most important changes are grouped below:
Security Enhancements:
[ValidateAntiForgeryToken]attribute to bothIndexModelandProcessedFilesModelclasses to protect against CSRF attacks (Index.cshtml.cs,ProcessedFiles.cshtml.cs). [1] [2]Program.csto enforce the use of HTTPS for all incoming requests.Program.cs).Resource Management:
vis-network.min.jsscript inIndex.cshtmlfrom an external CDN to a local file, improving reliability and security.Implements a Content Security Policy (CSP) and localizes the vis-network dependency to ensure compliance. Additionally, this adds anti-forgery token validation to page models and forces the HTTPS scheme in middleware.