Skip to content

[upgrade-lifecycle] Upgrade panic when package manifest is absent in checkUpgrade #14850

@github-actions

Description

@github-actions

Findings

1. Nil manifest dereference in FIPS compatibility check causes upgrade panic (HIGH, P1)

Platform(s): Linux, Windows, macOS (shared upgrade path)

Location

  • internal/pkg/agent/application/upgrade/upgrade.go:269-275
  • internal/pkg/agent/application/upgrade/upgrade.go:439-446
  • internal/pkg/agent/application/upgrade/step_unpack.go:90-117
  • internal/pkg/agent/application/upgrade/step_unpack.go:622-647

Evidence
checkUpgrade unconditionally dereferences metadata.manifest.Package.Fips:

if currentVersion.fips && !metadata.manifest.Package.Fips { ... }
if !currentVersion.fips && metadata.manifest.Package.Fips { ... }

Upgrade calls checkUpgrade immediately after getPackageMetadata.

getPackageMetadata does not require a manifest to exist (it only rejects empty VersionedHome when manifest != nil), and tar metadata parsing explicitly allows no manifest (ret.manifest remains nil).

This means a valid artifact with commit hash but no manifest reaches checkUpgrade and panics on nil pointer dereference.

What is wrong
The upgrade path supports metadata without a manifest, but checkUpgrade assumes manifest is always present.

Why it matters
A normal user upgrade path (especially legacy/older artifact formats) can crash the agent during upgrade orchestration, leaving upgrade incomplete and requiring manual recovery/rollback handling.

Suggested fix direction

  1. Make checkUpgrade nil-safe:
    • If metadata.manifest == nil, skip FIPS compatibility checks (or apply a documented default policy), and continue version/hash checks.
  2. Add explicit unit coverage for nil-manifest behavior.

Test code to add (failing before fix)

  • File: internal/pkg/agent/application/upgrade/upgrade_test.go (extend TestCheckUpgrade)
  • Add cases where metadata.manifest == nil for both FIPS and non-FIPS current versions.
  • Assert checkUpgrade does not panic and returns deterministic error/allow behavior.

Example shape:

assert.NotPanics(t, func() {
    err := checkUpgrade(log, current, newVersion, packageMetadata{manifest:nil, hash:"abcdef"})
    // assert expected err based on chosen nil-manifest policy
})

Priority ranking

  1. P1 (common upgrade path risk): Nil-manifest panic in checkUpgrade (all platforms).

Upgrade paths audited and found safe in this run

  • Marker write/read and cleanup flow around step_mark.go, marker_watcher.go, cleanup.go.
  • Watcher startup/rollback integration path in internal/pkg/agent/cmd/watch.go and watch_impl.go (excluding already-tracked clock-skew grace issue).
  • Rollback orchestration paths in rollback.go and platform variants; no additional verified HIGH defects found in this pass.

What is this? | From workflow: Sweeper: Upgrade and Rollback Lifecycle

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

  • expires on Jun 16, 2026, 10:26 AM UTC

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions