Skip to content

Commit b2181e2

Browse files
vbilopavclaude
vbilopav
and
claude
committed
fix: fail-fast non-text claim-bind param + WARN on req-value collision
A claim-mapped parameter declared with a known non-text PostgreSQL type (e.g. `_company_id int` mapped to a `company_id` claim) now throws ArgumentException at UseNpgsqlRest time instead of crashing every authenticated request with a misleading `InvalidCastException` from Npgsql ("Writing values of 'System.String' is not supported for parameters having NpgsqlDbType 'Integer'"). NpgsqlDbType.Unknown is allowed — the SqlFileSource path where param types are not inferred and Npgsql resolves them server-side. The runtime path also emits a LogLevel.Warning when a request body or query string supplies a value for a parameter that is auto-bound from a claim. The claim still wins (preserves security semantics for routines that key off the caller's identity), but the silent override is now visible in logs. 5 new tests, 1937/1937 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a2e38f1 commit b2181e2

7 files changed

Lines changed: 434 additions & 2 deletions

File tree

‎NpgsqlRest/Log.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,4 +331,7 @@ public static partial class Log
331331

332332
[LoggerMessage(Level = LogLevel.Trace, Message = "Column decryption failed; falling back to raw value. Error: {error}")]
333333
public static partial void DecryptColumnFailed(this ILogger logger, string error);
334+
335+
[LoggerMessage(Level = LogLevel.Warning, Message = "Endpoint {path} parameter {paramName} received a {source} value but is auto-bound from claim '{claimName}'. The supplied value is being ignored.")]
336+
public static partial void ClaimMappedParamReceivedRequestValue(this ILogger logger, string path, string paramName, string source, string claimName);
334337
}

‎NpgsqlRest/NpgsqlRestBuilder.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,29 @@ private static Metadata Build(IApplicationBuilder? builder, RetryStrategy? defau
325325
}
326326
}
327327

328+
// Fail-fast if a claim-mapped parameter is declared with a known non-text type.
329+
// Claim values are strings, so binding to e.g. an Integer parameter would crash
330+
// every authenticated request with a misleading InvalidCastException deep inside
331+
// Npgsql ("Writing values of 'System.String' is not supported for parameters
332+
// having NpgsqlDbType '<X>'"). NpgsqlDbType.Unknown is allowed — Npgsql resolves
333+
// it server-side, which is the SqlFileSource path where param types aren't
334+
// inferred. There is no scenario where the known-non-text configuration is valid,
335+
// so surface it as a hard error at startup rather than letting it ship to prod.
336+
if (endpoint.UseUserParameters is true && Options.AuthenticationOptions.ParameterNameClaimsMapping.Count > 0)
337+
{
338+
for (int p = 0; p < routine.Parameters.Length; p++)
339+
{
340+
var param = routine.Parameters[p];
341+
if (param.UserClaim is not null
342+
&& param.TypeDescriptor.IsText is false
343+
&& param.TypeDescriptor.BaseDbType != NpgsqlTypes.NpgsqlDbType.Unknown)
344+
{
345+
throw new ArgumentException(
346+
$"Endpoint {method} {endpoint.Path} parameter {param.ActualName} is mapped to claim '{param.UserClaim}' but its type is '{param.TypeDescriptor.OriginalType}' which is not text-compatible. Claim values are strings, so binding would fail at runtime with InvalidCastException. Declare the parameter as text/varchar/char/json/jsonb/xml/jsonpath, or remove '{param.ActualName}' from ParameterNameClaimsMapping.");
347+
}
348+
}
349+
}
350+
328351
// Warn if @table_format is set on non-applicable endpoints
329352
if (Options.TableFormatHandlers is not null
330353
&& endpoint.CustomParameters is not null

‎NpgsqlRest/NpgsqlRestEndpoint.cs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,15 @@ currentEndpoint.SseEventNoticeLevel is not null &&
362362
}
363363
else if (context.User?.Identity?.IsAuthenticated is true && Options.AuthenticationOptions.ParameterNameClaimsMapping.TryGetValue(parameter.ActualName, out var claimType))
364364
{
365+
// Claim auto-bind always wins (security routines depend on it). If the
366+
// request also supplied a value, it would otherwise be silently dropped —
367+
// surface that as a WARN so a parameter naming collision is visible.
368+
if (queryCollection is not null
369+
&& (queryCollection.ContainsKey(parameter.ConvertedName) || queryCollection.ContainsKey(parameter.ActualName)))
370+
{
371+
Logger?.ClaimMappedParamReceivedRequestValue(
372+
endpoint.Path, parameter.ActualName, "query", claimType);
373+
}
365374
parameter.Value = claimsDict!.GetClaimDbParam(claimType);
366375
}
367376
else if (context.User?.Identity?.IsAuthenticated is true && parameter.IsUserClaims is true)
@@ -1028,6 +1037,15 @@ await Options.ValidateParametersAsync(new ParameterValidationValues(
10281037
}
10291038
else if (context.User?.Identity?.IsAuthenticated is true && Options.AuthenticationOptions.ParameterNameClaimsMapping.TryGetValue(parameter.ActualName, out var claimType))
10301039
{
1040+
// Claim auto-bind always wins (security routines depend on it). If the
1041+
// request also supplied a value, it would otherwise be silently dropped —
1042+
// surface that as a WARN so a parameter naming collision is visible.
1043+
if (bodyDict is not null
1044+
&& (bodyDict.ContainsKey(parameter.ConvertedName) || bodyDict.ContainsKey(parameter.ActualName)))
1045+
{
1046+
Logger?.ClaimMappedParamReceivedRequestValue(
1047+
endpoint.Path, parameter.ActualName, "body", claimType);
1048+
}
10311049
parameter.Value = claimsDict!.GetClaimDbParam(claimType);
10321050
}
10331051
else if (context.User?.Identity?.IsAuthenticated is true && parameter.IsUserClaims is true)

‎NpgsqlRestTests/AuthTests/ClaimAutoBindWarningsTests.cs‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
using Microsoft.AspNetCore.Authentication.Cookies;
2+
using Microsoft.AspNetCore.Builder;
3+
using Microsoft.AspNetCore.Hosting;
4+
using Microsoft.Extensions.DependencyInjection;
5+
using Microsoft.Extensions.Logging;
6+
using NpgsqlRest.Auth;
7+
8+
namespace NpgsqlRestTests;
9+
10+
public static partial class Database
11+
{
12+
public static void ClaimAutoBindWarningsTests()
13+
{
14+
// The cab_* routines are loaded by ClaimAutoBindTestFixture (good config). The cabx_*
15+
// routine is *not* loaded by that fixture — it is only referenced by the dedicated
16+
// fail-fast test, which spins up its own WebApplication to assert UseNpgsqlRest throws
17+
// when a non-text parameter is in ParameterNameClaimsMapping.
18+
script.Append("""
19+
20+
create function cab_get_user_id(
21+
_user_id text
22+
) returns text
23+
language sql immutable as $$
24+
select _user_id;
25+
$$;
26+
27+
create function cab_get_user_id_post(
28+
_user_id text,
29+
_other text = null
30+
) returns text
31+
language sql immutable as $$
32+
select _user_id || ':' || coalesce(_other, '');
33+
$$;
34+
comment on function cab_get_user_id_post(text, text) is 'HTTP POST';
35+
36+
create function cabx_create_widget(
37+
_company_id int,
38+
_widget_name text
39+
) returns int
40+
language sql immutable as $$
41+
select _company_id;
42+
$$;
43+
""");
44+
}
45+
}
46+
47+
[Collection("ClaimAutoBindTestFixture")]
48+
public class ClaimAutoBindWarningsTests(ClaimAutoBindTestFixture test)
49+
{
50+
private static IEnumerable<LogEntry> WarningsContaining(IEnumerable<LogEntry> logs, params string[] needles) =>
51+
logs.Where(e => e.Level == LogLevel.Warning && needles.All(n => e.Message.Contains(n)));
52+
53+
[Fact]
54+
public async Task QueryStringSuppliesClaimMappedParam_LogsRuntimeWarningAndClaimWins()
55+
{
56+
// Sign in (cookie carries name_identifier=42, company_id=7).
57+
using var client = test.CreateClient();
58+
(await client.GetAsync("/cab-login")).StatusCode.Should().Be(HttpStatusCode.OK);
59+
60+
var before = test.CurrentLogCount;
61+
62+
// Request supplies userId=999 in the query string. The endpoint is auto-bound from the
63+
// name_identifier claim (=42), so the claim must win and the body value is silently dropped.
64+
// The fix: a WARN is emitted naming the endpoint, parameter, source ("query"), and claim
65+
// so the developer can see the collision in logs.
66+
using var response = await client.GetAsync("/api/cab-get-user-id/?userId=999");
67+
response.StatusCode.Should().Be(HttpStatusCode.OK);
68+
(await response.Content.ReadAsStringAsync()).Should().Be("42",
69+
"claim must always win over a request-supplied value for an auto-bound parameter");
70+
71+
var matches = WarningsContaining(test.RequestLogsSince(before),
72+
"/api/cab-get-user-id",
73+
"_user_id",
74+
"query",
75+
"name_identifier").ToList();
76+
77+
matches.Should().ContainSingle();
78+
}
79+
80+
[Fact]
81+
public async Task BodySuppliesClaimMappedParam_LogsRuntimeWarningAndClaimWins()
82+
{
83+
using var client = test.CreateClient();
84+
(await client.GetAsync("/cab-login")).StatusCode.Should().Be(HttpStatusCode.OK);
85+
86+
var before = test.CurrentLogCount;
87+
88+
// POST endpoint: body sends {"userId": "999", "other": "x"}. The claim auto-bind for _user_id
89+
// overrides 999, but we log a WARN so the developer notices the silent drop.
90+
using var response = await client.PostAsync(
91+
"/api/cab-get-user-id-post/",
92+
new StringContent("{\"userId\":\"999\",\"other\":\"x\"}", System.Text.Encoding.UTF8, "application/json"));
93+
response.StatusCode.Should().Be(HttpStatusCode.OK);
94+
(await response.Content.ReadAsStringAsync()).Should().Be("42:x",
95+
"claim must win for _user_id, but the unmapped _other parameter must still come from the body");
96+
97+
var matches = WarningsContaining(test.RequestLogsSince(before),
98+
"/api/cab-get-user-id-post",
99+
"_user_id",
100+
"body",
101+
"name_identifier").ToList();
102+
103+
matches.Should().ContainSingle();
104+
}
105+
106+
[Fact]
107+
public async Task NoCollisionMeansNoRuntimeWarning()
108+
{
109+
using var client = test.CreateClient();
110+
(await client.GetAsync("/cab-login")).StatusCode.Should().Be(HttpStatusCode.OK);
111+
112+
var before = test.CurrentLogCount;
113+
114+
// Request omits userId entirely — no collision, so no WARN should be emitted.
115+
using var response = await client.GetAsync("/api/cab-get-user-id/");
116+
response.StatusCode.Should().Be(HttpStatusCode.OK);
117+
(await response.Content.ReadAsStringAsync()).Should().Be("42");
118+
119+
var matches = WarningsContaining(test.RequestLogsSince(before),
120+
"_user_id",
121+
"auto-bound from claim").ToList();
122+
123+
matches.Should().BeEmpty(
124+
"no WARN must fire when the request does not supply a value for the claim-mapped parameter");
125+
}
126+
}
127+
128+
/// <summary>
129+
/// Fail-fast test: a parameter listed in <see cref="NpgsqlRestAuthenticationOptions.ParameterNameClaimsMapping"/>
130+
/// must be declared with a text-compatible PostgreSQL type. Otherwise every authenticated request
131+
/// would crash with a misleading <see cref="System.InvalidCastException"/> from Npgsql ("Writing
132+
/// values of 'System.String' is not supported for parameters having NpgsqlDbType '<X>'"). The
133+
/// configuration has no valid runtime, so we surface it at startup instead of letting it ship.
134+
/// </summary>
135+
public class ClaimAutoBindFailFastTests
136+
{
137+
[Fact]
138+
public void NonTextClaimMappedParameter_ThrowsAtStartup()
139+
{
140+
// Make sure the cabx_create_widget routine (with `_company_id int`) exists in the DB — it is
141+
// shared across the suite via the static SQL script. We then build a minimal WebApplication
142+
// that points NpgsqlRest at this routine with `_company_id` in ParameterNameClaimsMapping.
143+
// Construction must throw — the misdeclaration is caught before a request ever arrives.
144+
var connectionString = Database.Create();
145+
146+
var builder = WebApplication.CreateBuilder();
147+
builder.WebHost.UseUrls("http://127.0.0.1:0");
148+
builder.Services.AddAuthentication().AddCookie();
149+
150+
var app = builder.Build();
151+
152+
Action act = () => app.UseNpgsqlRest(new(connectionString)
153+
{
154+
IncludeSchemas = ["public"],
155+
NameSimilarTo = "cabx[_]create[_]widget",
156+
CommentsMode = CommentsMode.ParseAll,
157+
RequiresAuthorization = false,
158+
AuthenticationOptions = new()
159+
{
160+
UseUserParameters = true,
161+
ParameterNameClaimsMapping = new()
162+
{
163+
{ "_company_id", "company_id" },
164+
},
165+
},
166+
});
167+
168+
act.Should().Throw<ArgumentException>()
169+
.WithMessage("*_company_id*company_id*not text-compatible*");
170+
171+
app.DisposeAsync().AsTask().GetAwaiter().GetResult();
172+
}
173+
174+
[Fact]
175+
public void TextClaimMappedParameter_DoesNotThrowAtStartup()
176+
{
177+
// Sanity check: the same wiring with a text-typed claim-mapped parameter must build cleanly.
178+
// (The runtime collision tests in ClaimAutoBindWarningsTests rely on this fixture working.)
179+
var connectionString = Database.Create();
180+
181+
var builder = WebApplication.CreateBuilder();
182+
builder.WebHost.UseUrls("http://127.0.0.1:0");
183+
builder.Services.AddAuthentication().AddCookie();
184+
185+
var app = builder.Build();
186+
187+
Action act = () => app.UseNpgsqlRest(new(connectionString)
188+
{
189+
IncludeSchemas = ["public"],
190+
NameSimilarTo = "cab[_]get[_]user[_]id",
191+
CommentsMode = CommentsMode.ParseAll,
192+
RequiresAuthorization = false,
193+
AuthenticationOptions = new()
194+
{
195+
UseUserParameters = true,
196+
ParameterNameClaimsMapping = new()
197+
{
198+
{ "_user_id", "name_identifier" },
199+
},
200+
},
201+
});
202+
203+
act.Should().NotThrow();
204+
205+
app.DisposeAsync().AsTask().GetAwaiter().GetResult();
206+
}
207+
}

0 commit comments

Comments
 (0)