Skip to content

fix: the remaining findings of the main audit - #41

Merged
alexey-igrychev merged 12 commits into
mainfrom
fix/audit-second-batch
Sep 18, 2026
Merged

alexey-igrychev merged 12 commits into
mainfrom
fix/audit-second-batch

Conversation

@alexey-igrychev

Copy link
Copy Markdown
Member

The rest of the audit of main, one commit per finding.

Two more findings filed under #29 as "worth a second look", both promoted because they change what gets deployed:

  • semver sorts v2.0.0-rc1 above v1.9.0, so a release candidate became the newest tag and was deployed as a release. Pre-releases are skipped unless repo.allowPrerelease is set.
  • a forced checkout leaves untracked files in place, so build artifacts and stray .env files of the previous release took part in the next deployment. The worktree is cleaned.

Behaviour changes for operators

  • repo.env no longer overrides the operator env — the README always documented the opposite.
  • Pre-release tags are not deployed unless repo.allowPrerelease: true.
  • The worktree is cleaned on checkout; gitignored files survive, as with git clean -fd.
  • An interrupted run is no longer recorded as a failed tag, so it is retried without --force.

Closes #18, closes #19, closes #20, closes #21, closes #23, closes #24, closes #25, closes #26, closes #27, closes #29.

Commands, hooks and env values were rendered with html/template, so the
values were HTML-escaped on their way to sh: the tag v1.0.0+build.3 became
v1.0.0+build.3, and & or quotes in RepoUrl were mangled the same way.
HTML escaping protects nothing in a shell context.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
GetLastSemverTag returned the hash of the reference without peeling it, so
{{ .RepoCommit }} was the hash of the tag object for annotated tags, which
is what signed releases use: commands pinning an image tag or reporting a
commit got a SHA that is not a commit at all. Checkout peeled the tag
already; both now share one helper.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
Viper lower-cases every key it reads. NewExecutor compensated, but the
repository-config path replaced the environment wholesale, so a repo
trx.yaml with env: {FOO: bar} reached the command as foo=bar, which werf
and kubectl ignore, and the operator variables lost their upper-casing
with it.

The rendering now lives in one helper used by both paths. mergeEnvs also
returns a new map instead of writing into the operator config, and the
operator config overrides the repository one as the README documents.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
name is optional and validation substitutes <unnamed>, but verification
dereferenced the pointer, so a config without it panicked inside an
errgroup goroutine, after the checkout and before any hook could run. Both
places now go through one accessor.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
lock.HostLocker failing left a nil locker that panicked on Acquire, and an
unreadable ssh key file produced "unable to get ssh public key" without
naming the file or the I/O error. Both are startup errors now.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
commands and env were nested under repo in the README example, but they
live at the top level and decoding rejects unused keys, so copy-pasting the
example failed with "'repo' has invalid keys: commands, env". A test loads
the example straight out of the README now.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
Four WARNING lines used log.Println with a %w verb, so they printed the
literal %w instead of the error, the quorum hook was reported as
onCommandSkipped, and onCommandStarted logged itself as onStartedSuccess.
The quorum error returned to the operator also dropped the quorum name,
which only the hook could see.

Removed with it: the unused command.Vars, StorageOpts.StorageType that
nothing ever set, the accepted but never read
initial_last_published_git_commit, the unreachable _default(), and a branch
of IsNewerVersion already covered by the preceding comparison.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
semver sorts v2.0.0-rc1 above v1.9.0, so a release candidate pushed to the
repository became the newest tag and was deployed as if it were a release.
Pre-release tags are skipped when the target tag is selected, and the skip
is logged.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
A forced checkout overwrites tracked files but leaves untracked ones in
place, so build artifacts, generated manifests or stray .env files from the
previous release took part in the next deployment.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The state file was truncated and rewritten in place, so a crash or power
loss in the middle left a value such as "v1." behind, and every later run
failed with "invalid last processed tag" until an operator deleted the
file by hand. The value is written to a temp file in the same directory and
renamed over the old one.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
Neither took a context or a timeout while the execution lock was held, so a
remote that stops responding stalled the run and every run queued behind
it. Both are bounded now.

An interrupted clone also left a directory that PlainOpen then failed on
forever: the clone goes to a temporary directory and is renamed into place,
and a directory that cannot be opened is removed and cloned again. A clone
of a different repository is still refused.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
On SIGTERM the context was cancelled and exec killed sh, while the actual
workload kept running as an orphan outside the execution lock, so the next
scheduled run started a second deployment beside it. The command now runs
in its own process group and the group is signalled on cancellation.

The cancelled context also made every hook fail instantly with "context
canceled", so nothing reported the interrupted run. Hooks run on a context
derived without the cancellation, bounded by a timeout of their own.

Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
@alexey-igrychev
alexey-igrychev merged commit 11e8e8e into main Sep 18, 2026
2 checks passed
@alexey-igrychev
alexey-igrychev deleted the fix/audit-second-batch branch September 18, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment