fix(review): close client load-TOCTOU + canonicalize If-Match parse (Codex High/Medium)
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m39s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m30s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m39s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m30s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Codex independent review of #263 surfaced two defects the fork review missed: - High — client load TOCTOU: BlockEditor read root metadata (getBlock) and items+ETag (getBlockItemsWithMeta) concurrently, so a concurrent write landing between them (with the items read resolving last) left a stale root paired with a current ETag → the save silently overwrote the concurrent change with no 412. Fix: read items+ETag FIRST, then the root metadata, so the captured ETag is never newer than the root version and any inconsistency fails safe (save 412s → conflict dialog → reload). - Medium — `ParseIfMatch` accepted non-canonical strong tags ("03", "+3", " 3 ") as version 3. An ETag is opaque; only the exact emitted form is valid. Fix: canonical decimal only (`NumberStyles.None` + no leading zeros) → else 400. Tests: new `ConcurrencyHeadersTests` (canonical parse + padded/signed/whitespace/weak/ unquoted/list/overflow/empty → malformed); `ApiResultsTests` gains the 412 mapping case. Existing BlocksScreen tests still green (load reordering is behavior-preserving for the non-concurrent path). Refs #253 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,3 +1,4 @@
|
||||
using System.Globalization;
|
||||
using LanguageExt;
|
||||
using Microsoft.AspNetCore.Http;
|
||||
using Microsoft.Extensions.Primitives;
|
||||
@@ -48,11 +49,18 @@ public static class ConcurrencyHeaders
|
||||
}
|
||||
|
||||
// Strong entity-tag of a decimal version, e.g. "3". Weak tags (W/"…") are not honored:
|
||||
// this contract's ETags are always strong.
|
||||
if (value.Length >= 2 && value[0] == '"' && value[^1] == '"' &&
|
||||
int.TryParse(value[1..^1], out int version))
|
||||
// this contract's ETags are always strong. An ETag is an opaque token, so only the exact
|
||||
// canonical form we emit is accepted — a non-negative decimal with no sign, surrounding
|
||||
// whitespace, or leading zeros (`NumberStyles.None` + the leading-zero guard reject "+3",
|
||||
// " 3 ", and "03", which must NOT be treated as equal to the emitted "3").
|
||||
if (value.Length >= 2 && value[0] == '"' && value[^1] == '"')
|
||||
{
|
||||
return new IfMatchCondition(IfMatchKind.Version, version);
|
||||
string inner = value[1..^1];
|
||||
if (inner.Length > 0 && (inner.Length == 1 || inner[0] != '0') &&
|
||||
int.TryParse(inner, NumberStyles.None, CultureInfo.InvariantCulture, out int version))
|
||||
{
|
||||
return new IfMatchCondition(IfMatchKind.Version, version);
|
||||
}
|
||||
}
|
||||
|
||||
return new IfMatchCondition(IfMatchKind.Malformed, 0);
|
||||
|
||||
Reference in New Issue
Block a user