diff --git a/e2e-tests/cypress/tests/integration/channels/enterprise/saml/saml_metadata_spec.ts b/e2e-tests/cypress/tests/integration/channels/enterprise/saml/saml_metadata_spec.ts index 324a21e3593..d10e4200913 100644 --- a/e2e-tests/cypress/tests/integration/channels/enterprise/saml/saml_metadata_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/enterprise/saml/saml_metadata_spec.ts @@ -16,9 +16,14 @@ import {AdminConfig} from '@mattermost/types/config'; * Note: This test requires Enterprise license to be uploaded */ const testSamlMetadataUrl = 'http://test_saml_metadata_url'; +const testSamlMetadataSuccessUrl = 'http://test_saml_metadata_success_url'; const testIdpURL = 'http://test_idp_url'; const testIdpDescriptorURL = 'http://test_idp_descriptor_url'; +const testFetchedIdpURL = 'http://test_fetched_idp_url'; +const testFetchedIdpDescriptorURL = 'http://test_fetched_idp_descriptor_url'; +const testIdpPublicCertificate = 'MIICozCCAYsCBgGNzWfMwjANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDDAptYXR0ZXJtb3N0'; const getSamlMetadataErrorMessage = 'SAML Metadata URL did not connect and pull data successfully'; +const getSamlMetadataSuccessMessage = 'SAML Metadata retrieved successfully. Two fields and one certificate have been updated'; let config: AdminConfig; @@ -82,4 +87,58 @@ describe('SystemConsole->SAML 2.0 - Get Metadata from Idp Flow', () => { // * Verify that we can successfully save the settings (we have not affected previous state) cy.get('#saveSetting').click(); }); + + it('fetches metadata and sets the IdP certificate from Idp Metadata Url', () => { + cy.apiUpdateConfig({ + SamlSettings: { + Enable: true, + IdpMetadataURL: testSamlMetadataSuccessUrl, + IdpURL: testIdpURL, + IdpDescriptorURL: testIdpDescriptorURL, + AssertionConsumerServiceURL: Cypress.config('baseUrl') + '/login/sso/saml', + ServiceProviderIdentifier: Cypress.config('baseUrl') + '/login/sso/saml', + }, + }); + + cy.visit('/admin_console/authentication/saml'); + + cy.intercept('POST', '**/api/v4/saml/metadatafromidp', (req) => { + req.reply({ + statusCode: 200, + body: { + idp_url: testFetchedIdpURL, + idp_descriptor_url: testFetchedIdpDescriptorURL, + idp_public_certificate: testIdpPublicCertificate, + }, + }); + }).as('getSamlMetadataFromIdp'); + + cy.intercept('POST', '**/api/v4/saml/certificate/idp', (req) => { + expect(req.headers['content-type']).to.eq('application/x-pem-file'); + expect(req.body).to.eq(testIdpPublicCertificate); + + req.reply({ + statusCode: 200, + body: {status: 'OK'}, + }); + }).as('setSamlIdpCertificateFromMetadata'); + + // # Click on the Get SAML Metadata Button + cy.get('#getSamlMetadataFromIDPButton button').scrollIntoView().should('be.visible').and('be.enabled').click(); + + // * Verify that the metadata and certificate endpoints are called + cy.wait('@getSamlMetadataFromIdp'); + cy.wait('@setSamlIdpCertificateFromMetadata'); + + // * Verify that the IdP URL fields have been updated + cy.findByTestId('SamlSettings.IdpURLinput').should('have.value', testFetchedIdpURL); + cy.findByTestId('SamlSettings.IdpDescriptorURLinput').should('have.value', testFetchedIdpDescriptorURL); + + // * Verify that the success message reflects the updated fields and certificate + cy.get('#getSamlMetadataFromIDPButton').should('be.visible').contains(getSamlMetadataSuccessMessage); + + // * Verify that the IdP certificate row shows the remove certificate view + cy.contains('.remove-filename', 'saml-idp.crt').should('be.visible'); + cy.contains('button', 'Remove Identity Provider Certificate').should('be.visible'); + }); }); diff --git a/e2e-tests/playwright/lib/src/server/default_config.ts b/e2e-tests/playwright/lib/src/server/default_config.ts index b4448dd89b9..8e965d4d195 100644 --- a/e2e-tests/playwright/lib/src/server/default_config.ts +++ b/e2e-tests/playwright/lib/src/server/default_config.ts @@ -779,7 +779,6 @@ const defaultServerConfig: AdminConfig = { AttributeBasedAccessControl: true, PermissionPolicies: true, ContentFlagging: true, - InteractiveDialogAppsForm: true, EnableMattermostEntry: true, MobileSSOCodeExchange: false, AutoTranslation: true, diff --git a/server/.go-version b/server/.go-version index c7c3f3333e1..f8f73814096 100644 --- a/server/.go-version +++ b/server/.go-version @@ -1 +1 @@ -1.26.2 +1.26.3 diff --git a/server/build/Dockerfile.buildenv b/server/build/Dockerfile.buildenv index dde37dbddc6..54583233d27 100644 --- a/server/build/Dockerfile.buildenv +++ b/server/build/Dockerfile.buildenv @@ -1,4 +1,4 @@ -FROM mattermost/golang-bullseye:1.26.2@sha256:fc2cc64f035c74f14b0e5921971bcda4b6dbd281430d3cbfb0bf539ebb1bacd5 +FROM mattermost/golang-bullseye:1.26.3@sha256:3ae112b7dc291665c5582b9d768fc2adb4cdc3afbbd3fc82e03a10cd711e1a60 ARG NODE_VERSION=20.11.1 RUN apt-get update && apt-get install -y make git apt-transport-https ca-certificates curl software-properties-common build-essential zip xmlsec1 jq pgloader gnupg diff --git a/server/build/Dockerfile.buildenv-fips b/server/build/Dockerfile.buildenv-fips index 17f8ceadfff..9a986d75132 100644 --- a/server/build/Dockerfile.buildenv-fips +++ b/server/build/Dockerfile.buildenv-fips @@ -1,4 +1,4 @@ -FROM cgr.dev/mattermost.com/go-msft-fips:1.26.2-dev@sha256:cdd5bd448fcf61654893846572f006aff7349aa21027de590fc2bd020f8126f1 +FROM cgr.dev/mattermost.com/go-msft-fips:1.26.3-dev@sha256:48ab99fede7fb33e132a0636072971e1ec4a69520865bfa1e4b517ee9cfdef34 ARG NODE_VERSION=20.11.1 RUN apk add curl ca-certificates mailcap unrtf wv poppler-utils tzdata gpg xmlsec diff --git a/server/channels/api4/integration_action.go b/server/channels/api4/integration_action.go index c0ab52462a1..70a9f4c0f23 100644 --- a/server/channels/api4/integration_action.go +++ b/server/channels/api4/integration_action.go @@ -5,7 +5,8 @@ package api4 import ( "encoding/json" - "fmt" + "errors" + "io" "net/http" "github.com/mattermost/mattermost/server/public/model" @@ -20,22 +21,6 @@ func (api *API) InitAction() { api.BaseRoutes.APIRoot.Handle("/actions/dialogs/lookup", api.APISessionRequired(lookupDialog)).Methods(http.MethodPost) } -// getStringValue safely converts an interface{} value to a string with logging for failures. -// It handles nil values gracefully and logs warnings when conversion fails. -func getStringValue(val any, fieldName string, logger *mlog.Logger) string { - if val == nil { - return "" - } - if str, ok := val.(string); ok { - return str - } - logger.Warn("Failed to convert field to string", - mlog.String("field", fieldName), - mlog.String("type", fmt.Sprintf("%T", val)), - mlog.Any("value", val)) - return "" -} - func doPostAction(c *Context, w http.ResponseWriter, r *http.Request) { c.RequirePostId() if c.Err != nil { @@ -43,9 +28,26 @@ func doPostAction(c *Context, w http.ResponseWriter, r *http.Request) { } var actionRequest model.DoPostActionRequest - err := json.NewDecoder(r.Body).Decode(&actionRequest) - if err != nil { - c.Logger.Warn("Error decoding the action request", mlog.Err(err)) + dec := json.NewDecoder(r.Body) + err := dec.Decode(&actionRequest) + if err != nil && !errors.Is(err, io.EOF) { + // Empty body is allowed for backward-compatibility with older clients. + // Any other decode failure means the request cannot be trusted — in + // particular, a wrong-type query would otherwise fall through as nil + // and silently execute the action without the caller's params. + c.SetInvalidParamWithErr("action_request", err) + return + } + if err == nil { + // Reject trailing JSON values after the first object (e.g. + // `{"query":{"k":"v"}}{"cookie":"x"}`). json.Decoder.Decode + // stops at the first complete value and would otherwise silently + // ignore the rest, leaving the caller's intent ambiguous. + var trailing any + if extraErr := dec.Decode(&trailing); !errors.Is(extraErr, io.EOF) { + c.SetInvalidParamWithErr("action_request", extraErr) + return + } } var cookie *model.PostActionCookie @@ -82,7 +84,7 @@ func doPostAction(c *Context, w http.ResponseWriter, r *http.Request) { resp := &model.PostActionAPIResponse{Status: "OK"} resp.TriggerId, appErr = c.App.DoPostActionWithCookie(c.AppContext, c.Params.PostId, c.Params.ActionId, c.AppContext.Session().UserId, - actionRequest.SelectedOption, cookie) + actionRequest.SelectedOption, cookie, actionRequest.Query) if appErr != nil { c.Err = appErr return @@ -204,8 +206,8 @@ func lookupDialog(c *Context, w http.ResponseWriter, r *http.Request) { mlog.String("user_id", lookup.UserId), mlog.String("channel_id", lookup.ChannelId), mlog.String("team_id", lookup.TeamId), - mlog.String("selected_field", getStringValue(lookup.Submission["selected_field"], "selected_field", c.Logger)), - mlog.String("query", getStringValue(lookup.Submission["query"], "query", c.Logger)), + mlog.Any("selected_field", lookup.Submission["selected_field"]), + mlog.Any("query", lookup.Submission["query"]), ) resp, err := c.App.LookupInteractiveDialog(c.AppContext, lookup) diff --git a/server/channels/api4/integration_action_test.go b/server/channels/api4/integration_action_test.go index d9b6cd7fd9d..dd61ecc973c 100644 --- a/server/channels/api4/integration_action_test.go +++ b/server/channels/api4/integration_action_test.go @@ -6,9 +6,11 @@ package api4 import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -533,3 +535,189 @@ func TestLookupDialog(t *testing.T) { assert.Empty(t, lookupResp.Items) }) } + +// newAttachmentActionPost posts an attachment action pointing at upstreamURL, +// attributed to th.BasicUser so th.Client has access to call the action. +func newAttachmentActionPost(t *testing.T, th *TestHelper, upstreamURL string) (*model.Post, string) { + t.Helper() + basicPost := &model.Post{ + Message: "attachment action post", + ChannelId: th.BasicChannel.Id, + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + model.PostPropsAttachments: []*model.MessageAttachment{ + { + Text: "hello", + Actions: []*model.PostAction{ + { + Type: model.PostActionTypeButton, + Name: "click", + Integration: &model.PostActionIntegration{ + URL: upstreamURL, + }, + }, + }, + }, + }, + }, + } + created, _, appErr := th.App.CreatePostAsUser(th.Context, basicPost, "", true) + require.Nil(t, appErr) + + attachments, ok := created.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) + require.True(t, ok) + require.NotEmpty(t, attachments) + require.NotEmpty(t, attachments[0].Actions) + require.NotEmpty(t, attachments[0].Actions[0].Id) + return created, attachments[0].Actions[0].Id +} + +func TestDoPostActionQuery_ValidationErrors(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + client := th.Client + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + created, actionID := newAttachmentActionPost(t, th, ts.URL) + route := "/posts/" + created.Id + "/actions/" + actionID + + t.Run("too many entries returns 400 with expected error id", func(t *testing.T) { + ctxMap := make(map[string]string, model.MaxActionQueryEntries+1) + for i := range model.MaxActionQueryEntries + 1 { + ctxMap[fmt.Sprintf("k%d", i)] = "v" + } + payload, err := json.Marshal(model.DoPostActionRequest{Query: ctxMap}) + require.NoError(t, err) + + resp, err := client.DoAPIPost(context.Background(), route, string(payload)) + require.Error(t, err) + CheckBadRequestStatus(t, model.BuildResponse(resp)) + CheckErrorID(t, err, "api.post.do_action.query.app_error") + }) + + t.Run("oversized key returns 400", func(t *testing.T) { + ctxMap := map[string]string{strings.Repeat("k", model.MaxActionQueryKeyLength+1): "v"} + payload, err := json.Marshal(model.DoPostActionRequest{Query: ctxMap}) + require.NoError(t, err) + + resp, err := client.DoAPIPost(context.Background(), route, string(payload)) + require.Error(t, err) + CheckBadRequestStatus(t, model.BuildResponse(resp)) + CheckErrorID(t, err, "api.post.do_action.query.app_error") + }) + + t.Run("oversized value returns 400", func(t *testing.T) { + ctxMap := map[string]string{"k": strings.Repeat("v", model.MaxActionQueryValueLength+1)} + payload, err := json.Marshal(model.DoPostActionRequest{Query: ctxMap}) + require.NoError(t, err) + + resp, err := client.DoAPIPost(context.Background(), route, string(payload)) + require.Error(t, err) + CheckBadRequestStatus(t, model.BuildResponse(resp)) + CheckErrorID(t, err, "api.post.do_action.query.app_error") + }) + + t.Run("small valid context returns 200", func(t *testing.T) { + payload, err := json.Marshal(model.DoPostActionRequest{Query: map[string]string{"tail": "214"}}) + require.NoError(t, err) + + resp, err := client.DoAPIPost(context.Background(), route, string(payload)) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) +} + +func TestDoPostActionQuery_OmitempyCompat(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + client := th.Client + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + created, actionID := newAttachmentActionPost(t, th, ts.URL) + route := "/posts/" + created.Id + "/actions/" + actionID + + // Older clients do not know about query — their request body has no such + // key. The omitempty tag should make this equivalent to sending a nil + // map, which ValidateActionQuery accepts. + payload := `{"selected_option":""}` + resp, err := client.DoAPIPost(context.Background(), route, payload) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) + + // Completely empty body should also be accepted — same as older clients + // calling DoPostActionWithCookie with no selection and no cookie. + resp, err = client.DoAPIPost(context.Background(), route, "") + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) +} + +// TestDoPostActionMalformedBody verifies non-EOF JSON decode errors now +// return 400 instead of silently running the action with an empty request. +// A body like `{"query":{"k":1}}` (value is not a string) would otherwise +// deserialize to a zero-value Query and skip validation. +func TestDoPostActionMalformedBody(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + client := th.Client + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + created, actionID := newAttachmentActionPost(t, th, ts.URL) + route := "/posts/" + created.Id + "/actions/" + actionID + + t.Run("wrong type for query value returns 400", func(t *testing.T) { + // query must be map[string]string; passing an int value triggers a + // json UnmarshalTypeError which must not fall through. + resp, err := client.DoAPIPost(context.Background(), route, `{"query":{"k":1}}`) + require.Error(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + }) + + t.Run("syntactically invalid JSON returns 400", func(t *testing.T) { + resp, err := client.DoAPIPost(context.Background(), route, `{not json`) + require.Error(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + }) + + t.Run("trailing JSON values after the first object return 400", func(t *testing.T) { + // json.Decoder.Decode stops after the first complete value, so a + // body like `{"query":{}}{"cookie":"x"}` would otherwise execute + // the action with the first object's intent and silently drop the + // rest. The handler explicitly rejects trailing values. + resp, err := client.DoAPIPost(context.Background(), route, `{"query":{}}{"cookie":"x"}`) + require.Error(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + }) +} diff --git a/server/channels/app/integration_action.go b/server/channels/app/integration_action.go index 5b1ea52a61f..f6c8f935646 100644 --- a/server/channels/app/integration_action.go +++ b/server/channels/app/integration_action.go @@ -24,6 +24,7 @@ import ( "errors" "fmt" "io" + "maps" "net/http" "net/url" "path" @@ -39,7 +40,57 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/utils" ) -func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, selectedOption string, cookie *model.PostActionCookie) (string, *model.AppError) { +// maxMmBlocksActionsCloneDepth caps recursion in cloneMmBlocksActionsProp. +// ValidateMmBlocksActions bounds top-level entry count and key length but +// does not bound nesting depth inside spec.Context — a bot/plugin could +// otherwise stash a pathologically nested object that drives stack +// exhaustion on the restore path. 64 is well past any plausible legitimate +// nesting; deeper input is treated as malicious and truncated. +const maxMmBlocksActionsCloneDepth = 64 + +// cloneMmBlocksActionsProp deep-clones the post.props.mm_blocks_actions value. +// Each per-action entry can carry nested context / query maps (and arrays +// inside those), so the clone walks the structure recursively — a shallow +// clone at any level would leave nested objects aliased back to the live +// post's props, defeating the restore-after-invalid-response guarantee. +func cloneMmBlocksActionsProp(v any) any { + return cloneMmBlocksActionsPropAt(v, 0) +} + +func cloneMmBlocksActionsPropAt(v any, depth int) any { + if depth > maxMmBlocksActionsCloneDepth { + // Defense-in-depth: drop the subtree rather than risk stack + // exhaustion. The restore path that calls this helper is on a + // rare branch (plugin response is invalid), and pathological + // nesting at this depth is not a legitimate use case. + return nil + } + switch typed := v.(type) { + case map[string]any: + out := make(map[string]any, len(typed)) + for k, child := range typed { + out[k] = cloneMmBlocksActionsPropAt(child, depth+1) + } + return out + case []any: + out := make([]any, len(typed)) + for i, child := range typed { + out[i] = cloneMmBlocksActionsPropAt(child, depth+1) + } + return out + default: + // Scalars (string/number/bool/nil) are immutable — safe to share. + return v + } +} + +func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, selectedOption string, cookie *model.PostActionCookie, query map[string]string) (string, *model.AppError) { + // Bound the per-click query at the App boundary so any caller — REST + // handler, plugin, future internal trigger — gets the same enforcement. + if err := model.ValidateActionQuery(query); err != nil { + return "", model.NewAppError("DoPostActionWithCookie", "api.post.do_action.query.app_error", nil, "", http.StatusBadRequest).Wrap(err) + } + // PostAction may result in the original post being updated. For the // updated post, we need to unconditionally preserve the original // IsPinned and HasReaction attributes, and preserve its entire @@ -121,10 +172,17 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, upstreamRequest.ChannelName = channel.Name upstreamRequest.TeamId = channel.TeamId upstreamRequest.Type = cookie.Type - upstreamRequest.Context = cookie.Integration.Context + // Clone the Context map — later code may add selected_option to + // it, and we must not mutate the shared source. + // + // query is intentionally not merged on the cookie path: cookies are + // only baked for attachment action buttons, not for mm_blocks + // actions, so this branch is never reached by a click that carries + // per-click query params. + upstreamRequest.Context = maps.Clone(cookie.Integration.Context) datasource = cookie.DataSource - retain = cookie.RetainProps + retain = maps.Clone(cookie.RetainProps) remove = cookie.RemoveProps rootPostId = cookie.RootPostId upstreamURL = cookie.Integration.URL @@ -132,7 +190,7 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, post := result.Data chResult := <-cchan if chResult.NErr != nil { - return "", model.NewAppError("DoPostActionWithCookie", "app.channel.get_for_post.app_error", nil, "", http.StatusInternalServerError).Wrap(result.NErr) + return "", model.NewAppError("DoPostActionWithCookie", "app.channel.get_for_post.app_error", nil, "", http.StatusInternalServerError).Wrap(chResult.NErr) } channel := chResult.Data @@ -145,7 +203,12 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, upstreamRequest.ChannelName = channel.Name upstreamRequest.TeamId = channel.TeamId upstreamRequest.Type = action.Type - upstreamRequest.Context = action.Integration.Context + // Clone the Context map — the action pointer returned from + // post.GetAction may alias post.props state (attachment action) or + // the synthesized mm_blocks_actions spec. Mutating it directly + // would leak per-click values (selected_option) into the post's + // cached integration for subsequent clickers. + upstreamRequest.Context = maps.Clone(action.Integration.Context) datasource = action.DataSource // Save the original values that may need to be preserved (including selected @@ -158,7 +221,10 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, remove = append(remove, key) } } - originalProps = post.GetProps() + // Clone — originalProps may be passed to response.Update.SetProps, + // which would otherwise have response.Update alias the original + // post's props map. + originalProps = maps.Clone(post.GetProps()) originalIsPinned = post.IsPinned originalHasReactions = post.HasReactions @@ -234,6 +300,18 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, return "", model.NewAppError("DoPostActionWithCookie", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) } + // Merge per-click query into the upstream URL. This is the canonical + // transport for mm_blocks_actions external clicks; for legacy attachment + // clicks `query` is empty so this is a no-op. Done before the request + // log so operators see the URL actually sent on the wire. + if len(query) > 0 { + mergedURL, mergeErr := model.MergeQueryIntoURL(upstreamURL, query) + if mergeErr != nil { + return "", model.NewAppError("DoPostActionWithCookie", "api.post.do_action.merge_query.app_error", nil, "", http.StatusBadRequest).Wrap(mergeErr) + } + upstreamURL = mergedURL + } + // Log request, regardless of whether destination is internal or external rctx.Logger().Info("DoPostActionWithCookie POST request, through DoActionRequest", mlog.String("url", upstreamURL), @@ -281,7 +359,44 @@ func (a *App) DoPostActionWithCookie(rctx request.CTX, postID, actionId, userID, response.Update.IsPinned = originalIsPinned response.Update.HasReactions = originalHasReactions - if _, _, appErr = a.UpdatePost(rctx, response.Update, &model.UpdatePostOptions{SafeUpdate: false}); appErr != nil { + // Validate mm_blocks_actions on update responses. Since + // AllowMmBlocksActionsUpdate bypasses the non-integration guard in + // UpdatePost, and mm_blocks_actions are not in + // PostActionRetainPropKeys, a bad response would otherwise + // permanently replace the post's valid mm_blocks_actions. Keep the + // original value (if any) and log a warning so integration authors + // can diagnose. + // + // Contract (matches the attachments contract): a plugin update + // response that returns a non-nil Props map MUST echo + // mm_blocks_actions back if it wants the buttons to survive. + // Omitting the key drops the prop. This is intentional symmetry + // with attachments and matches the behavior in the mm_blocks + // framework PR. + if response.Update.GetProp(model.PostPropsMmBlocksActions) != nil { + if originalProps[model.PostPropsMmBlocksActions] == nil { + rctx.Logger().Info("Dropping mm_blocks_actions from plugin update response: original post had none", + mlog.String("post_id", postID), + mlog.String("url", upstreamURL), + ) + response.Update.DelProp(model.PostPropsMmBlocksActions) + } else if err := model.ValidateMmBlocksActions(response.Update); err != nil { + rctx.Logger().Info("Restoring original mm_blocks_actions: plugin update response was invalid", + mlog.String("post_id", postID), + mlog.String("url", upstreamURL), + mlog.Err(err), + ) + // originalProps came from maps.Clone(post.GetProps()) + // which is a shallow clone — the nested + // mm_blocks_actions map is still aliased to + // post.Props. Deep-clone before reattaching so a + // later mutation through response.Update can't + // reach back into the original post's prop map. + response.Update.AddProp(model.PostPropsMmBlocksActions, cloneMmBlocksActionsProp(originalProps[model.PostPropsMmBlocksActions])) + } + } + + if _, _, appErr = a.UpdatePost(rctx, response.Update, &model.UpdatePostOptions{SafeUpdate: false, AllowMmBlocksActionsUpdate: true}); appErr != nil { return "", appErr } } diff --git a/server/channels/app/integration_action_test.go b/server/channels/app/integration_action_test.go index 1389b24b18b..68aa21fd51b 100644 --- a/server/channels/app/integration_action_test.go +++ b/server/channels/app/integration_action_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strconv" "strings" "testing" "time" @@ -67,7 +68,7 @@ func TestPostActionInvalidURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.ErrorContains(t, err, "missing protocol scheme") } @@ -119,7 +120,7 @@ func TestPostActionEmptyResponse(t *testing.T) { attachments, ok := post.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) require.True(t, ok) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) }) @@ -167,7 +168,7 @@ func TestPostActionEmptyResponse(t *testing.T) { cfg.ServiceSettings.OutgoingIntegrationRequestsTimeout = new(int64(1)) }) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.ErrorContains(t, err, "context deadline exceeded") }) @@ -236,7 +237,7 @@ func TestPostActionResponseSizeLimit(t *testing.T) { // Should return error due to truncated JSON, but NOT crash or OOM _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, - attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) // Truncated JSON causes unmarshal error assert.Equal(t, "api.post.do_action.action_integration.app_error", err.Id) @@ -279,7 +280,7 @@ func TestPostActionResponseSizeLimit(t *testing.T) { // Should return error due to invalid JSON, but NOT crash or OOM _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, - attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.Equal(t, "api.post.do_action.action_integration.app_error", err.Id) }) @@ -425,16 +426,16 @@ func TestPostAction(t *testing.T) { require.NotEmpty(t, attachments2[0].Actions) require.NotEmpty(t, attachments2[0].Actions[0].Id) - clientTriggerID, err := th.App.DoPostActionWithCookie(th.Context, post.Id, "notavalidid", th.BasicUser.Id, "", nil) + clientTriggerID, err := th.App.DoPostActionWithCookie(th.Context, post.Id, "notavalidid", th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.Equal(t, http.StatusNotFound, err.StatusCode) assert.Len(t, clientTriggerID, 0) - clientTriggerID, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + clientTriggerID, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) assert.Len(t, clientTriggerID, 26) - clientTriggerID, err = th.App.DoPostActionWithCookie(th.Context, post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected", nil) + clientTriggerID, err = th.App.DoPostActionWithCookie(th.Context, post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected", nil, nil) require.Nil(t, err) assert.Len(t, clientTriggerID, 26) @@ -442,7 +443,7 @@ func TestPostAction(t *testing.T) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "" }) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.ErrorContains(t, err, "address forbidden") @@ -480,14 +481,14 @@ func TestPostAction(t *testing.T) { attachmentsPlugin, ok := postplugin.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) require.True(t, ok) - _, err = th.App.DoPostActionWithCookie(th.Context, postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Equal(t, "api.post.do_action.action_integration.app_error", err.Id) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" }) - _, err = th.App.DoPostActionWithCookie(th.Context, postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) th.App.UpdateConfig(func(cfg *model.Config) { @@ -528,7 +529,7 @@ func TestPostAction(t *testing.T) { attachmentsSiteURL, ok := postSiteURL.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) require.True(t, ok) - _, err = th.App.DoPostActionWithCookie(th.Context, postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) assert.ErrorContains(t, err, "connection refused") @@ -570,7 +571,7 @@ func TestPostAction(t *testing.T) { attachmentsSubpath, ok := postSubpath.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) require.True(t, ok) - _, err = th.App.DoPostActionWithCookie(th.Context, postSubpath.Id, attachmentsSubpath[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, postSubpath.Id, attachmentsSubpath[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) }) } @@ -644,7 +645,7 @@ func TestPostActionProps(t *testing.T) { attachments, ok := post.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) require.True(t, ok) - clientTriggerId, err := th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + clientTriggerId, err := th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) assert.Len(t, clientTriggerId, 26) @@ -830,7 +831,7 @@ func TestPostActionRelativeURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) @@ -870,7 +871,7 @@ func TestPostActionRelativeURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) @@ -910,7 +911,7 @@ func TestPostActionRelativeURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) @@ -950,7 +951,7 @@ func TestPostActionRelativeURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) @@ -990,7 +991,7 @@ func TestPostActionRelativeURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) } @@ -1067,7 +1068,7 @@ func TestPostActionRelativePluginURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.NotNil(t, err) }) @@ -1107,7 +1108,7 @@ func TestPostActionRelativePluginURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) }) @@ -1147,7 +1148,7 @@ func TestPostActionRelativePluginURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) }) @@ -1187,7 +1188,7 @@ func TestPostActionRelativePluginURL(t *testing.T) { require.NotEmpty(t, attachments[0].Actions) require.NotEmpty(t, attachments[0].Actions[0].Id) - _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil) + _, err = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) require.Nil(t, err) }) } @@ -1757,7 +1758,7 @@ func TestDoPostActionWithCookieEdgeCases(t *testing.T) { }, } - _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", th.BasicUser.Id, "", cookie) + _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", th.BasicUser.Id, "", cookie, nil) require.Nil(t, err) }) @@ -1771,7 +1772,7 @@ func TestDoPostActionWithCookieEdgeCases(t *testing.T) { }, } - _, err := th.App.DoPostActionWithCookie(th.Context, "actual_post_id", "action_id", th.BasicUser.Id, "", cookie) + _, err := th.App.DoPostActionWithCookie(th.Context, "actual_post_id", "action_id", th.BasicUser.Id, "", cookie, nil) require.NotNil(t, err) assert.Contains(t, err.Error(), "postId doesn't match") }) @@ -1784,7 +1785,7 @@ func TestDoPostActionWithCookieEdgeCases(t *testing.T) { Integration: nil, } - _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", th.BasicUser.Id, "", cookie) + _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", th.BasicUser.Id, "", cookie, nil) require.NotNil(t, err) assert.Contains(t, err.Error(), "no Integration in action cookie") }) @@ -1805,10 +1806,129 @@ func TestDoPostActionWithCookieEdgeCases(t *testing.T) { }, } - _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", "nonexistent_user_id", "", cookie) + _, err := th.App.DoPostActionWithCookie(th.Context, "nonexistent_post_id", "action_id", "nonexistent_user_id", "", cookie, nil) require.NotNil(t, err) assert.Contains(t, err.Error(), "Unable to find the user.") }) + + t.Run("rejects oversized query at the App boundary (independent of API handler)", func(t *testing.T) { + // ValidateActionQuery is called at the top of DoPostActionWithCookie, + // not just in the API handler. Direct App-layer callers (plugins, + // tests, internal triggers) get the same enforcement as REST clients. + oversized := make(map[string]string, model.MaxActionQueryEntries+1) + for i := range model.MaxActionQueryEntries + 1 { + oversized["k"+strconv.Itoa(i)] = "v" + } + + _, err := th.App.DoPostActionWithCookie(th.Context, "any_post", "any_action", th.BasicUser.Id, "", nil, oversized) + require.NotNil(t, err) + assert.Equal(t, http.StatusBadRequest, err.StatusCode) + assert.Equal(t, "api.post.do_action.query.app_error", err.Id) + }) +} + +// TestCloneMmBlocksActionsProp guards the deep-clone semantics used when +// restoring an original spec after a plugin update response is rejected. +// A shallow clone would alias the nested per-action map back into post.Props, +// so a later mutation through response.Update could reach into the live post. +func TestCloneMmBlocksActionsProp(t *testing.T) { + t.Run("nil and non-map values are returned unchanged", func(t *testing.T) { + assert.Nil(t, cloneMmBlocksActionsProp(nil)) + assert.Equal(t, "string", cloneMmBlocksActionsProp("string")) + }) + + t.Run("top-level and nested mutations on the clone do not leak", func(t *testing.T) { + original := map[string]any{ + "btn1": map[string]any{ + "type": "external", + "url": "http://example.com/hook", + }, + } + + cloned, ok := cloneMmBlocksActionsProp(original).(map[string]any) + require.True(t, ok) + + // Mutating the top-level map on the clone (adding a key) must not + // reach the original. + cloned["btn2"] = map[string]any{"type": "external", "url": "http://example.com/other"} + assert.NotContains(t, original, "btn2") + + // Mutating a nested per-action map on the clone (changing the URL) + // must not reach the original — this is the case the shallow-clone + // bug actually exposed. + clonedEntry, ok := cloned["btn1"].(map[string]any) + require.True(t, ok) + clonedEntry["url"] = "http://attacker.example/" + + originalEntry, ok := original["btn1"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "http://example.com/hook", originalEntry["url"]) + }) + + t.Run("deeply nested context and array mutations on the clone do not leak", func(t *testing.T) { + // Per-action specs can carry nested context maps and arrays. A + // shallow per-entry clone would still alias these structures back + // to the live post's props. + original := map[string]any{ + "btn1": map[string]any{ + "type": "external", + "url": "http://example.com/hook", + "context": map[string]any{"team": "alpha", "tags": []any{"a", "b"}}, + }, + } + + cloned, ok := cloneMmBlocksActionsProp(original).(map[string]any) + require.True(t, ok) + + clonedEntry := cloned["btn1"].(map[string]any) + clonedContext := clonedEntry["context"].(map[string]any) + + // Mutate the nested context map on the clone. + clonedContext["team"] = "tampered" + clonedContext["new"] = "added" + + // Mutate the nested array on the clone. + clonedTags := clonedContext["tags"].([]any) + clonedTags[0] = "tampered" + + // Original must be untouched at every level. + originalEntry := original["btn1"].(map[string]any) + originalContext := originalEntry["context"].(map[string]any) + assert.Equal(t, "alpha", originalContext["team"]) + assert.NotContains(t, originalContext, "new") + assert.Equal(t, []any{"a", "b"}, originalContext["tags"]) + }) + + t.Run("pathologically nested input is truncated past maxMmBlocksActionsCloneDepth", func(t *testing.T) { + // ValidateMmBlocksActions doesn't bound nesting depth inside + // spec.Context — defense-in-depth against stack exhaustion if a + // bot/plugin author crafts deeply nested input. + var leaf any = "leaf" + const tooDeep = maxMmBlocksActionsCloneDepth + 100 + for range tooDeep { + leaf = map[string]any{"n": leaf} + } + + // Must not stack-overflow / panic. + var cloned any + require.NotPanics(t, func() { + cloned = cloneMmBlocksActionsProp(leaf) + }) + + // Walk the clone; should hit nil before reaching the leaf string. + current := cloned + for i := range tooDeep { + m, ok := current.(map[string]any) + if !ok { + assert.Greater(t, i, maxMmBlocksActionsCloneDepth-2, + "truncation should kick in at or near maxMmBlocksActionsCloneDepth") + assert.Nil(t, current, "subtree past depth cap must be nil, not aliased to source") + return + } + current = m["n"] + } + t.Fatalf("clone walked %d levels without hitting truncation", tooDeep) + }) } func TestDoPluginRequest(t *testing.T) { @@ -2002,3 +2122,859 @@ func TestDoPluginRequest(t *testing.T) { } }) } + +// buildMmBlocksActionsProp returns a mm_blocks_actions map (an "external"-type +// action) suitable for use as a post prop in tests. +func buildMmBlocksActionsProp(id, url string, context map[string]any) map[string]any { + entry := map[string]any{ + "type": model.MmBlocksActionTypeExternal, + "url": url, + } + if context != nil { + entry["context"] = context + } + return map[string]any{id: entry} +} + +// setupBotInChannel creates a bot, joins it to the team and channel, and +// returns the resolved *model.User for the bot. +func setupBotInChannel(t *testing.T, th *TestHelper) *model.User { + t.Helper() + bot := th.CreateBot(t) + botUser, appErr := th.App.GetUser(bot.UserId) + require.Nil(t, appErr) + _, _, appErr = th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, botUser.Id, "") + require.Nil(t, appErr) + _, appErr = th.App.AddUserToChannel(th.Context, botUser, th.BasicChannel, false) + require.Nil(t, appErr) + return botUser +} + +func TestMmBlocksActionsStrippedOnCreate(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + post := &model.Post{ + Message: "hello with inline actions", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "actionone", + "http://127.0.0.1/plugins/myplugin/doit", + map[string]any{"operation": "STORM"}, + ), + }, + } + + created, _, err := th.App.CreatePostAsUser(th.Context, post, "", true) + require.Nil(t, err) + assert.Nil(t, created.GetProp(model.PostPropsMmBlocksActions), "non-bot, non-integration user should have mm_blocks_actions stripped") + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + assert.Nil(t, stored.GetProp(model.PostPropsMmBlocksActions), "stored post should not carry mm_blocks_actions") +} + +func TestMmBlocksActionsKeptForBotIntegration(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + + // IsOAuth=true makes Session.IsIntegration() return true without needing + // a full bot-token session. + intSession := &model.Session{UserId: botUser.Id, IsOAuth: true} + intCtx := th.Context.WithSession(intSession) + + post := &model.Post{ + Message: "hello from a bot", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "actiontwo", + "http://127.0.0.1/plugins/myplugin/doit", + map[string]any{"operation": "STORM"}, + ), + }, + } + + created, _, err := th.App.CreatePostAsUser(intCtx, post, "", true) + require.Nil(t, err) + require.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions), "bot post via integration session should preserve mm_blocks_actions") + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + require.NotNil(t, stored.GetProp(model.PostPropsMmBlocksActions), "stored bot post should carry mm_blocks_actions") + + spec := stored.GetMmBlocksActionSpec("actiontwo") + require.NotNil(t, spec) + assert.Equal(t, "http://127.0.0.1/plugins/myplugin/doit", spec.URL) +} + +// TestPluginAPICreatePostKeepsMmBlocksActions locks the contract that a +// plugin creating a post via PluginAPI.CreatePost retains mm_blocks_actions. +// Plugins are server-trusted code, but their static activation-time rctx +// has an unmarked session — without pluginIntegrationCtx the strip in +// CreatePost would delete the prop and clicks would 404 with +// "invalid action id". +func TestPluginAPICreatePostKeepsMmBlocksActions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + botUser := setupBotInChannel(t, th) + + manifest := &model.Manifest{Id: "com.mattermost.test-plugin"} + api := NewPluginAPI(th.App, th.Context, manifest) + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + UserId: botUser.Id, + Message: "issue tracker post", + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "triage", + "/plugins/com.mattermost.test-plugin/inline_action/triage", + map[string]any{"project": "Demo Project"}, + ), + }, + } + + created, appErr := api.CreatePost(post) + require.Nil(t, appErr) + require.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions), + "plugin-created post must preserve mm_blocks_actions; the strip in CreatePost should not fire because PluginAPI marks the session as integration") + + // Re-read from the store to confirm persistence (not just in-memory). + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + spec := stored.GetMmBlocksActionSpec("triage") + require.NotNil(t, spec, "stored plugin post must resolve the action spec at click time") + assert.Equal(t, "/plugins/com.mattermost.test-plugin/inline_action/triage", spec.URL) +} + +// TestMmBlocksActionsKeptForWebhookImpersonation verifies that an integration +// session is sufficient on its own — the post's author does not need to be a +// bot. This is the webhook-impersonation flow: a webhook posts as a regular +// user with from_webhook=true, and we must not strip the prop just because +// user.IsBot is false. +func TestMmBlocksActionsKeptForWebhookImpersonation(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + // Integration session for a regular (non-bot) user. + intSession := &model.Session{UserId: th.BasicUser.Id, IsOAuth: true} + intCtx := th.Context.WithSession(intSession) + + post := &model.Post{ + Message: "post from impersonating webhook", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "webhook1", + "http://127.0.0.1/plugins/myplugin/wh", + nil, + ), + }, + } + + created, _, err := th.App.CreatePostAsUser(intCtx, post, "", true) + require.Nil(t, err) + require.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions), + "non-bot author via integration session must preserve mm_blocks_actions (webhook flow)") + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + require.NotNil(t, stored.GetProp(model.PostPropsMmBlocksActions)) +} + +// TestMmBlocksActionsStripGate locks the create-time strip policy: keep +// when the post is bot-authored OR the session is an integration; strip +// when neither signal is present. The bot-author signal covers +// PluginAPI.CreatePost (whose static rctx is unmarked) where the post is +// authored by the plugin's bot user; the integration-session signal +// covers REST callers using bot tokens, PATs, or OAuth apps. +func TestMmBlocksActionsStripGate(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + + inline := buildMmBlocksActionsProp( + "mx", + "http://127.0.0.1/plugins/myplugin/mx", + nil, + ) + + t.Run("bot author via non-integration session is kept", func(t *testing.T) { + // Models the PluginAPI.CreatePost path: post.UserId is the plugin's + // bot user but rctx.Session() is the unmarked plugin context. The + // bot-author signal alone must be sufficient to keep the prop. + post := &model.Post{ + Message: "hello", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{model.PostPropsMmBlocksActions: inline}, + } + created, _, err := th.App.CreatePostAsUser(th.Context, post, "", true) + require.Nil(t, err) + assert.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions), + "bot-authored post must keep mm_blocks_actions even without an integration session") + }) + + t.Run("regular user via non-integration session is stripped", func(t *testing.T) { + // Neither signal present: the prop must be removed. Catches the + // baseline user-content case. + post := &model.Post{ + Message: "hello", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{model.PostPropsMmBlocksActions: inline}, + } + created, _, err := th.App.CreatePostAsUser(th.Context, post, "", true) + require.Nil(t, err) + assert.Nil(t, created.GetProp(model.PostPropsMmBlocksActions), + "regular-user post via non-integration session must strip mm_blocks_actions") + }) +} + +func TestUpdatePostMmBlocksActionsGuard(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + + // Bot posts with mm_blocks_actions must be CREATED via an integration + // session — see the matching create-time strip in CreatePostAsUser. + intSeedSession := &model.Session{UserId: botUser.Id, IsOAuth: true} + intSeedCtx := th.Context.WithSession(intSeedSession) + + // originalInline is the mm_blocks_actions value we expect the bot post to + // keep after non-integration edits. + originalInline := buildMmBlocksActionsProp( + "keep", + "http://127.0.0.1/plugins/myplugin/original", + map[string]any{"k": "orig"}, + ) + + t.Run("non-integration edit of bot post reverts mm_blocks_actions", func(t *testing.T) { + botPost := &model.Post{ + Message: "bot post with inline actions", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: originalInline, + }, + } + created, _, cErr := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, cErr) + require.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions)) + + // A non-integration session tries to swap mm_blocks_actions wholesale. + newInline := buildMmBlocksActionsProp( + "swap", + "http://127.0.0.1/plugins/myplugin/swapped", + map[string]any{"k": "attacker"}, + ) + edit := created.Clone() + edit.Message = "edited message" + edit.AddProp(model.PostPropsMmBlocksActions, newInline) + + // th.Context has an empty/zero session — not an integration. + updated, _, uErr := th.App.UpdatePost(th.Context, edit, &model.UpdatePostOptions{SafeUpdate: false}) + require.Nil(t, uErr) + + // mm_blocks_actions should revert to the original value. + got := updated.GetMmBlocksActionSpec("keep") + require.NotNil(t, got, "original inline action should still be reachable") + assert.Equal(t, "http://127.0.0.1/plugins/myplugin/original", got.URL) + + // The attacker's swapped action should not be present. + assert.Nil(t, updated.GetMmBlocksActionSpec("swap")) + + // Message change should still be applied. + assert.Equal(t, "edited message", updated.Message) + }) + + t.Run("non-integration edit cannot add mm_blocks_actions when original had none", func(t *testing.T) { + plainBotPost := &model.Post{ + Message: "bot post without inline actions", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + } + created, _, cErr := th.App.CreatePostAsUser(intSeedCtx, plainBotPost, "", true) + require.Nil(t, cErr) + require.Nil(t, created.GetProp(model.PostPropsMmBlocksActions)) + + newInline := buildMmBlocksActionsProp( + "added", + "http://127.0.0.1/plugins/myplugin/added", + nil, + ) + edit := created.Clone() + edit.AddProp(model.PostPropsMmBlocksActions, newInline) + + updated, _, uErr := th.App.UpdatePost(th.Context, edit, &model.UpdatePostOptions{SafeUpdate: false}) + require.Nil(t, uErr) + assert.Nil(t, updated.GetProp(model.PostPropsMmBlocksActions), "non-integration update must not introduce mm_blocks_actions") + }) + + t.Run("integration session alone cannot modify mm_blocks_actions", func(t *testing.T) { + // Even with an integration session (PAT / OAuth / bot-token), the + // UpdatePost path requires AllowMmBlocksActionsUpdate to modify + // mm_blocks_actions. A PAT-holding user could otherwise inject + // mm_blocks_actions on any post they can edit. + botPost := &model.Post{ + Message: "bot post for integration edit", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: originalInline, + }, + } + created, _, cErr := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, cErr) + + intSession := &model.Session{UserId: th.BasicUser.Id, IsOAuth: true} + intCtx := th.Context.WithSession(intSession) + require.True(t, intCtx.Session().IsIntegration()) + + newInline := buildMmBlocksActionsProp( + "replaced", + "http://127.0.0.1/plugins/myplugin/new", + map[string]any{"k": "integration"}, + ) + edit := created.Clone() + edit.AddProp(model.PostPropsMmBlocksActions, newInline) + + updated, _, uErr := th.App.UpdatePost(intCtx, edit, &model.UpdatePostOptions{SafeUpdate: false}) + require.Nil(t, uErr) + + // The attacker's "replaced" entry must not land; the original stays. + assert.Nil(t, updated.GetMmBlocksActionSpec("replaced"), "integration session alone must not overwrite mm_blocks_actions") + keep := updated.GetMmBlocksActionSpec("keep") + require.NotNil(t, keep, "original inline action must be preserved") + assert.Equal(t, "http://127.0.0.1/plugins/myplugin/original", keep.URL) + }) + + t.Run("AllowMmBlocksActionsUpdate option accepts new mm_blocks_actions", func(t *testing.T) { + botPost := &model.Post{ + Message: "bot post for plugin-path edit", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: originalInline, + }, + } + created, _, cErr := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, cErr) + + newInline := buildMmBlocksActionsProp( + "plugin", + "http://127.0.0.1/plugins/myplugin/plugin", + map[string]any{"k": "plugin"}, + ) + edit := created.Clone() + edit.AddProp(model.PostPropsMmBlocksActions, newInline) + + // Non-integration session, but AllowMmBlocksActionsUpdate grants write. + updated, _, uErr := th.App.UpdatePost(th.Context, edit, &model.UpdatePostOptions{SafeUpdate: false, AllowMmBlocksActionsUpdate: true}) + require.Nil(t, uErr) + + assert.Nil(t, updated.GetMmBlocksActionSpec("keep")) + integration := updated.GetMmBlocksActionSpec("plugin") + require.NotNil(t, integration) + assert.Equal(t, "http://127.0.0.1/plugins/myplugin/plugin", integration.URL) + }) +} + +// TestCreateWebhookPostStripsMmBlocksActions locks the contract that an +// incoming webhook cannot persist mm_blocks_actions even if the payload +// includes the prop in its `props` map. CreateWebhookPost's prop iteration +// has no explicit blocklist entry for mm_blocks_actions; it falls through +// to AddProp and would land on the post object. The strip in CreatePost +// (post.go) then fires because the webhook flow has no integration session +// (incomingWebhook is registered with RequireSession: false). If a future +// refactor changes the webhook session model, this test catches it. +func TestCreateWebhookPostStripsMmBlocksActions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableIncomingWebhooks = true }) + + hook, hookErr := th.App.CreateIncomingWebhookForChannel(th.BasicUser.Id, th.BasicChannel, &model.IncomingWebhook{ChannelId: th.BasicChannel.Id}) + require.Nil(t, hookErr) + defer func() { + _ = th.App.DeleteIncomingWebhook(hook.Id) + }() + + inline := buildMmBlocksActionsProp( + "actx", + "http://127.0.0.1/plugins/myplugin/x", + nil, + ) + + post, appErr := th.App.CreateWebhookPost(th.Context, hook.UserId, th.BasicChannel, "hello", "user", "http://iconurl", "", + model.StringInterface{ + model.PostPropsMmBlocksActions: inline, + }, + "", "", nil) + require.Nil(t, appErr) + + assert.Nil(t, post.GetProp(model.PostPropsMmBlocksActions), + "incoming webhook payload must not be able to persist mm_blocks_actions; the strip in CreatePost should fire because the webhook session has IsIntegration()==false") + + // Belt and suspenders: read back from the DB to confirm the prop is + // not persisted either. + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, post.Id, false) + require.NoError(t, nErr) + assert.Nil(t, stored.GetProp(model.PostPropsMmBlocksActions), + "stored webhook post must not carry mm_blocks_actions") +} + +func TestSendEphemeralPostStripsMmBlocksActions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + ephemeral := &model.Post{ + ChannelId: th.BasicChannel.Id, + UserId: th.BasicUser.Id, + Message: "ephemeral with inline actions", + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "eph", + "http://127.0.0.1/plugins/myplugin/eph", + map[string]any{"k": "v"}, + ), + }, + } + + result, _ := th.App.SendEphemeralPost(th.Context, th.BasicUser.Id, ephemeral) + require.NotNil(t, result) + assert.Nil(t, result.GetProp(model.PostPropsMmBlocksActions), "SendEphemeralPost must drop mm_blocks_actions") + + // UpdateEphemeralPost path + ephemeral2 := &model.Post{ + Id: result.Id, + ChannelId: th.BasicChannel.Id, + UserId: th.BasicUser.Id, + Message: "updated ephemeral with inline actions", + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: buildMmBlocksActionsProp( + "eph2", + "http://127.0.0.1/plugins/myplugin/eph2", + nil, + ), + }, + } + updated, _ := th.App.UpdateEphemeralPost(th.Context, th.BasicUser.Id, ephemeral2) + require.NotNil(t, updated) + assert.Nil(t, updated.GetProp(model.PostPropsMmBlocksActions), "UpdateEphemeralPost must drop mm_blocks_actions") +} + +func TestDoPostActionQueryMergedIntoURL(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + intSeedCtx := th.Context.WithSession(&model.Session{UserId: botUser.Id, IsOAuth: true}) + + // Capture both the upstream integration request body and the URL the + // server saw, so we can assert that per-click query lands in the URL + // (mm_blocks transport) and not in the upstream Context map. + var ( + capturedReq model.PostActionIntegrationRequest + capturedRawQuery string + ) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedRawQuery = r.URL.RawQuery + body, readErr := io.ReadAll(r.Body) + require.NoError(t, readErr) + require.NoError(t, json.Unmarshal(body, &capturedReq)) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + inlineActions := buildMmBlocksActionsProp( + "inline1", + ts.URL, + map[string]any{"operation": "STORM"}, + ) + botPost := &model.Post{ + Message: "mm_blocks action post", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: inlineActions, + }, + } + created, _, err := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, err) + require.NotNil(t, created.GetProp(model.PostPropsMmBlocksActions)) + + query := map[string]string{"tail": "214"} + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, "inline1", th.BasicUser.Id, "", nil, query) + require.Nil(t, err) + + // Query was appended to the upstream URL. + parsedQuery, qErr := url.ParseQuery(capturedRawQuery) + require.NoError(t, qErr) + assert.Equal(t, "214", parsedQuery.Get("tail"), "per-click query should land in the upstream URL") + + // Original action Context is forwarded as the upstream request's + // Context, untouched by the query merge. + assert.Equal(t, "STORM", capturedReq.Context["operation"]) + _, leakedInlineParams := capturedReq.Context["inline_params"] + assert.False(t, leakedInlineParams, "query must not be injected into upstream Context") +} + +func TestDoPostActionStaticQueryMergedWithPerClickQuery(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + intSeedCtx := th.Context.WithSession(&model.Session{UserId: botUser.Id, IsOAuth: true}) + + var capturedRawQuery string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedRawQuery = r.URL.RawQuery + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + // Spec carries a static query (source=fleet) AND a key (tail=999) that + // the per-click query will override. Per-click should win. + botPost := &model.Post{ + Message: "mm_blocks action post with static query", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: map[string]any{ + "inline1": map[string]any{ + "type": model.MmBlocksActionTypeExternal, + "url": ts.URL, + "query": map[string]any{"source": "fleet", "tail": "999"}, + }, + }, + }, + } + created, _, err := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, err) + + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, "inline1", th.BasicUser.Id, "", nil, map[string]string{"tail": "214"}) + require.Nil(t, err) + + parsedQuery, qErr := url.ParseQuery(capturedRawQuery) + require.NoError(t, qErr) + assert.Equal(t, "fleet", parsedQuery.Get("source"), "spec static query should land in the upstream URL") + assert.Equal(t, "214", parsedQuery.Get("tail"), "per-click query should override spec static query on overlapping keys") +} + +func TestDoPostActionContextMapNotMutated(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + intSeedCtx := th.Context.WithSession(&model.Session{UserId: botUser.Id, IsOAuth: true}) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer ts.Close() + + originalContext := map[string]any{"operation": "STORM"} + inlineActions := buildMmBlocksActionsProp("inline1", ts.URL, originalContext) + botPost := &model.Post{ + Message: "mm_blocks action post", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsMmBlocksActions: inlineActions, + }, + } + created, _, err := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, err) + + // First click: carries one set of per-click query values. + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, "inline1", th.BasicUser.Id, "", nil, map[string]string{"tail": "214"}) + require.Nil(t, err) + + // Post's stored mm_blocks_actions Context must not be mutated by the click. + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + spec := stored.GetMmBlocksActionSpec("inline1") + require.NotNil(t, spec) + assert.Equal(t, "STORM", spec.Context["operation"]) + assert.Equal(t, ts.URL, spec.URL, "stored URL must not absorb per-click query") + + // Second click with a different per-click query. + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, "inline1", th.BasicUser.Id, "", nil, map[string]string{"tail": "999"}) + require.Nil(t, err) + + stored, nErr = th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + spec = stored.GetMmBlocksActionSpec("inline1") + require.NotNil(t, spec) + assert.Equal(t, "STORM", spec.Context["operation"]) + assert.Equal(t, ts.URL, spec.URL, "stored URL must not absorb per-click query") +} + +func TestDoPostActionPluginResponseMmBlocksActionsDropped(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + + // Plugin returns an update that tries to add mm_blocks_actions, even + // though the original post had none. + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + resp := `{ + "update": { + "message": "updated message", + "props": { + "mm_blocks_actions": { + "sneaky": {"type": "external", "url": "http://127.0.0.1/plugins/myplugin/sneak"} + } + } + } + }` + _, _ = w.Write([]byte(resp)) + })) + defer ts.Close() + + // Bot post has an ATTACHMENT action (not an mm_blocks action), and no + // mm_blocks_actions prop. The plugin's response to clicking the + // attachment should not be able to introduce mm_blocks_actions. + botPost := &model.Post{ + Message: "attachment-only bot post", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsAttachments: []*model.MessageAttachment{ + { + Text: "hello", + Actions: []*model.PostAction{ + { + Type: model.PostActionTypeButton, + Name: "click", + Integration: &model.PostActionIntegration{ + URL: ts.URL, + }, + }, + }, + }, + }, + }, + } + created, _, err := th.App.CreatePostAsUser(th.Context, botPost, "", true) + require.Nil(t, err) + attachments, ok := created.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) + require.True(t, ok) + require.NotEmpty(t, attachments[0].Actions) + require.NotEmpty(t, attachments[0].Actions[0].Id) + require.Nil(t, created.GetProp(model.PostPropsMmBlocksActions)) + + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) + require.Nil(t, err) + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + assert.Nil(t, stored.GetProp(model.PostPropsMmBlocksActions), "plugin response must not be able to add mm_blocks_actions where none existed") + assert.Equal(t, "updated message", stored.Message) +} + +func TestDoPostActionPluginResponseInvalidMmBlocksActionsRestored(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + botUser := setupBotInChannel(t, th) + intSeedCtx := th.Context.WithSession(&model.Session{UserId: botUser.Id, IsOAuth: true}) + + // Plugin returns an update where mm_blocks_actions contains an entry + // with an empty URL — invalid; the original prop should be restored + // with a warning, while the message update still succeeds. + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + resp := `{ + "update": { + "message": "updated via plugin", + "props": { + "mm_blocks_actions": { + "broken": {"type": "external", "url": ""} + } + } + } + }` + _, _ = w.Write([]byte(resp)) + })) + defer ts.Close() + + // The original post has VALID mm_blocks_actions, so the "drop because + // original had none" branch is bypassed and we exercise the validation + // branch. + originalInline := buildMmBlocksActionsProp( + "orig", + "http://127.0.0.1/plugins/myplugin/orig", + nil, + ) + botPost := &model.Post{ + Message: "bot post with valid inline actions", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: botUser.Id, + Props: model.StringInterface{ + model.PostPropsAttachments: []*model.MessageAttachment{ + { + Text: "hello", + Actions: []*model.PostAction{ + { + Type: model.PostActionTypeButton, + Name: "click", + Integration: &model.PostActionIntegration{ + URL: ts.URL, + }, + }, + }, + }, + }, + model.PostPropsMmBlocksActions: originalInline, + }, + } + created, _, err := th.App.CreatePostAsUser(intSeedCtx, botPost, "", true) + require.Nil(t, err) + attachments, ok := created.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) + require.True(t, ok) + require.NotEmpty(t, attachments[0].Actions) + require.NotEmpty(t, attachments[0].Actions[0].Id) + + _, err = th.App.DoPostActionWithCookie(th.Context, created.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) + require.Nil(t, err) + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, created.Id, false) + require.NoError(t, nErr) + // Message update still applied — the invalid mm_blocks_actions were + // restored to the original value with a warning, so the rest of the + // response.Update is persisted. + assert.Equal(t, "updated via plugin", stored.Message) + // The broken action from the plugin response must never be stored. + assert.Nil(t, stored.GetMmBlocksActionSpec("broken"), "invalid mm_blocks action from plugin response must not be persisted") + // The original valid mm_blocks_actions must survive — an invalid plugin + // response must never wipe a post's existing buttons. + require.NotNil(t, stored.GetMmBlocksActionSpec("orig"), "original valid mm_blocks action must be preserved when plugin response is invalid") + assert.Equal(t, "http://127.0.0.1/plugins/myplugin/orig", stored.GetMmBlocksActionSpec("orig").URL) +} + +// TestPostActionRetainsFromBotAndFromPlugin verifies that from_bot and +// from_plugin props are retained across a plugin-returned post update even +// when the plugin's response.Props omits them. This matters because the +// webapp's allowInlineActions gate is derived from these markers; losing +// them on first update would hide every inline button on subsequent renders. +func TestPostActionRetainsFromBotAndFromPlugin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + // Plugin response deliberately omits from_bot / from_plugin from props. + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"update": {"message": "updated", "props": {"A": "AA"}}}`) + })) + defer ts.Close() + + interactivePost := model.Post{ + Message: "interactive", + ChannelId: th.BasicChannel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + model.PostPropsAttachments: []*model.MessageAttachment{{ + Text: "hello", + Actions: []*model.PostAction{{ + Type: model.PostActionTypeButton, + Name: "click", + Integration: &model.PostActionIntegration{ + URL: ts.URL, + }, + }}, + }}, + model.PostPropsFromBot: "true", + model.PostPropsFromPlugin: "true", + }, + } + + post, _, appErr := th.App.CreatePostAsUser(th.Context, &interactivePost, "", true) + require.Nil(t, appErr) + attachments, ok := post.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) + require.True(t, ok) + + _, appErr = th.App.DoPostActionWithCookie(th.Context, post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "", nil, nil) + require.Nil(t, appErr) + + stored, nErr := th.App.Srv().Store().Post().GetSingle(th.Context, post.Id, false) + require.NoError(t, nErr) + + assert.Equal(t, "true", stored.GetProp(model.PostPropsFromBot), "from_bot must be retained across plugin update response") + assert.Equal(t, "true", stored.GetProp(model.PostPropsFromPlugin), "from_plugin must be retained across plugin update response") + assert.Equal(t, "AA", stored.GetProp("A"), "plugin-supplied prop applied") +} diff --git a/server/channels/app/plugin_api.go b/server/channels/app/plugin_api.go index fa6dad2bec4..b57b3a2ef2f 100644 --- a/server/channels/app/plugin_api.go +++ b/server/channels/app/plugin_api.go @@ -874,7 +874,19 @@ func (api *PluginAPI) GetPostsForChannel(channelID string, page, perPage int) (* } func (api *PluginAPI) UpdatePost(post *model.Post) (*model.Post, *model.AppError) { - post, _, appErr := api.app.UpdatePost(api.ctx, post, &model.UpdatePostOptions{SafeUpdate: false}) + // Grant mm_blocks_actions write access only when the plugin's update + // actually includes the prop, AND the value passes validation. + // Otherwise the freeze in UpdatePost preserves whatever the original + // post had — plugins that update unrelated fields don't accidentally + // drop or corrupt mm_blocks_actions. + allowMmBlocksActionsUpdate := false + if post.GetProp(model.PostPropsMmBlocksActions) != nil { + if err := model.ValidateMmBlocksActions(post); err != nil { + return nil, model.NewAppError("UpdatePost", "plugin.api.update_post.mm_blocks_actions.app_error", nil, "", http.StatusBadRequest).Wrap(err) + } + allowMmBlocksActionsUpdate = true + } + post, _, appErr := api.app.UpdatePost(api.ctx, post, &model.UpdatePostOptions{SafeUpdate: false, AllowMmBlocksActionsUpdate: allowMmBlocksActionsUpdate}) if post != nil { post = post.ForPlugin() } diff --git a/server/channels/app/post.go b/server/channels/app/post.go index ff61bd923c3..71ede54b87a 100644 --- a/server/channels/app/post.go +++ b/server/channels/app/post.go @@ -255,6 +255,24 @@ func (a *App) CreatePost(rctx request.CTX, post *model.Post, channel *model.Chan post.AddProp(model.PostPropsFromOAuthApp, "true") } + // Strip mm_blocks_actions from posts that are neither bot-authored nor + // created via an integration session. Either signal is sufficient: + // - user.IsBot (DB-verified) covers PluginAPI.CreatePost where the + // plugin's static rctx has no integration markers but the post + // is authored by a bot user. + // - rctx.Session().IsIntegration() (server-derived, unspoofable) + // covers REST callers using bot tokens, PATs, or OAuth apps. + // + // Webhooks are handled separately at their entry point + // (CreateWebhookPost) — webhook payloads are user-controlled even + // when bound to a bot user, so the prop is dropped before the post + // reaches CreatePost. See TestCreateWebhookPostStripsMmBlocksActions. + if post.GetProp(model.PostPropsMmBlocksActions) != nil { + if !user.IsBot && !rctx.Session().IsIntegration() { + post.DelProp(model.PostPropsMmBlocksActions) + } + } + var ephemeralPost *model.Post if post.Type == "" { if hasPermission, _ := a.HasPermissionToChannel(rctx, user.Id, channel.Id, model.PermissionUseChannelMentions); !hasPermission { @@ -710,6 +728,13 @@ func (a *App) SendEphemeralPost(rctx request.CTX, userID string, post *model.Pos post.SetProps(make(model.StringInterface)) } + // mm_blocks_actions cannot be resolved on click for ephemeral posts (no + // DB row, no per-action cookie transport). Drop the prop here so the + // client doesn't render a non-functional button. + if post.GetProp(model.PostPropsMmBlocksActions) != nil { + post.DelProp(model.PostPropsMmBlocksActions) + } + post.GenerateActionIds() message := model.NewWebSocketEvent(model.WebsocketEventEphemeralMessage, "", post.ChannelId, userID, nil, "") post = a.PreparePostForClientWithEmbedsAndImages(rctx, post, &model.PreparePostForClientOpts{IsNewPost: true, IncludePriority: true}) @@ -744,6 +769,13 @@ func (a *App) UpdateEphemeralPost(rctx request.CTX, userID string, post *model.P post.SetProps(make(model.StringInterface)) } + // mm_blocks_actions cannot be resolved on click for ephemeral posts (no + // DB row, no per-action cookie transport). Drop the prop here so the + // client doesn't render a non-functional button. + if post.GetProp(model.PostPropsMmBlocksActions) != nil { + post.DelProp(model.PostPropsMmBlocksActions) + } + post.GenerateActionIds() message := model.NewWebSocketEvent(model.WebsocketEventPostEdited, "", post.ChannelId, userID, nil, "") post = a.PreparePostForClientWithEmbedsAndImages(rctx, post, &model.PreparePostForClientOpts{IsNewPost: true, IncludePriority: true}) @@ -862,6 +894,21 @@ func (a *App) UpdatePost(rctx request.CTX, receivedUpdatedPost *model.Post, upda newPost.HasReactions = receivedUpdatedPost.HasReactions newPost.SetProps(receivedUpdatedPost.GetProps()) + // mm_blocks_actions can only be modified by trusted paths that have + // pre-validated the new value (AllowMmBlocksActionsUpdate). Session + // type is intentionally not a sufficient signal: a PAT/OAuth session + // from a regular user would otherwise bypass the freeze and inject + // mm_blocks_actions on edit, since from_bot on the original post is + // user-forgeable. All other callers keep whatever mm_blocks_actions + // the original post had (or none). + if !updatePostOptions.AllowMmBlocksActionsUpdate { + if oldVal, ok := oldPost.GetProps()[model.PostPropsMmBlocksActions]; ok { + newPost.AddProp(model.PostPropsMmBlocksActions, oldVal) + } else { + newPost.DelProp(model.PostPropsMmBlocksActions) + } + } + var fileIds []string fileIds, appErr = a.processPostFileChanges(rctx, receivedUpdatedPost, oldPost, updatePostOptions) if appErr != nil { diff --git a/server/channels/app/webhook.go b/server/channels/app/webhook.go index 92775e70d50..68f5655c04e 100644 --- a/server/channels/app/webhook.go +++ b/server/channels/app/webhook.go @@ -367,6 +367,12 @@ func (a *App) CreateWebhookPost(rctx request.CTX, userID string, channel *model. model.PostPropsOverrideUsername, model.PostPropsFromWebhook: // Do nothing + case model.PostPropsMmBlocksActions: + // Webhook payloads are user-controlled even when the + // webhook is bound to a bot user, so the bot-author + // signal in CreatePost's strip rule cannot distinguish + // them. Drop here so mm_blocks_actions never reaches + // the post object. default: post.AddProp(key, val) } diff --git a/server/go.mod b/server/go.mod index c9a998b1530..54cf7d011fd 100644 --- a/server/go.mod +++ b/server/go.mod @@ -1,6 +1,6 @@ module github.com/mattermost/mattermost/server/v8 -go 1.26.2 +go 1.26.3 require ( code.sajari.com/docconv/v2 v2.0.0-pre.4 diff --git a/server/i18n/en.json b/server/i18n/en.json index 1f58855805c..862b1b859ab 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -2940,6 +2940,14 @@ "id": "api.post.do_action.action_integration.app_error", "translation": "Action integration error." }, + { + "id": "api.post.do_action.merge_query.app_error", + "translation": "Failed to merge query into action URL." + }, + { + "id": "api.post.do_action.query.app_error", + "translation": "Invalid action query." + }, { "id": "api.post.error_get_post_id.pending", "translation": "Unable to get the pending post." @@ -12866,6 +12874,10 @@ "id": "plugin.api.get_users_in_channel", "translation": "Unable to get the users, invalid sorting criteria." }, + { + "id": "plugin.api.update_post.mm_blocks_actions.app_error", + "translation": "Invalid mm_blocks_actions in plugin post update." + }, { "id": "plugin.api.update_user_status.bad_status", "translation": "Unable to set the user status. Unknown user status." diff --git a/server/public/go.mod b/server/public/go.mod index c93244d8c7e..ad6e4f6ad88 100644 --- a/server/public/go.mod +++ b/server/public/go.mod @@ -1,6 +1,6 @@ module github.com/mattermost/mattermost/server/public -go 1.26.2 +go 1.26.3 require ( github.com/Masterminds/semver/v3 v3.4.0 diff --git a/server/public/model/feature_flags.go b/server/public/model/feature_flags.go index 3b0e698d168..4d4b6a43c1e 100644 --- a/server/public/model/feature_flags.go +++ b/server/public/model/feature_flags.go @@ -79,9 +79,6 @@ type FeatureFlags struct { ContentFlagging bool - // Enable AppsForm for Interactive Dialogs instead of legacy dialog implementation - InteractiveDialogAppsForm bool - EnableMattermostEntry bool // DEPRECATED: Mobile SSO SAML code-exchange flow - disabled by default @@ -159,7 +156,6 @@ func (f *FeatureFlags) SetDefaults() { f.AttributeValueMasking = false f.PermissionPolicies = false f.ContentFlagging = true - f.InteractiveDialogAppsForm = true f.EnableMattermostEntry = true // DEPRECATED: Disabled by default - mobile clients use direct SSO callback flow diff --git a/server/public/model/integration_action.go b/server/public/model/integration_action.go index a8e00646442..03e8875cb8b 100644 --- a/server/public/model/integration_action.go +++ b/server/public/model/integration_action.go @@ -16,7 +16,9 @@ import ( "io" "math/big" "net/http" + "net/url" "reflect" + "regexp" "slices" "strconv" "strings" @@ -55,16 +57,30 @@ var commonDateTimeFormats = []string{ ISODateTimeNoSecondsFormat, // ISO datetime without seconds } -var PostActionRetainPropKeys = []string{PostPropsFromWebhook, PostPropsOverrideUsername, PostPropsOverrideIconURL} +var PostActionRetainPropKeys = []string{ + PostPropsFromWebhook, + PostPropsFromBot, + PostPropsFromPlugin, + PostPropsOverrideUsername, + PostPropsOverrideIconURL, +} type DoPostActionRequest struct { - SelectedOption string `json:"selected_option,omitempty"` - Cookie string `json:"cookie,omitempty"` + SelectedOption string `json:"selected_option,omitempty"` + Cookie string `json:"cookie,omitempty"` + Query map[string]string `json:"query,omitempty"` } const ( PostActionDataSourceUsers = "users" PostActionDataSourceChannels = "channels" + + MaxMmBlocksActionsPerPost = 50 + MaxMmBlocksActionKeyLength = 64 + + MaxActionQueryEntries = 50 + MaxActionQueryKeyLength = 128 + MaxActionQueryValueLength = 2048 ) type PostAction struct { @@ -873,6 +889,7 @@ func (o *Post) StripActionIntegrations() { action.Integration = nil } } + o.StripMmBlocksActionSecrets() } func (o *Post) GetAction(id string) *PostAction { @@ -883,6 +900,137 @@ func (o *Post) GetAction(id string) *PostAction { } } } + if spec := o.GetMmBlocksActionSpec(id); spec != nil && spec.Type == MmBlocksActionTypeExternal && spec.URL != "" { + // Synthesize a PostAction so the existing click pipeline can + // dispatch without branching on action source. Pre-merge the + // spec's static per-action query into the URL here; per-click + // query (from DoPostActionRequest.Query) is merged on top by the + // caller via MergeQueryIntoURL, with per-click overriding static + // values on overlapping keys. + url := spec.URL + if len(spec.Query) > 0 { + merged, err := MergeQueryIntoURL(spec.URL, spec.Query) + if err != nil { + // Spec URL is malformed. ValidateMmBlocksActions + // should have rejected it at save time, so this is a + // belt-and-suspenders guard. Returning nil routes the + // caller through the standard "action not found" + // 404 path rather than firing a request to a URL + // that's missing the static query params. + return nil + } + url = merged + } + return &PostAction{ + Id: id, + Type: PostActionTypeButton, + Integration: &PostActionIntegration{ + URL: url, + Context: spec.Context, + }, + } + } + return nil +} + +var mmBlocksActionIDRegex = regexp.MustCompile(`^[A-Za-z0-9]+$`) + +// ValidateMmBlocksActions verifies the post's mm_blocks_actions prop has the +// expected shape and bounds. Each entry must coerce to a valid spec via +// mmBlocksEntryMapToSpec. +func ValidateMmBlocksActions(o *Post) error { + raw := o.GetProp(PostPropsMmBlocksActions) + if raw == nil { + return nil + } + actions, ok := coerceToStringAnyMap(raw) + if !ok { + return fmt.Errorf("mm_blocks_actions must be a map") + } + if len(actions) > MaxMmBlocksActionsPerPost { + return fmt.Errorf("mm_blocks_actions exceeds maximum of %d entries", MaxMmBlocksActionsPerPost) + } + for key, entry := range actions { + if len(key) > MaxMmBlocksActionKeyLength { + return fmt.Errorf("mm_blocks_actions key exceeds %d chars", MaxMmBlocksActionKeyLength) + } + if !mmBlocksActionIDRegex.MatchString(key) { + return fmt.Errorf("mm_blocks_actions key %q must be alphanumeric", key) + } + entryMap, ok := coerceToStringAnyMap(entry) + if !ok { + return fmt.Errorf("mm_blocks_actions entry %q must be an object", key) + } + spec := mmBlocksEntryMapToSpec(entryMap) + if spec == nil { + return fmt.Errorf("mm_blocks_actions entry %q has invalid type or shape", key) + } + if spec.Type == MmBlocksActionTypeExternal { + if err := validateIntegrationURL(spec.URL); err != nil { + return fmt.Errorf("mm_blocks_actions entry %q: %w", key, err) + } + // Bound the per-spec static query so a bot cannot stash + // unbounded data in the post that gets merged into the + // outgoing URL on every click. + if err := ValidateActionQuery(spec.Query); err != nil { + return fmt.Errorf("mm_blocks_actions entry %q static query: %w", key, err) + } + // Bound entry count and key length on the static context. + // Values are arbitrary JSON, so size is constrained by the + // outer post-size limit; we cap entries to prevent crafted + // posts from inflating GetAction's clone cost. + if len(spec.Context) > MaxActionQueryEntries { + return fmt.Errorf("mm_blocks_actions entry %q context exceeds maximum of %d entries", key, MaxActionQueryEntries) + } + for k := range spec.Context { + if len(k) > MaxActionQueryKeyLength { + return fmt.Errorf("mm_blocks_actions entry %q context key exceeds %d chars", key, MaxActionQueryKeyLength) + } + } + } + } + return nil +} + +// ValidateActionQuery bounds the size of user-supplied per-click query +// parameters so a crafted post cannot trigger unbounded memory use in the +// plugin-request path. +func ValidateActionQuery(q map[string]string) error { + if len(q) > MaxActionQueryEntries { + return fmt.Errorf("query exceeds maximum of %d entries", MaxActionQueryEntries) + } + for key, value := range q { + if len(key) > MaxActionQueryKeyLength { + return fmt.Errorf("query key exceeds %d chars", MaxActionQueryKeyLength) + } + if len(value) > MaxActionQueryValueLength { + return fmt.Errorf("query value for %q exceeds %d chars", key, MaxActionQueryValueLength) + } + } + return nil +} + +func validateIntegrationURL(rawURL string) error { + if rawURL == "" { + return fmt.Errorf("must have a non-empty URL") + } + if !(strings.HasPrefix(rawURL, "/plugins/") || strings.HasPrefix(rawURL, "plugins/") || IsValidHTTPURL(rawURL)) { + return fmt.Errorf("must have a valid integration URL") + } + // Reject path-traversal segments. /plugins/ URLs are routed by the + // local server, so a `..` segment can escape the plugin namespace and + // hit unrelated server routes. url.Parse decodes percent-encoded path + // bytes into u.Path, which is the same single decode pass that + // doPluginRequest performs at dispatch — so encoded forms like + // %2e%2e%2f are caught here symmetrically with how the router would + // resolve them. + u, parseErr := url.Parse(rawURL) + if parseErr != nil { + return fmt.Errorf("must have a valid integration URL: %w", parseErr) + } + if strings.Contains(u.Path, "/../") || strings.HasSuffix(u.Path, "/..") { + return fmt.Errorf("integration URL must not contain path traversal segments") + } return nil } diff --git a/server/public/model/integration_action_test.go b/server/public/model/integration_action_test.go index baefc9f968b..69a751982d2 100644 --- a/server/public/model/integration_action_test.go +++ b/server/public/model/integration_action_test.go @@ -12,6 +12,7 @@ import ( "encoding/json" "io" "math/big" + "strconv" "strings" "testing" "time" @@ -1676,3 +1677,742 @@ func TestDialogElementDateTimeValidation(t *testing.T) { assert.True(t, effective.ManualTimeEntry, "deprecated field alone should enable manual entry after EffectiveDateTimeConfig") }) } + +func TestValidateActionQuery(t *testing.T) { + t.Run("nil map is valid", func(t *testing.T) { + assert.NoError(t, ValidateActionQuery(nil)) + }) + + t.Run("empty map is valid", func(t *testing.T) { + assert.NoError(t, ValidateActionQuery(map[string]string{})) + }) + + t.Run("within bounds is valid", func(t *testing.T) { + ctx := map[string]string{ + "alpha": "one", + "beta": "two", + } + assert.NoError(t, ValidateActionQuery(ctx)) + }) + + t.Run("exceeds MaxActionQueryEntries", func(t *testing.T) { + ctx := make(map[string]string, MaxActionQueryEntries+1) + for i := range MaxActionQueryEntries + 1 { + ctx[strconv.Itoa(i)] = "v" + } + err := ValidateActionQuery(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum") + }) + + t.Run("key length exactly MaxActionQueryKeyLength is allowed", func(t *testing.T) { + ctx := map[string]string{ + strings.Repeat("k", MaxActionQueryKeyLength): "value", + } + assert.NoError(t, ValidateActionQuery(ctx)) + }) + + t.Run("key length MaxActionQueryKeyLength+1 is rejected", func(t *testing.T) { + ctx := map[string]string{ + strings.Repeat("k", MaxActionQueryKeyLength+1): "value", + } + err := ValidateActionQuery(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "key exceeds") + }) + + t.Run("value length exactly MaxActionQueryValueLength is allowed", func(t *testing.T) { + ctx := map[string]string{ + "key": strings.Repeat("v", MaxActionQueryValueLength), + } + assert.NoError(t, ValidateActionQuery(ctx)) + }) + + t.Run("value length MaxActionQueryValueLength+1 is rejected", func(t *testing.T) { + ctx := map[string]string{ + "key": strings.Repeat("v", MaxActionQueryValueLength+1), + } + err := ValidateActionQuery(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "value for") + }) + + t.Run("multiple violations triggers an error", func(t *testing.T) { + // Too many entries AND every value is over-length. First detected + // violation wins; only assert that an error is returned. + ctx := make(map[string]string, MaxActionQueryEntries+1) + for i := range MaxActionQueryEntries + 1 { + ctx[strconv.Itoa(i)] = strings.Repeat("v", MaxActionQueryValueLength+1) + } + err := ValidateActionQuery(ctx) + require.Error(t, err) + }) +} + +func mmBlocksExternalEntry(url string, context map[string]any) map[string]any { + entry := map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": url, + } + if context != nil { + entry["context"] = context + } + return entry +} + +func TestGetMmBlocksActionSpec(t *testing.T) { + t.Run("prop absent returns nil", func(t *testing.T) { + p := &Post{} + assert.Nil(t, p.GetMmBlocksActionSpec("btn1")) + }) + + t.Run("empty action id returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + assert.Nil(t, p.GetMmBlocksActionSpec("")) + }) + + t.Run("id not found returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + assert.Nil(t, p.GetMmBlocksActionSpec("missing")) + }) + + t.Run("external entry returns spec with url and context", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", map[string]any{"k": "v"}), + }) + got := p.GetMmBlocksActionSpec("btn1") + require.NotNil(t, got) + assert.Equal(t, MmBlocksActionTypeExternal, got.Type) + assert.Equal(t, "http://example.com/hook", got.URL) + assert.Equal(t, "v", got.Context["k"]) + }) + + t.Run("entry missing type returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{"url": "http://example.com/hook"}, + }) + assert.Nil(t, p.GetMmBlocksActionSpec("btn1")) + }) + + t.Run("entry with unknown type returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": "bogus", + "url": "http://example.com/hook", + }, + }) + assert.Nil(t, p.GetMmBlocksActionSpec("btn1")) + }) + + t.Run("wrong-shape prop returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, "not-a-map") + assert.Nil(t, p.GetMmBlocksActionSpec("btn1")) + }) + + t.Run("entry value not an object returns nil", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": "not-an-object", + }) + assert.Nil(t, p.GetMmBlocksActionSpec("btn1")) + }) +} + +func TestValidateMmBlocksActions(t *testing.T) { + t.Run("absent prop returns no error", func(t *testing.T) { + p := &Post{} + assert.NoError(t, ValidateMmBlocksActions(p)) + }) + + t.Run("string prop is rejected (cookie transport not yet supported)", func(t *testing.T) { + // The cookie-transport PR will add proper validation for + // encrypted-string payloads. Until then, any string value is + // rejected so an integration session cannot bypass the + // alphanumeric-key, URL, and bounds checks by simply storing a + // raw string at the prop key. + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, "encrypted-cookie-blob") + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a map") + }) + + t.Run("valid external entries return no error", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + "btn2": mmBlocksExternalEntry("/plugins/myplugin/action", nil), + "btn3": mmBlocksExternalEntry("plugins/myplugin/action", nil), + }) + assert.NoError(t, ValidateMmBlocksActions(p)) + }) + + t.Run("exceeding MaxMmBlocksActionsPerPost returns error", func(t *testing.T) { + actions := make(map[string]any, MaxMmBlocksActionsPerPost+1) + for i := range MaxMmBlocksActionsPerPost + 1 { + actions["btn"+strconv.Itoa(i)] = mmBlocksExternalEntry("http://example.com/hook", nil) + } + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, actions) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum") + }) + + t.Run("action id with hyphen is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "foo-bar": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be alphanumeric") + }) + + t.Run("action id at MaxMmBlocksActionKeyLength is allowed", func(t *testing.T) { + key := strings.Repeat("a", MaxMmBlocksActionKeyLength) + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + key: mmBlocksExternalEntry("http://example.com/hook", nil), + }) + assert.NoError(t, ValidateMmBlocksActions(p)) + }) + + t.Run("action id over MaxMmBlocksActionKeyLength is rejected", func(t *testing.T) { + key := strings.Repeat("a", MaxMmBlocksActionKeyLength+1) + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + key: mmBlocksExternalEntry("http://example.com/hook", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds") + }) + + t.Run("action id with underscore is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "foo_bar": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be alphanumeric") + }) + + t.Run("action id with space is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "FOO bar": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be alphanumeric") + }) + + t.Run("empty URL is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty URL") + }) + + t.Run("path traversal in /plugins/ URL is rejected", func(t *testing.T) { + // Defense-in-depth: a `..` segment in a /plugins/ URL can escape the + // plugin namespace at request time. Bot-authored mm_blocks specs are + // the origin point so we reject at save. + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("/plugins/../../../etc/passwd", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "path traversal") + }) + + t.Run("trailing /.. in /plugins/ URL is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("/plugins/myplugin/..", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "path traversal") + }) + + t.Run("percent-encoded traversal in /plugins/ URL is rejected", func(t *testing.T) { + // doPluginRequest decodes the path via url.Parse before path.Clean, + // so an encoded "%2e%2e%2f" would otherwise route to a different + // plugin than the validator thinks it's protecting. Validator must + // decode symmetrically to catch this at save time. + for _, encoded := range []string{ + "/plugins/innocent/%2e%2e%2f/target/handler", + "/plugins/innocent/%2E%2E%2F/target/handler", + "/plugins/innocent/..%2f/target/handler", + "/plugins/innocent/%2e%2e/", + "/plugins/innocent/%2e%2e", + } { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry(encoded, nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err, "url=%q must be rejected", encoded) + assert.Contains(t, err.Error(), "path traversal", "url=%q", encoded) + } + }) + + t.Run("entry missing type is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{"url": "http://example.com/hook"}, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid type or shape") + }) + + t.Run("entry with unknown type is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": "bogus", + "url": "http://example.com/hook", + }, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid type or shape") + }) + + t.Run("entry value not an object is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": "not-an-object", + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be an object") + }) + + t.Run("javascript URL is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("javascript://alert(1)", nil), + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "valid integration URL") + }) + + t.Run("http URL is accepted", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://legit.com", nil), + }) + assert.NoError(t, ValidateMmBlocksActions(p)) + }) + + t.Run("/plugins/ URL is accepted", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("/plugins/foo", nil), + }) + assert.NoError(t, ValidateMmBlocksActions(p)) + }) + + t.Run("wrong-shape raw prop is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, []string{"not-a-map"}) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a map") + }) + + t.Run("static query exceeding entry cap is rejected", func(t *testing.T) { + query := make(map[string]any, MaxActionQueryEntries+1) + for i := range MaxActionQueryEntries + 1 { + query["k"+strconv.Itoa(i)] = "v" + } + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + "query": query, + }, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "static query") + }) + + t.Run("static query value exceeding length cap is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + "query": map[string]any{"k": strings.Repeat("a", MaxActionQueryValueLength+1)}, + }, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "static query") + }) + + t.Run("static context exceeding entry cap is rejected", func(t *testing.T) { + ctx := make(map[string]any, MaxActionQueryEntries+1) + for i := range MaxActionQueryEntries + 1 { + ctx["k"+strconv.Itoa(i)] = "v" + } + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + "context": ctx, + }, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "context exceeds maximum") + }) + + t.Run("static context key exceeding length cap is rejected", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + "context": map[string]any{strings.Repeat("a", MaxActionQueryKeyLength+1): "v"}, + }, + }) + err := ValidateMmBlocksActions(p) + require.Error(t, err) + assert.Contains(t, err.Error(), "context key exceeds") + }) +} + +func TestStripActionIntegrations_MmBlocksActions(t *testing.T) { + t.Run("strips mm_blocks_actions prop", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + p.StripActionIntegrations() + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + }) + + t.Run("post without mm_blocks_actions prop does not panic", func(t *testing.T) { + p := &Post{} + assert.NotPanics(t, func() { + p.StripActionIntegrations() + }) + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + }) + + t.Run("post with both attachments and mm_blocks_actions cleans both", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsAttachments, []*MessageAttachment{ + { + Actions: []*PostAction{ + { + Id: "a1", + Name: "Button", + Type: PostActionTypeButton, + Integration: &PostActionIntegration{URL: "http://example.com/hook"}, + }, + }, + }, + }) + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + + p.StripActionIntegrations() + + // mm_blocks_actions prop should be removed entirely. + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + + // Attachment actions should remain but with nil Integration. + attachments := p.Attachments() + require.Len(t, attachments, 1) + require.Len(t, attachments[0].Actions, 1) + assert.Nil(t, attachments[0].Actions[0].Integration) + }) +} + +func TestGetAction_MmBlocksFallback(t *testing.T) { + t.Run("returns attachment action when present", func(t *testing.T) { + attachmentAction := &PostAction{ + Id: "a1", + Name: "Attach Button", + Type: PostActionTypeButton, + Integration: &PostActionIntegration{URL: "http://example.com/attach"}, + } + p := &Post{} + p.AddProp(PostPropsAttachments, []*MessageAttachment{ + {Actions: []*PostAction{attachmentAction}}, + }) + + got := p.GetAction("a1") + require.NotNil(t, got) + assert.Same(t, attachmentAction, got) + }) + + t.Run("synthesizes PostAction from mm_blocks_actions when no attachment match", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", map[string]any{"k": "v"}), + }) + + got := p.GetAction("btn1") + require.NotNil(t, got) + assert.Equal(t, "btn1", got.Id) + assert.Equal(t, PostActionTypeButton, got.Type) + require.NotNil(t, got.Integration) + assert.Equal(t, "http://example.com/hook", got.Integration.URL) + assert.Equal(t, "v", got.Integration.Context["k"]) + }) + + t.Run("synthesized URL pre-merges spec static query", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + "query": map[string]any{"source": "fleet-status"}, + }, + }) + + got := p.GetAction("btn1") + require.NotNil(t, got) + require.NotNil(t, got.Integration) + assert.Equal(t, "http://example.com/hook?source=fleet-status", got.Integration.URL) + }) + + t.Run("synthesized URL preserves existing query and adds spec static query", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook?team=alpha", + "query": map[string]any{"source": "fleet-status"}, + }, + }) + + got := p.GetAction("btn1") + require.NotNil(t, got) + require.NotNil(t, got.Integration) + // url.Values.Encode() sorts keys alphabetically. + assert.Contains(t, got.Integration.URL, "source=fleet-status") + assert.Contains(t, got.Integration.URL, "team=alpha") + }) + + t.Run("attachment wins when id matches both attachment and mm_blocks action", func(t *testing.T) { + attachmentAction := &PostAction{ + Id: "btn1", + Name: "Attach Button", + Type: PostActionTypeButton, + Integration: &PostActionIntegration{URL: "http://example.com/attach"}, + } + p := &Post{} + p.AddProp(PostPropsAttachments, []*MessageAttachment{ + {Actions: []*PostAction{attachmentAction}}, + }) + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/inline", nil), + }) + + got := p.GetAction("btn1") + require.NotNil(t, got) + assert.Same(t, attachmentAction, got) + assert.Equal(t, "http://example.com/attach", got.Integration.URL) + }) + + t.Run("returns nil when id matches neither", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsAttachments, []*MessageAttachment{ + {Actions: []*PostAction{{Id: "other", Name: "X", Type: PostActionTypeButton, Integration: &PostActionIntegration{URL: "http://example.com"}}}}, + }) + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "something": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + + assert.Nil(t, p.GetAction("missing")) + }) + + t.Run("returns nil when spec URL is unparseable and static query merge fails", func(t *testing.T) { + // Defense-in-depth: ValidateMmBlocksActions should reject this at + // save time, but if a malformed URL slips through, GetAction must + // not silently fire the bare URL with the static query dropped. + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/%%%bad", + "query": map[string]any{"source": "fleet"}, + }, + }) + + assert.Nil(t, p.GetAction("btn1")) + }) +} + +func TestMergeQueryIntoURL(t *testing.T) { + t.Run("empty query returns rawURL unchanged", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook", nil) + require.NoError(t, err) + assert.Equal(t, "http://example.com/hook", got) + + got, err = MergeQueryIntoURL("http://example.com/hook", map[string]string{}) + require.NoError(t, err) + assert.Equal(t, "http://example.com/hook", got) + }) + + t.Run("URL without existing query gets query appended", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook", map[string]string{"a": "1"}) + require.NoError(t, err) + assert.Equal(t, "http://example.com/hook?a=1", got) + }) + + t.Run("URL with existing query merges non-overlapping keys", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook?team=alpha", map[string]string{"source": "fleet"}) + require.NoError(t, err) + // Encode() sorts keys alphabetically. + assert.Contains(t, got, "team=alpha") + assert.Contains(t, got, "source=fleet") + }) + + t.Run("query map overrides existing key on overlap", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook?tail=999", map[string]string{"tail": "214"}) + require.NoError(t, err) + assert.Equal(t, "http://example.com/hook?tail=214", got) + }) + + t.Run("URL fragment is preserved", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook#anchor", map[string]string{"a": "1"}) + require.NoError(t, err) + assert.Equal(t, "http://example.com/hook?a=1#anchor", got) + }) + + t.Run("special characters in values are URL-encoded", func(t *testing.T) { + got, err := MergeQueryIntoURL("http://example.com/hook", map[string]string{"q": "a b&c=d"}) + require.NoError(t, err) + // space → +, & and = → %26 / %3D + assert.Contains(t, got, "q=a+b%26c%3Dd") + }) + + t.Run("relative URL with empty path accepts query merge", func(t *testing.T) { + got, err := MergeQueryIntoURL("/plugins/myplugin/action", map[string]string{"a": "1"}) + require.NoError(t, err) + assert.Equal(t, "/plugins/myplugin/action?a=1", got) + }) + + t.Run("malformed URL returns parse error", func(t *testing.T) { + _, err := MergeQueryIntoURL("://not-a-url", map[string]string{"a": "1"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "parse url") + }) +} + +func TestMmBlocksContextMap(t *testing.T) { + t.Run("empty string returns nil", func(t *testing.T) { + assert.Nil(t, MmBlocksContextMap("")) + }) + + t.Run("valid JSON object string is parsed into a map", func(t *testing.T) { + got := MmBlocksContextMap(`{"k":"v","n":1}`) + require.NotNil(t, got) + assert.Equal(t, "v", got["k"]) + // JSON numbers decode to float64. + assert.Equal(t, float64(1), got["n"]) + }) + + t.Run("non-JSON string is wrapped under context key", func(t *testing.T) { + got := MmBlocksContextMap("hello world") + require.NotNil(t, got) + assert.Equal(t, "hello world", got["context"]) + }) + + t.Run("JSON null falls back to wrap (m is nil after unmarshal)", func(t *testing.T) { + got := MmBlocksContextMap("null") + require.NotNil(t, got) + assert.Equal(t, "null", got["context"]) + }) + + t.Run("JSON array falls back to wrap (target type mismatch)", func(t *testing.T) { + got := MmBlocksContextMap("[1,2,3]") + require.NotNil(t, got) + assert.Equal(t, "[1,2,3]", got["context"]) + }) + + t.Run("JSON number falls back to wrap (target type mismatch)", func(t *testing.T) { + got := MmBlocksContextMap("42") + require.NotNil(t, got) + assert.Equal(t, "42", got["context"]) + }) + + t.Run("malformed JSON falls back to wrap", func(t *testing.T) { + got := MmBlocksContextMap(`{"unclosed":`) + require.NotNil(t, got) + assert.Equal(t, `{"unclosed":`, got["context"]) + }) +} + +func TestStripMmBlocksActionSecrets(t *testing.T) { + t.Run("absent prop is a no-op", func(t *testing.T) { + p := &Post{} + assert.NotPanics(t, func() { + p.StripMmBlocksActionSecrets() + }) + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + }) + + t.Run("map-form prop is deleted", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + p.StripMmBlocksActionSecrets() + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + }) + + t.Run("string-form prop is deleted (cookie transport not yet supported)", func(t *testing.T) { + // Until the cookie-transport PR ships proper handling, any string + // value is treated as opaque garbage and stripped wholesale — + // matches the validator's reject-strings policy. + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, "encrypted-cookie-blob") + p.StripMmBlocksActionSecrets() + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + }) + + t.Run("other props on the post are not touched", func(t *testing.T) { + p := &Post{} + p.AddProp(PostPropsMmBlocksActions, map[string]any{ + "btn1": mmBlocksExternalEntry("http://example.com/hook", nil), + }) + p.AddProp(PostPropsAttachments, []*MessageAttachment{{Text: "keep me"}}) + p.AddProp(PostPropsFromBot, "true") + + p.StripMmBlocksActionSecrets() + + assert.Nil(t, p.GetProp(PostPropsMmBlocksActions)) + assert.NotNil(t, p.GetProp(PostPropsAttachments)) + assert.Equal(t, "true", p.GetProp(PostPropsFromBot)) + }) +} diff --git a/server/public/model/mm_blocks_actions.go b/server/public/model/mm_blocks_actions.go new file mode 100644 index 00000000000..dbea4c869aa --- /dev/null +++ b/server/public/model/mm_blocks_actions.go @@ -0,0 +1,154 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +// Server-side definitions for the post.props.mm_blocks_actions registry that +// underpins the markdown-actions feature. Mirrors the canonical model +// landing in the broader mm_blocks framework PR; cookie transport +// (MmBlocksActionCookie, AddMmBlocksActionCookies, ParseDecryptedActionCookiePayload) +// is intentionally omitted here and will be filled in by that PR. Until then, +// mm_blocks_actions is resolved on click via DB lookup +// (GetMmBlocksActionSpec) and stripped from ephemeral broadcasts so dead +// buttons don't render. + +package model + +import ( + "encoding/json" + "fmt" + "maps" + "net/url" +) + +const ( + MmBlocksActionTypeExternal = "external" +) + +// MmBlocksActionSpec is the server-side definition for one entry in props.mm_blocks_actions. +type MmBlocksActionSpec struct { + Type string + URL string + Query map[string]string + Context map[string]any +} + +// GetMmBlocksActionSpec returns the action definition for actionID from props.mm_blocks_actions, if present. +func (o *Post) GetMmBlocksActionSpec(actionID string) *MmBlocksActionSpec { + raw := o.GetProp(PostPropsMmBlocksActions) + if raw == nil || actionID == "" { + return nil + } + actionsTop, ok := coerceToStringAnyMap(raw) + if !ok { + return nil + } + entry, ok := actionsTop[actionID] + if !ok || entry == nil { + return nil + } + entryMap, ok := coerceToStringAnyMap(entry) + if !ok { + return nil + } + return mmBlocksEntryMapToSpec(entryMap) +} + +// mmBlocksEntryMapToSpec maps one props.mm_blocks_actions[actionID] object to MmBlocksActionSpec. +func mmBlocksEntryMapToSpec(entryMap map[string]any) *MmBlocksActionSpec { + typ, _ := entryMap["type"].(string) + if typ == "" { + return nil + } + if typ != MmBlocksActionTypeExternal { + return nil + } + spec := &MmBlocksActionSpec{Type: typ} + spec.URL, _ = entryMap["url"].(string) + spec.Context = contextMapFromProp(entryMap["context"]) + spec.Query = stringMapFromPropValue(entryMap["query"]) + return spec +} + +// MmBlocksContextMap parses a context JSON string or treats a non-JSON string as a single context value. +func MmBlocksContextMap(contextString string) map[string]any { + if contextString == "" { + return nil + } + var m map[string]any + if err := json.Unmarshal([]byte(contextString), &m); err == nil && m != nil { + return m + } + return map[string]any{"context": contextString} +} + +// MergeQueryIntoURL merges q into rawURL's query string; existing keys are overwritten by q. +func MergeQueryIntoURL(rawURL string, q map[string]string) (string, error) { + if len(q) == 0 { + return rawURL, nil + } + u, err := url.Parse(rawURL) + if err != nil { + return "", fmt.Errorf("parse url: %w", err) + } + values := u.Query() + for k, v := range q { + values.Set(k, v) + } + u.RawQuery = values.Encode() + return u.String(), nil +} + +// StripMmBlocksActionSecrets removes server-only fields from +// props.mm_blocks_actions for wire serialization. The current +// implementation deletes the prop wholesale; the cookie-transport PR will +// extend this to preserve encrypted-string cookie payloads in place. +func (o *Post) StripMmBlocksActionSecrets() { + if o.GetProp(PostPropsMmBlocksActions) == nil { + return + } + o.DelProp(PostPropsMmBlocksActions) +} + +// contextMapFromProp normalizes props.mm_blocks_actions[*].context to map[string]any (JSON object or string). +func contextMapFromProp(v any) map[string]any { + if v == nil { + return nil + } + if s, ok := v.(string); ok { + return MmBlocksContextMap(s) + } + if m, ok := coerceToStringAnyMap(v); ok { + // Clone so callers cannot mutate the live post.Props map. A + // nested mutation through the returned map would otherwise race + // with concurrent post.Props readers. + return maps.Clone(m) + } + return nil +} + +func stringMapFromPropValue(v any) map[string]string { + m, ok := coerceToStringAnyMap(v) + if !ok || len(m) == 0 { + return nil + } + out := make(map[string]string, len(m)) + for k, val := range m { + if s, ok := val.(string); ok { + out[k] = s + } + } + if len(out) == 0 { + return nil + } + return out +} + +func coerceToStringAnyMap(v any) (map[string]any, bool) { + if v == nil { + return nil, false + } + m, ok := v.(map[string]any) + if ok { + return m, true + } + return nil, false +} diff --git a/server/public/model/post.go b/server/public/model/post.go index 3c1d52cb9ae..50f41586673 100644 --- a/server/public/model/post.go +++ b/server/public/model/post.go @@ -93,6 +93,7 @@ const ( PostPropsFromOAuthApp = "from_oauth_app" PostPropsWebhookDisplayName = "webhook_display_name" PostPropsAttachments = "attachments" + PostPropsMmBlocksActions = "mm_blocks_actions" PostPropsFromPlugin = "from_plugin" PostPropsMentionHighlightDisabled = "mentionHighlightDisabled" PostPropsGroupHighlightDisabled = "disable_group_highlight" @@ -619,6 +620,7 @@ func ContainsIntegrationsReservedProps(props StringInterface) []string { PostPropsWebhookDisplayName, PostPropsOverrideIconURL, PostPropsOverrideIconEmoji, + PostPropsMmBlocksActions, } for _, key := range reservedProps { @@ -843,6 +845,12 @@ func (o *Post) propsIsValid() error { } } + if props[PostPropsMmBlocksActions] != nil { + if err := ValidateMmBlocksActions(o); err != nil { + multiErr = multierror.Append(multiErr, fmt.Errorf("invalid mm_blocks_actions: %w", err)) + } + } + for i, a := range o.Attachments() { if err := a.IsValid(); err != nil { multiErr = multierror.Append(multiErr, multierror.Prefix(err, fmt.Sprintf("message attachtment at index %d is invalid:", i))) @@ -1197,6 +1205,14 @@ func (o *Post) CleanPost() *Post { type UpdatePostOptions struct { SafeUpdate bool IsRestorePost bool + + // AllowMmBlocksActionsUpdate grants the caller permission to add, + // remove, or modify the mm_blocks_actions prop. Without it, + // non-integration sessions cannot change mm_blocks_actions and the + // prop is reset to its prior value. Set only from trusted paths (e.g. + // the post-action integration response handler which has already + // validated the incoming value). + AllowMmBlocksActionsUpdate bool } func DefaultUpdatePostOptions() *UpdatePostOptions { diff --git a/server/public/model/post_test.go b/server/public/model/post_test.go index 1d0b17b2d73..62739b184b9 100644 --- a/server/public/model/post_test.go +++ b/server/public/model/post_test.go @@ -171,10 +171,17 @@ func TestPost_ContainsIntegrationsReservedProps(t *testing.T) { PostPropsOverrideUsername: "overridden_username", PostPropsOverrideIconURL: "a-custom-url", PostPropsOverrideIconEmoji: ":custom_emoji_name:", + PostPropsMmBlocksActions: map[string]any{ + "btn1": map[string]any{ + "type": MmBlocksActionTypeExternal, + "url": "http://example.com/hook", + }, + }, }, } keys2 := post2.ContainsIntegrationsReservedProps() - require.Len(t, keys2, 5) + require.Len(t, keys2, 6) + require.Contains(t, keys2, PostPropsMmBlocksActions) } func TestPostPatch_ContainsIntegrationsReservedProps(t *testing.T) { diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index bcf95f93107..985989f4b10 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/webapp/channels/src/components/admin_console/admin_definition.tsx @@ -4229,7 +4229,7 @@ const AdminDefinition: AdminDefinitionType = { label: defineMessage({id: 'admin.saml.getSamlMetadataFromIDPUrl', defaultMessage: 'Get SAML Metadata from IdP'}), loading: defineMessage({id: 'admin.saml.getSamlMetadataFromIDPFetching', defaultMessage: 'Fetching...'}), error_message: defineMessage({id: 'admin.saml.getSamlMetadataFromIDPFail', defaultMessage: 'SAML Metadata URL did not connect and pull data successfully'}), - success_message: defineMessage({id: 'admin.saml.getSamlMetadataFromIDPSuccess', defaultMessage: 'SAML Metadata retrieved successfully. Two fields below have been updated'}), + success_message: defineMessage({id: 'admin.saml.getSamlMetadataFromIDPSuccess', defaultMessage: 'SAML Metadata retrieved successfully. Two fields and one certificate have been updated'}), isDisabled: it.any( it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.AUTHENTICATION.SAML)), it.stateIsFalse('SamlSettings.Enable'), diff --git a/webapp/channels/src/components/admin_console/schema_admin_settings.tsx b/webapp/channels/src/components/admin_console/schema_admin_settings.tsx index 524eef4413e..801f5a70a85 100644 --- a/webapp/channels/src/components/admin_console/schema_admin_settings.tsx +++ b/webapp/channels/src/components/admin_console/schema_admin_settings.tsx @@ -397,8 +397,22 @@ export class SchemaAdminSettings extends React.PureComponent { + this.handleChange(key, filename); + this.setState({[key]: filename, [`${key}Error`]: null}); + }; + const onError = (err: {message: string}) => { + this.setState({[`${key}Error`]: err.message}); + }; + if (typeof inputData !== 'string' || inputData.trim() === '') { + onError({ + message: this.props.intl.formatMessage({id: 'admin.saml.getSamlMetadataFromIDPFail', defaultMessage: 'SAML Metadata URL did not connect and pull data successfully'}), + }); + return; + } + tsetting.set_action(onSuccess, onError, inputData); } } } diff --git a/webapp/channels/src/components/admin_console/types.ts b/webapp/channels/src/components/admin_console/types.ts index e747eb3619d..5341f61f6b9 100644 --- a/webapp/channels/src/components/admin_console/types.ts +++ b/webapp/channels/src/components/admin_console/types.ts @@ -108,7 +108,11 @@ export type AdminDefinitionSettingFileUpload = AdminDefinitionSettingBase & { uploading_text: string | MessageDescriptor; fileType: string; upload_action: (file: File, success: (data: any) => void, error: (err: any) => void) => void; - set_action?: () => void; + set_action?: ( + success: (filename: string) => void, + error: (err: {message: string}) => void, + data: string, + ) => void; setFromMetadataField?: string; remove_action: (success: (data: any) => void, error: (err: any) => void) => void; } diff --git a/webapp/channels/src/components/dialog_router/dialog_router.test.tsx b/webapp/channels/src/components/dialog_router/dialog_router.test.tsx index 2bb23c6bb02..4d4fb8f04e6 100644 --- a/webapp/channels/src/components/dialog_router/dialog_router.test.tsx +++ b/webapp/channels/src/components/dialog_router/dialog_router.test.tsx @@ -8,19 +8,6 @@ import DialogRouter from './dialog_router'; import type {PropsFromRedux} from './index'; -// Mock the components -jest.mock('components/interactive_dialog/interactive_dialog', () => { - return function MockInteractiveDialog(props: any) { - return ( -
-
{props.title}
-
{props.url}
-
{props.callbackId}
-
- ); - }; -}); - jest.mock('./interactive_dialog_adapter', () => { return function MockInteractiveDialogAdapter(props: any) { return ( @@ -34,7 +21,7 @@ jest.mock('./interactive_dialog_adapter', () => { }); describe('components/dialog_router/DialogRouter', () => { - const baseProps: Partial & Pick & {onExited?: () => void} = { + const baseProps: Partial & Pick & {onExited?: () => void} = { url: 'http://example.com', callbackId: 'abc123', elements: [], @@ -45,7 +32,6 @@ describe('components/dialog_router/DialogRouter', () => { notifyOnCancel: true, state: 'test-state', emojiMap: new (require('utils/emoji_map').default)(new Map()), - isAppsFormEnabled: true, hasUrl: true, actions: { submitInteractiveDialog: jest.fn(), @@ -63,7 +49,7 @@ describe('components/dialog_router/DialogRouter', () => { }); describe('Component Selection Logic', () => { - test('should render InteractiveDialogAdapter when apps form is enabled and URL is present', () => { + test('should render InteractiveDialogAdapter when URL is present', () => { const {getByTestId} = render( , ); @@ -74,22 +60,6 @@ describe('components/dialog_router/DialogRouter', () => { expect(getByTestId('adapter-callback-id')).toHaveTextContent('abc123'); }); - test('should render InteractiveDialog when apps form is disabled', () => { - const propsWithAppsFormDisabled = { - ...baseProps, - isAppsFormEnabled: false, - }; - - const {getByTestId} = render( - , - ); - - expect(getByTestId('interactive-dialog')).toBeInTheDocument(); - expect(getByTestId('legacy-title')).toHaveTextContent('Test Dialog'); - expect(getByTestId('legacy-url')).toHaveTextContent('http://example.com'); - expect(getByTestId('legacy-callback-id')).toHaveTextContent('abc123'); - }); - test('should return null when URL is missing (configuration error)', () => { const propsWithoutUrl = { ...baseProps, diff --git a/webapp/channels/src/components/dialog_router/dialog_router.tsx b/webapp/channels/src/components/dialog_router/dialog_router.tsx index 7522b17e512..a68bed9c835 100644 --- a/webapp/channels/src/components/dialog_router/dialog_router.tsx +++ b/webapp/channels/src/components/dialog_router/dialog_router.tsx @@ -3,21 +3,18 @@ import React from 'react'; -import InteractiveDialog from 'components/interactive_dialog/interactive_dialog'; - import InteractiveDialogAdapter from './interactive_dialog_adapter'; import type {PropsFromRedux} from './index'; -// Make props optional like the original InteractiveDialog, but keep required props -type OptionalPropsFromRedux = Partial & Pick; +type OptionalPropsFromRedux = Partial & Pick; type Props = OptionalPropsFromRedux & { onExited?: () => void; }; const DialogRouter: React.FC = (props) => { - const {isAppsFormEnabled, hasUrl} = props; + const {hasUrl} = props; // URL-less dialog = configuration error if (!hasUrl) { @@ -26,11 +23,7 @@ const DialogRouter: React.FC = (props) => { return null; // Let calling code show ephemeral error } - if (isAppsFormEnabled) { - return ; - } - - return ; + return ; }; export default DialogRouter; diff --git a/webapp/channels/src/components/dialog_router/index.ts b/webapp/channels/src/components/dialog_router/index.ts index 37e41e8312c..79b22002db0 100644 --- a/webapp/channels/src/components/dialog_router/index.ts +++ b/webapp/channels/src/components/dialog_router/index.ts @@ -6,7 +6,6 @@ import type {ConnectedProps} from 'react-redux'; import {bindActionCreators} from 'redux'; import type {Dispatch} from 'redux'; -import {interactiveDialogAppsFormEnabled} from 'mattermost-redux/selectors/entities/interactive_dialog'; import {getCurrentTimezone} from 'mattermost-redux/selectors/entities/timezone'; import {submitInteractiveDialog, lookupInteractiveDialog} from 'actions/integration_actions'; @@ -19,11 +18,9 @@ import DialogRouter from './dialog_router'; function mapStateToProps(state: GlobalState) { const data = state.entities.integrations.dialog; const emojiMap = getEmojiMap(state); - const isAppsFormEnabled = interactiveDialogAppsFormEnabled(state); if (!data || !data.dialog) { return { emojiMap, - isAppsFormEnabled: false, hasUrl: false, }; } @@ -40,7 +37,6 @@ function mapStateToProps(state: GlobalState) { state: data.dialog.state, sourceUrl: data.dialog.source_url, emojiMap, - isAppsFormEnabled, hasUrl: Boolean(data.url), timezone: getCurrentTimezone(state) || undefined, }; diff --git a/webapp/channels/src/components/inline_action_button/index.test.tsx b/webapp/channels/src/components/inline_action_button/index.test.tsx new file mode 100644 index 00000000000..bed76138270 --- /dev/null +++ b/webapp/channels/src/components/inline_action_button/index.test.tsx @@ -0,0 +1,418 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import {doPostActionWithQuery} from 'mattermost-redux/actions/posts'; + +import {act, fireEvent, renderWithContext, screen, userEvent} from 'tests/react_testing_utils'; + +import InlineActionButton from './index'; + +jest.mock('mattermost-redux/actions/posts', () => ({ + doPostActionWithQuery: jest.fn(), +})); + +const mockedDoPostActionWithQuery = doPostActionWithQuery as jest.MockedFunction; + +/** + * Creates a thunk-shaped mock whose inner promise is externally controllable. + * The thunk returned by `doPostActionWithQuery` is invoked by redux-thunk + * middleware; the returned promise is what the component awaits. + */ +function setupControllablePromise() { + let resolveFn: (value: unknown) => void = () => {}; + const promise = new Promise((resolve) => { + resolveFn = resolve; + }); + + mockedDoPostActionWithQuery.mockImplementation(() => { + return (() => promise) as unknown as ReturnType; + }); + + return {promise, resolve: () => resolveFn({data: {}})}; +} + +describe('InlineActionButton', () => { + const baseProps = { + href: 'mmaction://mx?tail=214&mds=C130J', + postId: 'abc', + children: 'Click me', + }; + + beforeEach(() => { + mockedDoPostActionWithQuery.mockReset(); + }); + + test('renders with children as button label', () => { + mockedDoPostActionWithQuery.mockImplementation( + () => (() => Promise.resolve({data: {}})) as unknown as ReturnType, + ); + + renderWithContext(); + + const button = screen.getByRole('button'); + expect(button).toBeVisible(); + expect(button).toHaveTextContent('Click me'); + }); + + test('dispatches thunk with parsed action ID and query on click', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext(); + + await userEvent.click(screen.getByRole('button')); + + expect(mockedDoPostActionWithQuery).toHaveBeenCalledTimes(1); + expect(mockedDoPostActionWithQuery).toHaveBeenCalledWith('abc', 'mx', {tail: '214', mds: 'C130J'}); + + // Resolve pending dispatch inside act so the trailing setState commits cleanly. + await act(async () => { + resolve(); + }); + }); + + test('href without query results in empty query', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button')); + + expect(mockedDoPostActionWithQuery).toHaveBeenCalledTimes(1); + expect(mockedDoPostActionWithQuery).toHaveBeenCalledWith('abc', 'mx', {}); + + await act(async () => { + resolve(); + }); + }); + + test('mixed-case action ID is preserved (URL.hostname would lowercase it)', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button')); + + // Server action ID regex allows [A-Za-z0-9]+; losing case would + // cause lookups to 404 when mm_blocks_actions keys are mixed-case. + expect(mockedDoPostActionWithQuery).toHaveBeenCalledWith('abc', 'MxPlan42', {tail: '214'}); + + await act(async () => { + resolve(); + }); + }); + + test('double-click prevented by ref guard', async () => { + // Use a never-resolving promise so the first dispatch stays in-flight. + mockedDoPostActionWithQuery.mockImplementation( + () => (() => new Promise(() => {})) as unknown as ReturnType, + ); + + renderWithContext(); + const button = screen.getByRole('button'); + + // Fire two synchronous clicks before any microtask can run. + // fireEvent.click invokes the handler synchronously; the ref guard + // must block the second invocation before setState re-render lands. + fireEvent.click(button); + fireEvent.click(button); + + expect(mockedDoPostActionWithQuery).toHaveBeenCalledTimes(1); + + // Let any pending microtasks settle so teardown is clean. The dispatch + // promise never resolves, which is fine — we only care about the guard. + await act(async () => { + await Promise.resolve(); + }); + }); + + test('button uses aria-disabled (not native disabled) while executing — keeps it focusable for screen readers', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext(); + const button = screen.getByRole('button'); + + await userEvent.click(button); + + // Native `disabled` would remove the button from tab order; use + // aria-disabled so keyboard / screen-reader users can still + // navigate to it and hear "executing" announced via aria-busy. + // WCAG 2.1.1 + 4.1.3. + expect(button).not.toBeDisabled(); + expect(button).toHaveAttribute('aria-disabled', 'true'); + expect(button).toHaveAttribute('aria-busy', 'true'); + + await act(async () => { + resolve(); + }); + + // Idle state omits aria-disabled and aria-busy entirely (using + // {executing || undefined} idiom) so screen readers don't + // announce "not busy" / "not disabled" superfluously. + expect(button).not.toBeDisabled(); + expect(button).not.toHaveAttribute('aria-disabled'); + expect(button).not.toHaveAttribute('aria-busy'); + }); + + test('ref guard no-ops repeat clicks while aria-disabled (no native disabled to suppress)', async () => { + // Without native `disabled`, the browser fires onClick on + // aria-disabled buttons. The component's executingRef guard must + // catch the second click and no-op. + mockedDoPostActionWithQuery.mockImplementation( + () => (() => new Promise(() => {})) as unknown as ReturnType, + ); + + renderWithContext(); + const button = screen.getByRole('button'); + + fireEvent.click(button); + fireEvent.click(button); + + expect(mockedDoPostActionWithQuery).toHaveBeenCalledTimes(1); + + await act(async () => { + await Promise.resolve(); + }); + }); + + test('unmount during dispatch does not warn about setState on unmounted component', async () => { + const {resolve} = setupControllablePromise(); + + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + const {unmount} = renderWithContext(); + + await userEvent.click(screen.getByRole('button')); + + unmount(); + + // Resolve the in-flight dispatch after unmount; the component's + // mountedRef guard should prevent a stale setState call. + await act(async () => { + resolve(); + }); + + const unmountedWarnings = consoleErrorSpy.mock.calls.filter((args) => { + const msg = args[0]; + return typeof msg === 'string' && msg.includes('unmounted'); + }); + expect(unmountedWarnings).toHaveLength(0); + + consoleErrorSpy.mockRestore(); + }); + + test('renders {children} as plain text when postId is empty', () => { + const {container} = renderWithContext( + , + ); + + // No button is rendered; the link body shows as plain text instead + // so the user sees something readable rather than a broken affordance. + expect(screen.queryByRole('button')).toBeNull(); + expect(container).toHaveTextContent('Click me'); + }); + + test('renders {children} as plain text when href has wrong scheme', () => { + const {container} = renderWithContext( + , + ); + + expect(screen.queryByRole('button')).toBeNull(); + expect(container).toHaveTextContent('Click me'); + }); + + test('renders {children} as plain text for opaque mmaction: URI (no //)', () => { + // getScheme()-style accept of "mmaction:foo" without "//" would + // mis-slice the authority. Component must reject and fall back. + const {container} = renderWithContext( + , + ); + + expect(screen.queryByRole('button')).toBeNull(); + expect(container).toHaveTextContent('Click me'); + }); + + test('renders {children} as plain text for non-alphanumeric action ID', () => { + // Server regex is ^[A-Za-z0-9]+$; URL authority chars (port, + // userinfo, dash, dot) would never resolve server-side. + for (const href of ['mmaction://plan:443', 'mmaction://user@plan', 'mmaction://my-plan', 'mmaction://my.plan']) { + const {container, unmount} = renderWithContext( + , + ); + expect(screen.queryByRole('button')).toBeNull(); + expect(container).toHaveTextContent('Click me'); + unmount(); + } + }); + + test('renders {children} as plain text when params exceed length cap', () => { + // 2049-char query string is over MAX_PARAMS_LENGTH (2048). + const {container} = renderWithContext( + , + ); + + expect(screen.queryByRole('button')).toBeNull(); + expect(container).toHaveTextContent('Click me'); + }); + + test('aria-label without label prop: idle has none (children carries the name), executing has executing-label', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext(); + const button = screen.getByRole('button'); + + // Idle, no label prop: accessible name comes from {children}. + expect(button).not.toHaveAttribute('aria-label'); + + await userEvent.click(button); + + expect(button).toHaveAttribute('aria-label', 'Executing...'); + + await act(async () => { + resolve(); + }); + + expect(button).not.toHaveAttribute('aria-label'); + }); + + test('aria-label uses label prop at idle (icon-only callers must pass it)', async () => { + const {resolve} = setupControllablePromise(); + + renderWithContext( + +