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:
UpdateStatus succeeds.
- The subsequent
backend.Get fails.
- The failed result is not cached.
- A later
Get does not return a zero-value Command.
ReportCommandHandled continues to work for the legitimate piped.
Description
commandstorecan cache an invalid zero-valueCommandwhenbackend.Getfails immediately afterUpdateStatussucceeds.Details
pkg/app/server/commandstore/store.go:106-120pkg/app/server/commandstore/cache.go:29-48If
backend.Getfails afterUpdateStatushas already succeeded, the error is effectively swallowed and the resultingnilcommand is passed to the cache.The
nilvalue is marshaled to JSON asnull. Whennullis later unmarshaled into a non-pointerCommandstruct, Go treats it as a no-op, leaving a zero-valueCommand, includingPipedId="".Subsequent
Getcalls can therefore return a valid-looking zero-value command instead of the actual command.This can cause
ReportCommandHandledto returnPermissionDeniedfor the legitimate piped. SinceReportCommandHandledis also the RPC that could otherwise repair the cache, this can leave manual-approval/cancel commands stuck indefinitely.Suggested fix
Either:
backend.Geterror instead of continuing and caching the invalid result, orcache.Putwhencmd == nil.Additionally, add a nil guard in
commandCache.Putso 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:
UpdateStatussucceeds.backend.Getfails.Getdoes not return a zero-valueCommand.ReportCommandHandledcontinues to work for the legitimate piped.