Skip to content

Commit 0a534b5

Browse files
cemililikclaude
andcommitted
fix(api): refuse a wildcard mixed with a tag, and bound every echoed segment
All four findings verified against the code; all four were real. `If-Match: *, "7"` was answered `AnyExisting`. RFC 9110 § 13.1.1 makes the wildcard an alternative to the tag list — `If-Match = "*" / #entity-tag` — not a member of it, so a mixed list is a malformed precondition. Measured: Microsoft's `TryParseStrictList` accepts it as a two-element list, and the loop returned on the first `*` it saw, in either order. The result was the *weaker* reading of a header the API could not understand — "any version, as long as it exists" answered to a client that had also named one — which is the exact failure this type refuses one paragraph above, wearing a different shape. Four cases cover both orderings through both entry points; all four go red against the old branch. `SortSpecification` bounded the value it quotes back on the term-count path and not on the per-segment one, so a `sort` with no commas — which never reaches that guard — arrived whole. An 8 KB single field was echoed into a Problem Details body and every log line carrying one. Both paths go through one `Bounded` helper now. Two more holes in my own scanner, both in the same method the last round touched. `CopyLiteral` treated a raw string literal's first quote as its terminator, so `"""…"""` put the scanner back into code while it was still inside a string — and a `//` there swallowed the rest of the line again, which is the bug the literal tracking was added to fix. It records the opening quote run and closes only on a run of the same length. A probe carrying a raw literal with a quote, a `//` and a banned literal now fails the rule, and so does a `Request.Host` on the line after it — which is what proves the scanner exits the literal rather than staying in it. And the frontend architecture document showed the middleware setting `x-learnstack-host` under a comment claiming it was "what the SDK states to the API", conflating the inward carry with the outward statement and never showing the hop secret at all. They are now two things: the middleware carries the host inward, and the SDK pairs it with the secret from server configuration on the way out — with the reason the secret is not in the middleware said outright, which is that a secret written into a forwarded request header travels further than the one hop it authenticates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent afd6596 commit 0a534b5

5 files changed

Lines changed: 136 additions & 15 deletions

File tree

‎backend/src/LearnStack.Api/Common/EntityTag.cs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,14 +113,23 @@ public static Assertion ReadAssertion(HttpRequest request, out IReadOnlyList<lon
113113
return Assertion.Malformed;
114114
}
115115

116+
// RFC 9110 § 13.1.1: `If-Match = "*" / #entity-tag`. The wildcard is one
117+
// of two alternatives, not a member of the list — so `*, "7"` is not a
118+
// wildcard with extra detail, it is a malformed precondition.
119+
// `TryParseStrictList` accepts it as a two-element list (measured), and
120+
// reading the `*` out of it would answer "any version, as long as it
121+
// exists" to a client that also named one. That is the same failure this
122+
// type refuses one paragraph up, wearing a different shape: the weaker
123+
// interpretation of something it could not understand.
124+
if (tags.Any(tag => tag.Equals(EntityTagHeaderValue.Any)))
125+
{
126+
return tags.Count == 1 ? Assertion.AnyExisting : Assertion.Malformed;
127+
}
128+
116129
var asserted = new List<long>(tags.Count);
117130

118131
foreach (var tag in tags)
119132
{
120-
if (tag.Equals(EntityTagHeaderValue.Any))
121-
{
122-
return Assertion.AnyExisting;
123-
}
124133

125134
// Strong comparison: a weak tag says "semantically equivalent", and
126135
// two versions of a row that are semantically equivalent are still

‎backend/src/LearnStack.SharedKernel/Pagination/SortSpecification.cs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,7 @@ public static bool TryParse(
127127
// whole client-supplied value — an 8 KB `sort` is exactly the input
128128
// this guard exists to reject, and echoing it whole puts it in a
129129
// Problem Details body and every log line that carries one.
130-
offendingSegment = raw.Length > MaxFieldLength
131-
? raw[..MaxFieldLength]
132-
: raw;
130+
offendingSegment = Bounded(raw);
133131
return false;
134132
}
135133

@@ -149,7 +147,11 @@ public static bool TryParse(
149147
// a key the client believes it asked for.
150148
if (!IsWellFormedField(field) || !seen.Add(field))
151149
{
152-
offendingSegment = segment;
150+
// Bounded for the same reason the term-count guard above is: a
151+
// `sort` with no commas never reaches that guard, so an 8 KB
152+
// single field arrives here whole and would be quoted back into
153+
// a Problem Details body and every log line carrying one.
154+
offendingSegment = Bounded(segment);
153155
return false;
154156
}
155157

@@ -224,6 +226,13 @@ public Result<SortSpecification> Restrict(IReadOnlyCollection<string> allowedFie
224226
/// rather than left for the endpoint's allow-list, so a value that could
225227
/// only have come from a malformed client never travels further in.
226228
/// </summary>
229+
/// <summary>
230+
/// The most of a rejected value this type will quote back. Nothing the
231+
/// client sent leaves here longer than a single field is allowed to be.
232+
/// </summary>
233+
private static string Bounded(string value) =>
234+
value.Length > MaxFieldLength ? value[..MaxFieldLength] : value;
235+
227236
private static bool IsWellFormedField(string field)
228237
{
229238
if (field.Length == 0 || field.Length > MaxFieldLength)

‎backend/tests/LearnStack.Tests.Architecture/TenancyConventionTests.cs‎

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,12 +194,33 @@ private static string WithoutComments(string source)
194194
}
195195

196196
/// <summary>Copies one string or character literal and returns the index after it.</summary>
197+
/// <remarks>
198+
/// Three shapes, because C# has three and they terminate differently: a
199+
/// normal literal ends at an unescaped quote, a verbatim one (<c>@"…"</c>)
200+
/// escapes a quote by doubling it, and a raw one opens with a <b>run</b> of
201+
/// three or more quotes and closes only on a run of the same length. Reading
202+
/// a raw literal's first quote as its terminator puts the scanner back
203+
/// inside code while it is still inside a string — which is how a <c>//</c>
204+
/// there would swallow the rest of the line again.
205+
/// </remarks>
197206
private static int CopyLiteral(string source, int start, System.Text.StringBuilder kept)
198207
{
199208
var quote = source[start];
200209

201-
// Raw string literals ("""…""") have no escapes and end on a matching
202-
// run of quotes; verbatim ones (@"…") escape a quote by doubling it.
210+
if (quote == '"')
211+
{
212+
var opening = 0;
213+
while (start + opening < source.Length && source[start + opening] == '"')
214+
{
215+
opening++;
216+
}
217+
218+
if (opening >= 3)
219+
{
220+
return CopyRawLiteral(source, start, opening, kept);
221+
}
222+
}
223+
203224
var verbatim = start > 0 && source[start - 1] == '@';
204225
var i = start;
205226

@@ -230,8 +251,8 @@ private static int CopyLiteral(string source, int start, System.Text.StringBuild
230251
return i + 1;
231252
}
232253

233-
// An unterminated literal cannot span a line unless it is verbatim;
234-
// bailing keeps a malformed file from swallowing the whole scan.
254+
// An unterminated non-verbatim literal cannot span a line; bailing
255+
// keeps a malformed file from swallowing the rest of the scan.
235256
if (!verbatim && c == '\n')
236257
{
237258
return i;
@@ -244,6 +265,41 @@ private static int CopyLiteral(string source, int start, System.Text.StringBuild
244265
return i;
245266
}
246267

268+
/// <summary>Copies a raw string literal, closing only on a run of the opening length.</summary>
269+
private static int CopyRawLiteral(
270+
string source, int start, int opening, System.Text.StringBuilder kept)
271+
{
272+
var i = start;
273+
kept.Append(source, i, opening);
274+
i += opening;
275+
276+
while (i < source.Length)
277+
{
278+
if (source[i] != '"')
279+
{
280+
kept.Append(source[i]);
281+
i++;
282+
continue;
283+
}
284+
285+
var run = 0;
286+
while (i + run < source.Length && source[i + run] == '"')
287+
{
288+
run++;
289+
}
290+
291+
kept.Append(source, i, run);
292+
i += run;
293+
294+
if (run >= opening)
295+
{
296+
return i;
297+
}
298+
}
299+
300+
return i;
301+
}
302+
247303
private static string WithoutWhitespace(string value) =>
248304
string.Concat(value.Where(character => !char.IsWhiteSpace(character)));
249305
}

‎backend/tests/LearnStack.Tests.Unit/Api/Common/EntityTagTests.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,29 @@ public void Every_Asserted_Version_In_A_List_Is_Read()
127127
versions.Should().Equal(5, 6, 7);
128128
}
129129

130+
[Theory]
131+
[InlineData("*, \"7\"")]
132+
[InlineData("\"7\", *")]
133+
public void A_Wildcard_Mixed_With_A_Tag_Is_Malformed_Not_A_Wildcard(string raw)
134+
{
135+
// RFC 9110 § 13.1.1 makes `*` an alternative to the tag list, not a
136+
// member of it. Microsoft's parser accepts the mixed form as a
137+
// two-element list, so reading the `*` out of it would answer "any
138+
// version, as long as it exists" to a client that also named one —
139+
// the weaker interpretation of a header the API could not understand,
140+
// which is the failure Evaluate refuses everywhere else.
141+
ReadAssertion(raw, out var versions).Should().Be(EntityTag.Assertion.Malformed);
142+
versions.Should().BeEmpty();
143+
}
144+
145+
[Theory]
146+
[InlineData("*, \"7\"")]
147+
[InlineData("\"7\", *")]
148+
public void And_Evaluate_Fails_It_Rather_Than_Matching(string raw)
149+
{
150+
Evaluate(raw, 7).Should().Be(EntityTag.Precondition.Failed);
151+
}
152+
130153
[Theory]
131154
[InlineData(null, EntityTag.Assertion.Absent)]
132155
[InlineData("*", EntityTag.Assertion.AnyExisting)]

‎docs/architecture/14-frontend-architecture.md‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,11 @@ export async function middleware(req: NextRequest) {
105105
if (resolved.organizationId) requestHeaders.set('x-organization-id', resolved.organizationId);
106106
requestHeaders.set('x-locale', locale);
107107

108-
// What the server-side SDK actually states to the API on the way out, over
109-
// the authenticated hop. The host — not the tenant — is the input the API
110-
// resolves from.
108+
// The visitor's host, carried INWARD so the server-side SDK can state it to
109+
// the API. The host — not the tenant — is the input the API resolves from.
110+
// The hop SECRET is deliberately not here: it is server configuration, and a
111+
// secret written into a forwarded request header travels further than the one
112+
// hop it authenticates.
111113
requestHeaders.set('x-learnstack-host', host);
112114

113115
return NextResponse.next({ request: { headers: requestHeaders } });
@@ -118,6 +120,28 @@ export const config = {
118120
};
119121
```
120122

123+
The SDK is what states the hop, on the way **out**. It reads the host the
124+
middleware carried inward and pairs it with the secret from server configuration
125+
— which is why `createServerSdk` is a server-only entry point and why the secret
126+
never appears in the middleware above:
127+
128+
```ts
129+
// packages/sdk/src/server.ts — the shape, once the first operation exists.
130+
const response = await fetch(new URL(path, options.apiBaseUrl), {
131+
headers: {
132+
// The trusted hop: both halves, or the API ignores the host header
133+
// entirely and resolves from its own Host (ADR-0036).
134+
'X-LearnStack-Host': options.host,
135+
'X-LearnStack-Hop-Secret': process.env.LEARNSTACK_HOP_SECRET!,
136+
137+
// An assertion, not a selector. The API compares it against the host it
138+
// resolved for itself; a mismatch is a 404.
139+
'X-Tenant-Id': options.tenantId,
140+
'Accept-Language': options.locale,
141+
},
142+
});
143+
```
144+
121145
`resolveHost` is backed by a short-TTL edge cache (~60 s) that calls the LearnStack API,
122146
which in turn reads the `platform_host_to_tenant` projection (populated by the Hub —
123147
[27-custom-domain-tls.md](27-custom-domain-tls.md)). Custom-domain activations and

0 commit comments

Comments
 (0)