From 11982c35a5e5038a6e4609dd48347b9a35113c4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Sat, 3 Oct 2026 22:16:23 -0700 Subject: [PATCH 1/2] fix(process): reject a pid that would signal a process group The process endpoints declare required,min=1 on their pid path parameter and nothing enforced it. Both handlers validated the hostname and passed request.Pid straight through, so a value below 1 reached the provider, which validated the signal name and then called syscall.Kill with the pid as given. min=1 is the rule that matters here, not required. kill(2) overloads the sign: 0 is every process in the caller's process group, -1 is every process the caller may signal, and below -1 is a whole process group. required only rejects 0, so without min=1 the sign turns one signal into a host-wide one, and the agent usually runs as root. Validated in both places, because the provider contract says the request path does not discharge the provider's own checks and the provider is the frame that calls kill(2). The provider's check runs before the signal name is looked up, so a bad pid is refused whatever signal came with it. The 400 now names the value and the bound. validation.Var has no field name to report and min carries no hint, so the bare message read "Field validation for '' failed on the 'min' tag", which tells a caller nothing about what to send instead. Tests at both levels. The provider cases set no mock expectation, so the controller fails the test if Kill is reached at all, which is the assertion that matters rather than the error string. Verified against a running controller and agent: before, pid -5 returned a job id and crossed NATS; now all of 0, -1, -5 and -42 are refused at the API with no job created. Found by checking, one call site at a time, whether min was load-bearing where an audit had reported it as untestable. On 146 plain string fields required does mask it. On this one it was the only guard, and it was not wired up. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FuKUsHFG1EqZXamffh9M2c --- .../api/node/process/process_get.go | 4 +++ .../node/process/process_get_public_test.go | 22 ++++++++++++ .../api/node/process/process_signal_post.go | 4 +++ .../process_signal_post_public_test.go | 36 +++++++++++++++++++ .../controller/api/node/process/validate.go | 33 ++++++++++++++++- internal/provider/node/process/debian.go | 29 +++++++++++++++ .../node/process/debian_public_test.go | 36 +++++++++++++++++++ 7 files changed, 163 insertions(+), 1 deletion(-) diff --git a/internal/controller/api/node/process/process_get.go b/internal/controller/api/node/process/process_get.go index 19500288f..5d46041fb 100644 --- a/internal/controller/api/node/process/process_get.go +++ b/internal/controller/api/node/process/process_get.go @@ -41,6 +41,10 @@ func (s *Process) GetNodeProcessByPid( return gen.GetNodeProcessByPid400JSONResponse{Error: &errMsg}, nil } + if errMsg, ok := validatePid(request.Pid); !ok { + return gen.GetNodeProcessByPid400JSONResponse{Error: &errMsg}, nil + } + hostname := request.Hostname pid := request.Pid diff --git a/internal/controller/api/node/process/process_get_public_test.go b/internal/controller/api/node/process/process_get_public_test.go index c10ea9ac6..c500d3089 100644 --- a/internal/controller/api/node/process/process_get_public_test.go +++ b/internal/controller/api/node/process/process_get_public_test.go @@ -398,6 +398,28 @@ func (s *ProcessGetPublicTestSuite) TestGetNodeProcessByPidValidationHTTP() { s.Contains(rec.Body.String(), `"results"`) }, }, + { + name: "when the pid is zero returns 400", + path: "/api/node/server1/process/0", + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "required") + }, + }, + { + name: "when the pid is minus one returns 400", + path: "/api/node/server1/process/-1", + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, { name: "when target agent not found", path: "/api/node/nonexistent/process/1234", diff --git a/internal/controller/api/node/process/process_signal_post.go b/internal/controller/api/node/process/process_signal_post.go index 5631b5020..efeff425c 100644 --- a/internal/controller/api/node/process/process_signal_post.go +++ b/internal/controller/api/node/process/process_signal_post.go @@ -42,6 +42,10 @@ func (s *Process) PostNodeProcessSignal( return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil } + if errMsg, ok := validatePid(request.Pid); !ok { + return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil + } + if errMsg, ok := validation.Struct(request.Body); !ok { return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil } diff --git a/internal/controller/api/node/process/process_signal_post_public_test.go b/internal/controller/api/node/process/process_signal_post_public_test.go index 8d8a9abfb..1934e0ed7 100644 --- a/internal/controller/api/node/process/process_signal_post_public_test.go +++ b/internal/controller/api/node/process/process_signal_post_public_test.go @@ -483,6 +483,42 @@ func (s *ProcessSignalPublicTestSuite) TestPostNodeProcessSignalValidationHTTP() s.Contains(rec.Body.String(), "oneof") }, }, + { + name: "when the pid is zero returns 400", + path: "/api/node/server1/process/0/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "required") + }, + }, + { + name: "when the pid is minus one returns 400", + path: "/api/node/server1/process/-1/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, + { + name: "when the pid is a process group returns 400", + path: "/api/node/server1/process/-42/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, { name: "when target agent not found", path: "/api/node/nonexistent/process/1234/signal", diff --git a/internal/controller/api/node/process/validate.go b/internal/controller/api/node/process/validate.go index 36e9e562f..48011c2c2 100644 --- a/internal/controller/api/node/process/validate.go +++ b/internal/controller/api/node/process/validate.go @@ -20,7 +20,11 @@ package process -import "github.com/osapi-io/osapi/internal/validation" +import ( + "fmt" + + "github.com/osapi-io/osapi/internal/validation" +) // validateHostname validates a hostname path parameter using the shared // validator. Returns the error message and false if invalid. @@ -32,3 +36,30 @@ func validateHostname( ) (string, bool) { return validation.Var(hostname, "required,min=1,valid_target") } + +// validatePid validates a pid path parameter against the bound the +// specification declares for it. Same upstream limitation as +// validateHostname. +// +// min=1 is the rule that matters, not required. A pid reaches kill(2), where a +// value below 1 does not mean "that process": 0 is every process in the +// caller's process group, -1 is every process the caller may signal, and +// anything lower is a whole process group. required only rejects 0, so without +// min=1 a negative pid turns one signal into a fleet-wide one. +func validatePid( + pid int, +) (string, bool) { + errMsg, ok := validation.Var(pid, "required,min=1") + if ok { + return "", true + } + + // validation.Var has no field name to report and min carries no hint, so + // the bare message is "Field validation for '' failed on the 'min' tag", + // which tells a caller nothing. Name the value and the bound, and keep the + // tag's own message so a test can still assert which rule rejected it. + return fmt.Sprintf( + "pid %d must be greater than zero: %s", + pid, errMsg, + ), false +} diff --git a/internal/provider/node/process/debian.go b/internal/provider/node/process/debian.go index 19a46a642..34d437faf 100644 --- a/internal/provider/node/process/debian.go +++ b/internal/provider/node/process/debian.go @@ -33,6 +33,27 @@ import ( ) // allowedSignals maps signal names to their syscall equivalents. +// validatePid rejects a pid that kill(2) would not read as one process. +// +// kill(2) overloads the sign: 0 means every process in the caller's process +// group, -1 means every process the caller may signal, and anything below -1 +// means a whole process group. The agent usually runs as root, so a value +// below 1 turns one signal into a host-wide one. The API validates this too, +// and this is the copy that matters, because this is the frame that calls +// kill(2). +func validatePid( + pid int, +) error { + if pid < 1 { + return fmt.Errorf( + "process: pid %d must be greater than zero, a lower value signals a process group rather than a process", + pid, + ) + } + + return nil +} + var allowedSignals = map[string]syscall.Signal{ "TERM": syscall.SIGTERM, "KILL": syscall.SIGKILL, @@ -156,6 +177,10 @@ func (d *Debian) Get( _ context.Context, pid int, ) (*Info, error) { + if err := validatePid(pid); err != nil { + return nil, err + } + p, err := d.lister.NewProcess(int32(pid)) if err != nil { return nil, fmt.Errorf("process: get: %w", err) @@ -175,6 +200,10 @@ func (d *Debian) Signal( pid int, signal string, ) (*SignalResult, error) { + if err := validatePid(pid); err != nil { + return nil, err + } + sig, ok := allowedSignals[signal] if !ok { return nil, fmt.Errorf("process: signal: invalid signal %q", signal) diff --git a/internal/provider/node/process/debian_public_test.go b/internal/provider/node/process/debian_public_test.go index 789709a5d..a732b71d8 100644 --- a/internal/provider/node/process/debian_public_test.go +++ b/internal/provider/node/process/debian_public_test.go @@ -394,6 +394,42 @@ func (suite *DebianPublicTestSuite) TestSignal() { wantErr: true, wantErrMsg: "process: signal: unexpected error", }, + // Each of these would reach kill(2) as a process-group signal rather + // than a process signal. setupMock sets no expectation, so the mock + // controller fails the test if Kill is called at all, which is the + // assertion that matters: the signal is never sent. + { + name: "when pid is zero it never reaches kill", + pid: 0, + signal: "KILL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid 0 must be greater than zero", + }, + { + name: "when pid is minus one it never reaches kill", + pid: -1, + signal: "KILL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, + { + name: "when pid is a process group it never reaches kill", + pid: -42, + signal: "TERM", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -42 must be greater than zero", + }, + { + name: "the pid is checked before the signal name", + pid: -1, + signal: "NOT-A-SIGNAL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, } for _, tc := range tests { From 30aa04fd2b87d198f2c9a8ec795b5efd81adceac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Wed, 7 Oct 2026 10:44:04 -0700 Subject: [PATCH 2/2] test(process): cover the pid guard in Get The guard was added to both Signal and Get, and only Signal's was tested, so `return nil, err` in Get was the one statement this change left uncovered. Codecov caught it: misses 10 -> 11. Two cases, shaped like Signal's. setupMock sets no expectation, so the controller fails the test if NewProcess is reached at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FuKUsHFG1EqZXamffh9M2c --- .../node/process/debian_public_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/provider/node/process/debian_public_test.go b/internal/provider/node/process/debian_public_test.go index a732b71d8..3c440fb2f 100644 --- a/internal/provider/node/process/debian_public_test.go +++ b/internal/provider/node/process/debian_public_test.go @@ -245,6 +245,24 @@ func (suite *DebianPublicTestSuite) TestGet() { wantErrMsg string validateFunc func(result *process.Info) }{ + // setupMock sets no expectation, so the controller fails the test if + // NewProcess is called at all. kill(2) is not reached from Get, but the + // guard is here because the API and the provider validate the same + // bound, and a pid below 1 is not a process either way. + { + name: "when pid is zero it never reaches the lister", + pid: 0, + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid 0 must be greater than zero", + }, + { + name: "when pid is negative it never reaches the lister", + pid: -1, + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, { name: "when successful returns process info", pid: 42,