pipe-cd / pipe-cd/pipecd

`commandstore` cache poisoning after transient `Get` failure following successful status update

Open
#7,230 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

kind/bug
Dominant language
Go
Stars
1.4k
Forks
364
Avg merge
1d 4h
Merged PRs (30d)
84

Description

Description

commandstore can cache an invalid zero-value Command when backend.Get fails immediately after UpdateStatus succeeds.

Details
  • pkg/app/server/commandstore/store.go:106-120
  • pkg/app/server/commandstore/cache.go:29-48

If backend.Get fails after UpdateStatus has already succeeded, the error is effectively swallowed and the resulting nil command is passed to the cache.

The nil value is marshaled to JSON as null. When null is later unmarshaled into a non-pointer Command struct, Go treats it as a no-op, leaving a zero-value Command, including PipedId="".

Subsequent Get calls can therefore return a valid-looking zero-value command instead of the actual command.

This can cause ReportCommandHandled to return PermissionDenied for the legitimate piped. Since ReportCommandHandled is also the RPC that could otherwise repair the cache, this can leave manual-approval/cancel commands stuck indefinitely.

Suggested fix

Either:

  • return the backend.Get error instead of continuing and caching the invalid result, or
  • skip cache.Put when cmd == nil.

Additionally, add a nil guard in commandCache.Put so a nil command cannot be cached accidentally.

Tests

There currently does not appear to be a test file for this package.

Add a regression test covering the following sequence:

  1. UpdateStatus succeeds.
  2. The subsequent backend.Get fails.
  3. The failed result is not cached.
  4. A later Get does not return a zero-value Command.
  5. ReportCommandHandled continues to work for the legitimate piped.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading pkg/app/server/commandstore/store.go:106-120 and pkg/app/server/commandstore/cache.go:29-48 to trace the UpdateStatus, backend.Get, and cache.Put sequence. Add a regression test for the failure sequence described in the issue, then run the commandstore package tests. Done means the failed result is not cached, later Get calls do not return a zero-value Command, and ReportCommandHandled still works for the legitimate piped.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.