From bb14c5da3e64b03f207c7f38139eb60e97c278fc Mon Sep 17 00:00:00 2001 From: jamesread Date: Wed, 4 Mar 2026 22:51:58 +0000 Subject: [PATCH 01/12] security: (MED) GHSA-fwhj-785h-43hh Crash on NPE by calling APIs with invalid bindings or log references --- service/internal/api/api.go | 115 +++++++++++++++++++---------- service/internal/api/apiActions.go | 52 ++++++++----- service/internal/api/dashboards.go | 5 +- 3 files changed, 111 insertions(+), 61 deletions(-) diff --git a/service/internal/api/api.go b/service/internal/api/api.go index 629d039..2b9fe42 100644 --- a/service/internal/api/api.go +++ b/service/internal/api/api.go @@ -70,20 +70,21 @@ func (api *oliveTinAPI) KillAction(ctx ctx.Context, req *connect.Request[apiv1.K execReqLogEntry, ret.Found = api.executor.GetLog(req.Msg.ExecutionTrackingId) if !ret.Found { - log.Warnf("Killing execution request not possible - not found by tracking ID: %v", req.Msg.ExecutionTrackingId) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } - log.Warnf("Killing execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + if execReqLogEntry.Binding == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("log entry has no binding for tracking ID %s", req.Msg.ExecutionTrackingId)) + } action := execReqLogEntry.Binding.Action if action == nil { - log.Warnf("Killing execution request not possible - action not found: %v", execReqLogEntry.ActionTitle) - ret.Killed = false - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } + log.Warnf("Killing execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + user := auth.UserFromApiCall(ctx, req, api.cfg) api.killActionByTrackingId(user, action, execReqLogEntry, ret) @@ -205,42 +206,58 @@ func (api *oliveTinAPI) LocalUserLogin(ctx ctx.Context, req *connect.Request[api return response, nil } -func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionAndWaitRequest]) (*connect.Response[apiv1.StartActionAndWaitResponse], error) { - args := make(map[string]string) - - for _, arg := range req.Msg.Arguments { - args[arg.Name] = arg.Value - } - - user := auth.UserFromApiCall(ctx, req, api.cfg) - +func (api *oliveTinAPI) startActionAndWaitRun(binding *executor.ActionBinding, args map[string]string, user *authpublic.AuthenticatedUser) (*executor.InternalLogEntry, bool) { execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: user, Cfg: api.cfg, } - wg, _ := api.executor.ExecRequest(&execReq) wg.Wait() + return api.executor.GetLog(execReq.TrackingID) +} - internalLogEntry, ok := api.executor.GetLog(execReq.TrackingID) - - if ok { - return connect.NewResponse(&apiv1.StartActionAndWaitResponse{ - LogEntry: api.internalLogEntryToPb(internalLogEntry, user), - }), nil - } else { - return nil, fmt.Errorf("execution not found") +func (api *oliveTinAPI) findBindingOrNotFound(actionId string) (*executor.ActionBinding, error) { + binding := api.executor.FindBindingByID(actionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", actionId)) } + return binding, nil +} + +func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionAndWaitRequest]) (*connect.Response[apiv1.StartActionAndWaitResponse], error) { + binding, err := api.findBindingOrNotFound(req.Msg.ActionId) + if err != nil { + return nil, err + } + + args := make(map[string]string) + for _, arg := range req.Msg.Arguments { + args[arg.Name] = arg.Value + } + user := auth.UserFromApiCall(ctx, req, api.cfg) + + internalLogEntry, ok := api.startActionAndWaitRun(binding, args, user) + if !ok { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) + } + return connect.NewResponse(&apiv1.StartActionAndWaitResponse{ + LogEntry: api.internalLogEntryToPb(internalLogEntry, user), + }), nil } func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetRequest]) (*connect.Response[apiv1.StartActionByGetResponse], error) { + binding := api.executor.FindBindingByID(req.Msg.ActionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId)) + } + args := make(map[string]string) execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: auth.UserFromApiCall(ctx, req, api.cfg), @@ -255,12 +272,17 @@ func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[a } func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetAndWaitRequest]) (*connect.Response[apiv1.StartActionByGetAndWaitResponse], error) { + binding := api.executor.FindBindingByID(req.Msg.ActionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId)) + } + args := make(map[string]string) user := auth.UserFromApiCall(ctx, req, api.cfg) execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: user, @@ -276,9 +298,8 @@ func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Re return connect.NewResponse(&apiv1.StartActionByGetAndWaitResponse{ LogEntry: api.internalLogEntryToPb(internalLogEntry, user), }), nil - } else { - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) } + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) } func calculateRateLimitExpires(api *oliveTinAPI, logEntry *executor.InternalLogEntry) string { @@ -436,7 +457,7 @@ func (api *oliveTinAPI) GetActionBinding(ctx ctx.Context, req *connect.Request[a binding := api.executor.FindBindingByID(req.Msg.BindingId) - if binding == nil { + if binding == nil || binding.Action == nil { return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.BindingId)) } @@ -646,7 +667,16 @@ error messages more quickly before starting the action. It uses the same validation logic as the executor, including mangling argument values (e.g., datetime formatting, checkbox title-to-value conversion). */ +func (api *oliveTinAPI) argumentNotFoundForValidation(msg *apiv1.ValidateArgumentTypeRequest) bool { + arg, _ := api.findArgumentForValidation(msg.BindingId, msg.ArgumentName) + return arg == nil && (msg.BindingId != "" || msg.ArgumentName != "") +} + func (api *oliveTinAPI) ValidateArgumentType(ctx ctx.Context, req *connect.Request[apiv1.ValidateArgumentTypeRequest]) (*connect.Response[apiv1.ValidateArgumentTypeResponse], error) { + if api.argumentNotFoundForValidation(req.Msg) { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action or argument not found for binding ID %s", req.Msg.BindingId)) + } + err := api.validateArgumentTypeInternal(req.Msg) desc := "" if err != nil { @@ -747,6 +777,13 @@ func (api *oliveTinAPI) DumpVars(ctx ctx.Context, req *connect.Request[apiv1.Dum return connect.NewResponse(res), nil } +func debugBindingActionTitle(binding *executor.ActionBinding) string { + if binding == nil || binding.Action == nil { + return "" + } + return binding.Action.Title +} + func (api *oliveTinAPI) DumpPublicIdActionMap(ctx ctx.Context, req *connect.Request[apiv1.DumpPublicIdActionMapRequest]) (*connect.Response[apiv1.DumpPublicIdActionMapResponse], error) { res := &apiv1.DumpPublicIdActionMapResponse{} res.Contents = make(map[string]*apiv1.DebugBinding) @@ -761,7 +798,7 @@ func (api *oliveTinAPI) DumpPublicIdActionMap(ctx ctx.Context, req *connect.Requ for k, v := range api.executor.MapActionBindings { res.Contents[k] = &apiv1.DebugBinding{ - ActionTitle: v.Action.Title, + ActionTitle: debugBindingActionTitle(v), } } @@ -1267,28 +1304,26 @@ func serializeEntityFields(data any) map[string]string { } func (api *oliveTinAPI) RestartAction(ctx ctx.Context, req *connect.Request[apiv1.RestartActionRequest]) (*connect.Response[apiv1.StartActionResponse], error) { - ret := &apiv1.StartActionResponse{ - ExecutionTrackingId: req.Msg.ExecutionTrackingId, - } - var execReqLogEntry *executor.InternalLogEntry execReqLogEntry, found := api.executor.GetLog(req.Msg.ExecutionTrackingId) if !found { - log.Warnf("Restarting execution request not possible - not found by tracking ID: %v", req.Msg.ExecutionTrackingId) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } - log.Warnf("Restarting execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + if execReqLogEntry.Binding == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("log entry has no binding for tracking ID %s", req.Msg.ExecutionTrackingId)) + } action := execReqLogEntry.Binding.Action if action == nil { - log.Warnf("Restarting execution request not possible - action not found: %v", execReqLogEntry.ActionTitle) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } + log.Warnf("Restarting execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + return api.StartAction(ctx, &connect.Request[apiv1.StartActionRequest]{ Msg: &apiv1.StartActionRequest{ BindingId: execReqLogEntry.GetBindingId(), diff --git a/service/internal/api/apiActions.go b/service/internal/api/apiActions.go index 355f179..13bcb4a 100644 --- a/service/internal/api/apiActions.go +++ b/service/internal/api/apiActions.go @@ -28,18 +28,19 @@ func (rr *DashboardRenderRequest) findAction(title string) *apiv1.Action { return rr.findActionForEntity(title, nil) } +func bindingMatchesTitleAndEntity(binding *executor.ActionBinding, title string, entity *entities.Entity) bool { + return binding != nil && binding.Action != nil && binding.Action.Title == title && matchesEntity(binding, entity) +} + func (rr *DashboardRenderRequest) findActionForEntity(title string, entity *entities.Entity) *apiv1.Action { rr.ex.MapActionBindingsLock.RLock() defer rr.ex.MapActionBindingsLock.RUnlock() for _, binding := range rr.ex.MapActionBindings { - if binding.Action.Title != title { + if !bindingMatchesTitleAndEntity(binding, title, entity) { continue } - - if matchesEntity(binding, entity) { - return buildAction(binding, rr) - } + return buildAction(binding, rr) } return nil @@ -117,26 +118,37 @@ func getDefaultArgumentValue(cfgArg config.ActionArgument, entity *entities.Enti return defaultValue } +func formatRateLimitExpiry(expiryUnix int64) string { + if expiryUnix <= 0 { + return "" + } + return time.Unix(expiryUnix, 0).Format("2006-01-02 15:04:05") +} + +func actionFromBinding(actionBinding *executor.ActionBinding) (*executor.ActionBinding, *config.Action) { + if actionBinding == nil || actionBinding.Action == nil { + return nil, nil + } + return actionBinding, actionBinding.Action +} + func buildAction(actionBinding *executor.ActionBinding, rr *DashboardRenderRequest) *apiv1.Action { - action := actionBinding.Action - - aclCanExec := acl.IsAllowedExec(rr.cfg, rr.AuthenticatedUser, action) - enabledExprCanExec := evaluateEnabledExpression(action, actionBinding.Entity) - - // Calculate rate limit expiry time - expiryUnix := rr.ex.GetTimeUntilAvailable(actionBinding) - datetimeRateLimitExpires := "" - if expiryUnix > 0 { - datetimeRateLimitExpires = time.Unix(expiryUnix, 0).Format("2006-01-02 15:04:05") + binding, action := actionFromBinding(actionBinding) + if binding == nil { + return nil } + aclCanExec := acl.IsAllowedExec(rr.cfg, rr.AuthenticatedUser, action) + enabledExprCanExec := evaluateEnabledExpression(action, binding.Entity) + datetimeRateLimitExpires := formatRateLimitExpiry(rr.ex.GetTimeUntilAvailable(binding)) + btn := apiv1.Action{ - BindingId: actionBinding.ID, - Title: tpl.ParseTemplateOfActionBeforeExec(action.Title, actionBinding.Entity), - Icon: tpl.ParseTemplateOfActionBeforeExec(action.Icon, actionBinding.Entity), + BindingId: binding.ID, + Title: tpl.ParseTemplateOfActionBeforeExec(action.Title, binding.Entity), + Icon: tpl.ParseTemplateOfActionBeforeExec(action.Icon, binding.Entity), CanExec: aclCanExec && enabledExprCanExec, PopupOnStart: action.PopupOnStart, - Order: int32(actionBinding.ConfigOrder), + Order: int32(binding.ConfigOrder), Timeout: int32(action.Timeout), DatetimeRateLimitExpires: datetimeRateLimitExpires, } @@ -147,7 +159,7 @@ func buildAction(actionBinding *executor.ActionBinding, rr *DashboardRenderReque Title: cfgArg.Title, Type: cfgArg.Type, Description: cfgArg.Description, - DefaultValue: getDefaultArgumentValue(cfgArg, actionBinding.Entity), + DefaultValue: getDefaultArgumentValue(cfgArg, binding.Entity), Choices: buildChoices(cfgArg), Suggestions: cfgArg.Suggestions, SuggestionsBrowserKey: cfgArg.SuggestionsBrowserKey, diff --git a/service/internal/api/dashboards.go b/service/internal/api/dashboards.go index 11c6cf3..2bc2244 100644 --- a/service/internal/api/dashboards.go +++ b/service/internal/api/dashboards.go @@ -130,7 +130,7 @@ func buildDefaultDashboard(rr *DashboardRenderRequest) *apiv1.Dashboard { } for _, binding := range rr.ex.MapActionBindings { - if binding.Action.Hidden { + if binding == nil || binding.Action == nil || binding.Action.Hidden { continue } @@ -139,6 +139,9 @@ func buildDefaultDashboard(rr *DashboardRenderRequest) *apiv1.Dashboard { } action := buildAction(binding, rr) + if action == nil { + continue + } fieldset.Contents = append(fieldset.Contents, &apiv1.DashboardComponent{ Type: "link", From 0c47564652367749c255b22013ce5af82085e4ad Mon Sep 17 00:00:00 2001 From: jamesread Date: Mon, 2 Mar 2026 13:48:22 +0000 Subject: [PATCH 02/12] chore: logs are written with 0600 instead of 0644 --- service/internal/executor/executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index 91312d4..65f7be6 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -1059,7 +1059,7 @@ func saveLogResults(req *ExecutionRequest, filename string) { } filepath := path.Join(dir, filename+".yaml") - err = os.WriteFile(filepath, data, 0644) + err = os.WriteFile(filepath, data, 0600) if err != nil { log.Warnf("%v", err) @@ -1073,7 +1073,7 @@ func saveLogOutput(req *ExecutionRequest, filename string) { if dir != "" { data := req.logEntry.Output filepath := path.Join(dir, filename+".log") - err := os.WriteFile(filepath, []byte(data), 0644) + err := os.WriteFile(filepath, []byte(data), 0600) if err != nil { log.Warnf("%v", err) From 00cb5a2abfd024e6de680af4256687056dc9a041 Mon Sep 17 00:00:00 2001 From: jamesread Date: Tue, 3 Mar 2026 22:20:45 +0000 Subject: [PATCH 03/12] fix: action triggers were broken #914 --- service/internal/executor/executor.go | 11 ++- service/internal/executor/executor_test.go | 90 ++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index 65f7be6..f6807cf 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -1015,8 +1015,15 @@ func stepTrigger(req *ExecutionRequest) bool { } func triggerLoop(req *ExecutionRequest) { - for _, triggerReq := range req.Binding.Action.Triggers { - binding := req.executor.FindBindingByID(triggerReq) + for _, triggerTitle := range req.Binding.Action.Triggers { + binding := req.executor.findBindingByActionTitle(triggerTitle, "") + if binding == nil { + log.WithFields(log.Fields{ + "triggerTitle": triggerTitle, + "fromAction": req.logEntry.ActionTitle, + }).Warnf("Trigger references unknown action title; skipping") + continue + } trigger := &ExecutionRequest{ Binding: binding, TrackingID: uuid.NewString(), diff --git a/service/internal/executor/executor_test.go b/service/internal/executor/executor_test.go index 2608bbe..16f491d 100644 --- a/service/internal/executor/executor_test.go +++ b/service/internal/executor/executor_test.go @@ -2,6 +2,7 @@ package executor import ( "testing" + "time" "github.com/stretchr/testify/assert" @@ -395,3 +396,92 @@ func TestFilterToDefinedArgumentsPreservesSystemArgs(t *testing.T) { assert.Equal(t, "track-123", req.Arguments["ot_executionTrackingId"]) assert.Equal(t, "webhook", req.Arguments["ot_username"]) } + +func TestTriggerExecutesTriggeredAction(t *testing.T) { + cfg := config.DefaultConfig() + e := DefaultExecutor(cfg) + helloAction := &config.Action{ + Title: "Hello world", + Shell: "echo 'Hello World!'", + } + triggerAction := &config.Action{ + Title: "Simple action that triggers another action", + Shell: "echo 'Hi'", + Triggers: []string{"Hello world"}, + } + cfg.Actions = append(cfg.Actions, helloAction, triggerAction) + cfg.Sanitize() + e.RebuildActionMap() + + finishedTitles := make(chan string, 4) + collector := &executionFinishedCollector{ch: finishedTitles} + e.AddListener(collector) + + req := &ExecutionRequest{ + AuthenticatedUser: auth.UserFromSystem(cfg, "testuser"), + Cfg: cfg, + Binding: e.FindBindingWithNoEntity(triggerAction), + } + wg, _ := e.ExecRequest(req) + wg.Wait() + + var got []string + for i := 0; i < 2; i++ { + select { + case title := <-finishedTitles: + got = append(got, title) + case <-time.After(2 * time.Second): + t.Fatalf("timed out waiting for execution %d; got %v", i+1, got) + } + } + assert.Contains(t, got, "Hello world", "triggered action must run") + assert.Contains(t, got, "Simple action that triggers another action", "triggering action must run") +} + +func TestTriggerUnknownActionTitleSkipsWithoutPanic(t *testing.T) { + cfg := config.DefaultConfig() + e := DefaultExecutor(cfg) + triggerAction := &config.Action{ + Title: "Action with bad trigger", + Shell: "echo 'ok'", + Triggers: []string{"Nonexistent action"}, + } + cfg.Actions = append(cfg.Actions, triggerAction) + cfg.Sanitize() + e.RebuildActionMap() + + finishedTitles := make(chan string, 4) + collector := &executionFinishedCollector{ch: finishedTitles} + e.AddListener(collector) + + req := &ExecutionRequest{ + AuthenticatedUser: auth.UserFromSystem(cfg, "testuser"), + Cfg: cfg, + Binding: e.FindBindingWithNoEntity(triggerAction), + } + wg, _ := e.ExecRequest(req) + wg.Wait() + + var got []string + select { + case title := <-finishedTitles: + got = append(got, title) + case <-time.After(500 * time.Millisecond): + } + assert.Len(t, got, 1, "only the triggering action runs; unknown trigger is skipped") + assert.Equal(t, "Action with bad trigger", got[0]) +} + +type executionFinishedCollector struct { + ch chan string +} + +func (c *executionFinishedCollector) OnExecutionStarted(_ *InternalLogEntry) {} + +func (c *executionFinishedCollector) OnExecutionFinished(entry *InternalLogEntry) { + c.ch <- entry.ActionTitle +} + +func (c *executionFinishedCollector) OnOutputChunk(_ []byte, _ string) {} + +func (c *executionFinishedCollector) OnActionMapRebuilt() {} From 92a1346edf0e8c6202b6e575806eaf57b9df4ef5 Mon Sep 17 00:00:00 2001 From: jamesread Date: Wed, 4 Mar 2026 23:00:28 +0000 Subject: [PATCH 04/12] docs: update security.md with the fix process --- SECURITY.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/SECURITY.md b/SECURITY.md index 13074e1..49fcf13 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -40,3 +40,16 @@ The following notes might be helpful when reporting a vulnerability: ## Disclosure of how vulnerabilities were found It is incredibly useful to not just patch security vulnerabilities, but also to understand how they were found. If you are able to share this information, it can help us and the community to better understand potential attack vectors and improve the overall security of the project. + +## Process + +Once a vulnerability is reported, the process is; + +* Accept or reject the report, and communicate with the reporter about next steps. +* If accepted, patch using a temporary branch, and code review will be requested from the original reporter if they are interested. +* The severity of the vulnerability will be assessed using CVSS, and the patch will be prioritized accordingly. +* Once the patch is ready, it will be queued for a release onto the `next` branch (3k) or `release/2k` branch (2k) +* The reporter will be credited in the advistory and the release notes, but not the commit message. +* The commit message will contain a reference to the CVSS score (eg: MED) and the advisory ID. + + From e97d8ecbd8d6ba468c418ca496fcd18f78131233 Mon Sep 17 00:00:00 2001 From: jamesread Date: Tue, 3 Mar 2026 22:47:03 +0000 Subject: [PATCH 05/12] security: GHSA-g962-2j28-3cg9 (HIGH) JWT Audience Validation Bypass in Local Key and HMAC Modes --- service/internal/auth/otjwt/jwt.go | 24 +++++++++++++++++++----- service/internal/auth/otjwt/jwt_test.go | 22 +++++++++++++++++++++- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/service/internal/auth/otjwt/jwt.go b/service/internal/auth/otjwt/jwt.go index 103224a..2b06da5 100644 --- a/service/internal/auth/otjwt/jwt.go +++ b/service/internal/auth/otjwt/jwt.go @@ -33,6 +33,13 @@ func parseJwtToken(cfg *config.Config, jwtString string) (*jwt.Token, error) { return parseJwtTokenWithHMAC(cfg, jwtString) } +func parserOptionsWithAudience(cfg *config.Config) []jwt.ParserOption { + if cfg.AuthJwtAud == "" { + return nil + } + return []jwt.ParserOption{jwt.WithAudience(cfg.AuthJwtAud)} +} + func getClaimsFromJwtToken(cfg *config.Config, jwtString string) (jwt.MapClaims, error) { token, err := parseJwtToken(cfg, jwtString) @@ -56,7 +63,8 @@ func parseJwtTokenWithRemoteKey(cfg *config.Config, jwtToken string) (*jwt.Token return nil, err } - return jwt.Parse(jwtToken, jwksVerifier.Keyfunc, jwt.WithAudience(cfg.AuthJwtAud)) + opts := parserOptionsWithAudience(cfg) + return jwt.Parse(jwtToken, jwksVerifier.Keyfunc, opts...) } var ( @@ -148,24 +156,30 @@ func parseJwtTokenWithLocalKey(cfg *config.Config, jwtString string) (*jwt.Token return nil, err } - return jwt.Parse(jwtString, func(token *jwt.Token) (interface{}, error) { + keyFunc := func(token *jwt.Token) (interface{}, error) { if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok { return nil, fmt.Errorf("parseJwt expected token algorithm RSA but got: %v", token.Header["alg"]) } return pubKey, nil - }) + } + + opts := parserOptionsWithAudience(cfg) + return jwt.Parse(jwtString, keyFunc, opts...) } // Hash-based Message Authentication Code func parseJwtTokenWithHMAC(cfg *config.Config, jwtString string) (*jwt.Token, error) { - return jwt.Parse(jwtString, func(token *jwt.Token) (interface{}, error) { + keyFunc := func(token *jwt.Token) (interface{}, error) { if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("parseJwt expected token algorithm HMAC but got: %v", token.Header["alg"]) } return []byte(cfg.AuthJwtHmacSecret), nil - }) + } + + opts := parserOptionsWithAudience(cfg) + return jwt.Parse(jwtString, keyFunc, opts...) } func lookupClaimValueOrDefault(claims jwt.MapClaims, key string, def string) string { diff --git a/service/internal/auth/otjwt/jwt_test.go b/service/internal/auth/otjwt/jwt_test.go index d3f48a8..ed4e3af 100644 --- a/service/internal/auth/otjwt/jwt_test.go +++ b/service/internal/auth/otjwt/jwt_test.go @@ -66,12 +66,19 @@ func newMux() *http.ServeMux { } func createJWTTokenWithExpiration(t *testing.T, privateKey *rsa.PrivateKey, expire int64) string { + return createJWTTokenWithExpirationAndAudience(t, privateKey, expire, "") +} + +func createJWTTokenWithExpirationAndAudience(t *testing.T, privateKey *rsa.PrivateKey, expire int64, audience string) string { token := jwt.New(jwt.SigningMethodRS256) claims := token.Claims.(jwt.MapClaims) claims["nbf"] = time.Now().Unix() - 1000 claims["exp"] = time.Now().Unix() + expire claims["sub"] = "test" claims["olivetinGroup"] = "test" + if audience != "" { + claims["aud"] = audience + } tokenStr, err := token.SignedString(privateKey) if err != nil { @@ -108,6 +115,10 @@ func verifyJWTResponse(t *testing.T, res *http.Response, expectCode int) { } func testJwkValidation(t *testing.T, expire int64, expectCode int) { + testJwkValidationWithAudience(t, expire, expectCode, "", "") +} + +func testJwkValidationWithAudience(t *testing.T, expire int64, expectCode int, configAudience, tokenAudience string) { privateKey, publicKeyPath := createKeys(t) defer os.Remove(publicKeyPath) @@ -116,8 +127,9 @@ func testJwkValidation(t *testing.T, expire int64, expectCode int) { cfg.AuthJwtClaimUsername = "sub" cfg.AuthJwtClaimUserGroup = "olivetinGroup" cfg.AuthJwtHeader = "Authorization" + cfg.AuthJwtAud = configAudience - tokenStr := createJWTTokenWithExpiration(t, privateKey, expire) + tokenStr := createJWTTokenWithExpirationAndAudience(t, privateKey, expire, tokenAudience) handler := setupJWTTestHandler(t, cfg) srv := httptest.NewServer(handler) @@ -135,6 +147,14 @@ func TestJWTSignatureVerificationFails(t *testing.T) { testJwkValidation(t, -500, 403) } +func TestJWTAudienceValidationRejectsWrongAudience(t *testing.T) { + testJwkValidationWithAudience(t, 1000, 403, "expected-audience", "wrong-audience") +} + +func TestJWTAudienceValidationAcceptsCorrectAudience(t *testing.T) { + testJwkValidationWithAudience(t, 1000, 200, "expected-audience", "expected-audience") +} + func createJWTTokenWithGroups(t *testing.T, privateKey *rsa.PrivateKey, groups interface{}) string { token := jwt.New(jwt.SigningMethodRS256) claims := token.Claims.(jwt.MapClaims) From d6a0abc3755d43107be1939567c52953bcbec3d5 Mon Sep 17 00:00:00 2001 From: jamesread Date: Wed, 4 Mar 2026 23:31:15 +0000 Subject: [PATCH 06/12] security: GHSA-gq2m-77hf-vwgh (MODERATE) Session Fixation: Logout Fails to Invalidate Server-Side Session --- service/internal/api/api.go | 2 + .../auth/otoauth2/restapi_auth_oauth2.go | 6 +++ service/internal/auth/sessions.go | 37 ++++++++++++++++++- service/internal/httpservers/frontend.go | 1 + 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/service/internal/api/api.go b/service/internal/api/api.go index 629d039..f2c81da 100644 --- a/service/internal/api/api.go +++ b/service/internal/api/api.go @@ -392,6 +392,8 @@ func (api *oliveTinAPI) ExecutionStatus(ctx ctx.Context, req *connect.Request[ap func (api *oliveTinAPI) Logout(ctx ctx.Context, req *connect.Request[apiv1.LogoutRequest]) (*connect.Response[apiv1.LogoutResponse], error) { user := auth.UserFromApiCall(ctx, req, api.cfg) + auth.RevokeSessionForProvider(api.cfg, user.Provider, user.SID) + log.WithFields(log.Fields{ "username": user.Username, "provider": user.Provider, diff --git a/service/internal/auth/otoauth2/restapi_auth_oauth2.go b/service/internal/auth/otoauth2/restapi_auth_oauth2.go index a84e1f6..351a13b 100644 --- a/service/internal/auth/otoauth2/restapi_auth_oauth2.go +++ b/service/internal/auth/otoauth2/restapi_auth_oauth2.go @@ -391,6 +391,12 @@ func (h *OAuth2Handler) lookupOAuth2UserByState(state string) (*authTypes.Authen return user, true } +func (h *OAuth2Handler) RevokeSession(sid string) { + h.mu.Lock() + defer h.mu.Unlock() + delete(h.registeredStates, sid) +} + func (h *OAuth2Handler) CheckUserFromOAuth2Cookie(context *authTypes.AuthCheckingContext) *authTypes.AuthenticatedUser { cookie, err := context.Request.Cookie("olivetin-sid-oauth") if err != nil || cookie.Value == "" { diff --git a/service/internal/auth/sessions.go b/service/internal/auth/sessions.go index f11fc8a..c8d7416 100644 --- a/service/internal/auth/sessions.go +++ b/service/internal/auth/sessions.go @@ -25,8 +25,9 @@ type SessionStorage struct { } var ( - sessionStorage *SessionStorage - sessionStorageMutex sync.RWMutex + sessionStorage *SessionStorage + sessionStorageMutex sync.RWMutex + oauth2SessionRevoker func(sid string) ) func init() { @@ -58,6 +59,38 @@ func RegisterUserSession(cfg *config.Config, provider string, sid string, userna saveUserSessions(cfg) } +// RegisterOAuth2SessionRevoker registers a callback to revoke OAuth2 sessions on logout. +// OAuth2 uses its own session storage; the API calls this when provider is oauth2. +func RegisterOAuth2SessionRevoker(fn func(sid string)) { + oauth2SessionRevoker = fn +} + +// RevokeSessionForProvider invalidates the session for the given provider and SID (e.g. on logout). +// Local auth uses shared SessionStorage; OAuth2 uses a separate storage and revoker. +func RevokeSessionForProvider(cfg *config.Config, provider string, sid string) { + if sid == "" { + return + } + if provider == "oauth2" && oauth2SessionRevoker != nil { + oauth2SessionRevoker(sid) + return + } + RevokeUserSession(cfg, provider, sid) +} + +// RevokeUserSession removes a session from storage so it can no longer be used (e.g. on logout). +func RevokeUserSession(cfg *config.Config, provider string, sid string) { + sessionStorageMutex.Lock() + defer sessionStorageMutex.Unlock() + + if sessionStorage.Providers[provider] != nil { + delete(sessionStorage.Providers[provider].Sessions, sid) + if cfg != nil { + saveUserSessions(cfg) + } + } +} + // GetUserSession retrieves a user session func GetUserSession(provider string, sid string) *UserSession { sessionStorageMutex.Lock() diff --git a/service/internal/httpservers/frontend.go b/service/internal/httpservers/frontend.go index c1c1a1f..399c6fd 100644 --- a/service/internal/httpservers/frontend.go +++ b/service/internal/httpservers/frontend.go @@ -101,6 +101,7 @@ func StartFrontendMux(cfg *config.Config, ex *executor.Executor) { oauth2handler := otoauth2.NewOAuth2Handler(cfg) auth.AddAuthChainFunction(oauth2handler.CheckUserFromOAuth2Cookie) + auth.RegisterOAuth2SessionRevoker(oauth2handler.RevokeSession) mux.HandleFunc("/oauth/login", oauth2handler.HandleOAuthLogin) mux.HandleFunc("/oauth/callback", oauth2handler.HandleOAuthCallback) From cb46a597b2465235839ed58cf034b5e7b70ef911 Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 00:04:58 +0000 Subject: [PATCH 07/12] security: GHSA-p443-p7w5-2f7f (MODERATE) RestartAction always runs actions as guest --- service/internal/api/api.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/service/internal/api/api.go b/service/internal/api/api.go index 629d039..4e0b1db 100644 --- a/service/internal/api/api.go +++ b/service/internal/api/api.go @@ -1271,8 +1271,6 @@ func (api *oliveTinAPI) RestartAction(ctx ctx.Context, req *connect.Request[apiv ExecutionTrackingId: req.Msg.ExecutionTrackingId, } - var execReqLogEntry *executor.InternalLogEntry - execReqLogEntry, found := api.executor.GetLog(req.Msg.ExecutionTrackingId) if !found { @@ -1289,12 +1287,21 @@ func (api *oliveTinAPI) RestartAction(ctx ctx.Context, req *connect.Request[apiv return connect.NewResponse(ret), nil } - return api.StartAction(ctx, &connect.Request[apiv1.StartActionRequest]{ - Msg: &apiv1.StartActionRequest{ - BindingId: execReqLogEntry.GetBindingId(), - UniqueTrackingId: req.Msg.ExecutionTrackingId, - }, - }) + authenticatedUser := auth.UserFromApiCall(ctx, req, api.cfg) + + // TrackingID is deliberately not passed to the executor, so that it generates a new one for the restarted execution. + // This is because the old execution (identified by the old TrackingID) is already used. + execReq := executor.ExecutionRequest{ + Binding: execReqLogEntry.Binding, + Arguments: make(map[string]string), + AuthenticatedUser: authenticatedUser, + Cfg: api.cfg, + } + + api.executor.ExecRequest(&execReq) + + ret.ExecutionTrackingId = execReq.TrackingID + return connect.NewResponse(ret), nil } func newServer(ex *executor.Executor) *oliveTinAPI { From 9d55d4a178573749ba93106748adf4c33269d10f Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 00:24:00 +0000 Subject: [PATCH 08/12] docs: Policy change, 2k will receive security updates much slower --- SECURITY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 49fcf13..9b1ec7f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,8 +6,8 @@ The following branches are currently being supported with security updates: | Version | Supported | | ------- | ------------------ | -| `main` (3k release branch) | :white_check_mark: | -| `release/2k` (2k release branch) | :white_check_mark: | +| `main` (3k release branch) | :white_check_mark: - advisories will be published when patched in this branch | +| `release/2k` (2k release branch) | :white_check_mark: - receives security updates, but much slower | To understand more about 2k vs 3k, see the following docs; https://docs.olivetin.app/upgrade/2k3k.html From 10f5ba62a261190efa472651b409cccd79b08d98 Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 08:07:56 +0000 Subject: [PATCH 09/12] docs: typos in SECURITY.md --- SECURITY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 9b1ec7f..f4268df 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -47,9 +47,9 @@ Once a vulnerability is reported, the process is; * Accept or reject the report, and communicate with the reporter about next steps. * If accepted, patch using a temporary branch, and code review will be requested from the original reporter if they are interested. -* The severity of the vulnerability will be assessed using CVSS, and the patch will be prioritized accordingly. +* The severity of the vulnerability will be assessed using CVSS, and the patch will be prioritised accordingly. * Once the patch is ready, it will be queued for a release onto the `next` branch (3k) or `release/2k` branch (2k) -* The reporter will be credited in the advistory and the release notes, but not the commit message. +* The reporter will be credited in the advisory and the release notes, but not the commit message. * The commit message will contain a reference to the CVSS score (eg: MED) and the advisory ID. From 9080577f2b7921add35dee70df8b6c24cfd6a184 Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 08:10:56 +0000 Subject: [PATCH 10/12] chore: potential crash in unit tests --- service/internal/executor/executor_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/service/internal/executor/executor_test.go b/service/internal/executor/executor_test.go index 16f491d..a36fc10 100644 --- a/service/internal/executor/executor_test.go +++ b/service/internal/executor/executor_test.go @@ -469,7 +469,10 @@ func TestTriggerUnknownActionTitleSkipsWithoutPanic(t *testing.T) { case <-time.After(500 * time.Millisecond): } assert.Len(t, got, 1, "only the triggering action runs; unknown trigger is skipped") - assert.Equal(t, "Action with bad trigger", got[0]) + + if len(got) > 0 { + assert.Equal(t, "Action with bad trigger", got[0]) + } } type executionFinishedCollector struct { From d7962710e7c46f6bdda4188b5b0cdbde4be665a0 Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 08:20:02 +0000 Subject: [PATCH 11/12] security: GHSA-jf73-858c-54pg (MODERATE) View permission not being checked when returning dashboards --- service/internal/api/api.go | 163 ++++++++++++++++++++--------- service/internal/api/apiActions.go | 53 ++++++---- service/internal/api/api_test.go | 115 ++++++++++++++++++++ service/internal/api/dashboards.go | 10 +- 4 files changed, 272 insertions(+), 69 deletions(-) diff --git a/service/internal/api/api.go b/service/internal/api/api.go index 629d039..a9e8c8a 100644 --- a/service/internal/api/api.go +++ b/service/internal/api/api.go @@ -70,20 +70,21 @@ func (api *oliveTinAPI) KillAction(ctx ctx.Context, req *connect.Request[apiv1.K execReqLogEntry, ret.Found = api.executor.GetLog(req.Msg.ExecutionTrackingId) if !ret.Found { - log.Warnf("Killing execution request not possible - not found by tracking ID: %v", req.Msg.ExecutionTrackingId) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } - log.Warnf("Killing execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + if execReqLogEntry.Binding == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("log entry has no binding for tracking ID %s", req.Msg.ExecutionTrackingId)) + } action := execReqLogEntry.Binding.Action if action == nil { - log.Warnf("Killing execution request not possible - action not found: %v", execReqLogEntry.ActionTitle) - ret.Killed = false - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } + log.Warnf("Killing execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + user := auth.UserFromApiCall(ctx, req, api.cfg) api.killActionByTrackingId(user, action, execReqLogEntry, ret) @@ -205,42 +206,58 @@ func (api *oliveTinAPI) LocalUserLogin(ctx ctx.Context, req *connect.Request[api return response, nil } -func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionAndWaitRequest]) (*connect.Response[apiv1.StartActionAndWaitResponse], error) { - args := make(map[string]string) - - for _, arg := range req.Msg.Arguments { - args[arg.Name] = arg.Value - } - - user := auth.UserFromApiCall(ctx, req, api.cfg) - +func (api *oliveTinAPI) startActionAndWaitRun(binding *executor.ActionBinding, args map[string]string, user *authpublic.AuthenticatedUser) (*executor.InternalLogEntry, bool) { execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: user, Cfg: api.cfg, } - wg, _ := api.executor.ExecRequest(&execReq) wg.Wait() + return api.executor.GetLog(execReq.TrackingID) +} - internalLogEntry, ok := api.executor.GetLog(execReq.TrackingID) - - if ok { - return connect.NewResponse(&apiv1.StartActionAndWaitResponse{ - LogEntry: api.internalLogEntryToPb(internalLogEntry, user), - }), nil - } else { - return nil, fmt.Errorf("execution not found") +func (api *oliveTinAPI) findBindingOrNotFound(actionId string) (*executor.ActionBinding, error) { + binding := api.executor.FindBindingByID(actionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", actionId)) } + return binding, nil +} + +func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionAndWaitRequest]) (*connect.Response[apiv1.StartActionAndWaitResponse], error) { + binding, err := api.findBindingOrNotFound(req.Msg.ActionId) + if err != nil { + return nil, err + } + + args := make(map[string]string) + for _, arg := range req.Msg.Arguments { + args[arg.Name] = arg.Value + } + user := auth.UserFromApiCall(ctx, req, api.cfg) + + internalLogEntry, ok := api.startActionAndWaitRun(binding, args, user) + if !ok { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) + } + return connect.NewResponse(&apiv1.StartActionAndWaitResponse{ + LogEntry: api.internalLogEntryToPb(internalLogEntry, user), + }), nil } func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetRequest]) (*connect.Response[apiv1.StartActionByGetResponse], error) { + binding := api.executor.FindBindingByID(req.Msg.ActionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId)) + } + args := make(map[string]string) execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: auth.UserFromApiCall(ctx, req, api.cfg), @@ -255,12 +272,17 @@ func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[a } func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetAndWaitRequest]) (*connect.Response[apiv1.StartActionByGetAndWaitResponse], error) { + binding := api.executor.FindBindingByID(req.Msg.ActionId) + if binding == nil || binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId)) + } + args := make(map[string]string) user := auth.UserFromApiCall(ctx, req, api.cfg) execReq := executor.ExecutionRequest{ - Binding: api.executor.FindBindingByID(req.Msg.ActionId), + Binding: binding, TrackingID: uuid.NewString(), Arguments: args, AuthenticatedUser: user, @@ -276,9 +298,8 @@ func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Re return connect.NewResponse(&apiv1.StartActionByGetAndWaitResponse{ LogEntry: api.internalLogEntryToPb(internalLogEntry, user), }), nil - } else { - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) } + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found")) } func calculateRateLimitExpires(api *oliveTinAPI, logEntry *executor.InternalLogEntry) string { @@ -392,6 +413,8 @@ func (api *oliveTinAPI) ExecutionStatus(ctx ctx.Context, req *connect.Request[ap func (api *oliveTinAPI) Logout(ctx ctx.Context, req *connect.Request[apiv1.LogoutRequest]) (*connect.Response[apiv1.LogoutResponse], error) { user := auth.UserFromApiCall(ctx, req, api.cfg) + auth.RevokeSessionForProvider(api.cfg, user.Provider, user.SID) + log.WithFields(log.Fields{ "username": user.Username, "provider": user.Provider, @@ -434,19 +457,38 @@ func (api *oliveTinAPI) GetActionBinding(ctx ctx.Context, req *connect.Request[a return nil, err } - binding := api.executor.FindBindingByID(req.Msg.BindingId) - - if binding == nil { - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.BindingId)) + resp, err := api.getActionBindingResponse(user, req.Msg.BindingId) + if err != nil { + return nil, err } + return connect.NewResponse(resp), nil +} - return connect.NewResponse(&apiv1.GetActionBindingResponse{ +func (api *oliveTinAPI) getActionBindingResponse(user *authpublic.AuthenticatedUser, bindingId string) (*apiv1.GetActionBindingResponse, error) { + binding := api.executor.FindBindingByID(bindingId) + if binding == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", bindingId)) + } + if binding.Action == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", bindingId)) + } + if !api.userCanViewAction(user, binding.Action) { + return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("permission denied")) + } + return &apiv1.GetActionBindingResponse{ Action: buildAction(binding, &DashboardRenderRequest{ cfg: api.cfg, AuthenticatedUser: user, ex: api.executor, }), - }), nil + }, nil +} + +func (api *oliveTinAPI) userCanViewAction(user *authpublic.AuthenticatedUser, action *config.Action) bool { + if user == nil { + return true + } + return acl.IsAllowedView(api.cfg, user, action) } func (api *oliveTinAPI) GetDashboard(ctx ctx.Context, req *connect.Request[apiv1.GetDashboardRequest]) (*connect.Response[apiv1.GetDashboardResponse], error) { @@ -646,7 +688,16 @@ error messages more quickly before starting the action. It uses the same validation logic as the executor, including mangling argument values (e.g., datetime formatting, checkbox title-to-value conversion). */ +func (api *oliveTinAPI) argumentNotFoundForValidation(msg *apiv1.ValidateArgumentTypeRequest) bool { + arg, _ := api.findArgumentForValidation(msg.BindingId, msg.ArgumentName) + return arg == nil && (msg.BindingId != "" || msg.ArgumentName != "") +} + func (api *oliveTinAPI) ValidateArgumentType(ctx ctx.Context, req *connect.Request[apiv1.ValidateArgumentTypeRequest]) (*connect.Response[apiv1.ValidateArgumentTypeResponse], error) { + if api.argumentNotFoundForValidation(req.Msg) { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action or argument not found for binding ID %s", req.Msg.BindingId)) + } + err := api.validateArgumentTypeInternal(req.Msg) desc := "" if err != nil { @@ -747,6 +798,13 @@ func (api *oliveTinAPI) DumpVars(ctx ctx.Context, req *connect.Request[apiv1.Dum return connect.NewResponse(res), nil } +func debugBindingActionTitle(binding *executor.ActionBinding) string { + if binding == nil || binding.Action == nil { + return "" + } + return binding.Action.Title +} + func (api *oliveTinAPI) DumpPublicIdActionMap(ctx ctx.Context, req *connect.Request[apiv1.DumpPublicIdActionMapRequest]) (*connect.Response[apiv1.DumpPublicIdActionMapResponse], error) { res := &apiv1.DumpPublicIdActionMapResponse{} res.Contents = make(map[string]*apiv1.DebugBinding) @@ -761,7 +819,7 @@ func (api *oliveTinAPI) DumpPublicIdActionMap(ctx ctx.Context, req *connect.Requ for k, v := range api.executor.MapActionBindings { res.Contents[k] = &apiv1.DebugBinding{ - ActionTitle: v.Action.Title, + ActionTitle: debugBindingActionTitle(v), } } @@ -1271,30 +1329,37 @@ func (api *oliveTinAPI) RestartAction(ctx ctx.Context, req *connect.Request[apiv ExecutionTrackingId: req.Msg.ExecutionTrackingId, } - var execReqLogEntry *executor.InternalLogEntry - execReqLogEntry, found := api.executor.GetLog(req.Msg.ExecutionTrackingId) if !found { - log.Warnf("Restarting execution request not possible - not found by tracking ID: %v", req.Msg.ExecutionTrackingId) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("execution not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } - log.Warnf("Restarting execution request by tracking ID: %v", req.Msg.ExecutionTrackingId) + if execReqLogEntry.Binding == nil { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("log entry has no binding for tracking ID %s", req.Msg.ExecutionTrackingId)) + } action := execReqLogEntry.Binding.Action if action == nil { - log.Warnf("Restarting execution request not possible - action not found: %v", execReqLogEntry.ActionTitle) - return connect.NewResponse(ret), nil + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action not found for tracking ID %s", req.Msg.ExecutionTrackingId)) } - return api.StartAction(ctx, &connect.Request[apiv1.StartActionRequest]{ - Msg: &apiv1.StartActionRequest{ - BindingId: execReqLogEntry.GetBindingId(), - UniqueTrackingId: req.Msg.ExecutionTrackingId, - }, - }) + authenticatedUser := auth.UserFromApiCall(ctx, req, api.cfg) + + // TrackingID is deliberately not passed to the executor, so that it generates a new one for the restarted execution. + // This is because the old execution (identified by the old TrackingID) is already used. + execReq := executor.ExecutionRequest{ + Binding: execReqLogEntry.Binding, + Arguments: make(map[string]string), + AuthenticatedUser: authenticatedUser, + Cfg: api.cfg, + } + + api.executor.ExecRequest(&execReq) + + ret.ExecutionTrackingId = execReq.TrackingID + return connect.NewResponse(ret), nil } func newServer(ex *executor.Executor) *oliveTinAPI { diff --git a/service/internal/api/apiActions.go b/service/internal/api/apiActions.go index 355f179..4a23125 100644 --- a/service/internal/api/apiActions.go +++ b/service/internal/api/apiActions.go @@ -28,18 +28,22 @@ func (rr *DashboardRenderRequest) findAction(title string) *apiv1.Action { return rr.findActionForEntity(title, nil) } +func bindingMatchesTitleAndEntity(binding *executor.ActionBinding, title string, entity *entities.Entity) bool { + return binding != nil && binding.Action != nil && binding.Action.Title == title && matchesEntity(binding, entity) +} + func (rr *DashboardRenderRequest) findActionForEntity(title string, entity *entities.Entity) *apiv1.Action { rr.ex.MapActionBindingsLock.RLock() defer rr.ex.MapActionBindingsLock.RUnlock() for _, binding := range rr.ex.MapActionBindings { - if binding.Action.Title != title { + if !bindingMatchesTitleAndEntity(binding, title, entity) { continue } - - if matchesEntity(binding, entity) { - return buildAction(binding, rr) + if !acl.IsAllowedView(rr.cfg, rr.AuthenticatedUser, binding.Action) { + return nil } + return buildAction(binding, rr) } return nil @@ -117,26 +121,37 @@ func getDefaultArgumentValue(cfgArg config.ActionArgument, entity *entities.Enti return defaultValue } +func formatRateLimitExpiry(expiryUnix int64) string { + if expiryUnix <= 0 { + return "" + } + return time.Unix(expiryUnix, 0).Format("2006-01-02 15:04:05") +} + +func actionFromBinding(actionBinding *executor.ActionBinding) (*executor.ActionBinding, *config.Action) { + if actionBinding == nil || actionBinding.Action == nil { + return nil, nil + } + return actionBinding, actionBinding.Action +} + func buildAction(actionBinding *executor.ActionBinding, rr *DashboardRenderRequest) *apiv1.Action { - action := actionBinding.Action - - aclCanExec := acl.IsAllowedExec(rr.cfg, rr.AuthenticatedUser, action) - enabledExprCanExec := evaluateEnabledExpression(action, actionBinding.Entity) - - // Calculate rate limit expiry time - expiryUnix := rr.ex.GetTimeUntilAvailable(actionBinding) - datetimeRateLimitExpires := "" - if expiryUnix > 0 { - datetimeRateLimitExpires = time.Unix(expiryUnix, 0).Format("2006-01-02 15:04:05") + binding, action := actionFromBinding(actionBinding) + if binding == nil { + return nil } + aclCanExec := acl.IsAllowedExec(rr.cfg, rr.AuthenticatedUser, action) + enabledExprCanExec := evaluateEnabledExpression(action, binding.Entity) + datetimeRateLimitExpires := formatRateLimitExpiry(rr.ex.GetTimeUntilAvailable(binding)) + btn := apiv1.Action{ - BindingId: actionBinding.ID, - Title: tpl.ParseTemplateOfActionBeforeExec(action.Title, actionBinding.Entity), - Icon: tpl.ParseTemplateOfActionBeforeExec(action.Icon, actionBinding.Entity), + BindingId: binding.ID, + Title: tpl.ParseTemplateOfActionBeforeExec(action.Title, binding.Entity), + Icon: tpl.ParseTemplateOfActionBeforeExec(action.Icon, binding.Entity), CanExec: aclCanExec && enabledExprCanExec, PopupOnStart: action.PopupOnStart, - Order: int32(actionBinding.ConfigOrder), + Order: int32(binding.ConfigOrder), Timeout: int32(action.Timeout), DatetimeRateLimitExpires: datetimeRateLimitExpires, } @@ -147,7 +162,7 @@ func buildAction(actionBinding *executor.ActionBinding, rr *DashboardRenderReque Title: cfgArg.Title, Type: cfgArg.Type, Description: cfgArg.Description, - DefaultValue: getDefaultArgumentValue(cfgArg, actionBinding.Entity), + DefaultValue: getDefaultArgumentValue(cfgArg, binding.Entity), Choices: buildChoices(cfgArg), Suggestions: cfgArg.Suggestions, SuggestionsBrowserKey: cfgArg.SuggestionsBrowserKey, diff --git a/service/internal/api/api_test.go b/service/internal/api/api_test.go index 790e315..6e24d33 100644 --- a/service/internal/api/api_test.go +++ b/service/internal/api/api_test.go @@ -6,6 +6,7 @@ import ( "connectrpc.com/connect" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" log "github.com/sirupsen/logrus" @@ -335,3 +336,117 @@ func testWithEntity(t *testing.T, binding *executor.ActionBinding, rr *Dashboard actionResult := buildAction(binding, rr) assert.Equal(t, expectedCanExec, actionResult.CanExec, message) } + +// buildViewPermissionTestConfig returns config and users for GHSA view-permission tests: +// one action "secret_action", ACL "restricted" (view:false) for user "low", ACL "full" (view:true) for user "admin". +func buildViewPermissionTestConfig(t *testing.T) (*config.Config, *authpublic.AuthenticatedUser, *authpublic.AuthenticatedUser) { + t.Helper() + cfg := config.DefaultConfig() + cfg.DefaultPermissions.View = false + cfg.DefaultPermissions.Exec = false + + cfg.Actions = append(cfg.Actions, &config.Action{ + ID: "secret_action", + Title: "Secret Action", + Shell: "echo sensitive", + Icon: "🔒", + }) + + cfg.AccessControlLists = append(cfg.AccessControlLists, + &config.AccessControlList{ + Name: "restricted", + MatchUsernames: []string{"low"}, + AddToEveryAction: true, + Permissions: config.PermissionsList{View: false, Exec: false, Logs: false, Kill: false}, + }, + &config.AccessControlList{ + Name: "full", + MatchUsernames: []string{"admin"}, + AddToEveryAction: true, + Permissions: config.PermissionsList{View: true, Exec: true, Logs: true, Kill: true}, + }, + ) + + lowUser := &authpublic.AuthenticatedUser{Username: "low"} + lowUser.BuildUserAcls(cfg) + adminUser := &authpublic.AuthenticatedUser{Username: "admin"} + adminUser.BuildUserAcls(cfg) + return cfg, lowUser, adminUser +} + +// TestViewPermissionExcludedFromDashboard (GHSA: view permission) asserts that when a user has view: false, +// the default dashboard must not include that action. Covers GetDashboard not leaking action metadata. +func TestViewPermissionExcludedFromDashboard(t *testing.T) { + cfg, lowUser, _ := buildViewPermissionTestConfig(t) + ex := executor.DefaultExecutor(cfg) + ex.RebuildActionMap() + + rr := &DashboardRenderRequest{ + AuthenticatedUser: lowUser, + cfg: cfg, + ex: ex, + } + db := buildDefaultDashboard(rr) + + bindingIdsInDashboard := bindingIdsInDashboardContents(db.Contents) + assert.NotContains(t, bindingIdsInDashboard, "secret_action", + "user with view:false must not see action in dashboard; got bindingIds: %v", bindingIdsInDashboard) +} + +// TestGetActionBindingDeniedWhenNoViewPermission (GHSA: view permission) asserts that GetActionBinding +// returns permission denied for a user with view: false. Covers GetActionBinding not exposing action details. +func TestGetActionBindingDeniedWhenNoViewPermission(t *testing.T) { + cfg, lowUser, _ := buildViewPermissionTestConfig(t) + ex := executor.DefaultExecutor(cfg) + ex.RebuildActionMap() + api := newServer(ex) + + _, err := api.getActionBindingResponse(lowUser, "secret_action") + require.Error(t, err) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err), + "user with view:false must get permission denied from GetActionBinding") +} + +// TestViewPermissionAllowedSeesAction (GHSA: view permission) asserts that a user with view: true +// still sees the action in the dashboard and can fetch it via GetActionBinding. +func TestViewPermissionAllowedSeesAction(t *testing.T) { + cfg, _, adminUser := buildViewPermissionTestConfig(t) + ex := executor.DefaultExecutor(cfg) + ex.RebuildActionMap() + api := newServer(ex) + + rr := &DashboardRenderRequest{ + AuthenticatedUser: adminUser, + cfg: cfg, + ex: ex, + } + db := buildDefaultDashboard(rr) + bindingIdsInDashboard := bindingIdsInDashboardContents(db.Contents) + assert.Contains(t, bindingIdsInDashboard, "secret_action", + "user with view:true must see action in dashboard; got bindingIds: %v", bindingIdsInDashboard) + + resp, err := api.getActionBindingResponse(adminUser, "secret_action") + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Action) + assert.Equal(t, "secret_action", resp.Action.BindingId) +} + +func bindingIdsInDashboardContents(contents []*apiv1.DashboardComponent) []string { + var ids []string + for _, c := range contents { + ids = append(ids, bindingIdsFromComponent(c)...) + } + return ids +} + +func bindingIdsFromComponent(c *apiv1.DashboardComponent) []string { + if c == nil { + return nil + } + var ids []string + if c.Action != nil && c.Action.BindingId != "" { + ids = append(ids, c.Action.BindingId) + } + return append(ids, bindingIdsInDashboardContents(c.Contents)...) +} diff --git a/service/internal/api/dashboards.go b/service/internal/api/dashboards.go index 11c6cf3..575994c 100644 --- a/service/internal/api/dashboards.go +++ b/service/internal/api/dashboards.go @@ -4,6 +4,7 @@ import ( "sort" apiv1 "github.com/OliveTin/OliveTin/gen/olivetin/api/v1" + acl "github.com/OliveTin/OliveTin/internal/acl" config "github.com/OliveTin/OliveTin/internal/config" entities "github.com/OliveTin/OliveTin/internal/entities" "github.com/OliveTin/OliveTin/internal/tpl" @@ -130,7 +131,7 @@ func buildDefaultDashboard(rr *DashboardRenderRequest) *apiv1.Dashboard { } for _, binding := range rr.ex.MapActionBindings { - if binding.Action.Hidden { + if binding == nil || binding.Action == nil || binding.Action.Hidden { continue } @@ -138,7 +139,14 @@ func buildDefaultDashboard(rr *DashboardRenderRequest) *apiv1.Dashboard { continue } + if !acl.IsAllowedView(rr.cfg, rr.AuthenticatedUser, binding.Action) { + continue + } + action := buildAction(binding, rr) + if action == nil { + continue + } fieldset.Contents = append(fieldset.Contents, &apiv1.DashboardComponent{ Type: "link", From b032ae5e5e3fc5f9a0d3c8f66b9dce7bd276c9ef Mon Sep 17 00:00:00 2001 From: jamesread Date: Thu, 5 Mar 2026 08:29:30 +0000 Subject: [PATCH 12/12] chore: fix regression on arguments not being found --- service/internal/api/api.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/service/internal/api/api.go b/service/internal/api/api.go index 20e41dd..83465aa 100644 --- a/service/internal/api/api.go +++ b/service/internal/api/api.go @@ -466,15 +466,15 @@ func (api *oliveTinAPI) GetActionBinding(ctx ctx.Context, req *connect.Request[a func (api *oliveTinAPI) getActionBindingResponse(user *authpublic.AuthenticatedUser, bindingId string) (*apiv1.GetActionBindingResponse, error) { binding := api.executor.FindBindingByID(bindingId) - + if binding == nil || binding.Action == nil { return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", bindingId)) } - + if !api.userCanViewAction(user, binding.Action) { return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("permission denied")) } - + return &apiv1.GetActionBindingResponse{ Action: buildAction(binding, &DashboardRenderRequest{ cfg: api.cfg, @@ -689,8 +689,13 @@ It uses the same validation logic as the executor, including mangling argument values (e.g., datetime formatting, checkbox title-to-value conversion). */ func (api *oliveTinAPI) argumentNotFoundForValidation(msg *apiv1.ValidateArgumentTypeRequest) bool { + if msg.BindingId == "" || msg.ArgumentName == "" { + return false + } + arg, _ := api.findArgumentForValidation(msg.BindingId, msg.ArgumentName) - return arg == nil && (msg.BindingId != "" || msg.ArgumentName != "") + + return arg == nil } func (api *oliveTinAPI) ValidateArgumentType(ctx ctx.Context, req *connect.Request[apiv1.ValidateArgumentTypeRequest]) (*connect.Response[apiv1.ValidateArgumentTypeResponse], error) {