Conversation
Mpdreamz
left a comment
There was a problem hiding this comment.
Security & logic review
Compared against the patterns established in changelog-validate.yml, changelog-submit.yml, changelog/submit/action.yml, and docs-deploy.yml.
Seven issues found — two are high severity and block merge; the rest are medium/low or best-practice gaps.
🔴 High
1. Missing top-level permissions: {} on the reusable workflow — .github/workflows/changelog-bundle.yml
Both sibling workflows (changelog-validate.yml:11, changelog-submit.yml:15) open with permissions: {} to deny all permissions at workflow scope, then grant only what is needed per job. This file has no top-level permissions block, so it inherits the repository default token permissions (typically includes contents: write, packages: read, etc.). The generate job silently runs with write access it does not need.
Fix: add permissions: {} between the on: block and jobs:.
2. Missing concurrency block — .github/workflows/changelog-bundle.yml
Both changelog-validate.yml (line 13) and changelog-submit.yml (lines 17–19) define concurrency groups. Without one here, two simultaneous calls for the same output path will race: both generate jobs upload an artifact named changelog-bundle (last write wins), and both create-pr jobs will race on the same branch with a force-push.
Fix: add something like:
concurrency:
group: changelog-bundle-${{ inputs.output }}-${{ github.run_id }}
cancel-in-progress: false🟡 Medium
3. Force-push without branch-name validation — changelog/bundle-pr/action.yml lines 39–58
BUNDLE_NAME=$(basename "$OUTPUT" .yaml) is then used directly as a branch name component and in a git push -f. basename strips the directory prefix but does not validate characters. Compare changelog/submit/action.yml lines 66–77, which has an explicit ref-name guard: ^[a-zA-Z0-9._/+-]+$.
The force-push (-f) is also destructive and inconsistent with the pattern in changelog/submit/action.yml which uses a plain git push. If a team member has pushed additional commits to the bundle branch, a force-push will silently discard them.
Fix: validate $BUNDLE_NAME against ^[a-zA-Z0-9._+-]+$ before constructing $BRANCH; replace -f with a regular push (or justify explicitly in a comment).
4. Unvalidated $OUTPUT used as cp destination — changelog/bundle-pr/action.yml lines 42–43
mkdir -p "$(dirname "$OUTPUT")"
cp ".bundle-artifact/$(basename "$OUTPUT")" "$OUTPUT"$OUTPUT is a caller-controlled string used directly as the copy destination. The action description says "Must match the path used by bundle-create" but there is no enforcement. A value containing ../ sequences or an absolute path outside the workspace would be silently honoured.
Fix: validate that $OUTPUT is a relative path with no .. components and that it ends in .yml or .yaml (as the docs promise but the code does not check).
5. Unvalidated URL passed to curl — changelog/bundle-create/action.yml lines 78–80
curl -fsSL "$REPORT" -o .bundle-report.html$REPORT is a caller-controlled URL fetched on the runner before --network none is applied to the Docker container. A misconfigured (or malicious) caller could pass an internal endpoint (e.g. the EC2 instance metadata service at http://169.254.169.254/...) and the runner would fetch it. The downloaded content then becomes input to the Docker container.
The attack surface is limited to workflow_call authors in the same org, but the risk should be documented or mitigated (e.g. allowlist known Buildkite URL prefixes).
🟠 Logic
6. No --base branch on gh pr create — changelog/bundle-pr/action.yml line 65–68
The PR is created without --base, so it targets the repo default branch at call time. If the workflow is ever used to produce bundles for a non-default release branch, this will silently target the wrong base. Making it explicit is a one-word fix and matches the defensive posture of the rest of the repo.
Fix: add --base "$GITHUB_REF_NAME" or a dedicated base-branch input.
7. Hardcoded artifact name changelog-bundle — changelog/bundle-create/action.yml line 91 / changelog/bundle-pr/action.yml line 29
If a consuming workflow ever calls bundle-create twice in parallel (e.g. matrix), the second upload will overwrite the first and bundle-pr will silently process the wrong file. Consider parameterising the artifact name or including the output basename.
Summary
| File | Line(s) | Severity | Finding |
|---|---|---|---|
changelog-bundle.yml |
after line 30 | 🔴 | Missing permissions: {} |
changelog-bundle.yml |
after line 30 | 🔴 | Missing concurrency block |
bundle-pr/action.yml |
39–58 | 🟡 | Force-push + no branch-name validation |
bundle-pr/action.yml |
42–43 | 🟡 | Unvalidated $OUTPUT as cp destination |
bundle-create/action.yml |
78–80 | 🟡 | Unvalidated URL to curl (SSRF potential) |
bundle-pr/action.yml |
65–68 | 🟠 | gh pr create missing --base |
bundle-create/action.yml |
91 | 🟠 | Hardcoded artifact name risks collision |
There was a problem hiding this comment.
Follow-up review focusing on generate-bundle.js — the workflow and bundle-pr action look good now. Five findings remain, three of which I'd like addressed before merge.
| # | Line(s) | Severity | Finding |
|---|---|---|---|
| 1 | 144 | 🟡 Medium | Profile mode missing --config flag |
| 2 | 161-163 | 🟡 Medium | Redundant loadConfig call |
| 3 | 165-169 | 🟡 Medium | Network isolation gap when report is HTTPS URL |
| 4 | 49-65 | 🟢 Low | No timeout on httpsGet |
| 5 | 54 | 🟢 Low | httpsGet follows redirects to any HTTPS host |
Items 1-3 have inline suggestions you can accept directly. Items 4-5 are low severity notes for consideration.
EDIT: I really think we should move away from the javascript all together see: #42 (comment)
| } | ||
|
|
||
| const dockerArgs = ['--rm', '-v', `${process.cwd()}:/github/workspace`, '-w', '/github/workspace']; | ||
| const bundleArgs = ['changelog', 'bundle', profile]; |
There was a problem hiding this comment.
Missing --config flag in profile mode.
In buildOptionMode() (line 78) the config path is passed through to the Docker container:
const bundleArgs = ['changelog', 'bundle', '--config', config, '--resolve'];Here in profile mode it's omitted. The JS script reads the correct config via loadConfig(config) for validation, but the actual docs-builder invocation inside Docker won't receive the custom config path. If a consumer sets config: custom/path/changelog.yml, profile mode will silently use the tool's default.
| const bundleArgs = ['changelog', 'bundle', profile]; | |
| const bundleArgs = ['changelog', 'bundle', '--config', config, profile]; |
| const cfg2 = loadConfig(config); | ||
| const profileConfig = cfg2?.bundle?.profiles?.[profile]; | ||
| const needsNetwork = profileConfig?.source === 'github_release'; |
There was a problem hiding this comment.
Redundant loadConfig call.
loadConfig(config) is already called on line 126 as cfg. This second call re-reads and re-parses the same file. Reuse cfg instead.
Note: if the first load returns null (file doesn't exist), the validation block on line 127 is skipped entirely. The code then continues here, and the second load also returns null — needsNetwork silently defaults to false. Reusing cfg makes this data flow explicit.
| const cfg2 = loadConfig(config); | |
| const profileConfig = cfg2?.bundle?.profiles?.[profile]; | |
| const needsNetwork = profileConfig?.source === 'github_release'; | |
| const profileConfig = cfg?.bundle?.profiles?.[profile]; | |
| const needsNetwork = profileConfig?.source === 'github_release'; |
| if (needsNetwork) { | ||
| dockerArgs.push('-e', 'GITHUB_TOKEN'); | ||
| } else if (!report || !report.startsWith('https://')) { | ||
| dockerArgs.push('--network', 'none'); | ||
| } |
There was a problem hiding this comment.
Network isolation gap in profile mode when report is an HTTPS URL.
When needsNetwork is false AND report is an HTTPS URL, neither branch executes — the container runs with full network access even though the report was already downloaded to .bundle-report.html on lines 150-153.
Compare with buildOptionMode() line 100, which correctly adds --network none unconditionally after handling the report download.
Since the download has already happened by this point, --network none should always apply when needsNetwork is false:
| if (needsNetwork) { | |
| dockerArgs.push('-e', 'GITHUB_TOKEN'); | |
| } else if (!report || !report.startsWith('https://')) { | |
| dockerArgs.push('--network', 'none'); | |
| } | |
| if (needsNetwork) { | |
| dockerArgs.push('-e', 'GITHUB_TOKEN'); | |
| } else { | |
| dockerArgs.push('--network', 'none'); | |
| } |
| function httpsGet(url, dest, maxRedirects = 5) { | ||
| return new Promise((resolve, reject) => { | ||
| if (maxRedirects <= 0) return reject(new Error('Too many redirects')); | ||
| https.get(url, (res) => { |
There was a problem hiding this comment.
Low: No timeout on HTTP requests.
httpsGet has no socket or response timeout. A slow or hanging Buildkite report server could stall the action until GitHub's job-level timeout (6 hours by default). Consider adding req.setTimeout(30000, () => req.destroy(...)) after the https.get call.
Also: res.headers.location on line 54 is followed without checking the target host. An HTTPS redirect to a cloud metadata endpoint would be followed. The risk is limited (callers are workflow_call authors in the same org, and https.get won't follow http:// redirects), but an allowlist on redirect domains would harden this.
| @@ -0,0 +1,189 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
I'm not keen on having this much JavaScript logic embedded in the action. generate-bundle.js reimplements config loading, profile validation, mutual exclusivity rules, path validation, and network-requirement detection — all of which docs-builder already owns in ChangelogCommand.cs. That's ~190 lines of JS (plus a js-yaml dependency) duplicating logic that has unit/integration test coverage on the docs-builder side.
The risk is that config schema changes or new profile options require updating two codebases in lockstep, and the JS side has no tests to catch regressions.
I think we should add a --plan (or --dry-run) mode to docs-builder changelog bundle that outputs structured JSON describing what the action needs to know:
{
"needs_network": true,
"needs_github_token": true,
"output_path": "docs/releases/v9.2.0.yaml",
"report_url": "https://...",
"bundle_args": ["changelog", "bundle", "--config", "docs/changelog.yml", "elasticsearch-release", "9.2.0"]
}Then the action becomes a thin shell script: run the plan, optionally download the report URL, construct docker args from the JSON, execute. No config parsing, no profile validation, no js-yaml, no discover-output.js.
This should be addressed before merging — I'd rather not ship this JS as-is and accumulate tech debt around it.
Mpdreamz
left a comment
There was a problem hiding this comment.
Review: request changes (directional)
Thanks for the solid groundwork here—--plan, Docker hardening, and splitting bundle-create / bundle-pr are all moving in a good direction.
I think we need to switch gears on the default story this PR optimizes for. Right now changelog-bundle.yml always runs bundle-create and then create-pr, which implicitly treats checking the bundle into git as the main outcome (artifacts are intermediate). For our release docs pipeline, the primary goal is to get the resolved bundle to S3; whether a repo commits a bundle file is a repo-owner choice, and if they want it in-tree it generally needs to land before tagging / publishing a GitHub release—not as a follow-up PR after the fact.
Suggested split:
-
Primary shared workflow (tag / GitHub Release): Trigger on tag and/or
release. The workflow should derive the version from the tag/release, and the caller should only need to supply the bundle profile (see profile argument types). Forrelease, we should drive the bundle from the GitHub release usingchangelog-gh-release(docs) instead of asking humans to thread version strings everywhere. Upload to S3 belongs here (likely alongside or after PR 43). -
Optional
bundle-prpath: Keep a maintainer-oriented flow (e.g.workflow_dispatch) for teams that use feature-freeze branches and want a PR with the bundle before a tag exists. That flow can stay narrow: profile + version (version is inherently manual until the release exists).
None of this diminishes the implementation work already in the branch—it is mostly re-centering the reusable workflow around release/tag + S3 and demoting the PR workflow to an explicit opt-in.
Happy to sync on the exact trigger matrix (tags vs published releases only) if useful.
|
Follow-up context (same thread as the directional review above):
|
8a22206 to
cf1de3d
Compare
Mpdreamz
left a comment
There was a problem hiding this comment.
LGTM (Actions design)
This addresses the directional feedback: S3-first path via changelog-bundle.yml (generate → bundle-upload) and an opt-in changelog-bundle-pr.yml for teams that want a committed bundle before a tag. README positions on: release + profile/version as the recommended story, which matches how we expect stack/product releases to run.
Workflow hygiene looks good: permissions: {}, scoped job permissions, concurrency with cancel-in-progress: false, Docker-based bundle-create (no ad-hoc generate-bundle.js in this tree), and sensible validation in bundle-pr / bundle-upload (paths, artifact names, persist-credentials: false where appropriate, --force-with-lease on push).
Prerequisite for end-to-end S3 behavior
The upload job runs docs-builder changelog upload --artifact-type bundle. That path must be implemented in the docs-builder version consumers run — tracked in elastic/docs-builder#3054 (still open at review time). Until that lands in a release (or you pin to a build that includes it), the job may succeed without actually uploading bundle objects.
Residual (docs-builder / infra, not blocking this PR)
- Symlink / path containment on upload (same theme as changelog fragment uploads).
- Promotion-report HTTPS fetching remains docs-builder’s responsibility after SSRF allowlisting was reverted in the Action.
Fine to merge docs-actions once you are comfortable the docs-builder side is aligned for your consumers (or document a temporary pin).
Depends on: elastic/docs-builder#3054
This pull request introduces two new reusable GitHub Actions workflows for changelog bundling and documents their usage. The main additions are workflows for generating changelog bundles (with optional S3 upload) and for opening pull requests with bundle files, along with comprehensive documentation in the repository. These changes are aimed at standardizing and simplifying the process of generating and distributing changelog bundles across repositories.
New reusable workflows for changelog bundling:
.github/workflows/changelog-bundle.yml, a reusable workflow that generates a changelog bundle (via various filtering modes) and uploads it to S3. It supports both profile-based and option-based filtering, as well as a "gh-release" mode for generating bundles from GitHub release notes..github/workflows/changelog-bundle-pr.yml, a reusable workflow that generates a changelog bundle and opens a pull request with the bundle file. This is intended for teams needing a committed bundle before a release tag exists.Documentation and usage guides:
changelog/README.mdto document the new bundling workflows, including setup instructions, usage examples for different modes, prerequisites, and notes on output and permissions.changelog/bundle-create/README.mdto document the inputs, outputs, and usage of thebundle-createcomposite action, covering all supported modes and options.