fix(odata): stop the batch path from misreporting keys and failures

[Mikeoso]

Ports two defects found on the support/1.3 hotfix line back to main, where the code was identical. One is a security fix.

1. CRLF injection into the embedded request line

The atomic $batch body is assembled as text — ODataBatchRequestBuilder.AppendOperation splices operation.RelativeUrl verbatim into METHOD url HTTP/1.1. Key values reached that line through FormatValue, whose string arm escapes single quotes and nothing else:

string s => $"'{s.Replace("'", "''")}'"

BuildCompositeKeyUrl validated key names via IsValidODataFieldName but never their values, and the scalar-key path validated nothing at all.

A key value of USMF') HTTP/1.1\r\nX-Injected: 1 terminates the request line early and injects headers, or a forged request, into that MIME part.

Only the atomic batch path was exposed. Traced every call site:

Path	Sends via	Exposed
atomic batch update / delete	text body → StringContent	yes
ContinueOnError per item	HttpRequestMessage → Uri	no
single-entity composite-key write bypass	HttpRequestMessage → Uri	no

System.Uri rejects a stray CR or LF; the text-assembled path is the one place that safety net is missing. Entity payloads were never affected — System.Text.Json escapes control characters, and batch boundaries are per-request GUIDs, so a payload cannot break out of its part or forge one.

FormatKeyLiteral now rejects rather than escapes, and lives in the adapter rather than in shared FormatValue: the $filter callers keep their own Uri-backed protection and do not need their behaviour changed.

2. A failed batch reported a success status code

A rolled-back changeset with no non-2xx sub-response borrowed its status from the first sub-response, or from the outer response when none parsed. Both are 2xx by then — a non-2xx outer status returns earlier — so a failed operation came back carrying 204 or 200.

The gap is wider than the review that found it described: it is not only the zero-sub-responses case. Any count mismatch where the present sub-responses are all 2xx lands here too, which is the more likely shape.

Such a batch reports 502 now. The failure is that the response could not be used, not that a particular operation was rejected, so borrowing a per-operation code would be inventing information.

Tests

Five new tests in ODataClientAdapterAtomicBatchTests: three tainted composite-key values, one tainted scalar key, and one unreconcilable response asserting the status code rather than only IsSuccess.

Proven non-vacuous: with both guards removed, all five fail (Fehler: 5, erfolgreich: 0); with them, all five pass.

dotnet build 0 errors, dotnet test 609 passed, 0 failed (OData.Tests 228 → 233).

Compatibility

Rejecting control characters tightens what the public surface accepts, which api-compatibility.md warns about. Two reasons it is acceptable here: main is heading to 3.0.0, a MAJOR, and a D365 entity key cannot legitimately contain CR or LF — the only inputs this rejects are the injection vectors.

Origin

Found by the security review of the identical code on support/1.3, which shipped as 1.3.7 to a customer on a production system. That line carries the same fix.

🤖 Generated with Claude Code

[claude]

Major — the new throw-based guard can turn already-successful ContinueOnError items into false failures.

FormatKeyLiteral is reached via BuildKeyUrl from two places: the atomic path (fail-fast, all keys built up-front via .Select(...).ToList() before anything is sent — safe) and the non-atomic per-item path in BatchUpdateAsync/BatchDeleteAsync (ODataClientAdapter.cs:334 and :358), where BuildKeyUrl(entitySet, itemList[index].Key) is evaluated lazily, once per iteration, inside SendPerItemBatchAsync's loop (:599-624). That loop has no per-iteration try/catch.

If item N in a ContinueOnError chunk has a control-character-tainted key and items 0..N-1 already succeeded (already PATCHed/DELETEd against D365), the throw unwinds SendPerItemBatchAsync, discarding the already-collected BatchOperationResults for the real successes. RunBatchInChunksAsync's catch (Exception ex) (ODataService.cs:599) then marks every operation in that chunk — including the ones that already succeeded on the server — as failed (StatusCode = 0, ErrorMessage = ex.Message). A caller that retries "failed" items based on this result risks double-applying already-successful updates/deletes.

Before this PR, a tainted value on the per-item path was simply handed to Uri per request and each outcome was reported independently — this guard makes ContinueOnError strictly worse for this scenario than before the fix, and it's the opposite of what defect #2 in this same PR is trying to achieve (accurate per-operation failure reporting). No test exercises the ContinueOnError interaction — only Atomic-mode tests were added for the new guard.

Fix: validate all keys in the item list up front before entering SendPerItemBatchAsync (mirroring the atomic path's fail-fast .ToList() validation), or catch per-iteration inside SendPerItemBatchAsync so a thrown exception fails only that one item and preserves already-collected results.

[claude]

Overview

This PR ports two defects found on the support/1.3 hotfix line back to main: (1) a CRLF-injection vector into the embedded METHOD url HTTP/1.1 request line of the atomic $batch body, closed by a new FormatKeyLiteral guard shared by BuildKeyUrl/BuildCompositeKeyUrl; and (2) a failed-but-unreconcilable atomic changeset previously borrowing a 2xx status code, now reporting 502 via a new IndeterminateBatchStatus constant. Both fixes are narrowly scoped, well-reasoned, and backed by five new regression tests that are proven non-vacuous (fail without the guards, pass with them). Overall risk is low-to-moderate: the security fix itself is sound for its stated scope (the atomic path), but the shared helper it introduces has an unintended side effect on the untouched ContinueOnError per-item path — see the Major finding below.

Findings

Major

• IntegratoR.OData/Common/Services/ODataClientAdapter.cs:580 (inline comment posted) — FormatKeyLiteral's new throw is reached not only by the atomic (fail-fast, validated-up-front) path but also by the non-atomic ContinueOnError per-item path (:334, :358), which evaluates BuildKeyUrl lazily per iteration inside SendPerItemBatchAsync's uncaught loop (:599-624). A single tainted key partway through a ContinueOnError chunk throws, discarding already-collected results for items that already succeeded against D365, and RunBatchInChunksAsync then reports the entire chunk — including the real successes — as failed. A caller retrying "failed" items on that basis risks double-applying already-successful writes. This is the opposite of what defect #2 in this same PR sets out to fix (accurate per-operation failure reporting), and it isn't covered by the new tests (only Atomic mode is tested). Fix: validate keys for the whole item list up front before entering SendPerItemBatchAsync, or isolate the per-iteration exception there so only the tainted item is marked failed.

Minor

• None beyond the above.

Nit

• CHANGELOG.md's new Security entry states the non-atomic write paths "hand the URL to System.Uri, which rejects a stray CR or LF." I could not independently verify in this session whether System.Uri throws vs. silently strips CR/LF/TAB for such input (a documented .NET quirk in some code paths). Either way the practical security conclusion is unaffected (no injection reaches the wire), so this is at most a documentation-precision nit worth double-checking, not a functional concern.

Stages run

• Stage 1 (correctness/hard-rules/architecture/tests): ran, see Major finding above. No hard-rule violations found — the new ArgumentException throws are consistent with this file's existing validation pattern (IsValidODataFieldName) and are correctly absorbed into Result<T> by ODataExceptionHandler/RunBatchInChunksAsync for all call paths except the one flagged above.
• Stage 2 (security): not required by the workflow's flag, but this PR is a security fix by nature — reviewed anyway. The CRLF-injection close is correct and precisely scoped to the actually-exposed atomic-batch path; no secrets/headers/auth surface touched.
• Stage 3 (API compatibility): required — reviewed. Tightening what key values the public ODataClientAdapter/IODataClientAdapter surface accepts (rejecting control characters) is a real, intentional breaking change per api-compatibility.md, but it is properly justified and already covered by the existing next-version: 3.0.0 MAJOR bump and a dedicated CHANGELOG "Security" entry under "Unreleased" — no additional action needed.
• Mechanical CI checks (build, dotnet format --verify-no-changes, vulnerable-package scan) live in build.yml and were not recomputed here; the overall build check is green. Note: I was unable to inspect the individual dotnet test step result (vs. overall job status) in this session due to tool restrictions — the PR description claims "609 passed, 0 failed," but that should be confirmed against the actual test-step log before merge given build.yml runs tests with continue-on-error: true.

Verdict

approve-with-changes

[claude]

Minor — stale/missing <exception> docs for the new throw paths.

EnsureValidEntitySet (here, and at lines 323 and 349) and FormatKeyLiteral (used transitively by UpdateAsync/DeleteAsync via BuildCompositeKeyUrl) add new ArgumentException throw paths, but the /// <inheritdoc /> blocks above BatchCreateAsync/BatchUpdateAsync/BatchDeleteAsync gained no <exception> tag — unlike this file's own established pattern (FindByKeyAsync, UpdateAsync, DeleteAsync already document the composite-key ArgumentException). Those existing docs are now stale too: they say "...or contains a field name that is not a valid OData property identifier," but a composite-key value with a control character now throws as well.

Per CLAUDE.md's hard rule ("Doc comments describe actual behaviour — never ship stale/aspirational docs"), please add <exception cref="ArgumentException"> blocks to the three Batch* methods (entity-set shape + control-character key values) and extend the UpdateAsync/DeleteAsync doc text to cover the value case.

[claude]

Overview

This PR ports two hardening fixes from support/1.3 back to main: (1) rejects control characters (CR/LF) in composite-key/scalar-key values and entity-set names before they are spliced verbatim into the atomic $batch request-line text — closing a header/request-injection vector that the per-item and single-entity write paths are already shielded from via Uri percent-encoding; (2) stops a rolled-back atomic changeset with an unreconcilable response (no sub-response explains the failure, or all present sub-responses are 2xx) from reporting a borrowed 2xx status, reporting 502 instead. The second commit (2d40852) is itself a self-correction after review found the first commit's entity-set guard was missing and its stated rationale for why other paths are safe was wrong — that's now fixed and the reasoning corrected in code comments and CHANGELOG. The change is small, well-targeted, and backed by five new regression tests that are proven non-vacuous (fail with the guards removed). Overall risk is low: the guard only tightens acceptance of malformed/malicious input that should never occur from framework-internal callers ([Table]-derived entity-set names are all simple PascalCase identifiers, verified against the current F&O entity set), and the CHANGELOG documents the tightening as an intentional, MAJOR-justified compatibility change.

Findings

Major

• IntegratoR.OData/Common/Services/ODataClientAdapter.cs:297,323,349 (inline comment posted) — The new EnsureValidEntitySet/FormatKeyLiteral throw paths (ArgumentException) aren't reflected in the /// <inheritdoc /> doc blocks for BatchCreateAsync/BatchUpdateAsync/BatchDeleteAsync, and the existing <exception> docs on UpdateAsync/DeleteAsync (lines 218–221, 255–258) are now stale — they describe only the field-name validation, not the new key-value control-character check. Per CLAUDE.md's doc-comment hard rule, add/extend <exception cref="ArgumentException"> tags to cover both cases.

Minor / Nit

• None beyond the above.

Stages run

• Stage 1 (correctness / hard-rule / architecture / test review): ran, findings above.
• Stage 2 (security): not required by the workflow flag, but reviewed anyway since this PR's primary content is a security fix. The fix is sound — it rejects rather than escapes control characters, the exception messages don't echo the tainted raw value (avoiding log/exception injection), and the threat-model reasoning (text-assembled batch body vs. Uri-backed request lines) checks out against the code. No new security surface concerns.
• Stage 3 (API compatibility): required. EnsureValidEntitySet/FormatKeyLiteral tighten what the public IODataClientAdapter/ODataClientAdapter surface accepts (new synchronous-into-Task ArgumentException for malformed entity-set names or control-character key values). This is exactly the kind of change api-compatibility.md says needs an explicit breaking-change plan — and it has one: CHANGELOG's [Unreleased] section already documents it under a dedicated ### Security entry alongside other ### Changed — Breaking entries for this same release, consistent with the PR's stated main → 3.0.0 MAJOR target. No further action needed here.
• Mechanical CI (build, format, tests, vulnerable-package scan) lives in build.yml and was not recomputed. The build check on this PR shows SUCCESS; step-level detail for the continue-on-error test step was not accessible through the tools available to this review, so that specific caveat from the review brief could not be independently confirmed here — worth a manual glance before merge.

Verdict

approve-with-changes