fix: defects found by a full review of internal/ and main.go - #48
Merged
Conversation
The Azure DevOps extension pinned brace-expansion to 5.0.8 through an overrides entry, and 5.0.8 is the version the current advisory covers: a denial of service through unbounded intermediate arrays that bypasses the earlier CVE-2026-14257 mitigation. The pin that once fixed a problem had become the problem, and nothing would move it because an override is absolute. Bumped to 5.0.9, the first patched release and the current latest, with the lockfile regenerated to match.
Every run warned that actions/checkout and actions/setup-go target Node 20, which GitHub now force-runs on Node 24 and will stop supporting. Moved to checkout v7, setup-go v7, and upload-artifact v7 in ci.yml and action-smoke.yml, which this pull request exercises. The release, helm publish, and Azure DevOps extension workflows are left alone deliberately. They run only on a tag or a manual dispatch, so no pull request proves a bump there, and a broken publish workflow is discovered at the worst possible moment. They are worth updating in a change that can be dispatched and watched.
Three defects that each made a whole path unusable. The engine handed prsync the rendered destination name, which is owner relative because CloneURL and EnsureRepo prepend the owner themselves. Every destination pull request and comment endpoint interpolates that value as a fully qualified path, so a GitHub destination built "/repos/widget/pulls" instead of "/repos/acme/widget/pulls" and answered 404 for every call. mirror_objects could not work at all against GitHub, GitLab, or Bitbucket. Remote gains QualifiedPath, each provider implements the convention its own ListRepos reports, and the engine qualifies the name before passing it on. The existing tests passed because their fake used the broken convention. Azure DevOps went wrong the other way: ListRepos reports project/name and every URL already carries the project as its own segment, so interpolating the qualified path whole produced a duplicated project and 404ed on every repository. Its pull request routes now use the repository segment alone. internal/prsync wrote pull request records into the shared GitState from inside the engine's worker pool, bypassing the lock the engine uses for its own access and documents as necessary. Two repositories finishing together was a concurrent map write, which aborts the process and cannot be recovered by the worker that caused it. GitState now guards its own maps, so no caller has to remember.
Authenticate logged in and threw the response away, so the JWT the Hub API authenticates with was never kept. Every later listing fell back to HTTP basic auth, which that API does not accept: a private repository with watch_tags on answered 404 after three retries, reported against the image rather than against the credentials. The session token is stored and sent as a bearer credential, ahead of a configured personal access token, with basic auth left as a last resort for any deployment still relying on it. An anonymous registry sends nothing.
…ilure Sources are listed open only, so a merged or closed pull request does not arrive with a finished state: it stops appearing. The close path was therefore unreachable, and a destination pull request stayed open forever while the mirror push pruned the branch under it. Records the source no longer lists are now reconciled and closed with a note that says only what is knowable from an open listing. GitHub keeps discussion comments and review comments in separate id spaces addressed through different routes, and both were updated through the issues route. Every pull request with a mirrored inline comment failed its conversation sync with a 404 on every run after the first, or edited an unrelated comment if the ids collided. The id now carries which space it belongs to, and a bare id from an older state file still resolves to the discussion route. A conversation that failed part way discarded the ids of the comments it had already posted, because the state write came after the error return, so the next run reposted all of them, forever. What succeeded is recorded before giving up. The repository fingerprint was recorded before the pull request pass ran, so a failed pass was marked done and skipped on the next run until an unrelated branch or tag happened to move. It is recorded after, and only when the pass had no failures. A pull request head that failed to fetch was dropped from the keep set, which pruned it from the cache and then deleted it at the destination. One transient network error removed a correct branch and pulled the head out from under its mirrored pull request. The previous copy is kept instead.
…limits The image sync cron had no SkipIfStillRunning, which the git cron has along with a comment explaining the hazard. A run that outlives its interval overlapped itself on a Syncer that keeps per run state and a state map with no locking: the second run's report was voided by the first, and two writers on that map abort the process. Both cron loops now also register the schedule before the first run, so a typo is caught immediately instead of after a full sync of every image, and install the signal handler before the first run, so a SIGTERM during it lets the run finish and persist rather than killing the process with nothing saved. A malformed exclude glob was silently ignored, so a repository somebody wrote a rule to keep out was mirrored to the destination. Patterns are validated at config load, and a pattern that cannot be parsed now drops the repository rather than mirroring it. A Slack section block could be emitted empty, and could exceed the size limit, from a single oversized failure line such as a transport error carrying a multi kilobyte HTML body. Either one makes Slack answer 400 and the entire notification is lost, which matters most for the failure alert that is the reason the integration exists. Sections are bounded at both ends, and truncate now counts characters rather than bytes so it neither trims non-ASCII text far short of the limit nor splits a rune. A failed state save on the fail fast path was discarded, so every image copied before the abort was re-pulled and re-pushed on the next run with nothing in the output to say why. The same failure is fatal on the normal exit path; it is at least reported here.
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
2 participants
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.
A high-effort review across all of
internal/andmain.go(~11k lines), rather than a single diff. It found 15 defects; all are fixed here with regression tests, plus two more found alongside.Four of these mean a feature could not work at all. Everything from #44 and #46 shipped in v0.2.1 with green CI, because the tests encoded the same wrong assumptions the code did.
Blocking
mirror_objectswas inoperable against GitHub, GitLab, and Bitbucket. The engine passed the rendered destination name, which is owner-relative becauseCloneURLandEnsureRepoprepend the owner themselves. Every destination pull request and comment endpoint interpolates that value as a fully qualified path, so a GitHub destination asked for/repos/widget/pullsinstead of/repos/acme/widget/pullsand every call 404d.RemotegainsQualifiedPath; each provider implements the convention its ownListReposreports. The existing test asserted the broken convention, which is why it passed.Azure DevOps failed the opposite way.
ListReposreportsproject/nameand every URL already carries the project as its own segment, so interpolating the qualified path whole produced a duplicated project and 404d for every repository. Its pull request routes use the repository segment alone.Mirroring several repositories at once could abort the process.
internal/prsyncwrote pull request records into the sharedGitStatefrom inside the engine's worker pool, bypassing the lock the engine uses for its own access and documents as necessary. Two repositories finishing together is a concurrent map write, which Go turns into an unrecoverable crash that the worker's ownrecovercannot catch.GitStatenow guards its own maps; there is a race-detector test.A private Docker Hub repository always failed.
Authenticatelogged in and discarded the response, so the JWT the Hub API authenticates with was never kept and every listing fell back to basic auth, which that API rejects. The failure surfaced as a 404 blamed on the image.Important
Syncerwith per-run state and an unlocked state map — the exact hazard the git cron documents and guards against. Both loops now skip rather than overlap, validate the schedule before the first run rather than after a full sync, and install the signal handler before the first run so a shutdown can still persist.excludeglob was ignored entirely, mirroring a repository the rule existed to keep out. Patterns are validated at config load and a bad pattern now drops rather than mirrors.truncatecounted bytes where Slack counts characters, so it trimmed non-ASCII text to a third of the limit and could split a rune.Found alongside the review
brace-expansion, which the Azure DevOps extension pinned to exactly the affected version viaoverrides. The pin that once fixed a problem had become the problem, and nothing could move it.govulncheckreports the Go side clean.actions/checkoutandactions/setup-gomoved to v7. The release, helm, and extension publish workflows are deliberately untouched — no pull request exercises them, and a broken publish surfaces at the worst moment.Testing
go vetclean,go test -race ./...passes. Every fix has a regression test that fails against the old behaviour, including a concurrent-writer test for the state race and a test asserting the destination receives a qualified path.Note
This is all unreleased work except the Docker Hub, cron, filter, and Slack fixes, which affect v0.2.1 and earlier. Worth a release once reviewed.