From c654f0502f2d029a638773a9de3352af826bc43f Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Fri, 27 Mar 2026 02:30:55 -0400 Subject: [PATCH] [MM-67143] cherry pick of #34922 (#35800) Automatic Merge --- server/channels/api4/command_test.go | 141 ++++++++++++++++++++ server/channels/app/command.go | 14 +- server/channels/app/command_autocomplete.go | 7 +- server/i18n/en.json | 8 ++ 4 files changed, 168 insertions(+), 2 deletions(-) diff --git a/server/channels/api4/command_test.go b/server/channels/api4/command_test.go index 953a1671b4..786f077ac9 100644 --- a/server/channels/api4/command_test.go +++ b/server/channels/api4/command_test.go @@ -6,6 +6,7 @@ package api4 import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -17,6 +18,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" + "github.com/mattermost/mattermost/server/v8/channels/testlib" ) func TestCreateCommand(t *testing.T) { @@ -956,14 +958,17 @@ func TestExecuteInvalidCommand(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { rc := &model.CommandResponse{} @@ -1029,14 +1034,17 @@ func TestExecuteGetCommand(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) token := model.NewId() expectedCommandResponse := &model.CommandResponse{ @@ -1092,14 +1100,17 @@ func TestExecutePostCommand(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) token := model.NewId() expectedCommandResponse := &model.CommandResponse{ @@ -1264,16 +1275,19 @@ func TestExecuteCommandInDirectMessageChannel(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) // create a team that the user isn't a part of team2 := th.CreateTeam() @@ -1329,16 +1343,19 @@ func TestExecuteCommandInTeamUserIsNotOn(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) // create a team that the user isn't a part of team2 := th.CreateTeam() @@ -1407,16 +1424,19 @@ func TestExecuteCommandReadOnly(t *testing.T) { enableCommands := *th.App.Config().ServiceSettings.EnableCommands allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) }() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) expectedCommandResponse := &model.CommandResponse{ Text: "test post command response", @@ -1494,3 +1514,124 @@ func TestExecuteCommandReadOnly(t *testing.T) { require.Error(t, err) CheckBadRequestStatus(t, resp) } + +func TestExecuteCommandResponseURLUsesSiteURL(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + client := th.Client + channel := th.BasicChannel + + enableCommands := *th.App.Config().ServiceSettings.EnableCommands + allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections + }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) + }() + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" }) + + // Set a SiteURL that differs from the test client's Host header (localhost). + // This verifies that response_url uses the configured SiteURL, not the Host header. + // Before the fix (MM-67142), response_url would contain "localhost" and fail this check. + expectedSiteURL := "http://mattermost.example.com" + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = expectedSiteURL }) + + var receivedResponseURL string + expectedCommandResponse := &model.CommandResponse{ + Text: "test response_url command response", + ResponseType: model.CommandResponseTypeInChannel, + } + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + err := r.ParseForm() + require.NoError(t, err) + + // Capture the response_url sent by the server + receivedResponseURL = r.FormValue("response_url") + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(expectedCommandResponse); err != nil { + th.TestLogger.Warn("Error while writing response", mlog.Err(err)) + } + })) + defer ts.Close() + + postCmd := &model.Command{ + CreatorId: th.BasicUser.Id, + TeamId: th.BasicTeam.Id, + URL: ts.URL, + Method: model.CommandMethodPost, + Trigger: "testrespurl", + } + + _, appErr := th.App.CreateCommand(postCmd) + require.Nil(t, appErr, "failed to create command") + + _, _, err := client.ExecuteCommand(context.Background(), channel.Id, "/testrespurl") + require.NoError(t, err) + + // Verify response_url starts with the configured SiteURL, not the Host header + require.True(t, strings.HasPrefix(receivedResponseURL, expectedSiteURL), + "response_url should start with configured SiteURL %q, but got %q", expectedSiteURL, receivedResponseURL) + + // Verify warning is logged when Host header differs from SiteURL + require.NoError(t, th.TestLogger.Flush()) + testlib.AssertLog(t, th.LogBuffer, mlog.LvlWarn.Name, "Request hostname differs from configured SiteURL. The configured SiteURL will be used for the slash command response URL.") +} + +func TestExecuteCustomCommandRequiresSiteURL(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + client := th.Client + channel := th.BasicChannel + + enableCommands := *th.App.Config().ServiceSettings.EnableCommands + allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + siteURL := *th.App.Config().ServiceSettings.SiteURL + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections + }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = &siteURL }) + }() + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" }) + + // Set SiteURL to a valid value first so we can create the command + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "http://localhost:8065" }) + + // Create a custom command + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintln(w, `{"text": "response"}`) + })) + defer ts.Close() + + postCmd := &model.Command{ + CreatorId: th.BasicUser.Id, + TeamId: th.BasicTeam.Id, + URL: ts.URL, + Method: model.CommandMethodPost, + Trigger: "customcmd", + } + _, appErr := th.App.CreateCommand(postCmd) + require.Nil(t, appErr, "failed to create command") + + // Now set SiteURL to empty + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = "" }) + + // Try to execute the custom command - should fail because SiteURL is required for custom commands + _, resp, err := client.ExecuteCommand(context.Background(), channel.Id, "/customcmd") + require.Error(t, err) + CheckBadRequestStatus(t, resp) + CheckErrorID(t, err, "api.command.execute_command.site_url_required.app_error") + + // Built-in commands should still work without SiteURL + _, _, err = client.ExecuteCommand(context.Background(), channel.Id, "/echo test") + require.NoError(t, err) +} diff --git a/server/channels/app/command.go b/server/channels/app/command.go index c857fe3a83..eaf3b3c4ac 100644 --- a/server/channels/app/command.go +++ b/server/channels/app/command.go @@ -475,11 +475,23 @@ func (a *App) tryExecuteCustomCommand(c request.CTX, args *model.CommandArgs, tr p[key] = values } + // Use configured SiteURL for response_url to prevent SSRF via Host header spoofing (MM-67142) + siteURL := *a.Config().ServiceSettings.SiteURL + if siteURL == "" { + return cmd, nil, model.NewAppError("tryExecuteCustomCommand", "api.command.execute_command.site_url_required.app_error", nil, "", http.StatusBadRequest) + } + if siteURL != args.SiteURL { + c.Logger().Warn(i18n.T("api.command.execute_command.provided_site_url_different.app_error"), + mlog.String("request_host", args.SiteURL), + mlog.String("configured_site_url", siteURL), + mlog.String("command", trigger)) + } + hook, appErr := a.CreateCommandWebhook(cmd.Id, args) if appErr != nil { return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]any{"Trigger": trigger}, "", http.StatusInternalServerError).Wrap(appErr) } - p.Set("response_url", args.SiteURL+"/hooks/commands/"+hook.Id) + p.Set("response_url", siteURL+"/hooks/commands/"+hook.Id) return a.DoCommandRequest(c, cmd, p) } diff --git a/server/channels/app/command_autocomplete.go b/server/channels/app/command_autocomplete.go index 17eee0958d..be6222ec72 100644 --- a/server/channels/app/command_autocomplete.go +++ b/server/channels/app/command_autocomplete.go @@ -269,7 +269,12 @@ func (a *App) getDynamicListArgument(c request.CTX, commandArgs *model.CommandAr params.Add("team_id", commandArgs.TeamId) params.Add("root_id", commandArgs.RootId) params.Add("user_id", commandArgs.UserId) - params.Add("site_url", commandArgs.SiteURL) + + // Use configured SiteURL to prevent SSRF via Host header spoofing (MM-67142) + siteURL := *a.Config().ServiceSettings.SiteURL + if siteURL != "" { + params.Add("site_url", siteURL) + } resp, err := a.doPluginRequest(c, "GET", dynamicArg.FetchURL, params, nil) diff --git a/server/i18n/en.json b/server/i18n/en.json index a773289a04..3e679f5d75 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -679,6 +679,14 @@ "id": "api.command.execute_command.not_found.app_error", "translation": "Command with a trigger of '{{.Trigger}}' not found. To send a message beginning with \"/\", try adding an empty space at the beginning of the message." }, + { + "id": "api.command.execute_command.provided_site_url_different.app_error", + "translation": "Request hostname differs from configured SiteURL. The configured SiteURL will be used for the slash command response URL." + }, + { + "id": "api.command.execute_command.site_url_required.app_error", + "translation": "SiteURL must be configured to use slash commands" + }, { "id": "api.command.execute_command.start.app_error", "translation": "No command trigger found."