Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
910c61e
feat(changelog): add link: field to changelog entries — marker read side
Mpdreamz Aug 25, 2026
7d6f32e
fix(changelog): marker entries serialize as link-only — omit title an…
Mpdreamz Aug 25, 2026
bcec261
fix(changelog): probe-based CDN sourcing replaces dead pool-registry …
Mpdreamz Aug 26, 2026
2122b44
fix(changelog): marker guard — allow link: alongside other fields, on…
Mpdreamz Aug 26, 2026
82c1e25
fix(changelog): restore marker-only guard; remove contradictory scrub…
Mpdreamz Aug 26, 2026
b25f62e
fix(changelog): propagate probe error from FetchPrEntryAsync — transi…
Mpdreamz Aug 26, 2026
3c68594
fix(changelog): TryExtractPrNumber validates repo name — kibana PRs n…
Mpdreamz Aug 26, 2026
ec766b1
feat(changelog): canonical upload keys and scrubber marker writing — …
Mpdreamz Aug 25, 2026
f94c26f
style: dotnet format
Mpdreamz Aug 25, 2026
1715905
fix: use InvariantCulture for int.ToString in canonical key derivation
Mpdreamz Aug 25, 2026
ad25cbe
fix(changelog): marker guard — allow link: alongside other fields, on…
Mpdreamz Aug 26, 2026
939ce0f
fix(changelog): restore marker-only guard in ScrubResult path
Mpdreamz Aug 26, 2026
06ff6c5
fix(changelog): update MarkerScrubTests to use result.Content after S…
Mpdreamz Aug 26, 2026
e4d287b
fix(changelog): address PR 3930 review comments
Mpdreamz Aug 26, 2026
3e72879
fix(changelog): source pointer + phantom-marker guard for canonical r…
Mpdreamz Aug 26, 2026
a9f1e33
chore: merge main into fix/changelog-pr3930-review
Mpdreamz Aug 26, 2026
23228a0
fix(changelog): add source-redirect discriminator and yaml-only numer…
Mpdreamz Aug 26, 2026
418f591
fix(changelog): strip source-redirect from private markers and check …
Mpdreamz Aug 26, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ public record ChangelogEntryDto
/// Written by the pipeline for non-primary PRs in a multi-PR entry; never hand-authored.
/// </summary>
public string? Link { get; set; }

/// <summary>
/// When true, this public-bucket object is a scrubber-written source pointer that traces back
/// to a canonical public key. Distinguishes source pointers from ordinary link-only PR markers
/// so the delete path does not spuriously follow a regular marker to its canonical target.
/// </summary>
[YamlMember(Alias = "source-redirect", ApplyNamingConventions = false)]
public bool? SourceRedirect { get; set; }
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ private static string ToYamlDoubleQuotedString(string s)
Action = dto.Action,
FeatureId = dto.FeatureId,
Highlight = dto.Highlight,
Link = dto.Link
Link = dto.Link,
SourceRedirect = dto.SourceRedirect ?? false
};

private static ChangelogEntry ToEntry(BundledEntry entry) => new()
Expand Down Expand Up @@ -287,8 +288,10 @@ private static ChangelogEntryType ParseEntryType(string? value)
private static ChangelogEntryDto ToDto(ChangelogEntry entry)
{
// Marker entries are link-only; emitting any other field would violate the marker contract.
// Source pointers also carry source-redirect: true so the delete path can distinguish them
// from ordinary PR markers without ambiguity.
if (entry.IsMarker)
return new ChangelogEntryDto { Link = entry.Link };
return new ChangelogEntryDto { Link = entry.Link, SourceRedirect = entry.SourceRedirect ? true : null };

return new ChangelogEntryDto
{
Expand Down
6 changes: 6 additions & 0 deletions src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ public record ChangelogEntry
/// <summary>True when this entry is a pipeline-written marker that redirects to another PR's entry.</summary>
public bool IsMarker => Link is not null;

/// <summary>
/// When true, this entry is a scrubber-written source pointer in the public bucket that traces
/// to a canonical key. Distinguishes source pointers from ordinary link-only PR markers.
/// </summary>
public bool SourceRedirect { get; init; }

/// <summary>
/// Converts this ChangelogEntry to a BundledEntry for embedding in bundles.
/// File property is set to null; set it separately using a 'with' expression.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ public record ScrubResult
/// Empty for single-PR entries and bundles.
/// </summary>
public IReadOnlyList<(string Key, string Content)> Markers { get; init; } = [];

/// <summary>
/// True when the content is a pass-through private marker (<c>link:</c> only).
/// The processor must not overwrite existing canonical public content at the same key
/// with a marker — private markers derived from raw (pre-allowlist) PR lists can be
/// stale and their canonical target may already be written to a different public key.
/// </summary>
public bool IsMarker { get; init; }
}

/// <summary>Rewrites private-bucket changelog YAML into its public, allowlist-scrubbed form.</summary>
Expand Down Expand Up @@ -97,7 +105,11 @@ private async Task<ScrubResult> ScrubChangelog(string key, string content, Cance
var normalized = ReleaseNotesSerialization.NormalizeYaml(content);
var entry = ReleaseNotesSerialization.DeserializeEntry(normalized);

// Marker: link: <pr_number> with no other content. Return unchanged — there is no URL to scrub.
// Marker: link: <pr_number> with no other content.
// Re-serialize rather than passing raw content through so private-authored fields
// (e.g. source-redirect: true) are stripped. source-redirect is processor-owned and
// must never be settable by private input — if it were passed through, a forged marker
// could impersonate a scrubber-written source pointer and trigger canonical deletion.
if (entry.Link != null)
{
var hasContent = !string.IsNullOrEmpty(entry.Title)
Expand All @@ -107,7 +119,8 @@ private async Task<ScrubResult> ScrubChangelog(string key, string content, Cance
if (hasContent)
throw new InvalidOperationException(
"Changelog entry has both 'link:' and content fields. A marker must contain only 'link: <pr_number>'.");
return new ScrubResult { Content = content };
var safeContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = entry.Link });
return new ScrubResult { Content = safeContent, IsMarker = true };
}

var bundledEntry = new BundledEntry
Expand Down
103 changes: 91 additions & 12 deletions src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information

using System.Globalization;
using System.Net;
using Amazon.S3;
using Amazon.S3.Model;
Expand Down Expand Up @@ -321,25 +322,81 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa
// marker objects it previously produced and delete any that are no longer needed.
var oldPublicContent = await TryGetPublicObject(publicKey, ctx);

await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx);
if (scrubResult.CanonicalKey is not null)
_logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey);
// Issue 1 guard: a private marker derived from raw (pre-allowlist) PRs can race
// with the scrubber writing canonical public content at the same key. If canonical
// content already occupies the public key, skip the marker write so it cannot
// overwrite the canonical entry that the primary object's scrub already produced.
// A null return from TryDeserializeEntry (unparseable content) is treated as
// canonical — the safest assumption when we cannot classify the existing object.
var skipWrite = scrubResult.IsMarker
&& oldPublicContent is not null
&& TryDeserializeEntry(oldPublicContent)?.IsMarker != true;

if (skipWrite)
{
_logger.LogInformation(
"Skipped pass-through marker {Key}: canonical content in public bucket takes precedence", key);
}
else
_logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key);

foreach (var (markerKey, markerContent) in scrubResult.Markers)
{
await PutPublicObject(markerKey, markerContent, "application/yaml", ctx);
_logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey);
await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx);
if (scrubResult.CanonicalKey is not null)
{
_logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey);
// Issue 2: write a source pointer at the source key in the public bucket so
// that a delete event for 'key' can trace back to the canonical key and clean
// it up (see delete path below). The pointer uses the same link: format as
// secondary-PR markers; the non-canonical filename makes it distinguishable.
await WriteSourcePointerAsync(key, scrubResult.CanonicalKey, ctx);
}
else
_logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key);

foreach (var (markerKey, markerContent) in scrubResult.Markers)
{
await PutPublicObject(markerKey, markerContent, "application/yaml", ctx);
_logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey);
}

// Remove marker objects that existed in the previous scrub result but are no
// longer produced (e.g. an entry shrank from 3 PRs to 1).
await DeleteStaleMarkersAsync(publicKey, oldPublicContent, scrubResult.Markers, ctx);
}

// Remove marker objects that existed in the previous scrub result but are no
// longer produced (e.g. an entry shrank from 3 PRs to 1).
await DeleteStaleMarkersAsync(publicKey, oldPublicContent, scrubResult.Markers, ctx);
}
}
else
{
// Private object is gone. Read the public bucket to discover what was previously
// written for this source key so we can clean it up completely.
//
// Two sub-cases:
// (a) Source key carries a scrubber-written source pointer (SourceRedirect == true):
// the scrubber routed canonical content to a different public key and left a
// breadcrumb here. Trace the pointer to the canonical key, delete it and all its
// markers, then delete the pointer itself. SourceRedirect is processor-owned —
// the scrubber strips it from private-authored markers on the way through.
// (b) Any other key (canonical entry, secondary-PR marker, note-* file): clean up
// any markers the canonical may have emitted and delete the key itself.
var publicContent = await TryGetPublicObject(key, ctx);
var publicEntry = publicContent is not null ? TryDeserializeEntry(publicContent) : null;

if (publicEntry?.SourceRedirect == true)
{
var lastSlash = key.LastIndexOf('/');
var keyPrefix = lastSlash >= 0 ? key[..(lastSlash + 1)] : string.Empty;
var canonicalKey = $"{keyPrefix}{publicEntry.Link}.yaml";
var canonicalContent = await TryGetPublicObject(canonicalKey, ctx);
await DeleteStaleMarkersAsync(canonicalKey, canonicalContent, [], ctx);
await DeletePublicObject(canonicalKey, ctx);
_logger.LogInformation(
"Source pointer {Key} traced to canonical {CanonicalKey}; deleted canonical and its markers",
key, canonicalKey);
}
else if (publicContent is not null)
{
await DeleteStaleMarkersAsync(key, publicContent, [], ctx);
}

await DeletePublicObject(key, ctx);
_logger.LogInformation("Private {Key} is gone; removed its public copy", key);
}
Expand Down Expand Up @@ -404,6 +461,28 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa
}
}

private static ChangelogEntry? TryDeserializeEntry(string? content)
{
if (content is null)
return null;
try
{ return ReleaseNotesSerialization.DeserializeEntry(content); }
catch { return null; }
}

private async Task WriteSourcePointerAsync(string sourceKey, string canonicalKey, Cancel ctx)
{
var lastSlash = canonicalKey.LastIndexOf('/');
var canonicalFileName = lastSlash >= 0 ? canonicalKey[(lastSlash + 1)..] : canonicalKey;
var stem = Path.GetFileNameWithoutExtension(canonicalFileName);
if (!int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _))
return; // Not a PR-based canonical key; nothing to point to.
var pointerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = stem, SourceRedirect = true });
await PutPublicObject(sourceKey, pointerContent, "application/yaml", ctx);
_logger.LogInformation("Wrote source pointer {SourceKey} → canonical {CanonicalKey}", sourceKey, canonicalKey);
}


private async Task<string?> TryGetPublicObject(string key, Cancel ctx)
{
try
Expand Down
18 changes: 13 additions & 5 deletions tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@

using AwesomeAssertions;
using Elastic.Changelog.Scrubbing;
using Elastic.Documentation.Configuration.ReleaseNotes;
using Microsoft.Extensions.Logging.Abstractions;

namespace Elastic.Changelog.Tests.Changelogs;

/// <summary>
/// Verifies that <see cref="ChangelogContentScrubber"/> handles <c>link:</c> markers correctly:
/// a bare marker passes through unchanged; a marker with content fields throws.
/// a bare marker is re-serialized with the link value preserved; a marker with content fields throws.
/// Re-serialization (rather than pass-through) strips private-authored fields such as
/// <c>source-redirect: true</c> so forged markers cannot impersonate scrubber-written source pointers.
/// </summary>
public class MarkerScrubTests
{
Expand All @@ -20,25 +23,30 @@ public class MarkerScrubTests
private Cancel Ctx => TestContext.Current.CancellationToken;

[Fact]
public async Task Marker_OnlyLink_PassesThroughUnchanged()
public async Task Marker_OnlyLink_PreservesLinkValue()
{
const string key = "changelog/elastic/elasticsearch/main/200.yaml";
const string content = "link: 100\n";

var result = await _scrubber.ScrubAsync(key, content, Ctx);

result.Content.Should().Be(content);
var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content);
entry.Link.Should().Be("100", "link: value must survive the scrub");
entry.SourceRedirect.Should().BeFalse("source-redirect must not appear in scrubbed markers");
result.IsMarker.Should().BeTrue();
}

[Fact]
public async Task Marker_OnlyLink_NoTrailingNewline_PassesThroughUnchanged()
public async Task Marker_OnlyLink_NoTrailingNewline_PreservesLinkValue()
{
const string key = "changelog/elastic/elasticsearch/main/200.yaml";
const string content = "link: 100";

var result = await _scrubber.ScrubAsync(key, content, Ctx);

result.Content.Should().Be(content);
var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content);
entry.Link.Should().Be("100", "link: value must survive the scrub even without a trailing newline");
result.IsMarker.Should().BeTrue();
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ private static ChangelogContentScrubber Scrubber(IReadOnlyList<string>? allowRep
[Fact]
public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved()
{
// A marker is link: only — the scrubber must pass it through unchanged.
// A marker is link: only — the scrubber must pass it through (re-serialized to strip
// any private-authored fields, but link: itself is preserved).
var yaml = "link: \"12345\"\n";
var scrubber = Scrubber();

Expand All @@ -32,6 +33,23 @@ public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved()
result.Markers.Should().BeEmpty();
}

[Fact]
public async Task ScrubAsync_MarkerEntryWithSourceRedirect_StripsSourceRedirectFromOutput()
{
// source-redirect: true is processor-owned metadata. If a private author adds it to a
// marker file, the scrubber must strip it so forged markers cannot impersonate
// scrubber-written source pointers and trigger spurious canonical deletion on delete.
var yaml = "link: \"12345\"\nsource-redirect: true\n";
var scrubber = Scrubber();

var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None);

var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content);
entry.Link.Should().Be("12345", "link: must survive the scrub round-trip");
entry.SourceRedirect.Should().BeFalse("source-redirect must be stripped from private-authored markers");
result.IsMarker.Should().BeTrue();
}

[Fact]
public async Task ScrubAsync_NormalEntry_LinkIsNull()
{
Expand Down
Loading
Loading