Skip to content

Auto-rebuild & auto-merge theme-asset Dependabot PRs (+ verify check) - #1265

Open
CybotTM wants to merge 5 commits into
TYPO3-Documentation:mainfrom
CybotTM:feat/dependabot-asset-rebuild
Open

Auto-rebuild & auto-merge theme-asset Dependabot PRs (+ verify check)#1265
CybotTM wants to merge 5 commits into
TYPO3-Documentation:mainfrom
CybotTM:feat/dependabot-asset-rebuild

Conversation

@CybotTM

@CybotTM CybotTM commented May 24, 2026

Copy link
Copy Markdown
Contributor

Problem

packages/typo3-docs-theme/ commits its built frontend assets (assets/js/, resources/public/). They drift out of sync when a contributor forgets to rebuild, or when Dependabot bumps a build dependency — Dependabot can't rebuild the assets (this is what made the manual follow-up #1264 necessary).

Changes

  1. check-built-assets.yaml — required check verify: on every PR, rebuilds the theme and fails if the committed assets don't match a fresh build. PRs not touching the theme short-circuit to success, so the check can be required without blocking unrelated PRs. Also runs on push to main and in the merge queue.
  2. dependabot-rebuild-assets.yaml — on a Dependabot theme bump: rebuilds the assets, commits them back as a GitHub-signed (Verified) commit via a GitHub App, then enables auto-merge for patch/minor updates.
  3. pr-auto-merge.yaml — excludes theme PRs from the generic Dependabot auto-merge; the workflow above handles them after the rebuild commit lands.
  4. Docs pointer in Documentation/Developer/ThemeCustomization.rst.

Required repository settings

  • GitHub App with Contents: write + Pull requests: write, exposed as secrets APP_ID / APP_PRIVATE_KEY (same names as reusable-backport.yml).
  • Allow auto-merge enabled.
  • Branch protection requiring the verify check.

Security

dependabot-rebuild-assets.yaml uses pull_request_target (same convention as pr-auto-merge.yaml). Guards: actor gated to dependabot[bot]; checkout pinned to the PR head SHA; lockfile-only diff guard; npm ci --ignore-scripts; read-only GITHUB_TOKEN; App token minted only after the build; commit created via createCommitOnBranch, so it is Verified and re-triggers verify.

Residual risk (accepted): patch/minor theme bumps merge without a human in the loop, so a compromised dependency's build-time code runs and its output merges. For a manual checkpoint, drop the "Enable auto-merge" step.

Validation

Proven end-to-end on a fork: non-theme PR → verify skips green; stale assets → verify blocks; Dependabot bump → Verified rebuild commit → verify re-runs → auto-merge with correct assets on main.

@garvinhicking

Copy link
Copy Markdown
Contributor

So, "allow auto merge" is enabled. The App secrets should exist. I'm not sure what you mean with "verify status" though.

The only "verify" I can find under branch protection is the one here:

Screenshot 2026-05-24 at 20 14 50

and I don't think we can require that...

@CybotTM

CybotTM commented May 24, 2026

Copy link
Copy Markdown
Contributor Author

@garvinhicking

"verify" status check means the "verify" job:

https://github.com/TYPO3-Documentation/render-guides/pull/1265/changes#diff-31e507d3ed5388405d0382e860458b4c02af7444aa1084a8aed89f3c5da70d36R22

It must be required to ensure the PR does not get (auto-)merged when it fails.

image

You may only be able to set it after merge.

And regarding the "signed commits", IMO they should be required along with signed-off commits.
git commit --gpg-sign --signoff ... (git commit -s -S ...) is not that hard.

You can easily enable it for all commits:

git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign true

after registering your ssh key in GitHub → SSH and GPG keys → Signing keys
and your commit email git config user.email must match one of your verified emails in GitHub.

@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from c0de53a to 35f1f45 Compare May 25, 2026 06:53
@CybotTM

CybotTM commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

I will check failing "Main / Validate monorepo structure (pull_request)Failing after 36s"

@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from 35f1f45 to 396fdcf Compare May 25, 2026 07:08
@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch 3 times, most recently from 1d089a0 to 482c8fb Compare June 24, 2026 15:31
@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from 482c8fb to e3f71cb Compare July 1, 2026 12:01
linawolf pushed a commit that referenced this pull request Jul 25, 2026
Adds a `github-actions` package ecosystem to `dependabot.yml`, using the
same schedule and conventions as the existing composer and npm entries
(daily 16:00 Europe/Berlin, `[TASK]` prefix, `dependencies` + ecosystem
label — the `github-actions` label has been created).

## Why

Action versions in the workflows have only ever been updated by hand —
`dependabot.yml` has covered composer and npm exclusively since it was
created in #236, and the pins were last refreshed manually (#1194, then
the #1184#1328/#1329 split). With this entry, Dependabot takes that
over: it bumps SHA pins **and keeps the trailing `# vX.Y.Z` comments in
sync** (see e.g. the actions bumps in TYPO3-Documentation/.github#19,
same org).

Minor/patch action bumps then flow through the existing
auto-approve/auto-merge workflows like composer/npm updates already do;
majors wait for review.

## Coordination

- Trivial textual conflict with #1265, which also edits `dependabot.yml`
(composer `ignore` key fix) — whichever lands second rebases.
- Best merged after #1328/#1329 so Dependabot starts from current,
annotated pins rather than re-proposing the same bumps.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from e3f71cb to f8a1fcd Compare July 25, 2026 13:19
@CybotTM

CybotTM commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and extended.

The [BUGFIX] Use the valid 'ignore' key in dependabot.yml commit dropped out of the branch — #1346 has since landed the same ignore block on main, so .github/dependabot.yml is now identical to it and this PR no longer touches that file.

Two gaps the branch had, both from things that happened after it was opened in May:

Advisory merges bypassed it. Fixes from a temporary private fork arrive on main as direct commits, never as a pull request here, so a pull_request-only trigger cannot see them. That is not hypothetical — it is how 0.40.0 came to ship theme assets built eleven days before the fixes it was tagged for (#1353).

The merge queue would have stalled. main gained a merge queue in #1326. A required status check without a merge_group trigger never reports on a queued merge, so every queued merge times out — the same failure mode this file's own header warns about for paths filters.

Both triggers are added. The base sha now comes out of the event payload via jq instead of being interpolated into the script, and an event that carries no base sha — a push — is verified in full rather than skipped.

Checked locally: actionlint clean, actionlint -shellcheck clean, yamllint reports the same 11 findings as before the change. The base-sha extraction was exercised against synthetic pull_request, merge_group and push payloads, which resolve to the PR base, the merge-group base, and full verification respectively.

linawolf pushed a commit that referenced this pull request Jul 26, 2026
The three advisory fixes are merged and tagged in 0.40.0, but the
published assets do not contain them. `assets/js/search.js` and
`resources/public/js/theme.min.js` are committed build artifacts and
were never rebuilt. Re-rendering a manual does not help — the theme
version is pinned in the asset URL, so a re-render fetches the same
vulnerable file.

This is why
https://docs.typo3.org/m/typo3/tutorial-getting-started/13.4/en-us/?q=%3Cimg%20src=x%20onerror=alert(document.domain)%3E
still fires after a fresh render.

## Evidence

The CDN serves exactly the artifact committed at tag 0.40.0:

```
CDN 0.40.0/js/theme.min.js       md5 67ee48dc0483752ddbcbe68b64639efb   239212 bytes
repo @ 0.40.0, theme.min.js      md5 67ee48dc0483752ddbcbe68b64639efb   239212 bytes
```

That artifact was last changed in f795385 on 14 July — eleven days
before ab24ff7, db881fd and a22f55b.

Markers that survive minification, old versus rebuilt:

| marker | origin | search.js | theme.min.js |
|---|---|---|---|
| `MARK` | `sanitizeHtml.js`, a22f55b | 0 → 1 | 0 → 1 |
| `&#39;` | `attrEscape()`, ab24ff7 | — | 0 → 2 |
| `https:` | `safeUrl()` allowlist, ab24ff7 | — | 3 → 5 |

Executed, not just read: loading each bundle in jsdom against a real
rendered page and opening the info modal with `data-details="<img src=x
onerror=…>"`

- deployed bundle: `<img src="x" onerror="window.__xss = true">` lands
in the DOM as a live element, `[onerror]` present
- rebuilt bundle: rendered as inert text, no `img`, no `[onerror]`

The build is reproducible — two consecutive `npm run build` runs
produced identical hashes, and the committed files match both.

## Scope

Only the two JS artifacts. `npm run build` also regenerates
`resources/public/css/theme.css` and the FontAwesome woff2 files, which
have drifted since #1333; that drift is older than this incident and is
reverted here so the security release carries nothing else. It needs its
own PR.

## Verification

```
npm test                     5 files, 23 tests passed
```

No source file changes, so the diff is the artifacts only.

## After merge

0.40.1 needs to be tagged and the manuals re-rendered — the version is
part of the asset URL, so the release is what actually ships the fix.

## How this got through

There is no CI job that builds the assets or compares them to the
committed ones. #1265 adds exactly that check
(`.github/workflows/check-built-assets.yaml`) and has been open since 24
May with all checks green. It triggers on `pull_request` only, so as
written it would not have caught these three commits either — advisory
merges land as direct pushes to `main`, without a PR. Worth merging with
a `push` trigger on `main` added.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from 92e5fae to 3d91b7b Compare July 26, 2026 10:00
CybotTM and others added 5 commits September 8, 2026 17:33
packages/typo3-docs-theme/ commits generated frontend assets (Vite
output in assets/js/, Grunt output in resources/public/). If a change
edits the theme sources but does not regenerate those assets, stale
assets reach the rendered docs.

This adds a status-check workflow that rebuilds the theme on a pull
request and fails if the committed assets differ from a fresh build —
catching both modified tracked files and new untracked output, with a
single actionable annotation.

It runs on every pull request (no paths filter) and short-circuits to
success when the theme package is untouched, so it can safely be a
required status check: a path-filtered required check never reports on
unrelated PRs and would leave them un-mergeable.

Least privilege: empty top-level permissions, job-level contents:read;
harden-runner egress in block mode.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
A Dependabot bump of a theme dependency changes the build output but
Dependabot cannot regenerate the committed assets, so the bump alone
fails the check-built-assets gate. This adds a workflow that, on those
Dependabot PRs, rebuilds the assets, commits them back, and enables
auto-merge.

* pull_request_target + 'if: github.actor == dependabot[bot]' (matches
  the existing pr-auto-merge.yaml convention).
* PR head pinned by SHA; a lockfile-only diff guard; npm ci
  --ignore-scripts; harden-runner egress in block mode.
* The App token is minted AFTER the build so a compromised build
  dependency cannot read it; the GITHUB_TOKEN stays read-only.
* The rebuilt files are committed through the GitHub API
  (createCommitOnBranch) with the App token, not git push, so the
  commit is GitHub-signed (Verified) and re-triggers the asset check on
  the new commit. Requires a GitHub App (Contents + Pull requests:
  write) and APP_ID / APP_PRIVATE_KEY secrets.
* Auto-merge is enabled here, after the rebuild commit is in place, so
  pr-auto-merge.yaml is updated to skip PRs that update the theme
  package's dependencies (otherwise it squashes the bump before the
  rebuild lands).

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Point the theme-customization developer docs at the asset check and
the Dependabot auto-rebuild, and state the local rebuild command
contributors run before committing.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The check only triggered on pull_request. Two paths past it:

Security advisory fixes are merged out of a temporary private fork and
land on "main" as direct commits, never as a pull request in this repo,
so the gate could not see them. That is how 0.40.0 shipped assets built
before the fixes it was tagged for.

"main" gained a merge queue in TYPO3-Documentation#1326 after this branch was opened. A
required status check without a merge_group trigger never reports on a
queued merge, which times the merge out.

Add both triggers. The base sha now comes from the event payload, and an
event without one, a push, is verified in full rather than skipped.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Both new workflows opened with step-security/harden-runner. The
repository restricts Actions to an allowlist and harden-runner is not on
it, so the run ended in startup_failure before any step executed. This
never showed while the branch was conflicting, because a pull request
that cannot be merged produces no runs.

Remove the step from both workflows. Every remaining action matches the
allowlist. Getting harden-runner added to it would be the better fix and
is noted in the workflow header.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
@CybotTM
CybotTM force-pushed the feat/dependabot-asset-rebuild branch from 3d91b7b to c27d2dc Compare September 8, 2026 15:34
@linawolf

Copy link
Copy Markdown
Member

Thanks for this — I went through it properly, including reproducing the build
locally to test the premise. The design holds up: npm ci --ignore-scripts && npm run build on the committed lockfile gives me zero diff and zero
untracked files under both asset trees, so verify is enforceable as it
stands, not aspirational. The reasoning in the comments is also genuinely
useful to read — the "a required check must never be paths-skipped" point and
the push-trigger rationale for advisory merges are both correct and not
obvious.

Four things I'd like to resolve before this goes in.

1. The pr-auto-merge.yaml hunk predates the merge queue

This is the main one, and it's a timing artefact rather than a mistake:

Both July commits touch only check-built-assets.yaml and
dependabot-rebuild-assets.yaml. pr-auto-merge.yaml is still the May version,
and its rationale —

if auto-merge fires here at PR-open time it squashes BEFORE the rebuild
commit lands

— described the repo accurately in May, when --squash --auto really did merge
directly. Under a merge queue it no longer does: enabling auto-merge enqueues,
and the merge group re-runs the required checks, so the rebuild commit would
already be on the branch by the time anything merges.

Which raises the question I'd most like your read on: is the theme exclusion
still needed at all?
If the queue won't merge until verify passes on the
rebuilt commit, the generic path may handle theme PRs correctly on its own, and
this hunk plus the ::notice:: step could just be deleted. You designed it —
tell me if there's a race I'm not seeing.

Related: #1399 rewires the same file, because enqueuing with GITHUB_TOKEN
produces a merge group that Main never runs on (token-created events don't
trigger workflows), so entries sit until check_response_timeout drops them.
That's visible in the queue right now — five Dependabot PRs, approved, CLEAN,
"Dependabot auto-merge" green, unmerged for two weeks. Worth noting your
rebuild workflow already gets this right and enqueues with the App token;
it's only the generic path that still uses GITHUB_TOKEN.

Suggested order: land #1399 first, then rebase this onto it, revisiting the
exclusion rather than resolving the conflict mechanically.

Same era, same file family: gh pr merge --squash --auto in
dependabot-rebuild-assets.yaml — under a merge queue the method comes from the
queue rule and gh warns the flag is ignored. Dropping --squash matches what
#1399 does.

2. Minting the token after the build doesn't close the hole

The security note argues the App token is safe because it's minted after
npm run build, so untrusted build code can't read it. That closes the direct
read, but not the job.

A build step can append to $GITHUB_ENV or $GITHUB_PATH — ordinary writable
files whose contents GitHub applies to subsequent steps. NODE_OPTIONS=--require /tmp/x.js in $GITHUB_ENV is picked up by actions/create-github-app-token and
actions/github-script, both Node actions, both of which then hold a
Contents:write + PRs:write token. The final gh pr merge step exports GH_TOKEN
into a shell that a hijacked PATH can intercept.

The fix realises what the current ordering is already reaching for: split into
two jobs. Job A checks out, builds, uploads the asset trees as an artifact —
no secrets in scope at all. Job B downloads the artifact, mints the token,
commits. Untrusted code and the token then never share a runner.

I'd rather do that than accept it as residual risk. "Patch/minor theme bumps
merge without a human in the loop" is exactly the case where the runner should
be assumed hostile.

3. Node version: the new workflows disagree with the existing one

main.yaml (JS tests)            node-version-file: .nvmrc        -> 24.16.0 exactly
check-built-assets.yaml         node-version-file: package.json  -> engines ">=24.16.0 <25.0.0"
dependabot-rebuild-assets.yaml  node-version-file: package.json  -> same range

setup-node resolves the range to the newest 24.x — 24.18.0 today. For a check
whose whole job is byte-comparing build output, the toolchain should be pinned
to what a contributor gets from .nvmrc. Not breaking right now (I built on
24.18.0 and got a clean diff), but it's a latent flake source aimed straight at
this PR's purpose. Two-line fix.

Smaller notes

  • Three texts, three stories. The new ::notice:: says theme PRs "require a
    manual review/merge"; dependabot-rebuild-assets.yaml enables auto-merge for
    patch/minor; the docs say "no manual rebuild is required there." Worth
    reconciling, especially if item 1 removes the exclusion.
  • verify is a very generic required-check name, and main.yaml carries an
    explicit warning that job names are wired into the branch ruleset.
    check-built-assets would be safer to live with.
  • The untracked-file hard-fail may be fragile under the bumps it automates.
    vite.config.js uses chunkFileNames: 'js/[name].[hash].js' with
    emptyOutDir: false. Today the search bundle is one chunk, so nothing hashed
    is emitted. If a vite or react bump ever triggers code-splitting, the new
    hashed file lands untracked, "Prepare commit inputs" refuses to commit, and
    superseded hashed files would never be cleaned up. Speculative, but the
    mechanism is concrete and vite/react bumps are precisely the traffic here.
  • dependabot/fetch-metadata runs with only contents: read — worth confirming
    it doesn't want pull-requests: read. It would fail loudly, so low risk.

What I verified locally

  • npm ci --ignore-scripts + npm run build on node 24 -> clean git status;
    no modified or untracked files under assets/ or resources/public/.
  • --ignore-scripts doesn't break the build (rolldown and sass need no install
    scripts).
  • steps.metadata.outputs.directory is /packages/typo3-docs-theme/ for the npm
    ecosystem, so the contains(...) exclusion matches; composer and
    github-actions both use / and are unaffected.
  • main.yaml is the only workflow with a merge_group trigger today, so adding
    one to check-built-assets.yaml is right.

The asset-drift problem is real and this is the right shape of solution — I'd
like to get it in. Item 1 is the one I need your answer on; 2 and 3 I'm happy to
push to your branch if that's easier than round-tripping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants