fix: the remaining findings of the main audit - #41
Merged
Merged
Conversation
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
force-pushed
the
fix/audit-second-batch
branch
from
September 18, 2026 14:15
b568d96 to
4524630
Compare
This was referenced Sep 18, 2026
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.
The rest of the audit of
main, one commit per finding.html/template, sov1.0.0+build.3reachedshasv1.0.0+build.3. HTML escaping protects nothing in a shell context.GetLastSemverTagreturned the reference hash unpeeled, so{{ .RepoCommit }}was the hash of the tag object for annotated tags, which is what signed releases use.trx.yamlwithenv: {FOO: bar}reached the command asfoo=bar. The rendering is in one helper now,mergeEnvsno longer writes into the operator config, and the operator config overrides the repository one as documented.nameof a quorum was dereferenced during verification and panicked inside an errgroup goroutine.lock.HostLockerleft a nil locker that panicked onAcquire, and an unreadable ssh key produced an error naming neither the file nor the cause.commandsandenvunderrepo, so copy-pasting it failed with'repo' has invalid keys: commands, env. A test now loads the example straight out of the README.WARNINGlines printed the literal%w, the quorum hook was reported asonCommandSkipped,onCommandStartedlogged itself asonStartedSuccess, and the quorum name was dropped from the error the operator sees. Dead code removed with it:command.Vars,StorageOpts.StorageType,initial_last_published_git_commit, the unreachable_default(), and a branch ofIsNewerVersioncovered by the preceding comparison.v1.behind and fail every later run. It is written to a temp file and renamed, and the directory is synced.shand left the workload running as an orphan outside the lock, while the cancelled context made every hook fail instantly withcontext canceled. The command runs in its own process group, the group is signalled and then killed, hooks run on a context derived without the cancellation, and a second Ctrl-C ends trx at once.Two more findings filed under #29 as "worth a second look", both promoted because they change what gets deployed:
v2.0.0-rc1abovev1.9.0, so a release candidate became the newest tag and was deployed as a release. Pre-releases are skipped unlessrepo.allowPrereleaseis set..envfiles of the previous release took part in the next deployment. The worktree is cleaned.Behaviour changes for operators
repo.envno longer overrides the operatorenv— the README always documented the opposite.repo.allowPrerelease: true.git clean -fd.--force.Closes #18, closes #19, closes #20, closes #21, closes #23, closes #24, closes #25, closes #26, closes #27, closes #29.