From 2956b4fdf9a2e4632a29d81d4a929bddbab89cab Mon Sep 17 00:00:00 2001 From: lcawl Date: Thu, 2 Apr 2026 19:42:20 -0700 Subject: [PATCH 1/2] Replace changelog sanitize_private_links with link_allow_repos --- config/changelog.example.yml | 13 +- .../changelog-private-link-sanitization.yaml | 11 +- docs/cli/changelog/bundle.md | 29 +- docs/contribute/changelog.md | 9 +- .../Changelog/BundleConfiguration.cs | 12 +- .../Bundling/ChangelogBundleAmendService.cs | 95 ++- .../Bundling/ChangelogBundlingService.cs | 82 +-- .../Bundling/LinkAllowlistSanitizer.cs | 275 ++++++++ .../Bundling/PrivateChangelogLinkSanitizer.cs | 227 ------- .../ChangelogConfigurationLoader.cs | 71 ++- .../ChangelogConfigurationYaml.cs | 9 +- .../docs-builder/Commands/ChangelogCommand.cs | 12 +- .../Changelogs/LinkAllowlistSanitizerTests.cs | 336 ++++++++++ .../PrivateChangelogLinkSanitizerTests.cs | 600 ------------------ 14 files changed, 783 insertions(+), 998 deletions(-) create mode 100644 src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs delete mode 100644 src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs create mode 100644 tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs delete mode 100644 tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs diff --git a/config/changelog.example.yml b/config/changelog.example.yml index 5c9266b50..5fb9529a4 100644 --- a/config/changelog.example.yml +++ b/config/changelog.example.yml @@ -212,9 +212,13 @@ bundle: output_directory: docs/releases # Whether to resolve (copy contents) by default resolve: true - # When true, PR/issue links to repos marked private in assembler.yml are rewritten in bundle output - # (requires resolve: true). Option-based: --sanitize-private-links / --no-sanitize-private-links. - sanitize_private_links: false + # PR/issue link allowlist: when set (including []), only links to these owner/repo pairs are kept + # in bundle output; others are rewritten to '# PRIVATE:' sentinels (requires resolve: true). + # When omitted, no link filtering is applied. + # Add your repository and any others whose PR/issue links should appear in published docs. + link_allow_repos: + - elastic/elasticsearch + - elastic/kibana # Optional: default GitHub repo name applied to all profiles that do not specify their own. # Used by the {changelog} directive to generate correct PR/issue links when the product ID # differs from the GitHub repository name. Can be overridden per profile. @@ -238,9 +242,6 @@ bundle: # output: "elasticsearch-{version}.yaml" # # Optional: override the products array written to the bundle output. # # output_products: "elasticsearch {version}" - # # Optional: override bundle.sanitize_private_links for this profile only (requires bundle.resolve: true). - # sanitize_private_links: true - # Example: GitHub release profile (fetches PR list directly from a GitHub release) # Use when you want to bundle or remove changelogs based on a published GitHub release. # elasticsearch-gh-release: diff --git a/docs/changelog/changelog-private-link-sanitization.yaml b/docs/changelog/changelog-private-link-sanitization.yaml index 96783f92e..ae884910e 100644 --- a/docs/changelog/changelog-private-link-sanitization.yaml +++ b/docs/changelog/changelog-private-link-sanitization.yaml @@ -1,5 +1,5 @@ type: feature -title: Add bundle-time private link sanitization and changelog directive link visibility +title: Add bundle-time link allowlist for PR and issue references products: - product: docs-builder target: 0.100.0 @@ -9,7 +9,8 @@ areas: prs: - https://github.com/elastic/docs-builder/pull/3002 description: | - Adds opt-in `bundle.sanitize_private_links` in changelog configuration (and per-profile override), - with `--sanitize-private-links` and `--no-sanitize-private-links` on option-based changelog bundle. - Rewrites PR/issue references targeting private `assembler.yml` repos to quoted `# PRIVATE` sentinels - when resolve is true. The `{changelog}` directive gains `:link-visibility:` (`auto`, `keep-links`, `hide-links`). + Replaces `bundle.sanitize_private_links` with explicit `bundle.link_allow_repos` (`owner/repo` list). + When set (including an empty list), PR/issue references not in the allowlist are rewritten to quoted + `# PRIVATE:` sentinels when the bundle is resolved. `bundle.repo` must be a single repository (no `+` syntax). + Optional assembler.yml warnings when an allowlisted repo is missing or marked private. + The `{changelog}` directive retains `:link-visibility:` (`auto`, `keep-links`, `hide-links`). diff --git a/docs/cli/changelog/bundle.md b/docs/cli/changelog/bundle.md index 882e857dc..5f07a2e43 100644 --- a/docs/cli/changelog/bundle.md +++ b/docs/cli/changelog/bundle.md @@ -97,9 +97,6 @@ The `--input-products` option determines which changelog files are gathered for : Each occurrence can be either comma-separated issues ( `--issues "https://github.com/owner/repo/issues/123,456"`) or a file path (for example `--issues /path/to/file.txt`). : When using a file, every line must be a fully-qualified GitHub issue URL such as `https://github.com/owner/repo/issues/123`. Bare numbers and short forms are not allowed in files. -`--no-sanitize-private-links` -: Optional: Explicitly turn off the `sanitize_private_links` option if it's specified in the changelog configuration file. - `--no-resolve` : Optional: Explicitly turn off the `resolve` option if it's specified in the changelog configuration file. @@ -140,13 +137,6 @@ The `--input-products` option determines which changelog files are gathered for : Optional: Copy the contents of each changelog file into the entries array. : By default, the bundle contains only the file names and checksums. -`--sanitize-private-links` -: Optional: Turn on [private link sanitization](#private-link-sanitization). -: Pull requests and issues that target repositories marked `private: true` in the `references` section of `assembler.yml` are rewritten as quoted `# PRIVATE:` sentinel strings in the bundle file. -: This option requires a resolved bundle: use `--resolve` or set `bundle.resolve: true` in the `changelog.yml`. -: If sanitization is enabled and the bundle is not resolved, the command fails. -: When you omit this option, it defaults to `bundle.sanitize_private_links` in your changelog configuration file, which defaults to `false`. - ## Output files Both modes use the same ordered fallback to determine where to write the bundle. The first value that is set wins: @@ -284,27 +274,26 @@ rules: - "Monitoring" ``` -## Private link sanitization [private-link-sanitization] +## PR and issue link allowlist [link-allowlist] -A changelog in a public repository might contain links to pull requests or issues in private repositories. -To prevent that information from appearing in the documentation, use `bundle.sanitize_private_links` in the changelog configuration file (or a product-specific profile override) or the `--sanitize-private-links` command option. +A changelog in a public repository might contain links to pull requests or issues in repositories that should not appear in published documentation. -This feature relies on the [`assembler.yml`](/configure/site/content.md) file and the existence of `private: true` to determine which repo links should be sanitized. -Every repository that appears in a PR or issue link must be listed under `assembler.yml` `references`. References to unknown repositories fail the command so you can fix the registry. -Repos are assumed to be `private: false` unless you specify otherwise. +Set `bundle.link_allow_repos` in `changelog.yml` to an explicit list of `owner/repo` strings (for example, `elastic/elasticsearch`). When this key is present (including as an empty list), PR and issue references are filtered at bundle time: only links whose resolved repository is in the list are kept; others are rewritten to quoted `# PRIVATE:` sentinel strings in the bundle YAML. :::{important} -When you use these options, you must also set `bundle.resolve: true` or specify `--resolve`. -Unresolved bundles that only store `file:` pointers do not get this rewrite; if you need private link sanitization, you must use a resolved bundle. +`bundle.link_allow_repos` requires a **resolved** bundle. Set `bundle.resolve: true` or pass `--resolve`. Unresolved bundles that only store `file:` pointers are not rewritten. ::: -The `changelog bundle`, `changelog gh-release`, and `changelog bundle-amend` commands rewrite PR and issue references that **target** private repositories into quoted sentinel strings such as `"# PRIVATE: …"` in the bundle file. -The changelog directive and `changelog render` command then omit these sentinels from the documentation. +When [`assembler.yml`](/configure/site/content.md) is available, docs-builder emits **warnings** (non-fatal) if an allowlisted repo is missing from `references` or is marked `private: true`, so you can verify the registry before publishing. + +The `changelog bundle`, `changelog gh-release`, and `changelog bundle-amend` commands apply the same rules. The changelog directive and `changelog render` command omit `# PRIVATE:` sentinels from rendered documentation. :::{warning} Sentinel values are omitted from rendered documentation but remain in bundle files; they are not cryptographic redaction. ::: +`bundle.repo` must name a **single** GitHub repository (do not use `repo1+repo2` merged-repo syntax). + ## Option-based examples ### Bundle by report or URL list diff --git a/docs/contribute/changelog.md b/docs/contribute/changelog.md index 379bdce82..4ca1dbc31 100644 --- a/docs/contribute/changelog.md +++ b/docs/contribute/changelog.md @@ -713,8 +713,8 @@ Top-level `bundle` fields: |---|---| | `repo` | Default GitHub repository name applied to all profiles. Falls back to product ID if not set at any level. | | `owner` | Default GitHub repository owner applied to all profiles. | -| `resolve` | When `true`, embeds full changelog entry content in the bundle (same as `--resolve`). Required when `sanitize_private_links` is enabled. | -| `sanitize_private_links` | When `true`, rewrites PR/issue references that target private repositories (per `assembler.yml` `references`) to quoted `# PRIVATE:` sentinel strings in bundle YAML. Requires `resolve: true` and a non-empty `references` section in `assembler.yml`. Default `false`. Refer to [Private link sanitization at bundle time](/cli/changelog/bundle.md#private-link-sanitization). | +| `resolve` | When `true`, embeds full changelog entry content in the bundle (same as `--resolve`). Required when `link_allow_repos` is set. | +| `link_allow_repos` | When set (including an empty list), only PR/issue links whose resolved repository is in this `owner/repo` list are kept; others are rewritten to `# PRIVATE:` sentinels in bundle YAML. When absent, no link filtering is applied. Requires `resolve: true`. Refer to [PR and issue link allowlist](/cli/changelog/bundle.md#link-allowlist). | Profile configuration fields in `bundle.profiles`: @@ -727,7 +727,6 @@ Profile configuration fields in `bundle.profiles`: | `repo` | Optional. Overrides `bundle.repo` for this profile only. Required when `source: github_release` is used and no `bundle.repo` is set. | | `owner` | Optional. Overrides `bundle.owner` for this profile only. | | `hide_features` | List of feature IDs to embed in the bundle as hidden. | -| `sanitize_private_links` | Optional. Overrides `bundle.sanitize_private_links` for this profile. | Example profile configuration: @@ -1048,7 +1047,7 @@ The `--hide-features` option on the `render` command and the `hide-features` fie A changelog can reference multiple pull requests and issues in the `prs` and `issues` array fields. -To comment out the private links in all changelogs in your bundles, refer to [changelog bundle](/cli/changelog/bundle.md#private-link-sanitization). +To comment out links that are not in your allowlist in all changelogs in your bundles, refer to [changelog bundle](/cli/changelog/bundle.md#link-allowlist). If you are working in a private repo and do not want any pull request or issue links to appear (even if they target a public repo), you also have the option to configure link visibiblity in the [changelog directive](/syntax/changelog.md) and [changelog render](/cli/changelog/render.md) command. @@ -1296,7 +1295,7 @@ docs-builder changelog remove elasticsearch-release 9.2.0 --dry-run The command automatically discovers `changelog.yml` by checking `./changelog.yml` then `./docs/changelog.yml` relative to your current directory. If no configuration file is found, the command returns an error with advice to create one or to run from the directory where the file exists. -The `output`, `output_products`, `hide_features`, `sanitize_private_links`, and `resolve` fields are bundle-specific and are always ignored for removal (along with other bundle-only settings that do not affect which changelog files match the filter). +The `output`, `output_products`, `hide_features`, `link_allow_repos`, and `resolve` fields are bundle-specific and are always ignored for removal (along with other bundle-only settings that do not affect which changelog files match the filter). Which other fields are used depends on the profile type: - Standard profiles: only the `products` field is used. The `repo` and `owner` fields are ignored (they only affect bundle output metadata). diff --git a/src/Elastic.Documentation.Configuration/Changelog/BundleConfiguration.cs b/src/Elastic.Documentation.Configuration/Changelog/BundleConfiguration.cs index 7a49b1b44..7f3717d82 100644 --- a/src/Elastic.Documentation.Configuration/Changelog/BundleConfiguration.cs +++ b/src/Elastic.Documentation.Configuration/Changelog/BundleConfiguration.cs @@ -39,10 +39,11 @@ public record BundleConfiguration public string? Owner { get; init; } /// - /// When true, PR/issue references targeting repositories marked private: true in - /// assembler.yml are rewritten to sentinel values at bundle time (requires ). + /// When set (including an empty list), PR/issue references whose resolved owner/repo is not listed + /// are rewritten to # PRIVATE: sentinels at bundle time. When absent, no link filtering is applied. + /// Requires . /// - public bool SanitizePrivateLinks { get; init; } + public IReadOnlyList? LinkAllowRepos { get; init; } /// /// Named bundle profiles for different release scenarios. @@ -104,9 +105,4 @@ public record BundleProfile /// Mutually exclusive with . /// public string? Source { get; init; } - - /// - /// When set, overrides for this profile. - /// - public bool? SanitizePrivateLinks { get; init; } } diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs index 4e8e3f44e..809ce81f5 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs @@ -134,22 +134,14 @@ public async Task AmendBundle(IDiagnosticsCollector collector, AmendBundle if (_configLoader != null) changelogConfig = await _configLoader.LoadChangelogConfiguration(collector, null, ctx); - var sanitizePrivateLinks = changelogConfig?.Bundle?.SanitizePrivateLinks == true; - Bundle? parentBundleForSanitize = null; - AssemblyConfiguration? assemblyForSanitize = null; + var linkAllowRepos = changelogConfig?.Bundle?.LinkAllowRepos; + var linkAllowlistActive = linkAllowRepos != null; + Bundle? parentBundleForAllowlist = null; - if (sanitizePrivateLinks) + if (linkAllowlistActive) { - if (configurationContext == null) - { - collector.EmitError( - string.Empty, - "Private link sanitization requires assembler configuration. Run docs-builder with a valid configuration context."); - return false; - } - if (parentBundleFromInfer != null) - parentBundleForSanitize = parentBundleFromInfer; + parentBundleForAllowlist = parentBundleFromInfer; else { var (ok, loaded) = await TryDeserializeParentBundleAsync( @@ -160,61 +152,47 @@ public async Task AmendBundle(IDiagnosticsCollector collector, AmendBundle if (!ok) return false; ArgumentNullException.ThrowIfNull(loaded); - parentBundleForSanitize = loaded; + parentBundleForAllowlist = loaded; } - ArgumentNullException.ThrowIfNull(parentBundleForSanitize); - if (!parentBundleForSanitize.IsResolved) + ArgumentNullException.ThrowIfNull(parentBundleForAllowlist); + if (!parentBundleForAllowlist.IsResolved) { collector.EmitError( string.Empty, - "Private link sanitization requires the parent bundle to be resolved (inline entry content). " + - "Re-create the bundle with resolve enabled, or disable bundle.sanitize_private_links."); + "bundle.link_allow_repos requires the parent bundle to be resolved (inline entry content). " + + "Re-create the bundle with resolve enabled, or remove bundle.link_allow_repos."); return false; } - var assemblyYaml = configurationContext.ConfigurationFileProvider.AssemblerFile.ReadToEnd(); - try - { - assemblyForSanitize = AssemblyConfiguration.Deserialize(assemblyYaml, skipPrivateRepositories: false); - } - catch (Exception ex) when (ex is not (OutOfMemoryException or StackOverflowException)) - { - collector.EmitError( - string.Empty, - $"Failed to parse assembler configuration YAML: {ex.Message}", - ex); - return false; - } - - var owner = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Owner ?? "elastic" : "elastic"; - var repo = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Repo : null; - if (!PrivateChangelogLinkSanitizer.TrySanitizeBundle( + var owner = parentBundleForAllowlist.Products.Count > 0 ? parentBundleForAllowlist.Products[0].Owner ?? "elastic" : "elastic"; + var repo = parentBundleForAllowlist.Products.Count > 0 ? parentBundleForAllowlist.Products[0].Repo : null; + if (!LinkAllowlistSanitizer.TryApplyBundle( collector, - parentBundleForSanitize, - assemblyForSanitize, + parentBundleForAllowlist, + linkAllowRepos!, owner, repo, out _, - out var parentHadUnsanitizedLinks)) + out var parentHadAllowlistChanges)) return false; - if (parentHadUnsanitizedLinks) + if (parentHadAllowlistChanges) { collector.EmitError( string.Empty, - "Private link sanitization requires the parent bundle to already reflect sanitized PR/issue references. " + - "Re-create the parent bundle with bundle.sanitize_private_links enabled and resolve enabled, " + - "or disable bundle.sanitize_private_links for amend."); + "bundle.link_allow_repos requires the parent bundle to already reflect filtered PR/issue references. " + + "Re-create the parent bundle with the same bundle.link_allow_repos and resolve enabled, " + + "or remove bundle.link_allow_repos for amend."); return false; } } - if (sanitizePrivateLinks && !shouldResolve) + if (linkAllowlistActive && !shouldResolve) { collector.EmitError( string.Empty, - "Private link sanitization requires resolved amend content. Use --resolve or ensure the original bundle is resolved, or disable bundle.sanitize_private_links."); + "bundle.link_allow_repos requires resolved amend content. Use --resolve or ensure the original bundle is resolved, or remove bundle.link_allow_repos."); return false; } @@ -236,23 +214,38 @@ public async Task AmendBundle(IDiagnosticsCollector collector, AmendBundle }; var bundleForWrite = amendBundle; - if (sanitizePrivateLinks && shouldResolve) + if (linkAllowlistActive && shouldResolve) { - ArgumentNullException.ThrowIfNull(parentBundleForSanitize); - ArgumentNullException.ThrowIfNull(assemblyForSanitize); - var owner = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Owner ?? "elastic" : "elastic"; - var repo = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Repo : null; + ArgumentNullException.ThrowIfNull(parentBundleForAllowlist); + var owner = parentBundleForAllowlist.Products.Count > 0 ? parentBundleForAllowlist.Products[0].Owner ?? "elastic" : "elastic"; + var repo = parentBundleForAllowlist.Products.Count > 0 ? parentBundleForAllowlist.Products[0].Repo : null; - if (!PrivateChangelogLinkSanitizer.TrySanitizeBundle( + if (!LinkAllowlistSanitizer.TryApplyBundle( collector, amendBundle, - assemblyForSanitize, + linkAllowRepos!, owner, repo, out var sanitized, out _)) return false; bundleForWrite = sanitized; + + if (configurationContext != null) + { + try + { + var assemblyYaml = configurationContext.ConfigurationFileProvider.AssemblerFile.ReadToEnd(); + var assembly = AssemblyConfiguration.Deserialize(assemblyYaml, skipPrivateRepositories: false); + LinkAllowlistSanitizer.EmitAssemblerDiagnostics(collector, linkAllowRepos!, assembly); + } + catch (Exception ex) when (ex is not (OutOfMemoryException or StackOverflowException)) + { + collector.EmitWarning( + string.Empty, + $"Could not load assembler.yml for bundle.link_allow_repos diagnostics: {ex.Message}"); + } + } } // Serialize and write the amend file diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs index f113e08e7..c5807409f 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs @@ -85,19 +85,9 @@ public record BundleChangelogsArguments public string[]? HideFeatures { get; init; } /// - /// Effective flag after merging CLI, profile, and config (see ). + /// When non-null (including empty), PR/issue links are filtered to this owner/repo allowlist (from changelog.yml bundle.link_allow_repos). /// - public bool SanitizePrivateLinks { get; init; } - - /// - /// CLI override for option-based bundling only. null = use changelog.yml bundle default. - /// - public bool? SanitizePrivateLinksCli { get; init; } - - /// - /// When true, forces sanitization off (overrides other sources). - /// - public bool NoSanitizePrivateLinks { get; init; } + public IReadOnlyList? LinkAllowRepos { get; init; } } /// @@ -176,7 +166,7 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle if (!ValidateInput(collector, input)) return false; - if (!ValidateSanitizePrivateLinks(collector, input)) + if (!ValidateLinkAllowlist(collector, input)) return false; // Load PR or issue filter values @@ -290,21 +280,34 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle return false; var bundleData = buildResult.Data; - if (input.SanitizePrivateLinks) + if (input.LinkAllowRepos != null) { - ArgumentNullException.ThrowIfNull(configurationContext); - var assemblyYaml = configurationContext.ConfigurationFileProvider.AssemblerFile.ReadToEnd(); - var assembly = AssemblyConfiguration.Deserialize(assemblyYaml, skipPrivateRepositories: false); - if (!PrivateChangelogLinkSanitizer.TrySanitizeBundle( + if (!LinkAllowlistSanitizer.TryApplyBundle( collector, bundleData, - assembly, + input.LinkAllowRepos, input.Owner ?? "elastic", input.Repo, out var sanitizedBundle, out _)) return false; bundleData = sanitizedBundle; + + if (configurationContext != null) + { + try + { + var assemblyYaml = configurationContext.ConfigurationFileProvider.AssemblerFile.ReadToEnd(); + var assembly = AssemblyConfiguration.Deserialize(assemblyYaml, skipPrivateRepositories: false); + LinkAllowlistSanitizer.EmitAssemblerDiagnostics(collector, input.LinkAllowRepos, assembly); + } + catch (Exception ex) when (ex is not (OutOfMemoryException or StackOverflowException)) + { + collector.EmitWarning( + string.Empty, + $"Could not load assembler.yml for bundle.link_allow_repos diagnostics: {ex.Message}"); + } + } } // Write bundle file @@ -347,7 +350,6 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle string? repo = null; string? owner = null; string[]? mergedHideFeatures = null; - var sanitizePrivateLinks = false; if (config?.Bundle?.Profiles != null && config.Bundle.Profiles.TryGetValue(input.Profile!, out var profile)) { @@ -389,7 +391,6 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle repo = profile.Repo ?? config.Bundle.Repo; owner = profile.Owner ?? config.Bundle.Owner; mergedHideFeatures = profile.HideFeatures?.Count > 0 ? [.. profile.HideFeatures] : null; - sanitizePrivateLinks = profile.SanitizePrivateLinks ?? config.Bundle.SanitizePrivateLinks; } return input with @@ -402,8 +403,7 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle OutputProducts = outputProducts, Repo = repo, Owner = owner, - HideFeatures = mergedHideFeatures, - SanitizePrivateLinks = sanitizePrivateLinks + HideFeatures = mergedHideFeatures }; } @@ -413,13 +413,7 @@ private static BundleChangelogsArguments ApplyConfigDefaults(BundleChangelogsArg var directory = input.Directory ?? config?.Bundle?.Directory ?? Directory.GetCurrentDirectory(); if (config?.Bundle == null) - { - var sanitizeNoConfig = !input.NoSanitizePrivateLinks && - (string.IsNullOrWhiteSpace(input.Profile) - ? input.SanitizePrivateLinksCli ?? false - : input.SanitizePrivateLinks); - return input with { Directory = directory, SanitizePrivateLinks = sanitizeNoConfig }; - } + return input with { Directory = directory, LinkAllowRepos = null }; // Apply output default when --output not specified: use bundle.output_directory if set var output = input.Output; @@ -433,12 +427,6 @@ private static BundleChangelogsArguments ApplyConfigDefaults(BundleChangelogsArg var repo = input.Repo ?? config.Bundle.Repo; var owner = input.Owner ?? config.Bundle.Owner; - // Profile mode forbids --sanitize-private-links on the CLI; SanitizePrivateLinksCli is only set for option-based bundle. - var sanitizePrivateLinks = !input.NoSanitizePrivateLinks && - (!string.IsNullOrWhiteSpace(input.Profile) - ? input.SanitizePrivateLinks - : input.SanitizePrivateLinksCli ?? config.Bundle.SanitizePrivateLinks); - return input with { Directory = directory, @@ -446,7 +434,7 @@ private static BundleChangelogsArguments ApplyConfigDefaults(BundleChangelogsArg Resolve = resolve, Repo = repo, Owner = owner, - SanitizePrivateLinks = sanitizePrivateLinks + LinkAllowRepos = config.Bundle.LinkAllowRepos }; } @@ -491,29 +479,17 @@ private bool ValidateInput(IDiagnosticsCollector collector, BundleChangelogsArgu return true; } - private bool ValidateSanitizePrivateLinks(IDiagnosticsCollector collector, BundleChangelogsArguments input) + private static bool ValidateLinkAllowlist(IDiagnosticsCollector collector, BundleChangelogsArguments input) { - if (!input.SanitizePrivateLinks) + if (input.LinkAllowRepos == null) return true; if (!(input.Resolve ?? false)) { collector.EmitError( string.Empty, - "Private link sanitization requires resolved bundle content. " + - "Use --resolve or set bundle.resolve: true in changelog.yml, or disable sanitization " + - "(bundle.sanitize_private_links / --sanitize-private-links)." - ); - return false; - } - - if (configurationContext == null) - { - collector.EmitError( - string.Empty, - "Private link sanitization requires assembler configuration (assembler.yml). " + - "Ensure docs-builder runs with a valid configuration source." - ); + "bundle.link_allow_repos requires resolved bundle content. " + + "Use --resolve or set bundle.resolve: true in changelog.yml, or remove bundle.link_allow_repos."); return false; } diff --git a/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs b/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs new file mode 100644 index 000000000..e1f4bb4fd --- /dev/null +++ b/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs @@ -0,0 +1,275 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// 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 Elastic.Documentation.Configuration.Assembler; +using Elastic.Documentation.Diagnostics; +using Elastic.Documentation.ReleaseNotes; + +namespace Elastic.Changelog.Bundling; + +/// +/// Rewrites PR/issue references that are not in bundle.link_allow_repos to # PRIVATE: sentinels for bundle YAML output. +/// +public static class LinkAllowlistSanitizer +{ + private const string SentinelPrefix = "# PRIVATE:"; + + /// + /// Applies the allowlist to PR/issue strings. References whose resolved owner/repo is not in + /// are rewritten to # PRIVATE: sentinels (warnings are emitted). + /// Existing sentinels are normalized: if the underlying reference targets an allowed repo, the plain reference is restored. + /// + public static bool TryApplyBundle( + IDiagnosticsCollector collector, + Bundle bundle, + IReadOnlyList allowRepos, + string defaultOwner, + string? defaultBundleRepo, + out Bundle sanitized, + out bool changesApplied) + { + sanitized = bundle; + changesApplied = false; + + var allow = BuildAllowSet(allowRepos); + var ownerDefault = string.IsNullOrWhiteSpace(defaultOwner) ? "elastic" : defaultOwner; + var anyRewritten = false; + var newEntries = new List(bundle.Entries.Count); + + foreach (var entry in bundle.Entries) + { + var prs = ApplyToReferenceList( + collector, + entry.Prs, + ownerDefault, + defaultBundleRepo, + allow, + "PR", + ref anyRewritten); + if (prs == null) + return false; + + var issues = ApplyToReferenceList( + collector, + entry.Issues, + ownerDefault, + defaultBundleRepo, + allow, + "issue", + ref anyRewritten); + if (issues == null) + return false; + + newEntries.Add(entry with { Prs = prs, Issues = issues }); + } + + sanitized = bundle with { Entries = newEntries }; + changesApplied = anyRewritten; + return true; + } + + /// + /// When assembler configuration is available, emits warnings for allowlist entries that are missing from + /// assembler.yml references or marked private: true. + /// + public static void EmitAssemblerDiagnostics( + IDiagnosticsCollector collector, + IReadOnlyList linkAllowRepos, + AssemblyConfiguration? assembly) + { + if (assembly == null || linkAllowRepos.Count == 0) + return; + + foreach (var entry in linkAllowRepos) + { + if (string.IsNullOrWhiteSpace(entry)) + continue; + + if (!TrySplitOwnerRepo(entry.Trim(), out var owner, out var repo)) + continue; + + if (!TryFindReferenceRepository(owner, repo, assembly, out var repository) || repository is null) + { + collector.EmitWarning( + string.Empty, + $"bundle.link_allow_repos entry '{entry}' is not listed in assembler.yml references (informational)."); + continue; + } + + if (repository.Private) + { + collector.EmitWarning( + string.Empty, + $"bundle.link_allow_repos entry '{entry}' is marked private in assembler.yml; verify that published links are intended."); + } + } + } + + private static HashSet BuildAllowSet(IReadOnlyList allowRepos) + { + var allow = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var r in allowRepos) + { + if (string.IsNullOrWhiteSpace(r)) + continue; + + if (TrySplitOwnerRepo(r.Trim(), out var o, out var repo)) + _ = allow.Add($"{o}/{repo}"); + } + + return allow; + } + + private static IReadOnlyList? ApplyToReferenceList( + IDiagnosticsCollector collector, + IReadOnlyList? refs, + string defaultOwner, + string? defaultBundleRepo, + HashSet allow, + string referenceKind, + ref bool anyRewritten) + { + if (refs is null || refs.Count == 0) + return refs ?? []; + + var list = new List(refs.Count); + foreach (var r in refs) + { + if (string.IsNullOrWhiteSpace(r)) + { + list.Add(r); + continue; + } + + if (r.StartsWith(SentinelPrefix, StringComparison.OrdinalIgnoreCase)) + { + var adjusted = ProcessSentinel(collector, r, defaultOwner, defaultBundleRepo, allow, referenceKind, ref anyRewritten); + if (adjusted == null) + return null; + + list.Add(adjusted); + continue; + } + + var plain = ProcessPlainReference(collector, r, defaultOwner, defaultBundleRepo, allow, referenceKind, ref anyRewritten); + if (plain == null) + return null; + + list.Add(plain); + } + + return list; + } + + private static string? ProcessSentinel( + IDiagnosticsCollector collector, + string sentinelRef, + string defaultOwner, + string? defaultBundleRepo, + HashSet allow, + string referenceKind, + ref bool anyRewritten) + { + var underlyingRef = sentinelRef.Substring(SentinelPrefix.Length).Trim(); + + if (string.IsNullOrWhiteSpace(underlyingRef)) + { + collector.EmitError( + string.Empty, + $"Invalid {referenceKind} sentinel '{sentinelRef}': no underlying reference found. " + + "Sentinels must have the format '# PRIVATE: '."); + return null; + } + + if (!ChangelogTextUtilities.TryGetGitHubRepo(underlyingRef, defaultOwner, defaultBundleRepo ?? string.Empty, out var owner, out var repo)) + { + collector.EmitError( + string.Empty, + $"Invalid {referenceKind} sentinel '{sentinelRef}': underlying reference '{underlyingRef}' could not be parsed. " + + "Use a full https://github.com/ URL, owner/repo#number, or a bare number with bundle owner/repo set."); + return null; + } + + var fullName = $"{owner}/{repo}"; + if (allow.Contains(fullName)) + { + if (!string.Equals(sentinelRef, underlyingRef, StringComparison.Ordinal)) + anyRewritten = true; + + return underlyingRef; + } + + return sentinelRef; + } + + private static string? ProcessPlainReference( + IDiagnosticsCollector collector, + string r, + string defaultOwner, + string? defaultBundleRepo, + HashSet allow, + string referenceKind, + ref bool anyRewritten) + { + if (!ChangelogTextUtilities.TryGetGitHubRepo(r, defaultOwner, defaultBundleRepo ?? string.Empty, out var owner, out var repo)) + { + collector.EmitError( + string.Empty, + $"Link allowlist filtering could not parse {referenceKind} reference '{r}'. " + + "Use a full https://github.com/ URL, owner/repo#number, or a bare number with bundle owner/repo set."); + return null; + } + + var fullName = $"{owner}/{repo}"; + if (allow.Contains(fullName)) + return r; + + anyRewritten = true; + collector.EmitWarning( + string.Empty, + $"PR/issue reference '{r}' targets repository '{fullName}', which is not in bundle.link_allow_repos. " + + "It was rewritten to a '# PRIVATE:' sentinel."); + return $"{SentinelPrefix} {r}"; + } + + private static bool TrySplitOwnerRepo(string entry, out string owner, out string repo) + { + owner = string.Empty; + repo = string.Empty; + + var slash = entry.IndexOf('/'); + if (slash <= 0 || slash >= entry.Length - 1) + return false; + + if (entry.IndexOf('/', slash + 1) >= 0) + return false; + + owner = entry[..slash]; + repo = entry[(slash + 1)..]; + return !string.IsNullOrWhiteSpace(owner) && !string.IsNullOrWhiteSpace(repo); + } + + private static bool TryFindReferenceRepository( + string owner, + string repo, + AssemblyConfiguration assembly, + out Repository? repository) + { + var fullName = $"{owner}/{repo}"; + var isElasticOwner = string.Equals(owner, "elastic", StringComparison.OrdinalIgnoreCase); + + foreach (var kvp in assembly.ReferenceRepositories) + { + if (string.Equals(kvp.Key, fullName, StringComparison.OrdinalIgnoreCase) || + (isElasticOwner && string.Equals(kvp.Key, repo, StringComparison.OrdinalIgnoreCase))) + { + repository = kvp.Value; + return true; + } + } + + repository = null; + return false; + } +} diff --git a/src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs b/src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs deleted file mode 100644 index 96c6b40ca..000000000 --- a/src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs +++ /dev/null @@ -1,227 +0,0 @@ -// Licensed to Elasticsearch B.V under one or more agreements. -// 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 Elastic.Documentation.Configuration.Assembler; -using Elastic.Documentation.Diagnostics; -using Elastic.Documentation.ReleaseNotes; - -namespace Elastic.Changelog.Bundling; - -/// -/// Rewrites PR/issue references that target private repositories to sentinel strings for bundle YAML output. -/// -public static class PrivateChangelogLinkSanitizer -{ - private const string SentinelPrefix = "# PRIVATE:"; - - /// - /// Rewrites PR/issue strings that target repositories marked private in to - /// # PRIVATE: sentinels. Emits errors for unknown repos. The empty references registry error is - /// emitted only when a parseable PR/issue reference requires classification. - /// - /// Diagnostic sink for validation errors. - /// Input bundle; unchanged when this method returns false. - /// Parsed assembler.yml (must list every referenced owner/repo). - /// Default GitHub organization for bare numeric references. - /// Bundle repo field (supports repo1+repo2; first segment used for defaults). - /// Bundle with updated Prs/Issues when return value is true. - /// True when at least one reference was rewritten to a sentinel. - /// True if all references were validated and any rewrites applied successfully. - public static bool TrySanitizeBundle( - IDiagnosticsCollector collector, - Bundle bundle, - AssemblyConfiguration assembly, - string defaultOwner, - string? defaultBundleRepo, - out Bundle sanitized, - out bool changesApplied) - { - sanitized = bundle; - changesApplied = false; - - var ownerDefault = string.IsNullOrWhiteSpace(defaultOwner) ? "elastic" : defaultOwner; - var anyRewritten = false; - var newEntries = new List(bundle.Entries.Count); - - foreach (var entry in bundle.Entries) - { - var prs = SanitizeReferenceList(collector, entry.Prs, ownerDefault, defaultBundleRepo, assembly, "PR", ref anyRewritten); - if (prs == null) - return false; - - var issues = SanitizeReferenceList(collector, entry.Issues, ownerDefault, defaultBundleRepo, assembly, "issue", ref anyRewritten); - if (issues == null) - return false; - - newEntries.Add(entry with { Prs = prs, Issues = issues }); - } - - sanitized = bundle with { Entries = newEntries }; - changesApplied = anyRewritten; - return true; - } - - private static IReadOnlyList? SanitizeReferenceList( - IDiagnosticsCollector collector, - IReadOnlyList? refs, - string defaultOwner, - string? defaultBundleRepo, - AssemblyConfiguration assembly, - string referenceKind, - ref bool anyRewritten) - { - if (refs is null || refs.Count == 0) - return refs ?? []; - - var list = new List(refs.Count); - foreach (var r in refs) - { - if (string.IsNullOrWhiteSpace(r)) - { - list.Add(r); - continue; - } - - if (r.StartsWith(SentinelPrefix, StringComparison.OrdinalIgnoreCase)) - { - if (!ValidateSentinelReference(collector, r, defaultOwner, defaultBundleRepo, assembly, referenceKind)) - return null; - - list.Add(r); - continue; - } - - if (!ChangelogTextUtilities.TryGetGitHubRepo(r, defaultOwner, defaultBundleRepo ?? string.Empty, out var o, out var repoName)) - { - collector.EmitError( - string.Empty, - $"Private link sanitization could not parse {referenceKind} reference '{r}'. " + - "Use a full https://github.com/ URL, owner/repo#number, or a bare number with bundle owner/repo set." - ); - return null; - } - - if (assembly.ReferenceRepositories.Count == 0) - { - collector.EmitError( - string.Empty, - "Private link sanitization requires a non-empty assembler.yml references section. " + - "Ensure configuration is loaded (for example ./config relative to the current directory, embedded defaults, or --configuration-source). " + - "See documentation for changelog bundle private link filtering." - ); - return null; - } - - if (!TryFindReferenceRepository(o, repoName, assembly, out var repository) || repository is null) - { - collector.EmitError( - string.Empty, - $"Repository '{o}/{repoName}' referenced in a changelog {referenceKind} is not listed in assembler.yml references. " + - "Add it under references with private: true or false. " + - "Ensure assembler configuration is up to date (local ./config, embedded binary, or --configuration-source)." - ); - return null; - } - - if (repository.Private) - { - list.Add($"{SentinelPrefix} {r}"); - anyRewritten = true; - } - else - list.Add(r); - } - - return list; - } - - private static bool ValidateSentinelReference( - IDiagnosticsCollector collector, - string sentinelRef, - string defaultOwner, - string? defaultBundleRepo, - AssemblyConfiguration assembly, - string referenceKind) - { - var underlyingRef = sentinelRef.Substring(SentinelPrefix.Length).Trim(); - - if (string.IsNullOrWhiteSpace(underlyingRef)) - { - collector.EmitError( - string.Empty, - $"Invalid {referenceKind} sentinel '{sentinelRef}': no underlying reference found. " + - "Sentinels must have the format '# PRIVATE: '." - ); - return false; - } - - if (!ChangelogTextUtilities.TryGetGitHubRepo(underlyingRef, defaultOwner, defaultBundleRepo ?? string.Empty, out var owner, out var repo)) - { - collector.EmitError( - string.Empty, - $"Invalid {referenceKind} sentinel '{sentinelRef}': underlying reference '{underlyingRef}' could not be parsed. " + - "Use a full https://github.com/ URL, owner/repo#number, or a bare number with bundle owner/repo set." - ); - return false; - } - - if (assembly.ReferenceRepositories.Count == 0) - { - collector.EmitError( - string.Empty, - "Private link sanitization requires a non-empty assembler.yml references section. " + - "Ensure configuration is loaded (for example ./config relative to the current directory, embedded defaults, or --configuration-source). " + - "See documentation for changelog bundle private link filtering." - ); - return false; - } - - if (!TryFindReferenceRepository(owner, repo, assembly, out var repository) || repository is null) - { - collector.EmitError( - string.Empty, - $"Invalid {referenceKind} sentinel '{sentinelRef}': repository '{owner}/{repo}' is not listed in assembler.yml references. " + - "Add it under references with private: true or false. " + - "Ensure assembler configuration is up to date (local ./config, embedded binary, or --configuration-source)." - ); - return false; - } - - if (!repository.Private) - { - collector.EmitError( - string.Empty, - $"Invalid {referenceKind} sentinel '{sentinelRef}': repository '{owner}/{repo}' is marked as public, not private. " + - "Sentinels must only wrap references to private repositories. " + - "Remove the sentinel or update assembler.yml to mark the repository as private: true." - ); - return false; - } - - return true; - } - - private static bool TryFindReferenceRepository( - string owner, - string repo, - AssemblyConfiguration assembly, - out Repository? repository) - { - var fullName = $"{owner}/{repo}"; - var isElasticOwner = string.Equals(owner, "elastic", StringComparison.OrdinalIgnoreCase); - - foreach (var kvp in assembly.ReferenceRepositories) - { - if (string.Equals(kvp.Key, fullName, StringComparison.OrdinalIgnoreCase) || - (isElasticOwner && string.Equals(kvp.Key, repo, StringComparison.OrdinalIgnoreCase))) - { - repository = kvp.Value; - return true; - } - } - - repository = null; - return false; - } -} diff --git a/src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs b/src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs index 5c5159977..797b7371c 100644 --- a/src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs +++ b/src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs @@ -286,7 +286,11 @@ internal static ChangelogConfigurationYaml DeserializeConfiguration(string yaml) // Process bundle configuration BundleConfiguration? bundleConfig = null; if (yamlConfig.Bundle != null) - bundleConfig = ParseBundleConfiguration(yamlConfig.Bundle); + { + bundleConfig = ParseBundleConfiguration(collector, configPath, yamlConfig.Bundle); + if (bundleConfig == null) + return null; + } // Process extract configuration var extract = new ExtractConfiguration @@ -441,8 +445,66 @@ private static PivotConfiguration ConvertPivot(PivotConfigurationYaml yamlPivot) }; } - private static BundleConfiguration ParseBundleConfiguration(BundleConfigurationYaml yaml) + private static BundleConfiguration? ParseBundleConfiguration(IDiagnosticsCollector collector, string configPath, BundleConfigurationYaml yaml) { + if (!string.IsNullOrWhiteSpace(yaml.Repo) && yaml.Repo.Contains('+', StringComparison.Ordinal)) + { + collector.EmitError( + configPath, + "bundle.repo must name a single GitHub repository. Remove '+' merged-repo syntax from bundle.repo."); + return null; + } + + if (yaml.Profiles is { Count: > 0 }) + { + foreach (var kvp in yaml.Profiles) + { + var profileRepo = kvp.Value?.Repo; + if (!string.IsNullOrWhiteSpace(profileRepo) && profileRepo.Contains('+', StringComparison.Ordinal)) + { + collector.EmitError( + configPath, + $"bundle.profiles.{kvp.Key}.repo must name a single GitHub repository. Remove '+' merged-repo syntax."); + return null; + } + } + } + + IReadOnlyList? linkAllowRepos = null; + if (yaml.LinkAllowRepos != null) + { + var raw = yaml.LinkAllowRepos.Values ?? []; + var list = new List(); + foreach (var v in raw) + { + if (string.IsNullOrWhiteSpace(v)) + continue; + + var trimmed = v.Trim(); + if (trimmed.IndexOf('/') < 0 || + trimmed.IndexOf('/') != trimmed.LastIndexOf('/')) + { + collector.EmitError( + configPath, + $"bundle.link_allow_repos: each entry must be exactly 'owner/repo' (one slash). Invalid: '{v}'."); + return null; + } + + var slash = trimmed.IndexOf('/'); + if (slash <= 0 || slash >= trimmed.Length - 1) + { + collector.EmitError( + configPath, + $"bundle.link_allow_repos: each entry must be exactly 'owner/repo' (one slash). Invalid: '{v}'."); + return null; + } + + list.Add(trimmed); + } + + linkAllowRepos = list; + } + Dictionary? profiles = null; if (yaml.Profiles is { Count: > 0 }) { @@ -458,8 +520,7 @@ private static BundleConfiguration ParseBundleConfiguration(BundleConfigurationY Repo = kvp.Value.Repo, Owner = kvp.Value.Owner, HideFeatures = kvp.Value.HideFeatures?.Values, - Source = kvp.Value.Source, - SanitizePrivateLinks = kvp.Value.SanitizePrivateLinks + Source = kvp.Value.Source }); } @@ -470,7 +531,7 @@ private static BundleConfiguration ParseBundleConfiguration(BundleConfigurationY Resolve = yaml.Resolve ?? true, Repo = yaml.Repo, Owner = yaml.Owner, - SanitizePrivateLinks = yaml.SanitizePrivateLinks ?? false, + LinkAllowRepos = linkAllowRepos, Profiles = profiles }; } diff --git a/src/services/Elastic.Changelog/Serialization/ChangelogConfigurationYaml.cs b/src/services/Elastic.Changelog/Serialization/ChangelogConfigurationYaml.cs index 34e2f085c..cc3ad0e22 100644 --- a/src/services/Elastic.Changelog/Serialization/ChangelogConfigurationYaml.cs +++ b/src/services/Elastic.Changelog/Serialization/ChangelogConfigurationYaml.cs @@ -291,9 +291,9 @@ internal record BundleConfigurationYaml public string? Owner { get; set; } /// - /// When true, sanitize private-repo PR/issue links at bundle time (requires resolve). + /// When set, only PR/issue links targeting these owner/repo values are kept; others become # PRIVATE: sentinels (requires resolve). /// - public bool? SanitizePrivateLinks { get; set; } + public YamlLenientList? LinkAllowRepos { get; set; } /// /// Named bundle profiles. @@ -346,11 +346,6 @@ internal record BundleProfileYaml /// Mutually exclusive with . /// public string? Source { get; set; } - - /// - /// When set, overrides bundle.sanitize_private_links for this profile. - /// - public bool? SanitizePrivateLinks { get; set; } } /// diff --git a/src/tooling/docs-builder/Commands/ChangelogCommand.cs b/src/tooling/docs-builder/Commands/ChangelogCommand.cs index 28b52241a..9b07f3497 100644 --- a/src/tooling/docs-builder/Commands/ChangelogCommand.cs +++ b/src/tooling/docs-builder/Commands/ChangelogCommand.cs @@ -501,8 +501,6 @@ async static (s, collector, state, ctx) => await s.CreateChangelog(collector, st /// GitHub release tag to use as a filter source (for example, "v9.2.0" or "latest"). When specified, fetches the release, parses PR references from the release notes, and uses those PRs as the filter — equivalent to passing the PR list via --prs. When --output-products is not specified, it is inferred from the release tag and repository name. /// Optional: Copy the contents of each changelog file into the entries array. Uses config bundle.resolve or defaults to false. /// Optional: Explicitly turn off resolve (overrides config). - /// Optional: Enable bundle-time private link sanitization (requires --resolve). Uses bundle.sanitize_private_links when omitted. - /// Optional: Disable private link sanitization even when enabled in config. /// [Command("bundle")] public async Task Bundle( @@ -524,8 +522,6 @@ public async Task Bundle( string? report = null, bool? resolve = null, bool noResolve = false, - bool? sanitizePrivateLinks = null, - bool noSanitizePrivateLinks = false, Cancel ctx = default ) { @@ -615,10 +611,6 @@ public async Task Bundle( forbidden.Add("--config"); if (!string.IsNullOrWhiteSpace(directory)) forbidden.Add("--directory"); - if (sanitizePrivateLinks.HasValue) - forbidden.Add("--sanitize-private-links"); - if (noSanitizePrivateLinks) - forbidden.Add("--no-sanitize-private-links"); if (forbidden.Count > 0) { @@ -783,9 +775,7 @@ public async Task Bundle( ProfileReport = isProfileMode ? profileReport : null, Report = !isProfileMode ? report : null, Config = config, - HideFeatures = allFeatureIdsForBundle.Count > 0 ? allFeatureIdsForBundle.ToArray() : null, - SanitizePrivateLinksCli = sanitizePrivateLinks, - NoSanitizePrivateLinks = noSanitizePrivateLinks + HideFeatures = allFeatureIdsForBundle.Count > 0 ? allFeatureIdsForBundle.ToArray() : null }; serviceInvoker.AddCommand(service, input, diff --git a/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs new file mode 100644 index 000000000..550b3421e --- /dev/null +++ b/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs @@ -0,0 +1,336 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// 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 AwesomeAssertions; +using Elastic.Changelog.Bundling; +using Elastic.Documentation.Configuration.Assembler; +using Elastic.Documentation.ReleaseNotes; + +namespace Elastic.Changelog.Tests.Changelogs; + +public class LinkAllowlistSanitizerTests(ITestOutputHelper output) : ChangelogTestBase(output) +{ + [Fact] + public void TryGetGitHubRepo_FullUrl_ParsesOwnerRepo() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "https://github.com/elastic/kibana-team/pull/456", + "elastic", + "elasticsearch", + out var owner, + out var repo); + + ok.Should().BeTrue(); + owner.Should().Be("elastic"); + repo.Should().Be("kibana-team"); + } + + [Fact] + public void TryGetGitHubRepo_ShortForm_ParsesOwnerRepo() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "elastic/security-team#789", + "elastic", + "elasticsearch", + out var owner, + out var repo); + + ok.Should().BeTrue(); + owner.Should().Be("elastic"); + repo.Should().Be("security-team"); + } + + [Fact] + public void TryGetGitHubRepo_PullUrl_InvalidNumber_ReturnsFalse() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "https://github.com/elastic/kibana-team/pull/not-a-number", + "elastic", + "elasticsearch", + out _, + out _); + + ok.Should().BeFalse(); + } + + [Fact] + public void TryGetGitHubRepo_ShortForm_NonNumericFragment_ReturnsFalse() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "elastic/kibana-team#abc", + "elastic", + "elasticsearch", + out _, + out _); + + ok.Should().BeFalse(); + } + + [Fact] + public void TryGetGitHubRepo_ShortForm_TooManySlashes_ReturnsFalse() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "a/b/c#123", + "elastic", + "elasticsearch", + out _, + out _); + + ok.Should().BeFalse(); + } + + [Fact] + public void TryGetGitHubRepo_BareNumber_UsesDefaults() + { + var ok = ChangelogTextUtilities.TryGetGitHubRepo( + "123", + "elastic", + "elasticsearch+kibana", + out var owner, + out var repo); + + ok.Should().BeTrue(); + owner.Should().Be("elastic"); + repo.Should().Be("elasticsearch"); + } + + [Fact] + public void FormatPrLink_Sentinel_ReturnsEmpty() + { + var s = ChangelogTextUtilities.FormatPrLink("# PRIVATE: https://github.com/elastic/x/pull/1", "x", hidePrivateLinks: false); + s.Should().BeEmpty(); + } + + [Fact] + public void TryApplyBundle_AllowedRepo_KeepsUrl() + { + var bundle = new Bundle + { + Entries = + [ + new() + { + Title = "t", + Prs = ["https://github.com/elastic/elasticsearch/pull/100"], + Issues = ["https://github.com/elastic/kibana/issues/200"] + } + ] + }; + + var allow = new[] { "elastic/elasticsearch", "elastic/kibana" }; + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + allow, + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeFalse(); + sanitized.Entries[0].Prs![0].Should().Be("https://github.com/elastic/elasticsearch/pull/100"); + sanitized.Entries[0].Issues![0].Should().Be("https://github.com/elastic/kibana/issues/200"); + Collector.Errors.Should().Be(0); + } + + [Fact] + public void TryApplyBundle_NotAllowed_ReplacesWithSentinel() + { + var bundle = new Bundle + { + Entries = + [ + new() + { + Title = "t", + Prs = ["https://github.com/elastic/secret-repo/pull/1"] + } + ] + }; + + var allow = new[] { "elastic/elasticsearch" }; + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + allow, + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeTrue(); + sanitized.Entries[0].Prs![0].Should().StartWith("# PRIVATE:"); + Collector.Warnings.Should().BeGreaterThan(0); + } + + [Fact] + public void TryApplyBundle_EmptyAllowlist_StripsAll() + { + var bundle = new Bundle + { + Entries = [new() { Title = "t", Prs = ["https://github.com/elastic/elasticsearch/pull/1"] }] + }; + + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + [], + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeTrue(); + sanitized.Entries[0].Prs![0].Should().StartWith("# PRIVATE:"); + } + + [Fact] + public void TryApplyBundle_UnparseableRef_EmitsError() + { + var bundle = new Bundle + { + Entries = [new() { Title = "t", Prs = ["not-a-valid-ref"] }] + }; + + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + ["elastic/elasticsearch"], + "elastic", + "elasticsearch", + out _, + out _); + + ok.Should().BeFalse(); + Collector.Errors.Should().BeGreaterThan(0); + } + + [Fact] + public void TryApplyBundle_SentinelAllowed_RestoresPlainRef() + { + var bundle = new Bundle + { + Entries = + [ + new() + { + Title = "t", + Prs = ["# PRIVATE: https://github.com/elastic/elasticsearch/pull/1"] + } + ] + }; + + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + ["elastic/elasticsearch"], + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeTrue(); + sanitized.Entries[0].Prs![0].Should().Be("https://github.com/elastic/elasticsearch/pull/1"); + } + + [Fact] + public void TryApplyBundle_SentinelNotAllowed_KeepsSentinel() + { + var bundle = new Bundle + { + Entries = + [ + new() + { + Title = "t", + Prs = ["# PRIVATE: https://github.com/elastic/other/pull/1"] + } + ] + }; + + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + ["elastic/elasticsearch"], + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeFalse(); + sanitized.Entries[0].Prs![0].Should().Be("# PRIVATE: https://github.com/elastic/other/pull/1"); + } + + [Fact] + public void EmitAssemblerDiagnostics_MissingRepo_EmitsWarning() + { + var asm = AssemblyConfiguration.Deserialize("references: {}", skipPrivateRepositories: false); + LinkAllowlistSanitizer.EmitAssemblerDiagnostics(Collector, ["elastic/foo"], asm); + Collector.Warnings.Should().BeGreaterThan(0); + } + + [Fact] + public void EmitAssemblerDiagnostics_PrivateRepo_EmitsWarning() + { + var yaml = + """ + references: + elastic/foo: + private: true + """; + var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); + LinkAllowlistSanitizer.EmitAssemblerDiagnostics(Collector, ["elastic/foo"], asm); + Collector.Warnings.Should().BeGreaterThan(0); + } + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_Null_ReturnsEmpty() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo(null).Should().BeEmpty(); + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_Empty_ReturnsEmpty() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("").Should().BeEmpty(); + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_Whitespace_ReturnsEmpty() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo(" ").Should().BeEmpty(); + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_SingleRepo_ReturnsSame() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("elasticsearch").Should().Be("elasticsearch"); + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_MergedRepo_ReturnsFirst() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("elasticsearch+kibana").Should().Be("elasticsearch"); + + [Fact] + public void GetFirstRepoSegmentFromBundleRepo_ThreeSegments_ReturnsFirst() => + ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("a+b+c").Should().Be("a"); + + [Fact] + public void FormatIssueLink_Sentinel_ReturnsEmpty() + { + var s = ChangelogTextUtilities.FormatIssueLink("# PRIVATE: https://github.com/elastic/x/issues/1", "x", hidePrivateLinks: false); + s.Should().BeEmpty(); + } + + [Fact] + public void FormatPrLinkAsciidoc_Sentinel_ReturnsEmpty() + { + var s = ChangelogTextUtilities.FormatPrLinkAsciidoc("# PRIVATE: https://github.com/elastic/x/pull/1", "x", hidePrivateLinks: false); + s.Should().BeEmpty(); + } + + [Fact] + public void FormatIssueLinkAsciidoc_Sentinel_ReturnsEmpty() + { + var s = ChangelogTextUtilities.FormatIssueLinkAsciidoc("# PRIVATE: https://github.com/elastic/x/issues/1", "x", hidePrivateLinks: false); + s.Should().BeEmpty(); + } +} diff --git a/tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs deleted file mode 100644 index cdfea528f..000000000 --- a/tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs +++ /dev/null @@ -1,600 +0,0 @@ -// Licensed to Elasticsearch B.V under one or more agreements. -// 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 AwesomeAssertions; -using Elastic.Changelog.Bundling; -using Elastic.Documentation.Configuration.Assembler; -using Elastic.Documentation.ReleaseNotes; - -namespace Elastic.Changelog.Tests.Changelogs; - -public class PrivateChangelogLinkSanitizerTests(ITestOutputHelper output) : ChangelogTestBase(output) -{ - [Fact] - public void TryGetGitHubRepo_FullUrl_ParsesOwnerRepo() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "https://github.com/elastic/kibana-team/pull/456", - "elastic", - "elasticsearch", - out var owner, - out var repo); - - ok.Should().BeTrue(); - owner.Should().Be("elastic"); - repo.Should().Be("kibana-team"); - } - - [Fact] - public void TryGetGitHubRepo_ShortForm_ParsesOwnerRepo() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "elastic/security-team#789", - "elastic", - "elasticsearch", - out var owner, - out var repo); - - ok.Should().BeTrue(); - owner.Should().Be("elastic"); - repo.Should().Be("security-team"); - } - - [Fact] - public void TryGetGitHubRepo_PullUrl_InvalidNumber_ReturnsFalse() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "https://github.com/elastic/kibana-team/pull/not-a-number", - "elastic", - "elasticsearch", - out _, - out _); - - ok.Should().BeFalse(); - } - - [Fact] - public void TryGetGitHubRepo_ShortForm_NonNumericFragment_ReturnsFalse() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "elastic/kibana-team#abc", - "elastic", - "elasticsearch", - out _, - out _); - - ok.Should().BeFalse(); - } - - [Fact] - public void TryGetGitHubRepo_ShortForm_TooManySlashes_ReturnsFalse() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "a/b/c#123", - "elastic", - "elasticsearch", - out _, - out _); - - ok.Should().BeFalse(); - } - - [Fact] - public void TryGetGitHubRepo_BareNumber_UsesDefaults() - { - var ok = ChangelogTextUtilities.TryGetGitHubRepo( - "123", - "elastic", - "elasticsearch+kibana", - out var owner, - out var repo); - - ok.Should().BeTrue(); - owner.Should().Be("elastic"); - repo.Should().Be("elasticsearch"); - } - - [Fact] - public void FormatPrLink_Sentinel_ReturnsEmpty() - { - var s = ChangelogTextUtilities.FormatPrLink("# PRIVATE: https://github.com/elastic/x/pull/1", "x", hidePrivateLinks: false); - s.Should().BeEmpty(); - } - - [Fact] - public void TrySanitizeBundle_PrivateRepo_ReplacesWithSentinel() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Elastic.Documentation.ReleaseNotes.Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["https://github.com/elastic/kibana-team/pull/1", "123"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, - bundle, - asm, - "elastic", - "elasticsearch", - out var sanitized, - out var changed); - - ok.Should().BeTrue(); - changed.Should().BeTrue(); - sanitized.Entries[0].Prs![0].Should().StartWith("# PRIVATE:"); - sanitized.Entries[0].Prs![1].Should().Be("123"); - } - - [Fact] - public void TrySanitizeBundle_ReferenceKey_ElasticSlashRepo_Resolves() - { - var yaml = - """ - references: - elastic/kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["https://github.com/elastic/kibana-team/pull/99"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, - bundle, - asm, - "elastic", - "elasticsearch", - out var sanitized, - out _); - - ok.Should().BeTrue(); - sanitized.Entries[0].Prs![0].Should().StartWith("# PRIVATE:"); - } - - [Fact] - public void TrySanitizeBundle_UnknownRepo_EmitsError() - { - var yaml = - """ - references: - elasticsearch: - private: false - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Elastic.Documentation.ReleaseNotes.Bundle - { - Entries = [new() { Title = "t", Prs = ["https://github.com/unknown-org/unknown-repo/pull/1"] }] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, - bundle, - asm, - "elastic", - "elasticsearch", - out _, - out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - } - - [Fact] - public void TrySanitizeBundle_EmptyReferences_NoPrIssueRefs_Succeeds() - { - var yaml = - """ - references: {} - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = [], - Issues = [] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, - bundle, - asm, - "elastic", - "elasticsearch", - out var sanitized, - out var changed); - - ok.Should().BeTrue(); - changed.Should().BeFalse(); - Collector.Errors.Should().Be(0); - sanitized.Entries.Should().HaveCount(1); - } - - [Fact] - public void TrySanitizeBundle_EmptyReferences_ParseableRef_EmitsEmptyRegistryError() - { - var yaml = - """ - references: {} - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = [new() { Title = "t", Prs = ["https://github.com/elastic/kibana/pull/1"] }] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, - bundle, - asm, - "elastic", - "elasticsearch", - out _, - out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - Collector.Diagnostics.Should().Contain(d => d.Message.Contains("non-empty assembler.yml references", StringComparison.Ordinal)); - } - - [Fact] - public void TrySanitizeBundle_AllPublicRefs_ChangesAppliedIsFalse() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana: - private: false - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["https://github.com/elastic/elasticsearch/pull/100"], - Issues = ["https://github.com/elastic/kibana/issues/200"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out var sanitized, out var changed); - - ok.Should().BeTrue(); - changed.Should().BeFalse(); - sanitized.Entries[0].Prs![0].Should().Be("https://github.com/elastic/elasticsearch/pull/100"); - sanitized.Entries[0].Issues![0].Should().Be("https://github.com/elastic/kibana/issues/200"); - } - - [Fact] - public void TrySanitizeBundle_AlreadySanitizedBundle_ChangesAppliedIsFalse() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["https://github.com/elastic/elasticsearch/pull/100", - "# PRIVATE: https://github.com/elastic/kibana-team/pull/1"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out _, out var changed); - - ok.Should().BeTrue(); - changed.Should().BeFalse(); - } - - [Fact] - public void TrySanitizeBundle_MixedPublicAndPrivateRefs_SanitizesOnlyPrivate() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "public entry", - Prs = ["https://github.com/elastic/elasticsearch/pull/1"], - Issues = ["https://github.com/elastic/elasticsearch/issues/2"] - }, - new() - { - Title = "mixed entry", - Prs = ["https://github.com/elastic/elasticsearch/pull/3"], - Issues = ["https://github.com/elastic/kibana-team/issues/4"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out var sanitized, out var changed); - - ok.Should().BeTrue(); - changed.Should().BeTrue(); - sanitized.Entries[0].Prs![0].Should().Be("https://github.com/elastic/elasticsearch/pull/1"); - sanitized.Entries[0].Issues![0].Should().Be("https://github.com/elastic/elasticsearch/issues/2"); - sanitized.Entries[1].Prs![0].Should().Be("https://github.com/elastic/elasticsearch/pull/3"); - sanitized.Entries[1].Issues![0].Should().StartWith("# PRIVATE:"); - } - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_Null_ReturnsEmpty() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo(null).Should().BeEmpty(); - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_Empty_ReturnsEmpty() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("").Should().BeEmpty(); - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_Whitespace_ReturnsEmpty() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo(" ").Should().BeEmpty(); - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_SingleRepo_ReturnsSame() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("elasticsearch").Should().Be("elasticsearch"); - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_MergedRepo_ReturnsFirst() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("elasticsearch+kibana").Should().Be("elasticsearch"); - - [Fact] - public void GetFirstRepoSegmentFromBundleRepo_ThreeSegments_ReturnsFirst() => - ChangelogTextUtilities.GetFirstRepoSegmentFromBundleRepo("a+b+c").Should().Be("a"); - - [Fact] - public void FormatIssueLink_Sentinel_ReturnsEmpty() - { - var s = ChangelogTextUtilities.FormatIssueLink("# PRIVATE: https://github.com/elastic/x/issues/1", "x", hidePrivateLinks: false); - s.Should().BeEmpty(); - } - - [Fact] - public void FormatPrLinkAsciidoc_Sentinel_ReturnsEmpty() - { - var s = ChangelogTextUtilities.FormatPrLinkAsciidoc("# PRIVATE: https://github.com/elastic/x/pull/1", "x", hidePrivateLinks: false); - s.Should().BeEmpty(); - } - - [Fact] - public void FormatIssueLinkAsciidoc_Sentinel_ReturnsEmpty() - { - var s = ChangelogTextUtilities.FormatIssueLinkAsciidoc("# PRIVATE: https://github.com/elastic/x/issues/1", "x", hidePrivateLinks: false); - s.Should().BeEmpty(); - } - - [Fact] - public void TrySanitizeBundle_SentinelWithValidPrivateRef_Succeeds() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["# PRIVATE: https://github.com/elastic/kibana-team/pull/1"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out var sanitized, out var changed); - - ok.Should().BeTrue(); - changed.Should().BeFalse(); - sanitized.Entries[0].Prs![0].Should().Be("# PRIVATE: https://github.com/elastic/kibana-team/pull/1"); - Collector.Errors.Should().Be(0); - } - - [Fact] - public void TrySanitizeBundle_SentinelWithPublicRepo_Fails() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["# PRIVATE: https://github.com/elastic/elasticsearch/pull/1"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out _, out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - Collector.Diagnostics.Should().Contain(d => d.Message.Contains("marked as public", StringComparison.Ordinal)); - } - - [Fact] - public void TrySanitizeBundle_SentinelWithUnknownRepo_Fails() - { - var yaml = - """ - references: - elasticsearch: - private: false - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["# PRIVATE: https://github.com/unknown-org/unknown-repo/pull/1"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out _, out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - Collector.Diagnostics.Should().Contain(d => d.Message.Contains("not listed in assembler.yml", StringComparison.Ordinal)); - } - - [Fact] - public void TrySanitizeBundle_SentinelWithMalformedRef_Fails() - { - var yaml = - """ - references: - elasticsearch: - private: false - kibana-team: - private: true - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["# PRIVATE: not-a-valid-ref"] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out _, out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - Collector.Diagnostics.Should().Contain(d => d.Message.Contains("could not be parsed", StringComparison.Ordinal)); - } - - [Fact] - public void TrySanitizeBundle_SentinelWithEmptyRef_Fails() - { - var yaml = - """ - references: - elasticsearch: - private: false - """; - - var asm = AssemblyConfiguration.Deserialize(yaml, skipPrivateRepositories: false); - var bundle = new Bundle - { - Entries = - [ - new() - { - Title = "t", - Prs = ["# PRIVATE: "] - } - ] - }; - - var ok = PrivateChangelogLinkSanitizer.TrySanitizeBundle( - Collector, bundle, asm, "elastic", "elasticsearch", - out _, out _); - - ok.Should().BeFalse(); - Collector.Errors.Should().BeGreaterThan(0); - Collector.Diagnostics.Should().Contain(d => d.Message.Contains("no underlying reference found", StringComparison.Ordinal)); - } -} From e590a0c30af82957a1259aa28b07b11fcf474dc4 Mon Sep 17 00:00:00 2001 From: lcawl Date: Mon, 6 Apr 2026 07:21:29 -0700 Subject: [PATCH 2/2] Address CodeRabbit comments --- config/changelog.example.yml | 6 ++-- .../Bundling/ChangelogBundleAmendService.cs | 2 +- .../Bundling/ChangelogBundlingService.cs | 2 +- .../Bundling/LinkAllowlistSanitizer.cs | 11 ++++--- .../Changelogs/LinkAllowlistSanitizerTests.cs | 32 +++++++++++++++++++ 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/config/changelog.example.yml b/config/changelog.example.yml index 5fb9529a4..b09d71b2e 100644 --- a/config/changelog.example.yml +++ b/config/changelog.example.yml @@ -216,9 +216,9 @@ bundle: # in bundle output; others are rewritten to '# PRIVATE:' sentinels (requires resolve: true). # When omitted, no link filtering is applied. # Add your repository and any others whose PR/issue links should appear in published docs. - link_allow_repos: - - elastic/elasticsearch - - elastic/kibana + # link_allow_repos: + # - elastic/elasticsearch + # - elastic/kibana # Optional: default GitHub repo name applied to all profiles that do not specify their own. # Used by the {changelog} directive to generate correct PR/issue links when the product ID # differs from the GitHub repository name. Can be overridden per profile. diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs index 809ce81f5..a744c8a22 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs @@ -231,7 +231,7 @@ public async Task AmendBundle(IDiagnosticsCollector collector, AmendBundle return false; bundleForWrite = sanitized; - if (configurationContext != null) + if (configurationContext != null && linkAllowRepos!.Count > 0) { try { diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs index c5807409f..c69b18c02 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs @@ -293,7 +293,7 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle return false; bundleData = sanitizedBundle; - if (configurationContext != null) + if (configurationContext != null && input.LinkAllowRepos.Count > 0) { try { diff --git a/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs b/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs index e1f4bb4fd..7d72d16ca 100644 --- a/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs +++ b/src/services/Elastic.Changelog/Bundling/LinkAllowlistSanitizer.cs @@ -47,7 +47,7 @@ public static bool TryApplyBundle( allow, "PR", ref anyRewritten); - if (prs == null) + if (prs == null && entry.Prs is not null) return false; var issues = ApplyToReferenceList( @@ -58,7 +58,7 @@ public static bool TryApplyBundle( allow, "issue", ref anyRewritten); - if (issues == null) + if (issues == null && entry.Issues is not null) return false; newEntries.Add(entry with { Prs = prs, Issues = issues }); @@ -130,8 +130,11 @@ private static HashSet BuildAllowSet(IReadOnlyList allowRepos) string referenceKind, ref bool anyRewritten) { - if (refs is null || refs.Count == 0) - return refs ?? []; + if (refs is null) + return null; + + if (refs.Count == 0) + return refs; var list = new List(refs.Count); foreach (var r in refs) diff --git a/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs index 550b3421e..51bf684d1 100644 --- a/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs +++ b/tests/Elastic.Changelog.Tests/Changelogs/LinkAllowlistSanitizerTests.cs @@ -135,6 +135,38 @@ public void TryApplyBundle_AllowedRepo_KeepsUrl() Collector.Errors.Should().Be(0); } + [Fact] + public void TryApplyBundle_NullPrsAndIssues_PreservesNull_WhenUnchanged() + { + var bundle = new Bundle + { + Entries = + [ + new() + { + Title = "t", + Prs = null, + Issues = null + } + ] + }; + + var allow = new[] { "elastic/elasticsearch" }; + var ok = LinkAllowlistSanitizer.TryApplyBundle( + Collector, + bundle, + allow, + "elastic", + "elasticsearch", + out var sanitized, + out var changed); + + ok.Should().BeTrue(); + changed.Should().BeFalse(); + sanitized.Entries[0].Prs.Should().BeNull(); + sanitized.Entries[0].Issues.Should().BeNull(); + } + [Fact] public void TryApplyBundle_NotAllowed_ReplacesWithSentinel() {