Skip to content

Commit 2890ecb

Browse files
committed
[fix] address security & correctness audit findings
Critical - StaticFiles: prevent path traversal by normalizing the resolved path with Path.GetFullPath, rejecting '..' / NUL sequences, and verifying containment inside the configured site root (case-insensitive on Windows/macOS). Both IsValidPath() and the data-reading methods now use the safe resolver. - SimplifyWebSettings: default HideExceptionDetails to true so unhandled exception stack traces are no longer leaked to anonymous users by default. High - FilesInMemoryCache / InMemoryFilesCacheHandler: bound the in-memory cache (MaxItems, default 1024), normalize keys case-insensitively to defeat path case-variant OOM DoS, and invalidate stale entries based on the file's last-modification timestamp. - FileReader: replace static Dictionary caches guarded by an external lock with ConcurrentDictionary to eliminate read-while-write corruption. - TemplateFactory: replace Dictionary + dual lock/semaphore primitives with a ConcurrentDictionary cache and unify sync/async paths on a single SemaphoreSlim to avoid duplicate-add races. - WebContext: pass leaveOpen:true to the StreamReader used to consume the request body so subsequent middleware/model binders can still read it; add double-check inside semaphore-guarded sections; implement IDisposable so per-scope semaphores release their kernel handles. Medium - Redirector: harden Redirect(string) - accept same-origin relative paths only when they start with '/' (rejecting '//' and '/\' scheme-spoofs) and compare absolute URLs by scheme+host+port instead of substring StartsWith. All redirect/login/previous-page cookies are now HttpOnly, SameSite=Lax, Secure. - LanguageManager: language cookie switched from SameSite=None to Lax. - ControllerMetadata: detect [Authorize] on base controllers (inherit:true) so derived controllers don't silently become anonymous. - AuthRedirectExtensions: only issue the 401->redirect when the response has not started, avoiding InvalidOperationException 500s when upstream authentication middleware already flushed a challenge. Tests - Update RedirectorTests cases that relied on the old StartsWith-based redirect check to use realistic same-origin URLs. - Add tests covering path-traversal rejection, protocol-relative open redirects, host-prefix collisions, relative-path acceptance, and the hardened cookie options.
1 parent 841af8f commit 2890ecb

13 files changed

Lines changed: 317 additions & 137 deletions

File tree

src/Simplify.Web.Tests/Modules/Redirection/RedirectorTests.cs

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Security;
23
using Microsoft.AspNetCore.Http;
34
using Moq;
45
using NUnit.Framework;
@@ -57,11 +58,9 @@ public void Redirect_ToRedirectUrlHaveRedirectUrl_RedirectCalledWithCorrectLinkP
5758
// Arrange
5859

5960
var cookieCollection = new Mock<IRequestCookieCollection>();
60-
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.RedirectUrlCookieFieldName)]).Returns("foo");
61+
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.RedirectUrlCookieFieldName)]).Returns("http://localhost/my-website/foo");
6162
_context.SetupGet(x => x.Request.Cookies).Returns(cookieCollection.Object);
6263

63-
_context.SetupGet(x => x.SiteUrl).Returns("foo");
64-
6564
_responseCookies.Setup(x => x.Append(It.IsAny<string>(), It.IsAny<string>())).Callback<string, string>((key, value) =>
6665
{
6766
Assert.That(key, Is.EqualTo(Redirector.PreviousNavigatedUrlCookieFieldName));
@@ -72,7 +71,7 @@ public void Redirect_ToRedirectUrlHaveRedirectUrl_RedirectCalledWithCorrectLinkP
7271
_redirector.Redirect(RedirectionType.RedirectUrl);
7372

7473
// Assert
75-
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "foo")), Times.Once);
74+
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "http://localhost/my-website/foo")), Times.Once);
7675
}
7776

7877
[Test]
@@ -101,16 +100,14 @@ public void Redirect_ToLoginReturnUrl_NotNullOrEmpty_LoginReturnUrl()
101100
// Arrange
102101

103102
var cookieCollection = new Mock<IRequestCookieCollection>();
104-
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.LoginReturnUrlCookieFieldName)]).Returns("loginFoo");
103+
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.LoginReturnUrlCookieFieldName)]).Returns("http://localhost/my-website/loginFoo");
105104
_context.SetupGet(x => x.Request.Cookies).Returns(cookieCollection.Object);
106105

107-
_context.SetupGet(x => x.SiteUrl).Returns("loginFoo");
108-
109106
// Act
110107
_redirector.Redirect(RedirectionType.LoginReturnUrl);
111108

112109
// Assert
113-
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "loginFoo")), Times.Once);
110+
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "http://localhost/my-website/loginFoo")), Times.Once);
114111
}
115112

116113
[Test]
@@ -119,16 +116,14 @@ public void Redirect_ToPreviousPageHavePreviousPageUrl_RedirectCalledWithCorrect
119116
// Arrange
120117

121118
var cookieCollection = new Mock<IRequestCookieCollection>();
122-
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.PreviousPageUrlCookieFieldName)]).Returns("foo");
119+
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.PreviousPageUrlCookieFieldName)]).Returns("http://localhost/my-website/foo");
123120
_context.SetupGet(x => x.Request.Cookies).Returns(cookieCollection.Object);
124121

125-
_context.SetupGet(x => x.SiteUrl).Returns("foo");
126-
127122
// Act
128123
_redirector.Redirect(RedirectionType.PreviousPage);
129124

130125
// Assert
131-
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "foo")), Times.Once);
126+
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "http://localhost/my-website/foo")), Times.Once);
132127
}
133128

134129
[Test]
@@ -137,16 +132,14 @@ public void Redirect_ToPreviousPageWithBookmarkHaveUrl_RedirectCalledWithCorrect
137132
// Arrange
138133

139134
var cookieCollection = new Mock<IRequestCookieCollection>();
140-
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.PreviousPageUrlCookieFieldName)]).Returns("foo");
135+
cookieCollection.SetupGet(x => x[It.Is<string>(s => s == Redirector.PreviousPageUrlCookieFieldName)]).Returns("http://localhost/my-website/foo");
141136
_context.SetupGet(x => x.Request.Cookies).Returns(cookieCollection.Object);
142137

143-
_context.SetupGet(x => x.SiteUrl).Returns("foo");
144-
145138
// Act
146139
_redirector.Redirect(RedirectionType.PreviousPageWithBookmark, "bar");
147140

148141
// Assert
149-
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "foo#bar")), Times.Once);
142+
_context.Verify(x => x.Response.Redirect(It.Is<string>(c => c == "http://localhost/my-website/foo#bar")), Times.Once);
150143
}
151144

152145
[Test]
@@ -239,4 +232,47 @@ public void SetPreviousPageUrl_Set()
239232
// Act & Assert
240233
_redirector.PreviousPageUrl = "foo";
241234
}
235+
236+
[Test]
237+
public void Redirect_ProtocolRelativeUrl_ThrowsSecurityException() =>
238+
Assert.Throws<SecurityException>(() => _redirector.Redirect("//evil.com/path"));
239+
240+
[Test]
241+
public void Redirect_BackslashSchemeSpoof_ThrowsSecurityException() =>
242+
Assert.Throws<SecurityException>(() => _redirector.Redirect("/\\evil.com"));
243+
244+
[Test]
245+
public void Redirect_HostPrefixCollision_ThrowsSecurityException()
246+
{
247+
// The old StartsWith-based check would have accepted "http://localhost.attacker.com/"
248+
// because it shared the "http://localhost" prefix when SiteUrl lacked a trailing slash.
249+
_context.SetupGet(x => x.SiteUrl).Returns("http://localhost");
250+
251+
Assert.Throws<SecurityException>(() => _redirector.Redirect("http://localhost.attacker.com/path"));
252+
}
253+
254+
[Test]
255+
public void Redirect_RelativePath_Allowed()
256+
{
257+
_redirector.Redirect("/some/internal/path");
258+
259+
_context.Verify(x => x.Response.Redirect("/some/internal/path"), Times.Once);
260+
}
261+
262+
[Test]
263+
public void SetPreviousPageUrl_AppendedCookieIsHttpOnlyAndLax()
264+
{
265+
CookieOptions? captured = null;
266+
267+
_context.SetupGet(x => x.Response.Cookies).Returns(_responseCookies.Object);
268+
_responseCookies.Setup(x => x.Append(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CookieOptions>()))
269+
.Callback<string, string, CookieOptions>((_, _, opts) => captured = opts);
270+
271+
_redirector.PreviousPageUrl = "anything";
272+
273+
Assert.That(captured, Is.Not.Null);
274+
Assert.That(captured!.HttpOnly, Is.True);
275+
Assert.That(captured.SameSite, Is.EqualTo(SameSiteMode.Lax));
276+
Assert.That(captured.Secure, Is.True);
277+
}
242278
}

src/Simplify.Web.Tests/StaticFiles/IO/StaticFileTests.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,31 @@ public void GetLastModificationTime_ValidFile_MillisecondsTrimmed()
7373
Assert.That(result.Millisecond, Is.Zero);
7474
}
7575

76+
[Test]
77+
public void IsValidPath_ParentDirectoryTraversal_False()
78+
{
79+
// Act
80+
var result = _staticFile.IsValidPath("staticfiles/../StaticFiles/IO/TestFiles/TestStaticFile.html");
81+
82+
// Assert
83+
Assert.That(result, Is.False);
84+
}
85+
86+
[Test]
87+
public void IsValidPath_EncodedTraversalLooking_False()
88+
{
89+
// Act — ASP.NET Core decodes %2e%2e before this layer, so the raw '..' check fires.
90+
var result = _staticFile.IsValidPath("staticfiles/../../../etc/passwd");
91+
92+
// Assert
93+
Assert.That(result, Is.False);
94+
}
95+
96+
[Test]
97+
public void GetDataAsync_TraversalPath_Throws() =>
98+
Assert.ThrowsAsync<UnauthorizedAccessException>(async () =>
99+
await _staticFile.GetDataAsync("staticfiles/../StaticFiles/IO/TestFiles/TestStaticFile.html"));
100+
76101
[Test]
77102
public async Task GetDataAsync_ValidFile_DataReturned()
78103
{

src/Simplify.Web/Auth/Extensions/AuthRedirectExtensions.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ public static void UseAuthRedirect(this IApplicationBuilder app, string redirect
1717
{
1818
await next();
1919

20-
if (context.Response.StatusCode == 401)
20+
// Guard against InvalidOperationException ("StatusCode cannot be set, response
21+
// has already started") that occurs when an upstream authentication handler
22+
// already flushed a 401 challenge response body.
23+
if (context.Response.StatusCode == 401 && !context.Response.HasStarted)
2124
context.Response.Redirect(redirectUrl);
2225
});
2326
}

src/Simplify.Web/Controllers/Meta/ControllerMetadata.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ public abstract class ControllerMetadata(Type controllerType) : IControllerMetad
100100
var isAuthorizationRequired = false;
101101
IEnumerable<string>? requiredUserRoles = null;
102102

103-
var attributes = controllerType.GetCustomAttributes(typeof(AuthorizeAttribute), false);
103+
var attributes = controllerType.GetCustomAttributes(typeof(AuthorizeAttribute), true);
104104

105105
if (attributes.Length > 0)
106106
{

src/Simplify.Web/Modules/Context/WebContext.cs

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
using System;
1+
using System;
22
using System.IO;
3+
using System.Text;
34
using System.Threading;
45
using System.Threading.Tasks;
56
using Microsoft.AspNetCore.Http;
@@ -10,13 +11,14 @@ namespace Simplify.Web.Modules.Context;
1011
/// Provides the web context.
1112
/// </summary>
1213
/// <seealso cref="IWebContext" />
13-
public sealed class WebContext : IWebContext
14+
public sealed class WebContext : IWebContext, IDisposable
1415
{
1516
private readonly SemaphoreSlim _formReadSemaphore = new(1, 1);
1617
private readonly SemaphoreSlim _requestBodyReadSemaphore = new(1, 1);
1718

1819
private IFormCollection? _form;
1920
private string? _requestBody;
21+
private bool _disposed;
2022

2123
/// <summary>
2224
/// Initializes a new instance of the <see cref="WebContext" /> class.
@@ -164,6 +166,9 @@ public async Task ReadFormAsync()
164166

165167
try
166168
{
169+
if (_form != null)
170+
return;
171+
167172
_form = await Context.Request.ReadFormAsync();
168173
}
169174
finally
@@ -184,7 +189,16 @@ public async Task ReadRequestBodyAsync()
184189

185190
try
186191
{
187-
using var reader = new StreamReader(Context.Request.Body);
192+
if (_requestBody != null)
193+
return;
194+
195+
// Keep the underlying request stream open so that other middleware /
196+
// model binders downstream can still consume the body if it is buffered.
197+
#if NETSTANDARD2_0 || NETSTANDARD2_1
198+
using var reader = new StreamReader(Context.Request.Body, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, bufferSize: 1024, leaveOpen: true);
199+
#else
200+
using var reader = new StreamReader(Context.Request.Body, leaveOpen: true);
201+
#endif
188202

189203
_requestBody = await reader.ReadToEndAsync();
190204
}
@@ -193,4 +207,19 @@ public async Task ReadRequestBodyAsync()
193207
_requestBodyReadSemaphore.Release();
194208
}
195209
}
210+
211+
/// <summary>
212+
/// Releases unmanaged resources held by the web context (the semaphores used to
213+
/// serialize body / form reads).
214+
/// </summary>
215+
public void Dispose()
216+
{
217+
if (_disposed)
218+
return;
219+
220+
_disposed = true;
221+
222+
_formReadSemaphore.Dispose();
223+
_requestBodyReadSemaphore.Dispose();
224+
}
196225
}

src/Simplify.Web/Modules/Data/FileReader.cs

Lines changed: 18 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
using System;
1+
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.IO;
4-
using System.Linq;
55
using System.Xml.Linq;
66
using Simplify.Web.Modules.Localization;
77

@@ -21,13 +21,9 @@ namespace Simplify.Web.Modules.Data;
2121
public sealed class FileReader(string dataPhysicalPath, string defaultLanguage, ILanguageManagerProvider languageManagerProvider,
2222
bool disableCache = false) : IFileReader
2323
{
24-
private static readonly IDictionary<KeyValuePair<string, string>, XDocument?> XmlCache =
25-
new Dictionary<KeyValuePair<string, string>, XDocument?>();
24+
private static readonly ConcurrentDictionary<KeyValuePair<string, string>, XDocument?> XmlCache = new();
2625

27-
private static readonly IDictionary<KeyValuePair<string, string>, string?> TextCache =
28-
new Dictionary<KeyValuePair<string, string>, string?>();
29-
30-
private static readonly object Locker = new();
26+
private static readonly ConcurrentDictionary<KeyValuePair<string, string>, string?> TextCache = new();
3127

3228
private ILanguageManager _languageManager = null!;
3329

@@ -36,11 +32,8 @@ public sealed class FileReader(string dataPhysicalPath, string defaultLanguage,
3632
/// </summary>
3733
public static void ClearCache()
3834
{
39-
lock (Locker)
40-
{
41-
XmlCache.Clear();
42-
TextCache.Clear();
43-
}
35+
XmlCache.Clear();
36+
TextCache.Clear();
4437
}
4538

4639
/// <summary>
@@ -173,33 +166,11 @@ public string GetFilePath(string? fileName, string? language)
173166

174167
#endregion XML
175168

176-
private static bool TryToLoadTextFileFromCache(string fileName, string language, out string? data)
177-
{
178-
data = null;
179-
180-
var cacheItem = TextCache.FirstOrDefault(x => x.Key.Key == fileName && x.Key.Value == language);
181-
182-
if (cacheItem.Equals(default(KeyValuePair<KeyValuePair<string, string>, string>)))
183-
return false;
184-
185-
data = cacheItem.Value;
169+
private static bool TryToLoadTextFileFromCache(string fileName, string language, out string? data) =>
170+
TextCache.TryGetValue(new KeyValuePair<string, string>(fileName, language), out data);
186171

187-
return true;
188-
}
189-
190-
private static bool TryToLoadXDocumentFromCache(string fileName, string language, out XDocument? data)
191-
{
192-
data = null;
193-
194-
var cacheItem = XmlCache.FirstOrDefault(x => x.Key.Key == fileName && x.Key.Value == language);
195-
196-
if (cacheItem.Equals(default(KeyValuePair<KeyValuePair<string, string>, XDocument>)))
197-
return false;
198-
199-
data = cacheItem.Value;
200-
201-
return true;
202-
}
172+
private static bool TryToLoadXDocumentFromCache(string fileName, string language, out XDocument? data) =>
173+
XmlCache.TryGetValue(new KeyValuePair<string, string>(fileName, language), out data);
203174

204175
private bool LoadTextFileFromFileSystem(string fileName, string language, out string? data)
205176
{
@@ -220,18 +191,12 @@ private bool LoadTextFileCached(string fileName, string language, out string? da
220191
if (TryToLoadTextFileFromCache(fileName, language, out data))
221192
return true;
222193

223-
lock (Locker)
224-
{
225-
if (TryToLoadTextFileFromCache(fileName, language, out data))
226-
return true;
227-
228-
if (!LoadTextFileFromFileSystem(fileName, language, out data))
229-
return false;
194+
if (!LoadTextFileFromFileSystem(fileName, language, out data))
195+
return false;
230196

231-
TextCache.Add(new KeyValuePair<string, string>(fileName, language), data);
197+
TextCache.TryAdd(new KeyValuePair<string, string>(fileName, language), data);
232198

233-
return true;
234-
}
199+
return true;
235200
}
236201

237202
private bool LoadXDocumentFromFileSystem(string fileName, string language, out XDocument? data)
@@ -251,17 +216,11 @@ private bool LoadXDocumentCached(string fileName, string language, out XDocument
251216
if (TryToLoadXDocumentFromCache(fileName, language, out data))
252217
return true;
253218

254-
lock (Locker)
255-
{
256-
if (TryToLoadXDocumentFromCache(fileName, language, out data))
257-
return true;
258-
259-
if (!LoadXDocumentFromFileSystem(fileName, language, out data))
260-
return false;
219+
if (!LoadXDocumentFromFileSystem(fileName, language, out data))
220+
return false;
261221

262-
XmlCache.Add(new KeyValuePair<string, string>(fileName, language), data);
222+
XmlCache.TryAdd(new KeyValuePair<string, string>(fileName, language), data);
263223

264-
return true;
265-
}
224+
return true;
266225
}
267226
}

0 commit comments

Comments
 (0)