Skip to content

Commit 7c69c4e

Browse files
committed
fix(core): anchor the dynamic report name with \z, not $
In .NET, $ matches at the end of input AND immediately before a trailing newline, so ^[a-zA-Z][a-zA-Z0-9_-]{0,99}$ accepted a name ending in one. That name is remotely creatable through POST /api/reports, and this validator is the only thing standing in front of it. The concrete consequence is the log statements in ReportJobWorker and InMemoryJobScheduler: in a plain-text sink the trailing newline splits the line and lets the name forge a log entry of its own, which is what CodeQL cs/log-forging was pointing at. Path traversal was never reachable through it — /, \, . and : have never been in the character class — so this is the validator not doing what its own doc comment claims rather than a hole in the file layout. \z means end of input and nothing else. The type had NO tests at all, which is how an under-anchored pattern survived since it was written; it has sixteen now, including the trailing newline, embedded control characters, the length boundary and the path-traversal shapes the type exists to reject. One existing assertion had to change with it: ScheduleEndpointTests compared DynamicReportName.Pattern against the RAW response body, and the pattern now contains a backslash, which JSON doubles on the wire. It reads the parsed `error` field instead — stricter, since it also pins which field carries the message. Recorded in the backlog rather than fixed here: a code-registered name is never validated at all, and ReportRunner interpolates it into a temp file path. The obvious guard throws ArgumentException from ReportBuilder's constructor, and every ReportConfigCompiler.Compile call site catches only ConfigurationException — it would turn a bad name into a 500 on POST/PUT and on the validate endpoint, and stop a host with such a report stored from starting at all. That needs a design decision, and the input is the host developer's own literal rather than anything remote. Full suite: 1 703 green across 33 projects.
1 parent 625ed15 commit 7c69c4e

5 files changed

Lines changed: 108 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,18 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
88

99
## [Unreleased]
1010

11+
### Fixed
12+
- **A dynamic report name could end in a newline.** `DynamicReportName.Pattern` was anchored with
13+
`$`, which in .NET matches at the end of input **and** immediately before a trailing newline — so
14+
a name ending in one was accepted. That name is remotely creatable via `POST /api/reports`, and
15+
it reaches a file name on disk, a URL segment, a job record field and every log line about a run.
16+
The concrete consequence is the last one: in a plain-text log sink the newline splits the line and
17+
lets the name forge a log entry of its own (CodeQL `cs/log-forging` on `ReportJobWorker` and
18+
`InMemoryJobScheduler`). Path traversal was never reachable through it — `/`, `\`, `.` and `:` have
19+
never been in the character class — so this is the validator not doing what it documented rather
20+
than a hole in the file layout. Anchored with `\z`, which means end of input and nothing else. The
21+
validator had no tests at all, which is how this survived; it has sixteen now.
22+
1123
### Added
1224
- **Optimistic concurrency on report editing (ADR D87).** `GET /api/reports/{name}/config` now returns
1325
an `ETag`, and `PUT /api/reports/{name}` honours `If-Match`, answering `412 Precondition Failed`

docs/STATUS-AND-BACKLOG.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,21 @@ Fixed by excluding the rule itself via `query-filters`, not by a third path twea
135135
rule at a 100% false-positive rate across two different generators will keep firing every time a
136136
generator changes shape.
137137

138+
### 1d. A code-registered report name is never validated — open
139+
140+
`AddReport<T>(name, …)` accepts any non-blank string, and `ReportRunner` builds the run's output file
141+
as `Path.Combine(tempDir, $"{report.Name}.{ext}")`. So `AddReport<T>("../sales", …)` writes outside the
142+
run's temp directory and escapes its cleanup, and a name with a newline reaches the same log
143+
statements the dynamic-name fix above just closed.
144+
145+
**Not** fixed alongside that one, deliberately. The obvious guard — reject control characters in
146+
`ReportBuilder`'s constructor — throws `ArgumentException`, and every `ReportConfigCompiler.Compile`
147+
call site catches only `ConfigurationException`: it would turn a bad name into a 500 on `POST`/`PUT`
148+
and on the *validate* endpoint (which exists never to throw), and in `FileStoreRegistryHydrator` it
149+
would escape the per-report skip and stop the host from starting at all. Doing this properly means
150+
deciding where a code-first name is validated and with which exception, which is a design question,
151+
and the input is the host developer's own literal rather than anything remote.
152+
138153
### 2. CI hardening
139154
- **Fail (not skip) the Testcontainers integration tests when Docker is absent in CI.****done**:
140155
the five container `ServerFixture`s now swallow a start failure only through an exception filter,

src/NeoReports.Core/Configuration/DynamicReportName.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,15 @@ namespace NeoReports.Core.Configuration;
1111
public static partial class DynamicReportName
1212
{
1313
/// <summary>The validation pattern: starts with a letter, then up to 99 letters/digits/underscore/hyphen.</summary>
14-
public const string Pattern = "^[a-zA-Z][a-zA-Z0-9_-]{0,99}$";
14+
/// <remarks>
15+
/// Anchored with <c>\z</c>, not <c>$</c>. In .NET <c>$</c> also matches immediately BEFORE a
16+
/// trailing newline, so <c>^…$</c> accepted a name ending in one — and this validator is the only
17+
/// thing standing in front of it. The name reaches a file name on disk, a URL segment, a job
18+
/// record and every log line about a run; in a plain-text log sink the newline lets the name forge
19+
/// entries (CodeQL cs/log-forging on ReportJobWorker/InMemoryJobScheduler). <c>\z</c> means end
20+
/// of input and nothing else.
21+
/// </remarks>
22+
public const string Pattern = @"^[a-zA-Z][a-zA-Z0-9_-]{0,99}\z";
1523

1624
/// <summary>True when <paramref name="name"/> matches <see cref="Pattern"/>.</summary>
1725
/// <param name="name">The candidate report name.</param>

tests/NeoReports.AspNetCore.IntegrationTests/ScheduleEndpointTests.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,11 @@ public async Task Setting_a_schedule_for_a_name_no_override_store_can_key_is_ref
7474
// override under this name. That is the resource's state, not a malformed request — and it
7575
// must not be the ArgumentException-turned-500 it used to be.
7676
response.StatusCode.ShouldBe(HttpStatusCode.Conflict);
77-
(await response.Content.ReadAsStringAsync()).ShouldContain(DynamicReportName.Pattern);
77+
// Read through the parsed body, not the raw JSON: the pattern contains a backslash (it is
78+
// anchored with \z, since $ also matches before a trailing newline), and JSON doubles it on
79+
// the wire — a substring check against the raw text compares two different encodings.
80+
JsonElement body = await response.Content.ReadFromJsonAsync<JsonElement>(Json);
81+
body.GetProperty("error").GetString()!.ShouldContain(DynamicReportName.Pattern);
7882
}
7983

8084
[Fact]
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
using NeoReports.Core.Configuration;
2+
using Shouldly;
3+
using Xunit;
4+
5+
namespace NeoReports.Core.UnitTests;
6+
7+
/// <summary>
8+
/// The dynamic-report name validator is the only thing standing between a caller-supplied name and a
9+
/// file name on disk, a URL segment, a job record field and every log line about a run — and it had no
10+
/// tests at all, which is how an under-anchored pattern survived.
11+
/// </summary>
12+
public class DynamicReportNameTests
13+
{
14+
[Theory]
15+
[InlineData("sales")]
16+
[InlineData("Sales")]
17+
[InlineData("monthly-sales_2026")]
18+
[InlineData("a")]
19+
public void An_ordinary_name_is_accepted(string name) =>
20+
DynamicReportName.IsValid(name).ShouldBeTrue();
21+
22+
/// <summary>
23+
/// The reason this file exists. In .NET, <c>$</c> matches at the end of input <b>and</b>
24+
/// immediately before a trailing newline, so <c>^…$</c> accepted a name ending in one. That name
25+
/// is remotely creatable (POST /api/reports), and it reaches ReportJobWorker's and
26+
/// InMemoryJobScheduler's log statements — where, in a plain-text sink, the newline splits the
27+
/// line and lets the name forge a log entry of its own (CodeQL cs/log-forging).
28+
/// </summary>
29+
[Fact]
30+
public void A_name_ending_in_a_newline_is_rejected()
31+
{
32+
DynamicReportName.IsValid("sales" + (char)10).ShouldBeFalse();
33+
DynamicReportName.IsValid("sales" + (char)13 + (char)10).ShouldBeFalse();
34+
DynamicReportName.IsValid("sales" + (char)13).ShouldBeFalse();
35+
}
36+
37+
[Fact]
38+
public void A_name_with_an_embedded_control_character_is_rejected()
39+
{
40+
DynamicReportName.IsValid("sa" + (char)10 + "les").ShouldBeFalse();
41+
DynamicReportName.IsValid("sales" + (char)9 + "monthly").ShouldBeFalse();
42+
DynamicReportName.IsValid("sales" + (char)0).ShouldBeFalse();
43+
}
44+
45+
[Theory]
46+
[InlineData("")]
47+
[InlineData(" ")]
48+
[InlineData("1sales")] // must start with a letter
49+
[InlineData("-sales")]
50+
[InlineData("sales monthly")] // no spaces: it becomes a URL segment
51+
[InlineData("sales.monthly")] // no dots: it becomes a file name
52+
[InlineData("../sales")] // the path-traversal case the type was written for
53+
[InlineData("sales/../etc")]
54+
public void A_name_outside_the_grammar_is_rejected(string name) =>
55+
DynamicReportName.IsValid(name).ShouldBeFalse();
56+
57+
[Fact]
58+
public void Null_is_rejected_rather_than_throwing() =>
59+
DynamicReportName.IsValid(null).ShouldBeFalse();
60+
61+
[Fact]
62+
public void The_length_bound_is_enforced()
63+
{
64+
DynamicReportName.IsValid("a" + new string('b', 99)).ShouldBeTrue();
65+
DynamicReportName.IsValid("a" + new string('b', 100)).ShouldBeFalse();
66+
}
67+
}

0 commit comments

Comments
 (0)