fix(release): refuse a null entry in --assets-links
Description
When the JSON array given to --assets-links contained null, glab release create and glab release upload decoded it to a nil *upload.ReleaseAsset and later dereferenced it, so glab panicked with a nil pointer dereference and exit status 2. By then it could already have changed the release: upload added the links that came before the null one, and create updated an existing release and added them too. v1.124.0 also creates a new release and its first links before it panics; on main, since !4040 (merged), a new release panics before the release is created.
This MR moves the decoding of --assets-links into one helper, releaseutils.AssetLinksFromJSON, next to releaseutils.AssetsFromArgs, and calls it from both commands where they decoded the flag before. It refuses a null element with failed to parse JSON string: the asset link at index <n> is null, counting from 0, and keeps the error for malformed JSON as it was. Both commands parse the flag before their first request (create in complete, upload in validate), so the command now fails without touching the release.
I chose to refuse a null element rather than skip it, since it can only be a mistake in the input and malformed input is refused already. Tell me if you would rather skip it.
Related Issues
Closes #8684 (closed)
How has this been tested?
I added TestReleaseCreate_InvalidAssetsLinksJSON and TestReleaseUpload_InvalidAssetsLinksJSON, one in each command, each with three cases: [null], a valid link followed by null, and malformed JSON. They set no mock expectations, so a request of any kind fails them. On main at a6fd50ce the two null cases fail in both tests, because the commands go on to call GetTag and GetRelease, and the malformed JSON case passes. With the change all six cases pass, and so do the existing --assets-links tests of both commands.
With Go 1.27.3 and golangci-lint 2.14.0 on this branch: make test-race passes (6726 tests, 18 skipped), make lint reports 0 issues, golangci-lint fmt --diff is empty, go run ./scripts/comment-overlap -threshold 1.0 -max-findings 0 ./cmd/... ./internal/... reports 0 findings, make check-embed check-args is clean, and make gen-docs and make generate change no file. The commit message and the title pass commitlint with scripts/commit-lint/.commitlintrc.js.
Each of these changes to the code makes at least one of the new cases fail: removing the null check, inverting it, skipping null elements instead of refusing them, reporting the index plus one, and going back to json.Unmarshal in either command alone.
End to end, I built glab from v1.124.0, from main and from this branch, and ran the three commands of the issue against a small local HTTP server that answers the tag and release endpoints the way GitLab does and logs each request:
| Command | v1.124.0 | main |
This branch |
|---|---|---|---|
release create, new release |
panic, exit status 2, after creating the release and link a |
panic, exit status 2, after GET tag and GET release | error, exit status 1, no request |
release create, existing release |
panic, exit status 2, after updating the release and adding link b |
as v1.124.0 | error, exit status 1, no request |
release upload |
panic, exit status 2, after adding link c |
as v1.124.0 | error, exit status 1, no request |
On this branch each of the three prints failed to parse JSON string: the asset link at index 1 is null. and exits with status 1.
This comes from maintaining gitlab-mcp-server; its upstream-bugs.md records everything done on its dependencies and sibling projects.
Test output on main (without the change to the three production files)
create.go:357: Unexpected call to *testing.MockTagsServiceInterface.GetTag([OWNER/REPO 0.0.1]) at /tmp/w/internal/commands/release/create/create.go:357 because: there are no expected calls of the method "GetTag" for that receiver
create.go:357: Unexpected call to *testing.MockTagsServiceInterface.GetTag([OWNER/REPO 0.0.1]) at /tmp/w/internal/commands/release/create/create.go:357 because: there are no expected calls of the method "GetTag" for that receiver
--- FAIL: TestReleaseCreate_InvalidAssetsLinksJSON (0.00s)
--- FAIL: TestReleaseCreate_InvalidAssetsLinksJSON/only_a_null_link (0.00s)
--- PASS: TestReleaseCreate_InvalidAssetsLinksJSON/malformed_JSON (0.00s)
--- FAIL: TestReleaseCreate_InvalidAssetsLinksJSON/a_null_link_after_a_valid_one (0.00s)
FAIL gitlab.com/gitlab-org/cli/internal/commands/release/create 0.019s
upload.go:168: Unexpected call to *testing.MockReleasesServiceInterface.GetRelease([OWNER/REPO 0.0.1 0x8a3a20]) at /tmp/w/internal/commands/release/upload/upload.go:168 because: there are no expected calls of the method "GetRelease" for that receiver
upload.go:168: Unexpected call to *testing.MockReleasesServiceInterface.GetRelease([OWNER/REPO 0.0.1 0x8a3a20]) at /tmp/w/internal/commands/release/upload/upload.go:168 because: there are no expected calls of the method "GetRelease" for that receiver
--- FAIL: TestReleaseUpload_InvalidAssetsLinksJSON (0.00s)
--- PASS: TestReleaseUpload_InvalidAssetsLinksJSON/malformed_JSON (0.00s)
--- FAIL: TestReleaseUpload_InvalidAssetsLinksJSON/only_a_null_link (0.00s)
--- FAIL: TestReleaseUpload_InvalidAssetsLinksJSON/a_null_link_after_a_valid_one (0.00s)
FAIL gitlab.com/gitlab-org/cli/internal/commands/release/upload 0.020sThe output is cut down to the result lines and the Unexpected call lines, one per null case; everything else is left out. Command: go test -count=1 -run 'TestReleaseCreate_InvalidAssetsLinksJSON|TestReleaseUpload_InvalidAssetsLinksJSON' -v ./internal/commands/release/create/ ./internal/commands/release/upload/
Test output with the change
--- PASS: TestReleaseCreate_InvalidAssetsLinksJSON (0.00s)
--- PASS: TestReleaseCreate_InvalidAssetsLinksJSON/only_a_null_link (0.00s)
--- PASS: TestReleaseCreate_InvalidAssetsLinksJSON/malformed_JSON (0.00s)
--- PASS: TestReleaseCreate_InvalidAssetsLinksJSON/a_null_link_after_a_valid_one (0.00s)
ok gitlab.com/gitlab-org/cli/internal/commands/release/create 0.016s
--- PASS: TestReleaseUpload_InvalidAssetsLinksJSON (0.00s)
--- PASS: TestReleaseUpload_InvalidAssetsLinksJSON/only_a_null_link (0.00s)
--- PASS: TestReleaseUpload_InvalidAssetsLinksJSON/malformed_JSON (0.00s)
--- PASS: TestReleaseUpload_InvalidAssetsLinksJSON/a_null_link_after_a_valid_one (0.00s)
ok gitlab.com/gitlab-org/cli/internal/commands/release/upload 0.021sThe same command, with the output cut down to the result lines.
Screenshots (if appropriate):
Not applicable: a command-line change, with its output above.