Skip to content

Commit e9a57f7

Browse files
committed
fix: address CodeQL and code-quality scan findings
1 parent 93ae23c commit e9a57f7

6 files changed

Lines changed: 82 additions & 36 deletions

File tree

src/SEBT.Portal.Api/Controllers/Auth/OidcController.cs

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,13 @@ public async Task<IActionResult> GetConfig(
5050
[FromQuery] bool stepUp = false,
5151
CancellationToken cancellationToken = default)
5252
{
53-
if (!stateAllowlist.Contains(code))
53+
// Resolve the route parameter to the canonical allowlist value. TryResolve returns
54+
// a value from the allowlist itself (not derived from user input), breaking the
55+
// taint chain for CodeQL's "user input in log" analysis.
56+
var stateCode = stateAllowlist.TryResolve(code);
57+
if (stateCode == null)
5458
{
55-
logger.LogWarning(
56-
"OIDC GetConfig rejected: stateCode {StateCode} not in allowlist (reason=unknown_state)",
57-
SanitizeForLog(code));
59+
logger.LogWarning("OIDC GetConfig rejected: unknown stateCode (reason=unknown_state)");
5860
return BadRequest(new ErrorResponse("Unknown or unsupported stateCode."));
5961
}
6062

@@ -69,7 +71,7 @@ public async Task<IActionResult> GetConfig(
6971
{
7072
logger.LogWarning(
7173
"OIDC config missing for stateCode {StateCode} (reason=oidc_not_configured)",
72-
SanitizeForLog(code));
74+
stateCode);
7375
var hint = environment.IsDevelopment()
7476
? "Set Oidc:AuthorizationEndpoint, Oidc:ClientId, and Oidc:CallbackRedirectUri in appsettings."
7577
: "";
@@ -83,7 +85,7 @@ public async Task<IActionResult> GetConfig(
8385

8486
// Create the pre-auth session and set the cookie.
8587
var session = await sessionStore.CreateAsync(
86-
code.ToLowerInvariant(), state, codeVerifier, redirectUri, stepUp, cancellationToken);
88+
stateCode, state, codeVerifier, redirectUri, stepUp, cancellationToken);
8789
OidcSessionCookie.Set(Response, session.Id);
8890

8991
var languageParam = config["Oidc:LanguageParam"] ?? "en";
@@ -118,11 +120,12 @@ public async Task<IActionResult> Callback(
118120
if (body == null || string.IsNullOrEmpty(body.Code) || string.IsNullOrEmpty(body.StateCode))
119121
return BadRequest(new ErrorResponse("Missing code or stateCode."));
120122

121-
if (!stateAllowlist.Contains(body.StateCode))
123+
// Resolve stateCode to the canonical allowlist value (breaks taint chain).
124+
var requestedCode = body.Code;
125+
var requestedStateCode = stateAllowlist.TryResolve(body.StateCode);
126+
if (requestedStateCode == null)
122127
{
123-
logger.LogWarning(
124-
"OIDC Callback rejected: stateCode {StateCode} not in allowlist (reason=unknown_state)",
125-
SanitizeForLog(body.StateCode));
128+
logger.LogWarning("OIDC Callback rejected: unknown stateCode (reason=unknown_state)");
126129
return BadRequest(new ErrorResponse("Unknown or unsupported stateCode."));
127130
}
128131

@@ -150,7 +153,7 @@ public async Task<IActionResult> Callback(
150153
}
151154

152155
// --- Validate stateCode matches stored value (prevents tenant switching) ---
153-
if (!string.Equals(body.StateCode, session.StateCode, StringComparison.OrdinalIgnoreCase))
156+
if (!string.Equals(requestedStateCode, session.StateCode, StringComparison.OrdinalIgnoreCase))
154157
{
155158
logger.LogWarning(
156159
"OIDC Callback rejected: stateCode mismatch (reason=mismatched_stateCode, SessionId={SessionId})", sessionId);
@@ -168,7 +171,7 @@ public async Task<IActionResult> Callback(
168171

169172
// --- Exchange code using the stored code_verifier (never from the body) ---
170173
var result = await exchangeService.ExchangeCodeAsync(
171-
body.Code,
174+
requestedCode,
172175
session.CodeVerifier,
173176
session.RedirectUri,
174177
session.IsStepUp,
@@ -213,11 +216,12 @@ public async Task<IActionResult> CompleteLogin(
213216
if (body == null || string.IsNullOrEmpty(body.StateCode) || string.IsNullOrEmpty(body.CallbackToken))
214217
return BadRequest(new ErrorResponse("Missing stateCode or callbackToken."));
215218

216-
if (!stateAllowlist.Contains(body.StateCode))
219+
// Resolve stateCode via allowlist (breaks taint chain); bind callbackToken locally.
220+
var requestedStateCode = stateAllowlist.TryResolve(body.StateCode);
221+
var callbackToken = body.CallbackToken;
222+
if (requestedStateCode == null)
217223
{
218-
logger.LogWarning(
219-
"OIDC CompleteLogin rejected: stateCode {StateCode} not in allowlist (reason=unknown_state)",
220-
SanitizeForLog(body.StateCode));
224+
logger.LogWarning("OIDC CompleteLogin rejected: unknown stateCode (reason=unknown_state)");
221225
return BadRequest(new ErrorResponse("Unknown or unsupported stateCode."));
222226
}
223227

@@ -230,7 +234,7 @@ public async Task<IActionResult> CompleteLogin(
230234
}
231235

232236
// --- Verify the callback token matches this session and hasn't been consumed ---
233-
var tokenHash = IPreAuthSessionStore.HashCallbackToken(body.CallbackToken);
237+
var tokenHash = IPreAuthSessionStore.HashCallbackToken(callbackToken);
234238
var advanced = await sessionStore.TryAdvanceToLoginCompletedAsync(sessionId, tokenHash, cancellationToken);
235239
if (!advanced)
236240
{
@@ -244,7 +248,7 @@ public async Task<IActionResult> CompleteLogin(
244248
OidcSessionCookie.Clear(Response);
245249
await sessionStore.RemoveAsync(sessionId, cancellationToken);
246250

247-
var stateKey = body.StateCode.ToLowerInvariant();
251+
var stateKey = requestedStateCode.ToLowerInvariant();
248252
var signingKey = config["Oidc:CompleteLoginSigningKey"];
249253
if (string.IsNullOrEmpty(signingKey))
250254
{
@@ -274,12 +278,12 @@ public async Task<IActionResult> CompleteLogin(
274278
ClaimsPrincipal principal;
275279
try
276280
{
277-
principal = handler.ValidateToken(body.CallbackToken, validationParams, out _);
281+
principal = handler.ValidateToken(callbackToken, validationParams, out _);
278282
}
279283
catch (Exception ex)
280284
{
281285
logger.LogWarning(ex, "Invalid or expired callback token for state {StateCode}",
282-
SanitizeForLog(body.StateCode));
286+
SanitizeForLog(requestedStateCode));
283287
return BadRequest(new ErrorResponse("Invalid or expired callback token."));
284288
}
285289

src/SEBT.Portal.Api/Services/OidcExchangeService.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ public async Task<OidcExchangeResult> ExchangeCodeAsync(
163163

164164
// --- Exchange authorization code for tokens ---
165165
using var client = _httpFactory.CreateClient();
166-
var tokenParams = new FormUrlEncodedContent(new Dictionary<string, string>
166+
using var tokenParams = new FormUrlEncodedContent(new Dictionary<string, string>
167167
{
168168
["grant_type"] = "authorization_code",
169169
["code"] = code,
@@ -332,7 +332,15 @@ void TrySet(string jsonKey, string claimName)
332332

333333
TrySet("sub", "sub");
334334
TrySet("email", "email");
335-
TrySet("preferred_username", "email"); // fallback for IdPs that put email here
335+
// Some IdPs put email in preferred_username — only use it as email if it looks like one.
336+
if (!claims.ContainsKey("email")
337+
&& doc.RootElement.TryGetProperty("preferred_username", out var pu)
338+
&& pu.ValueKind == JsonValueKind.String
339+
&& !string.IsNullOrEmpty(pu.GetString())
340+
&& pu.GetString()!.Contains('@'))
341+
{
342+
claims.TryAdd("email", pu.GetString()!);
343+
}
336344
TrySet("phone", "phone");
337345
TrySet("phone_number", "phone_number");
338346
TrySet("given_name", "givenName");

src/SEBT.Portal.Api/Services/PreAuthSessionStore.cs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ public async Task<PreAuthSession> CreateAsync(
123123
KnownSessionIds.TryAdd(sessionId, 0);
124124
_logger.LogInformation(
125125
"Pre-auth session created: SessionId={SessionId}, StateCode={StateCode} (reason=session_created)",
126-
sessionId, stateCode);
126+
sessionId, SanitizeForLog(stateCode));
127127
return session;
128128
}
129129

@@ -141,6 +141,14 @@ public async Task<PreAuthSession> CreateAsync(
141141
_ => ValueTask.FromResult<PreAuthSession?>(null),
142142
CacheOptions,
143143
cancellationToken: cancellationToken);
144+
145+
// Session expired in cache — clean up tracking dictionaries so they don't grow unbounded.
146+
if (result == null)
147+
{
148+
KnownSessionIds.TryRemove(sessionId, out _);
149+
SessionLocks.TryRemove(sessionId, out _);
150+
}
151+
144152
return result;
145153
}
146154

@@ -231,6 +239,16 @@ public async Task RemoveAsync(string sessionId, CancellationToken cancellationTo
231239

232240
private static string CacheKey(string sessionId) => $"{CacheKeyPrefix}{sessionId}";
233241

242+
private static string SanitizeForLog(string? value)
243+
{
244+
if (string.IsNullOrEmpty(value)) return string.Empty;
245+
return value
246+
.Replace("\r", string.Empty, StringComparison.Ordinal)
247+
.Replace("\n", string.Empty, StringComparison.Ordinal)
248+
.Replace("\u2028", string.Empty, StringComparison.Ordinal)
249+
.Replace("\u2029", string.Empty, StringComparison.Ordinal);
250+
}
251+
234252
private static string GenerateSessionId()
235253
{
236254
Span<byte> bytes = stackalloc byte[32];

src/SEBT.Portal.Api/Services/StateAllowlist.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,13 @@ public interface IStateAllowlist
1212
/// <summary>True if <paramref name="stateCode"/> (case-insensitive) is a configured OIDC tenant.</summary>
1313
bool Contains(string? stateCode);
1414

15+
/// <summary>
16+
/// Resolves a user-provided stateCode to the canonical (lowercased) value from the
17+
/// allowlist. Returns null if not found. The returned value is from the allowlist itself,
18+
/// not derived from user input — safe for logging and downstream use without taint.
19+
/// </summary>
20+
string? TryResolve(string? stateCode);
21+
1522
/// <summary>Lowercased, case-insensitive set of allowed state codes.</summary>
1623
IReadOnlySet<string> All { get; }
1724
}
@@ -33,6 +40,15 @@ public StateAllowlist(IEnumerable<string> states)
3340
public bool Contains(string? stateCode) =>
3441
!string.IsNullOrWhiteSpace(stateCode) && _states.Contains(stateCode.ToLowerInvariant());
3542

43+
/// <inheritdoc/>
44+
public string? TryResolve(string? stateCode)
45+
{
46+
if (string.IsNullOrWhiteSpace(stateCode)) return null;
47+
var normalized = stateCode.ToLowerInvariant();
48+
// Return the canonical value from the set, breaking any taint chain from user input.
49+
return _states.TryGetValue(normalized, out var canonical) ? canonical : null;
50+
}
51+
3652
/// <inheritdoc/>
3753
public IReadOnlySet<string> All => _states;
3854
}

test/SEBT.Portal.Tests/Integration/AuthCookieAuthenticationTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public async Task GetStatus_WithExpiredSessionCookie_ReturnsUnauthorized()
7070
private async Task<HttpResponseMessage> GetStatusWithCookie(string? token)
7171
{
7272
var client = _factory.CreateClient();
73-
var request = new HttpRequestMessage(HttpMethod.Get, "/api/auth/status");
73+
using var request = new HttpRequestMessage(HttpMethod.Get, "/api/auth/status");
7474
if (token != null)
7575
request.Headers.Add("Cookie", $"{AuthCookies.AuthCookieName}={token}");
7676
return await client.SendAsync(request);

test/SEBT.Portal.Tests/Integration/OidcPreAuthSecurityTests.cs

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public async Task V01_T03_CompleteLogin_WithoutSessionCookie_Returns403()
4343
{
4444
var client = _factory.CreateClient();
4545
var callbackToken = MintCallbackToken("user@example.com");
46-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
46+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
4747
{
4848
Content = JsonContent.Create(new { stateCode = "co", callbackToken }),
4949
Headers = { { "Origin", "http://localhost:3000" } }
@@ -60,7 +60,7 @@ public async Task V01_T03_CompleteLogin_WithSpoofedOrigin_Returns403()
6060
{
6161
var client = _factory.CreateClient();
6262
var callbackToken = MintCallbackToken("user@example.com");
63-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
63+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
6464
{
6565
Content = JsonContent.Create(new { stateCode = "co", callbackToken }),
6666
Headers = { { "Origin", "https://attacker.example.com" } }
@@ -77,7 +77,7 @@ public async Task V01_T03_CompleteLogin_WithMissingOrigin_Returns403()
7777
{
7878
var client = _factory.CreateClient();
7979
var callbackToken = MintCallbackToken("user@example.com");
80-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
80+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
8181
{
8282
Content = JsonContent.Create(new { stateCode = "co", callbackToken })
8383
// No Origin header at all — curl from command line
@@ -119,7 +119,7 @@ public async Task V02_T02_CompleteLogin_SecondCallWithConsumedSession_Returns403
119119
await sessionStore.TryAdvanceToLoginCompletedAsync(session.Id, tokenHash);
120120

121121
// Replay attempt: exact same token + cookie → must fail
122-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
122+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
123123
{
124124
Content = JsonContent.Create(new { stateCode = "co", callbackToken }),
125125
Headers =
@@ -146,7 +146,7 @@ public async Task V03_T04_Callback_WithTamperedState_Returns400()
146146
var session = await sessionStore.CreateAsync("co", "real-state-value", "verifier1", "http://localhost:3000/callback", false);
147147

148148
var client = _factory.CreateClient();
149-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
149+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
150150
{
151151
Content = JsonContent.Create(new
152152
{
@@ -187,7 +187,7 @@ public async Task V05_T07a_CompleteLogin_WithInvalidStateCode_Returns400(string
187187
{
188188
var client = _factory.CreateClient();
189189
var callbackToken = MintCallbackToken("user@example.com");
190-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
190+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/complete-login")
191191
{
192192
Content = JsonContent.Create(new { stateCode, callbackToken }),
193193
Headers = { { "Origin", "http://localhost:3000" } }
@@ -204,7 +204,7 @@ public async Task V05_T07a_CompleteLogin_WithInvalidStateCode_Returns400(string
204204
public async Task V05_T07a_Callback_WithInvalidStateCode_Returns400(string stateCode)
205205
{
206206
var client = _factory.CreateClient();
207-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
207+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
208208
{
209209
Content = JsonContent.Create(new { code = "code", state = "state", stateCode }),
210210
Headers = { { "Origin", "http://localhost:3000" } }
@@ -225,7 +225,7 @@ public async Task V05_T07a_Callback_WithInvalidStateCode_Returns400(string state
225225
public async Task V06_T08aA_Callback_WithoutSessionCookie_Returns403()
226226
{
227227
var client = _factory.CreateClient();
228-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
228+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
229229
{
230230
Content = JsonContent.Create(new
231231
{
@@ -246,7 +246,7 @@ public async Task V06_T08aA_Callback_WithoutSessionCookie_Returns403()
246246
public async Task V06_T08aA_Callback_WithSpoofedOrigin_Returns403()
247247
{
248248
var client = _factory.CreateClient();
249-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
249+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
250250
{
251251
Content = JsonContent.Create(new
252252
{
@@ -274,7 +274,7 @@ public async Task V06_T08aB_Callback_WithValidSessionButWrongState_Returns400()
274274
var session = await sessionStore.CreateAsync("co", "correct-state", "verifier", "http://localhost:3000/callback", false);
275275

276276
var client = _factory.CreateClient();
277-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
277+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
278278
{
279279
Content = JsonContent.Create(new
280280
{
@@ -308,7 +308,7 @@ public async Task V06_T08aE_Callback_WithValidSessionButWrongStateCode_Returns40
308308
var client = _factory.CreateClient();
309309
// Session was created for "co" but we send "az" (stateCode mismatch)
310310
// Note: "az" isn't in the allowlist either, so this gets caught at both layers
311-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
311+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
312312
{
313313
Content = JsonContent.Create(new
314314
{
@@ -344,7 +344,7 @@ public async Task SessionReplay_CallbackOnAlreadyUsedSession_Returns400()
344344

345345
var client = _factory.CreateClient();
346346
// Try to use the same session for another callback — should fail
347-
var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
347+
using var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/oidc/callback")
348348
{
349349
Content = JsonContent.Create(new
350350
{

0 commit comments

Comments
 (0)