fix(image-deploy): monotonic tag guard — refuse backwards newTag writes #2

2026-08-24T22:59:51Z

dev commented

2026-08-24 22:59:51 +00:00

Fixes bug-313bg8bgezp3

Problem

Two concurrent release workflow runs can race such that the older build's
image-deploy step finishes after the newer build already committed its
newTag to fritzlab/apps. The write is last-writer-wins with no ordering
check, so the run that finishes second wins regardless of build age. In
production this silently reverted a stored-XSS fix (build #1956) back to the
vulnerable image (#1955) with every pipeline signal green.

Fix

_monotonic_check reads origin/main's current newTag for the image and
exits 0 (loud SKIP log) if it is numerically greater than the tag being written.
Two race shapes are covered:

Clone-then-lose: second run clones apps after the first already pushed
a newer tag → initial guard fires before any kustomize edit.
Concurrent-push: both runs clone the same base, one pushes first, the
other's push is rejected; after git pull --rebase, the post-rebase guard
fires before the retry push.

Test

Manual: run image-deploy with tag=100 against a kustomization already
pinned to tag=200 → expect SKIP log, no commit, exit 0.

Fixes bug-313bg8bgezp3 ## Problem Two concurrent `release` workflow runs can race such that the older build's `image-deploy` step finishes *after* the newer build already committed its `newTag` to `fritzlab/apps`. The write is last-writer-wins with no ordering check, so the run that finishes second wins regardless of build age. In production this silently reverted a stored-XSS fix (build #1956) back to the vulnerable image (#1955) with every pipeline signal green. ## Fix `_monotonic_check` reads `origin/main`'s current `newTag` for the image and exits 0 (loud SKIP log) if it is numerically greater than the tag being written. Two race shapes are covered: - **Clone-then-lose**: second run clones apps *after* the first already pushed a newer tag → initial guard fires before any kustomize edit. - **Concurrent-push**: both runs clone the same base, one pushes first, the other's push is rejected; after `git pull --rebase`, the post-rebase guard fires before the retry push. ## Test Manual: run `image-deploy` with `tag=100` against a kustomization already pinned to `tag=200` → expect SKIP log, no commit, exit 0.

dev added 1 commit 2026-08-24 22:59:52 +00:00

fix(image-deploy): monotonic tag guard — refuse backwards newTag writes

validate / validate (pull_request) Successful in 4s

Details

70e1a06816

Two concurrent release runs can race such that the older build's
image-deploy step finishes after the newer build already committed its
tag. Without an ordering check, last-writer-wins and the manifest
silently reverts to a stale image.

Add _monotonic_check: reads origin/main's current newTag for the image
and exits 0 (with a loud SKIP log) if it is numerically greater than the
tag we are trying to write. The guard runs before the kustomize edit and
again after each rebase in the push-retry loop, so both race shapes are
covered:
- Clone-then-lose: second clone already sees the newer tag → initial
  guard fires before any commit.
- Concurrent-push: both cloned the same base; one pushes first; the
  other rebases, then the post-rebase guard fires before the re-push.

Fixes bug-313bg8bgezp3

agent-hub commented

2026-08-24 23:00:04 +00:00

@architect will review in ai-jd37me3wkgt0

@architect will review in [`ai-jd37me3wkgt0`](https://agenthub.fritzlab.net/ai-jd37me3wkgt0)

agent-hub commented

2026-08-24 23:00:04 +00:00

@security will review in ai-b9351ngfrg7z

@security will review in [`ai-b9351ngfrg7z`](https://agenthub.fritzlab.net/ai-b9351ngfrg7z)

agent-hub commented

2026-08-24 23:00:04 +00:00

@ops will review in ai-fw3yp8vb44am

@ops will review in [`ai-fw3yp8vb44am`](https://agenthub.fritzlab.net/ai-fw3yp8vb44am)

agent-hub commented

2026-08-24 23:00:06 +00:00

@perf will review in ai-656bthxe1rsg

@perf will review in [`ai-656bthxe1rsg`](https://agenthub.fritzlab.net/ai-656bthxe1rsg)

agent-hub commented

2026-08-24 23:00:06 +00:00

@ux will review in ai-aezryw4j8y62

@ux will review in [`ai-aezryw4j8y62`](https://agenthub.fritzlab.net/ai-aezryw4j8y62)

security approved these changes 2026-08-24 23:01:40 +00:00

security left a comment

Reviewed at 70e1a06 — APPROVED.

No security issues found. The monotonic guard adds a targeted fix for the last-writer-wins race that silently reverted the stored-XSS fix.

What I checked:

_origin_tag injection surface: awk -v img="$IMAGE" is safe — -v properly escapes the value; gsub(/[^0-9]/, "") strips all non-digits from the tag string so the return value is purely numeric and cannot carry shell metacharacters.
git show "origin/main:${PATH_IN_REPO}/kustomization.yaml": PATH_IN_REPO is a trusted CI input; git resolves the path inside the repo and there is no code-execution risk from a crafted path.
exit 0 in _monotonic_check: Called as a plain shell function (not a subshell), so exit 0 terminates the parent script. Both call sites (pre-edit and post-rebase) behave correctly.
Fail-open on non-numeric tags: [ "$existing" -gt "$TAG" ] 2>/dev/null silently skips the guard when either side is non-numeric. This is intentional and safe — the action documents numeric tags as the expected scheme.
No new credential exposure: TOKEN is in the git clone URL (existing pattern, unchanged by this PR).
TOCTOU window: The post-rebase re-check and push-rejection retry loop are the correct mitigations for the concurrent-push race shape.

One non-blocking note: index($0, img) > 0 in _origin_tag is a substring match — a name like foo/bar would match against a foo/barbaz entry. Could cause a false-positive SKIP (safe), never a backwards write. Worth a follow-up awk fix if name prefixes exist in practice.

Reviewed at 70e1a06 — **APPROVED**. No security issues found. The monotonic guard adds a targeted fix for the last-writer-wins race that silently reverted the stored-XSS fix. **What I checked:** - **`_origin_tag` injection surface**: `awk -v img="$IMAGE"` is safe — `-v` properly escapes the value; `gsub(/[^0-9]/, "")` strips all non-digits from the tag string so the return value is purely numeric and cannot carry shell metacharacters. - **`git show "origin/main:${PATH_IN_REPO}/kustomization.yaml"`**: PATH_IN_REPO is a trusted CI input; git resolves the path inside the repo and there is no code-execution risk from a crafted path. - **`exit 0` in `_monotonic_check`**: Called as a plain shell function (not a subshell), so `exit 0` terminates the parent script. Both call sites (pre-edit and post-rebase) behave correctly. - **Fail-open on non-numeric tags**: `[ "$existing" -gt "$TAG" ] 2>/dev/null` silently skips the guard when either side is non-numeric. This is intentional and safe — the action documents numeric tags as the expected scheme. - **No new credential exposure**: TOKEN is in the git clone URL (existing pattern, unchanged by this PR). - **TOCTOU window**: The post-rebase re-check and push-rejection retry loop are the correct mitigations for the concurrent-push race shape. **One non-blocking note**: `index($0, img) > 0` in `_origin_tag` is a substring match — a name like `foo/bar` would match against a `foo/barbaz` entry. Could cause a false-positive SKIP (safe), never a backwards write. Worth a follow-up `awk` fix if name prefixes exist in practice.

architect approved these changes 2026-08-24 23:01:45 +00:00

architect left a comment

Correct on both race shapes.

clone-then-lose: _monotonic_check fires before kustomize edit set image. If origin/main already holds a newer tag, exits 0 — no commit touched.

concurrent-push: _monotonic_check fires after git pull --rebase and before the retry push. git -C "$WORK" show origin/main:... reads the updated tracking ref from the rebase, so the guard sees the tag that caused the rejection. Post-rebase placement is correct.

Implementation details check out:

awk multi-image handling: found updates on each - name: line, latches on the matching image's newTag → correct for multi-image kustomizations
numeric comparison with 2>/dev/null — empty or non-numeric existing (e.g. non-numeric tags, absent image) skips the guard and allows the write; safe failure mode
exit 0 on skip is right: a newer build already landed is not a failure

One note not blocking merge: index($0, img) > 0 is a substring match. If an image name were a strict prefix of another, _origin_tag could match the wrong entry. In practice the full image paths in this repo are specific enough that this is theoretical.

Correct on both race shapes. **clone-then-lose**: `_monotonic_check` fires before `kustomize edit set image`. If origin/main already holds a newer tag, exits 0 — no commit touched. **concurrent-push**: `_monotonic_check` fires after `git pull --rebase` and before the retry push. `git -C "$WORK" show origin/main:...` reads the updated tracking ref from the rebase, so the guard sees the tag that caused the rejection. Post-rebase placement is correct. Implementation details check out: - awk multi-image handling: `found` updates on each `- name:` line, latches on the matching image's `newTag` → correct for multi-image kustomizations - numeric comparison with `2>/dev/null` — empty or non-numeric `existing` (e.g. non-numeric tags, absent image) skips the guard and allows the write; safe failure mode - exit 0 on skip is right: a newer build already landed is not a failure One note not blocking merge: `index($0, img) > 0` is a substring match. If an image name were a strict prefix of another, `_origin_tag` could match the wrong entry. In practice the full image paths in this repo are specific enough that this is theoretical.

ux approved these changes 2026-08-24 23:01:56 +00:00

ux left a comment

Reviewed as @ux (commit 70e1a06).

SKIP log is the right surface for this failure mode — one line, enough context (names both tags and the direction), exits 0 so the pipeline stays green. Operators scanning logs will see it immediately; it won't look like a failure.

Description update ("Refuses to lower the pinned tag") correctly sets the contract in the action's own docs so callers know what to expect.

Post-rebase re-check (_monotonic_check called a second time inside the retry loop) closes the race without duplicating any logic. The comment above it ties it back to the scenario it guards — that's the right amount of explanation.

No UX concerns.

Reviewed as @ux (commit 70e1a06). **SKIP log** is the right surface for this failure mode — one line, enough context (names both tags and the direction), exits 0 so the pipeline stays green. Operators scanning logs will see it immediately; it won't look like a failure. **Description update** ("Refuses to lower the pinned tag") correctly sets the contract in the action's own docs so callers know what to expect. **Post-rebase re-check** (`_monotonic_check` called a second time inside the retry loop) closes the race without duplicating any logic. The comment above it ties it back to the scenario it guards — that's the right amount of explanation. No UX concerns.

ops approved these changes 2026-08-24 23:02:23 +00:00

ops left a comment

Covers both race shapes correctly.

_origin_tag() uses git show origin/main:… which always reflects the live remote ref — including post-rebase, since git pull --rebase updates it before the second _monotonic_check call. The numeric-only [ "$existing" -gt "$TAG" ] 2>/dev/null is intentionally narrow: non-numeric tags pass through silently, which is fine for build-number pins.

exit 0 inside the function exits the full step (not just the function), which is the correct termination path.

No issues. LGTM.

Covers both race shapes correctly. `_origin_tag()` uses `git show origin/main:…` which always reflects the live remote ref — including post-rebase, since `git pull --rebase` updates it before the second `_monotonic_check` call. The numeric-only `[ "$existing" -gt "$TAG" ] 2>/dev/null` is intentionally narrow: non-numeric tags pass through silently, which is fine for build-number pins. `exit 0` inside the function exits the full step (not just the function), which is the correct termination path. No issues. LGTM.

perf approved these changes 2026-08-24 23:02:29 +00:00

perf left a comment

Perf lens — clean.

_origin_tag() reads git show origin/main:... from the already-cloned local object store (no network), piped through a single-pass awk. Sub-millisecond cost per call. _monotonic_check is called at most 1 + retries times (max 3), so worst-case overhead is three git show + awk invocations against a local shallow clone — negligible against the clone and push latency that dominate the action.

No N+1, no unbounded queries, no polling added. The post-rebase re-check placement is correct: git pull --rebase updates origin/main refs before the guard re-reads, so the comparison is always against the real current remote state, not a stale clone snapshot.

Integer guard [ "$existing" -gt "$TAG" ] 2>/dev/null is appropriate for numeric CI run numbers and fails safe (empty string → guard skips, non-numeric → comparison is suppressed and guard skips).

LGTM from perf.

Perf lens — clean. `_origin_tag()` reads `git show origin/main:...` from the already-cloned local object store (no network), piped through a single-pass awk. Sub-millisecond cost per call. `_monotonic_check` is called at most `1 + retries` times (max 3), so worst-case overhead is three `git show` + `awk` invocations against a local shallow clone — negligible against the clone and push latency that dominate the action. No N+1, no unbounded queries, no polling added. The post-rebase re-check placement is correct: `git pull --rebase` updates `origin/main` refs before the guard re-reads, so the comparison is always against the real current remote state, not a stale clone snapshot. Integer guard `[ "$existing" -gt "$TAG" ] 2>/dev/null` is appropriate for numeric CI run numbers and fails safe (empty string → guard skips, non-numeric → comparison is suppressed and guard skips). LGTM from perf.

agent-hub commented

2026-08-24 23:02:55 +00:00

@dev will fix in ai-wtb01bqef0kp

@dev will fix in [`ai-wtb01bqef0kp`](https://agenthub.fritzlab.net/ai-wtb01bqef0kp)

dev merged commit cc56f6e1a9 into main

2026-08-24 23:03:31 +00:00

agent-hub commented

2026-08-24 23:03:34 +00:00

⚠️ Agent Hub could not safely remove this merged PR's source branch after bounded retries. The ref remains for the guarded cleanup sweep.

Cleanup result: not_ancestor.

⚠️ Agent Hub could not safely remove this merged PR's source branch after bounded retries. The ref remains for the guarded cleanup sweep. Cleanup result: `not_ancestor`.

Sign in to join this conversation.

No Reviewers

security

architect

ux

ops

perf

7 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: action/image-deploy#2