From c74e51f35ec77c14b0283c88449bb701cb03cb7e Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 20 May 2026 11:56:19 -0300 Subject: [PATCH 1/5] chore(ci): upgrade Go to 1.26.3 in build container Dockerfiles (#36648) --- server/build/Dockerfile.buildenv | 2 +- server/build/Dockerfile.buildenv-fips | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 From 0790fc7281d7e31ba070ae8bc126df70501ef1d1 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 20 May 2026 13:21:14 -0300 Subject: [PATCH 2/5] Upgrade Go to 1.26.3 (#36656) --- server/.go-version | 2 +- server/go.mod | 2 +- server/public/go.mod | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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/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/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 From 6189a3f54a072a339b9e2943072e11ac4b2f4083 Mon Sep 17 00:00:00 2001 From: Ben Cooke Date: Wed, 20 May 2026 14:51:24 -0400 Subject: [PATCH 3/5] [MM-66489] Pull and populate certificate from metadata endpoint (#36557) --- .../enterprise/saml/saml_metadata_spec.ts | 59 +++++++++++++++++++ .../admin_console/admin_definition.tsx | 2 +- .../admin_console/schema_admin_settings.tsx | 18 +++++- .../src/components/admin_console/types.ts | 6 +- webapp/channels/src/i18n/en.json | 2 +- 5 files changed, 82 insertions(+), 5 deletions(-) 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/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/i18n/en.json b/webapp/channels/src/i18n/en.json index 2fecc44d9ee..1eaeafd23b5 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -2714,7 +2714,7 @@ "admin.saml.firstnameAttrTitle": "First Name Attribute:", "admin.saml.getSamlMetadataFromIDPFail": "SAML Metadata URL did not connect and pull data successfully", "admin.saml.getSamlMetadataFromIDPFetching": "Fetching...", - "admin.saml.getSamlMetadataFromIDPSuccess": "SAML Metadata retrieved successfully. Two fields below have been updated", + "admin.saml.getSamlMetadataFromIDPSuccess": "SAML Metadata retrieved successfully. Two fields and one certificate have been updated", "admin.saml.getSamlMetadataFromIDPUrl": "Get SAML Metadata from IdP", "admin.saml.guestAttrDesc": "(Optional) Requires Guest Access to be enabled before being applied. The attribute in the SAML Assertion that will be used to apply a guest role to users in Mattermost. Guests are prevented from accessing teams or channels upon logging in until they are assigned a team and at least one channel.\n \nNote: If this attribute is removed/changed from your guest user in SAML and the user is still active, they will not be promoted to a member and will retain their Guest role. Guests can be promoted in **System Console > User Management**.\n \n \nExisting members that are identified by this attribute as a guest will be demoted from a member to a guest when they are asked to login next. The next login is based upon Session lengths set in **System Console > Session Lengths**. It is highly recommend to manually demote users to guests in **System Console > User Management ** to ensure access is restricted immediately.", "admin.saml.guestAttrEx": "E.g.: \"usertype=Guest\" or \"isGuest=true\"", From a84941bec1bdf4f069e75650ec48b0aff61bdcba Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Wed, 20 May 2026 13:34:13 -0600 Subject: [PATCH 4/5] Remove Legacy Interactive Dialog code (#35874) --- .../lib/src/server/default_config.ts | 1 - server/public/model/feature_flags.go | 4 - .../dialog_router/dialog_router.test.tsx | 34 +- .../dialog_router/dialog_router.tsx | 13 +- .../src/components/dialog_router/index.ts | 4 - .../dialog_introduction_text.test.tsx.snap | 45 --- .../dialog_element/dialog_element.test.tsx | 185 ----------- .../dialog_element/dialog_element.tsx | 227 -------------- .../dialog_element/index.ts | 22 -- .../dialog_introduction_text.test.tsx | 33 -- .../dialog_introduction_text.tsx | 34 -- .../components/interactive_dialog/index.ts | 47 --- .../interactive_dialog.test.tsx | 153 --------- .../interactive_dialog/interactive_dialog.tsx | 293 ------------------ .../entities/interactive_dialog.test.ts | 75 ----- .../selectors/entities/interactive_dialog.ts | 16 - webapp/platform/types/src/config.ts | 1 - 17 files changed, 5 insertions(+), 1182 deletions(-) delete mode 100644 webapp/channels/src/components/interactive_dialog/__snapshots__/dialog_introduction_text.test.tsx.snap delete mode 100644 webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.test.tsx delete mode 100644 webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.tsx delete mode 100644 webapp/channels/src/components/interactive_dialog/dialog_element/index.ts delete mode 100644 webapp/channels/src/components/interactive_dialog/dialog_introduction_text.test.tsx delete mode 100644 webapp/channels/src/components/interactive_dialog/dialog_introduction_text.tsx delete mode 100644 webapp/channels/src/components/interactive_dialog/index.ts delete mode 100644 webapp/channels/src/components/interactive_dialog/interactive_dialog.test.tsx delete mode 100644 webapp/channels/src/components/interactive_dialog/interactive_dialog.tsx delete mode 100644 webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.test.ts delete mode 100644 webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.ts 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/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/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/interactive_dialog/__snapshots__/dialog_introduction_text.test.tsx.snap b/webapp/channels/src/components/interactive_dialog/__snapshots__/dialog_introduction_text.test.tsx.snap deleted file mode 100644 index 715bbe65466..00000000000 --- a/webapp/channels/src/components/interactive_dialog/__snapshots__/dialog_introduction_text.test.tsx.snap +++ /dev/null @@ -1,45 +0,0 @@ -// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing - -exports[`components/DialogIntroductionText should not fail on empty value 1`] = ` -
- -
-`; - -exports[`components/DialogIntroductionText should render message with supported values 1`] = ` -
- -

- - bold - - - - italic - - - - link - - <br/> - - link target blank - -

- -
-`; diff --git a/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.test.tsx b/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.test.tsx deleted file mode 100644 index f6342f0b267..00000000000 --- a/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.test.tsx +++ /dev/null @@ -1,185 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; - -import {render, screen} from 'tests/react_testing_utils'; - -import DialogElement from './dialog_element'; - -describe('components/interactive_dialog/DialogElement', () => { - const baseDialogProps = { - displayName: 'Testing', - name: 'testing', - type: 'text', - maxLength: 100, - actions: { - autocompleteActiveChannels: jest.fn(), - autocompleteUsers: jest.fn(), - }, - onChange: jest.fn(), - }; - - it('type textarea', () => { - render( - , - ); - expect(document.querySelector('textarea')).toBeInTheDocument(); - }); - - it('subtype blank', () => { - render( - , - ); - - expect(screen.getByTestId('testinginput')).toHaveAttribute('type', 'text'); - }); - - describe('subtype number', () => { - test('value is 0', () => { - render( - , - ); - expect(screen.getByTestId('testingnumber')).toHaveValue(0); - }); - - test('value is 123', () => { - render( - , - ); - expect(screen.getByTestId('testingnumber')).toHaveValue(123); - }); - }); - - it('subtype email', () => { - render( - , - ); - expect(screen.getByTestId('testingemail')).toHaveAttribute('type', 'email'); - }); - - it('subtype password', () => { - render( - , - ); - expect(screen.getByTestId('testingpassword')).toHaveAttribute('type', 'password'); - }); - - describe('radioSetting', () => { - const radioOptions = [ - {value: 'foo', text: 'foo-text'}, - {value: 'bar', text: 'bar-text'}, - ]; - - test('RadioSetting is rendered when type is radio', () => { - render( - , - ); - - expect(screen.getAllByRole('radio')).toHaveLength(2); - }); - - test('RadioSetting is rendered when options are null', () => { - render( - , - ); - - expect(screen.getByTestId('testing')).toBeInTheDocument(); - }); - - test('RadioSetting is rendered when options are null and value is null', () => { - render( - , - ); - - expect(screen.getByTestId('testing')).toBeInTheDocument(); - }); - - test('RadioSetting is rendered when options are null and value is not null', () => { - render( - , - ); - - expect(screen.getByTestId('testing')).toBeInTheDocument(); - }); - - test('RadioSetting is rendered when value is not one of the options', () => { - render( - , - ); - - expect(screen.getByTestId('testing')).toBeInTheDocument(); - }); - - test('No default value is selected from the radio button list', () => { - render( - , - ); - const radios = screen.getAllByRole('radio'); - radios.forEach((radio) => { - expect(radio).not.toBeChecked(); - }); - }); - - test('The default value can be specified from the list', () => { - render( - , - ); - expect(screen.getByRole('radio', {name: radioOptions[1].text})).toBeChecked(); - }); - }); -}); diff --git a/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.tsx b/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.tsx deleted file mode 100644 index 79f274440a5..00000000000 --- a/webapp/channels/src/components/interactive_dialog/dialog_element/dialog_element.tsx +++ /dev/null @@ -1,227 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; -import {FormattedMessage} from 'react-intl'; - -import type {UserAutocomplete} from '@mattermost/types/autocomplete'; -import type {Channel} from '@mattermost/types/channels'; -import type {ServerError} from '@mattermost/types/errors'; -import type {UserProfile} from '@mattermost/types/users'; - -import type {ActionResult} from 'mattermost-redux/types/actions'; - -import AutocompleteSelector from 'components/autocomplete_selector'; -import type {Option, Selected} from 'components/autocomplete_selector'; -import GenericChannelProvider from 'components/suggestion/generic_channel_provider'; -import GenericUserProvider from 'components/suggestion/generic_user_provider'; -import MenuActionProvider from 'components/suggestion/menu_action_provider'; -import ModalSuggestionList from 'components/suggestion/modal_suggestion_list'; -import type Provider from 'components/suggestion/provider'; -import BoolSetting from 'components/widgets/settings/bool_setting'; -import RadioSetting from 'components/widgets/settings/radio_setting'; -import TextSetting from 'components/widgets/settings/text_setting'; -import type {InputTypes} from 'components/widgets/settings/text_setting'; - -const TEXT_DEFAULT_MAX_LENGTH = 150; -const TEXTAREA_DEFAULT_MAX_LENGTH = 3000; - -export type Props = { - displayName: string; - name: string; - type: string; - subtype?: string; - placeholder?: string; - helpText?: string; - errorText?: React.ReactNode; - maxLength?: number; - dataSource?: string; - optional?: boolean; - options?: Array<{ - text: string; - value: string; - }>; - value?: string | number | boolean; - onChange: (name: string, selected: string) => void; - autoFocus?: boolean; - actions: { - autocompleteActiveChannels: (term: string, success: (channels: Channel[]) => void, error?: (err: ServerError) => void) => (ActionResult | Promise); - autocompleteUsers: (search: string) => Promise; - }; -} - -type State = { - value: string; -} - -export default class DialogElement extends React.PureComponent { - private providers: Provider[]; - - constructor(props: Props) { - super(props); - - let defaultText = ''; - this.providers = []; - if (props.type === 'select') { - if (props.dataSource === 'users') { - this.providers = [new GenericUserProvider(props.actions.autocompleteUsers)]; - } else if (props.dataSource === 'channels') { - this.providers = [new GenericChannelProvider(props.actions.autocompleteActiveChannels)]; - } else if (props.options) { - this.providers = [new MenuActionProvider(props.options)]; - } - - if (props.value && props.options) { - const defaultOption = props.options.find( - (option) => option.value === props.value, - ); - defaultText = defaultOption ? defaultOption.text : ''; - } - } - - this.state = { - value: defaultText, - }; - } - - private handleSelected = (selected: Selected) => { - const {name, dataSource} = this.props; - - if (dataSource === 'users') { - const user = selected as UserProfile; - this.props.onChange(name, user.id); - this.setState({value: user.username}); - } else if (dataSource === 'channels') { - const channel = selected as Channel; - this.props.onChange(name, channel.id); - this.setState({value: channel.display_name}); - } else { - const option = selected as Option; - this.props.onChange(name, option.value); - this.setState({value: option.text}); - } - }; - - public render(): JSX.Element | null { - const { - name, - subtype, - displayName, - value, - placeholder, - onChange, - helpText, - errorText, - optional, - options, - type, - maxLength, - } = this.props; - - let displayNameContent: React.ReactNode = displayName; - if (optional) { - displayNameContent = ( - <> - {displayName + ' '} - - - - - ); - } else { - displayNameContent = ( - <> - {displayName} - {' *'} - - ); - } - - let helpTextContent: React.ReactNode = helpText; - if (errorText) { - helpTextContent = ( - <> - {helpText} -
- {errorText} -
- - ); - } - - if (type === 'text' || type === 'textarea') { - let textSettingMaxLength; - if (type === 'text') { - textSettingMaxLength = maxLength || TEXT_DEFAULT_MAX_LENGTH; - } else { - textSettingMaxLength = maxLength || TEXTAREA_DEFAULT_MAX_LENGTH; - } - - let assertedValue; - if (subtype === 'number' && typeof value === 'number') { - assertedValue = value as number; - } else { - assertedValue = value as string || ''; - } - - return ( - - ); - } else if (type === 'select') { - return ( - - ); - } else if (type === 'bool') { - const boolValue = value as boolean; - return ( - - ); - } else if (type === 'radio') { - const textValue = value as string; - return ( - - ); - } - - return null; - } -} diff --git a/webapp/channels/src/components/interactive_dialog/dialog_element/index.ts b/webapp/channels/src/components/interactive_dialog/dialog_element/index.ts deleted file mode 100644 index 48f4a2017b7..00000000000 --- a/webapp/channels/src/components/interactive_dialog/dialog_element/index.ts +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import {connect} from 'react-redux'; -import {bindActionCreators} from 'redux'; -import type {Dispatch} from 'redux'; - -import {autocompleteActiveChannels} from 'actions/channel_actions'; -import {autocompleteUsers} from 'actions/user_actions'; - -import DialogElement from './dialog_element'; - -function mapDispatchToProps(dispatch: Dispatch) { - return { - actions: bindActionCreators({ - autocompleteActiveChannels, - autocompleteUsers, - }, dispatch), - }; -} - -export default connect(null, mapDispatchToProps)(DialogElement); diff --git a/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.test.tsx b/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.test.tsx deleted file mode 100644 index 9e3b36e1c6e..00000000000 --- a/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.test.tsx +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; - -import {render} from 'tests/react_testing_utils'; -import EmojiMap from 'utils/emoji_map'; - -import DialogIntroductionText from './dialog_introduction_text'; - -describe('components/DialogIntroductionText', () => { - const emojiMap = new EmojiMap(new Map()); - - test('should render message with supported values', () => { - const descriptor = { - id: 'testsupported', - value: '**bold** *italic* [link](https://mattermost.com/)
[link target blank](!https://mattermost.com/)', - emojiMap, - }; - const {container} = render(); - expect(container).toMatchSnapshot(); - }); - - test('should not fail on empty value', () => { - const descriptor = { - id: 'testblankvalue', - value: '', - emojiMap, - }; - const {container} = render(); - expect(container).toMatchSnapshot(); - }); -}); diff --git a/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.tsx b/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.tsx deleted file mode 100644 index 7077423b11e..00000000000 --- a/webapp/channels/src/components/interactive_dialog/dialog_introduction_text.tsx +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; - -import type EmojiMap from 'utils/emoji_map'; -import * as Markdown from 'utils/markdown'; -import {getSiteURL} from 'utils/url'; - -type Props = { - id: string; - value: string; - emojiMap?: EmojiMap; -} - -export default function DialogIntroductionText({id, value, emojiMap}: Props) { - const formattedMessage = Markdown.format( - value, - { - breaks: true, - sanitize: true, - gfm: true, - siteURL: getSiteURL(), - }, - emojiMap, - ); - - return ( - - ); -} diff --git a/webapp/channels/src/components/interactive_dialog/index.ts b/webapp/channels/src/components/interactive_dialog/index.ts deleted file mode 100644 index 5caf968a159..00000000000 --- a/webapp/channels/src/components/interactive_dialog/index.ts +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import type {ConnectedProps} from 'react-redux'; -import {connect} from 'react-redux'; -import {bindActionCreators} from 'redux'; -import type {Dispatch} from 'redux'; - -import {submitInteractiveDialog, lookupInteractiveDialog} from 'actions/integration_actions'; -import {getEmojiMap} from 'selectors/emojis'; - -import type {GlobalState} from 'types/store'; - -import InteractiveDialog from './interactive_dialog'; - -function mapStateToProps(state: GlobalState) { - const data = state.entities.integrations.dialog; - if (!data || !data.dialog) { - return {}; - } - - return { - url: data.url, - callbackId: data.dialog.callback_id, - elements: data.dialog.elements, - title: data.dialog.title, - introductionText: data.dialog.introduction_text, - iconUrl: data.dialog.icon_url, - submitLabel: data.dialog.submit_label, - notifyOnCancel: data.dialog.notify_on_cancel, - state: data.dialog.state, - emojiMap: getEmojiMap(state), - }; -} -function mapDispatchToProps(dispatch: Dispatch) { - return { - actions: bindActionCreators({ - submitInteractiveDialog, - lookupInteractiveDialog, - }, dispatch), - }; -} -const connector = connect(mapStateToProps, mapDispatchToProps); - -export type PropsFromRedux = ConnectedProps; - -export default connector(InteractiveDialog); diff --git a/webapp/channels/src/components/interactive_dialog/interactive_dialog.test.tsx b/webapp/channels/src/components/interactive_dialog/interactive_dialog.test.tsx deleted file mode 100644 index 71777cc4b43..00000000000 --- a/webapp/channels/src/components/interactive_dialog/interactive_dialog.test.tsx +++ /dev/null @@ -1,153 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; - -import type {DialogElement as TDialogElement} from '@mattermost/types/integrations'; - -import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; -import EmojiMap from 'utils/emoji_map'; - -import type {Props} from './interactive_dialog'; -import InteractiveDialog from './interactive_dialog'; - -describe('components/interactive_dialog/InteractiveDialog', () => { - const baseProps: Props = { - url: 'http://example.com', - callbackId: 'abc', - elements: [], - title: 'test title', - iconUrl: 'http://example.com/icon.png', - submitLabel: 'Yes', - notifyOnCancel: true, - state: 'some state', - introductionText: 'Some introduction text', - onExited: jest.fn(), - actions: { - submitInteractiveDialog: jest.fn(), - lookupInteractiveDialog: jest.fn(), - }, - emojiMap: new EmojiMap(new Map()), - }; - - describe('generic error message', () => { - test('should appear when submit returns an error', async () => { - const props = { - ...baseProps, - actions: { - submitInteractiveDialog: jest.fn().mockResolvedValue({data: {error: 'This is an error.'}}), - lookupInteractiveDialog: jest.fn(), - }, - }; - renderWithContext(); - - const submitButton = screen.getByText('Yes'); - await userEvent.click(submitButton); - - await waitFor(() => { - expect(screen.getByText('This is an error.')).toBeInTheDocument(); - }); - - const errorElement = screen.getByText('This is an error.'); - expect(errorElement).toHaveClass('error-text'); - }); - - test('should not appear when submit does not return an error', async () => { - const props = { - ...baseProps, - actions: { - submitInteractiveDialog: jest.fn().mockResolvedValue({}), - lookupInteractiveDialog: jest.fn(), - }, - }; - renderWithContext(); - - const submitButton = screen.getByText('Yes'); - await userEvent.click(submitButton); - - expect(document.querySelector('.error-text')).not.toBeInTheDocument(); - }); - }); - - describe('default select element in Interactive Dialog', () => { - test('should be enabled by default', () => { - const selectElement: TDialogElement = { - data_source: '', - default: 'opt3', - display_name: 'Option Selector', - name: 'someoptionselector', - optional: false, - options: [ - {text: 'Option1', value: 'opt1'}, - {text: 'Option2', value: 'opt2'}, - {text: 'Option3', value: 'opt3'}, - ], - type: 'select', - subtype: '', - placeholder: '', - help_text: '', - min_length: 0, - max_length: 0, - }; - - const props = { - ...baseProps, - elements: [selectElement], - }; - - renderWithContext(); - - expect(screen.getByDisplayValue('Option3')).toBeInTheDocument(); - }); - }); - - describe('bool element in Interactive Dialog', () => { - const element: TDialogElement = { - data_source: '', - display_name: 'Boolean Selector', - name: 'somebool', - optional: false, - type: 'bool', - placeholder: 'Subscribe?', - subtype: '', - default: '', - help_text: '', - min_length: 0, - max_length: 0, - options: [], - }; - - const testCases = [ - {description: 'no default', expectedChecked: false}, - {description: 'unknown default', default: 'unknown', expectedChecked: false}, - {description: 'default of "false"', default: 'false', expectedChecked: false}, - {description: 'default of true', default: true, expectedChecked: true}, - {description: 'default of "true"', default: 'True', expectedChecked: true}, - {description: 'default of "True"', default: 'True', expectedChecked: true}, - {description: 'default of "TRUE"', default: 'TRUE', expectedChecked: true}, - ]; - - testCases.forEach((testCase) => test(`should interpret ${testCase.description}`, () => { - const testElement = {...element}; - if (testCase.default === undefined) { - delete (testElement as any).default; - } else { - (testElement as any).default = testCase.default; - } - - const props = { - ...baseProps, - elements: [testElement], - }; - - renderWithContext(); - - const checkbox = screen.getByRole('checkbox'); - if (testCase.expectedChecked) { - expect(checkbox).toBeChecked(); - } else { - expect(checkbox).not.toBeChecked(); - } - })); - }); -}); diff --git a/webapp/channels/src/components/interactive_dialog/interactive_dialog.tsx b/webapp/channels/src/components/interactive_dialog/interactive_dialog.tsx deleted file mode 100644 index 20790144fba..00000000000 --- a/webapp/channels/src/components/interactive_dialog/interactive_dialog.tsx +++ /dev/null @@ -1,293 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React from 'react'; -import {Modal} from 'react-bootstrap'; -import {FormattedMessage} from 'react-intl'; - -import {Button} from '@mattermost/shared/components/button'; -import type {DialogSubmission} from '@mattermost/types/integrations'; - -import { - checkDialogElementForError, - checkIfErrorsMatchElements, -} from 'mattermost-redux/utils/integration_utils'; - -import SpinnerButton from 'components/spinner_button'; - -import DialogElement from './dialog_element'; -import DialogIntroductionText from './dialog_introduction_text'; - -import type {PropsFromRedux} from './index'; - -// We are using Partial as we are returning empty object with dialog redux state is empty in connect -type OptionalProsFromRedux = Partial & Pick; - -export type Props = { - onExited?: () => void; -} & OptionalProsFromRedux; - -type State = { - show: boolean; - values: Record; - error: string | null; - errors: Record; - submitting: boolean; -} - -export default class InteractiveDialog extends React.PureComponent { - constructor(props: Props) { - super(props); - - const values: Record = {}; - if (props.elements != null) { - props.elements.forEach((e) => { - if (e.type === 'bool') { - values[e.name] = String(e.default).toLowerCase() === 'true'; - } else { - values[e.name] = e.default ?? null; - } - }); - } - - this.state = { - show: true, - values, - error: null, - errors: {}, - submitting: false, - }; - } - - handleSubmit = async (e: React.FormEvent) => { - e.preventDefault(); - - const {elements} = this.props; - const values = this.state.values; - const errors: Record = {}; - - if (elements) { - elements.forEach((elem) => { - const error = checkDialogElementForError( - elem, - values[elem.name], - ); - if (error) { - errors[elem.name] = ( - - ); - } - }); - } - - this.setState({errors}); - - if (Object.keys(errors).length !== 0) { - return; - } - - const {url, callbackId, state} = this.props; - - const dialog: DialogSubmission = { - url, - callback_id: callbackId ?? '', - state: state ?? '', - submission: values as { [x: string]: string }, - user_id: '', - channel_id: '', - team_id: '', - cancelled: false, - }; - - this.setState({submitting: true}); - - const {data} = await this.props.actions.submitInteractiveDialog(dialog) ?? {}; - - this.setState({submitting: false}); - - let hasErrors = false; - - if (data) { - if (data.error) { - hasErrors = true; - this.setState({error: data.error}); - } - - if ( - data.errors && - Object.keys(data.errors).length >= 0 && - checkIfErrorsMatchElements(data.errors, elements) - ) { - hasErrors = true; - this.setState({errors: data.errors}); - } - } - - if (!hasErrors) { - this.handleHide(true); - } - }; - - onHide = () => { - this.handleHide(false); - }; - - handleHide = (submitted = false) => { - const {url, callbackId, state, notifyOnCancel} = this.props; - - if (!submitted && notifyOnCancel) { - const dialog: DialogSubmission = { - url, - callback_id: callbackId ?? '', - state: state ?? '', - cancelled: true, - user_id: '', - channel_id: '', - team_id: '', - submission: {}, - }; - - this.props.actions.submitInteractiveDialog(dialog); - } - - this.setState({show: false}); - }; - - onChange = (name: string, value: string) => { - const values = {...this.state.values, [name]: value}; - this.setState({values}); - }; - - render() { - const { - title, - introductionText, - iconUrl, - submitLabel, - elements, - } = this.props; - - let submitText: JSX.Element | string = ( - - ); - - if (submitLabel) { - submitText = submitLabel; - } - - let icon; - if (iconUrl) { - icon = ( - {'modal - ); - } - - return ( - -
- - - {icon} - {title} - - - {(elements || introductionText) && ( - - {introductionText && ( - - )} - {elements && - elements.map((e, index) => { - return ( - - ); - })} - - )} - - {this.state.error && ( -
{this.state.error}
- )} - - - } - > - {submitText} - - -
- - ); - } -} diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.test.ts b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.test.ts deleted file mode 100644 index 4c672cfa694..00000000000 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.test.ts +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import type {GlobalState} from '@mattermost/types/store'; - -import {interactiveDialogAppsFormEnabled} from './interactive_dialog'; - -describe('interactive_dialog selectors', () => { - describe('interactiveDialogAppsFormEnabled', () => { - const createMockState = (config: Partial = {}): GlobalState => ({ - entities: { - general: { - config, - }, - }, - } as any); - - test('should return true when feature flag is enabled', () => { - const state = createMockState({ - FeatureFlagInteractiveDialogAppsForm: 'true', - }); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(true); - }); - - test('should return false when feature flag is disabled', () => { - const state = createMockState({ - FeatureFlagInteractiveDialogAppsForm: 'false', - }); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(false); - }); - - test('should return false when feature flag is not present', () => { - const state = createMockState({}); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(false); - }); - - test('should return false when feature flag is empty string', () => { - const state = createMockState({ - FeatureFlagInteractiveDialogAppsForm: '', - }); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(false); - }); - - test('should return false when feature flag is undefined', () => { - const state = createMockState({ - FeatureFlagInteractiveDialogAppsForm: undefined, - }); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(false); - }); - - test('should return false when config is empty', () => { - const state = createMockState(); - - expect(interactiveDialogAppsFormEnabled(state)).toBe(false); - }); - - test('should be case sensitive for true value', () => { - const stateUppercase = createMockState({ - FeatureFlagInteractiveDialogAppsForm: 'TRUE', - }); - - const stateMixed = createMockState({ - FeatureFlagInteractiveDialogAppsForm: 'True', - }); - - expect(interactiveDialogAppsFormEnabled(stateUppercase)).toBe(false); - expect(interactiveDialogAppsFormEnabled(stateMixed)).toBe(false); - }); - }); -}); diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.ts b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.ts deleted file mode 100644 index 246aeace00f..00000000000 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/interactive_dialog.ts +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import type {ClientConfig} from '@mattermost/types/config'; -import type {GlobalState} from '@mattermost/types/store'; - -import {createSelector} from 'mattermost-redux/selectors/create_selector'; -import {getConfig} from 'mattermost-redux/selectors/entities/general'; - -export const interactiveDialogAppsFormEnabled = createSelector( - 'interactiveDialogAppsFormEnabled', - (state: GlobalState) => getConfig(state), - (config: Partial) => { - return config?.FeatureFlagInteractiveDialogAppsForm === 'true'; - }, -); diff --git a/webapp/platform/types/src/config.ts b/webapp/platform/types/src/config.ts index c5e6fc8899e..1b54ce3f901 100644 --- a/webapp/platform/types/src/config.ts +++ b/webapp/platform/types/src/config.ts @@ -130,7 +130,6 @@ export type ClientConfig = { FeatureFlagAttributeBasedAccessControl: string; FeatureFlagPermissionPolicies: string; FeatureFlagWebSocketEventScope: string; - FeatureFlagInteractiveDialogAppsForm: string; FeatureFlagContentFlagging: string; FeatureFlagClassificationMarkings: string; FeatureFlagManagedChannelCategories: string; From 448a642835da50b82106ec8ad9ae6d0a200f6bae Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Wed, 20 May 2026 13:34:44 -0600 Subject: [PATCH 5/5] Add inline action buttons for bot-posted markdown (#36219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add inline action buttons for bot-posted markdown Bots, webhooks, and plugins can now embed clickable action buttons inside markdown (including table cells) using mmaction://actionId links, with row-specific parameters forwarded to the integration on click. This enables use cases like a per-row "Mx Plan" button in a fleet-status table that opens a dialog scoped to the clicked row. Design - New post prop inline_actions maps actionId (alphanumeric) to a PostActionIntegration {URL, Context}, capped at 50 entries. - Markdown link with scheme mmaction:// emits a placeholder span that messageHtmlToComponent converts to the InlineActionButton component. - Click POSTs inline_context (parsed from the URL query string) to the existing /posts/{id}/actions/{action_id} endpoint; the server merges it into the integration request as context.inline_params while preserving the post-level context. - Only bot, webhook, and plugin posts render the button; non-integration posts have inline_actions stripped on create, update, and ephemeral broadcast. Hardened-mode also covers the new prop. - Reuses the existing PostAction dialog pipeline: plugin handlers reply with a trigger_id and call /actions/dialogs/open as before. Security - InlineContext capped at 50 entries / 128-char keys / 2 KB values. - Integration Context cloned per click so per-click inline_params and selected_option cannot leak into the cached post for other clickers. - Plugin response updates cannot add inline_actions to a post that did not already have them; invalid entries are dropped with a warn log. - Label content and data attributes are escaped; labels are flattened to plain text (tags stripped, entities decoded, then escaped). - Malformed JSON request bodies now return 400 instead of falling through with an empty inline_context. Tests - Model: validators, normalization, GetInlineAction, strip, fallback. - App: create strip, update guard (4 subtests including AllowInlineActionsUpdate bypass), ephemeral strip, inline_params merge, context-map isolation, plugin-response guards, from_bot and from_plugin retention across plugin updates. - API: inline_context validation (size bounds + error id), omitempty backward compat, malformed JSON 400. - Webapp: renderer scheme handling, allow/deny flags, size caps, HTML escape, tag strip, entity decode, attribute-injection defense; component click dispatch, double-click race guard, unmount safety, error-result recovery, aria state. Co-Authored-By: Claude Opus 4.7 (1M context) * lint fix * i18n-extract * Review fixes for inline action buttons - renderer: preserve actionId case; reject opaque mmaction: URI - app: require bot AND integration session to preserve inline_actions - app: restore original inline_actions when plugin response is invalid - i18n: rename key to ...app_error to match convention Co-Authored-By: Claude Opus 4.7 (1M context) * Tighten UpdatePost inline_actions guard; fix test seeds - app: UpdatePost now requires AllowInlineActionsUpdate to modify inline_actions. Integration session alone is insufficient — a PAT-wielding user could otherwise inject inline_actions on any post they could edit. - tests: seed bot posts with inline_actions via an integration session (intSeedCtx) so they survive the create-time strip. - renderer: lint fix (blank line before comment block). Co-Authored-By: Claude Opus 4.7 (1M context) * Reject malformed inline-action authorities at render time - renderer: enforce ^[A-Za-z0-9]+$ on actionId, mirroring the server regex. Authorities like mmaction://plan:443 or mmaction://user@plan now fall through to plain text instead of rendering a dead button. - post: clarify in the strip comment that webhooks and plugins bypass CreatePostAsUser entirely (they call CreatePost / CreatePostMissingChannel directly), so the strip block does not apply to them. Co-Authored-By: Claude Opus 4.7 (1M context) * Tighten inline-action renderer tests - Replace oversized-params test with boundary pair (at-cap and over-cap) to lock in the > vs >= behavior of the size-limit check. - Add a "surrounding text survives" assertion for the tag-strip path so a future swap from regex strip to a DOM sanitizer won't silently drop legitimate content along with tags. Co-Authored-By: Claude Opus 4.7 (1M context) * Inline action buttons via mmaction:// markdown links Adds inline action buttons rendered from mmaction:// links in markdown, with the click pipeline reusing the existing post-action infrastructure. Aligned with the broader mm_blocks_actions framework (Daniel's PR). * fix lint, DoS hardening, fix and rename test * Address review feedback * lint fix * Reject percent-encoded path traversal in validateIntegrationURL (e.g. %2e%2e%2f) by parsing the URL and checking the decoded path. --------- Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: Mattermost Build --- server/channels/api4/integration_action.go | 48 +- .../channels/api4/integration_action_test.go | 188 +++ server/channels/app/integration_action.go | 129 ++- .../channels/app/integration_action_test.go | 1030 ++++++++++++++++- server/channels/app/plugin_api.go | 14 +- server/channels/app/post.go | 47 + server/channels/app/webhook.go | 6 + server/i18n/en.json | 12 + server/public/model/integration_action.go | 154 ++- .../public/model/integration_action_test.go | 740 ++++++++++++ server/public/model/mm_blocks_actions.go | 154 +++ server/public/model/post.go | 16 + server/public/model/post_test.go | 9 +- .../inline_action_button/index.test.tsx | 418 +++++++ .../components/inline_action_button/index.tsx | 214 ++++ .../inline_action_button.scss | 25 + .../src/components/markdown/markdown.tsx | 9 + .../post_markdown/post_markdown.tsx | 11 + webapp/channels/src/i18n/en.json | 2 + .../mattermost-redux/src/actions/posts.ts | 32 + .../src/utils/markdown/renderer.test.tsx | 15 + .../utils/message_html_to_component.test.tsx | 48 + .../src/utils/message_html_to_component.tsx | 24 + webapp/platform/client/src/client4.ts | 7 + 24 files changed, 3290 insertions(+), 62 deletions(-) create mode 100644 server/public/model/mm_blocks_actions.go create mode 100644 webapp/channels/src/components/inline_action_button/index.test.tsx create mode 100644 webapp/channels/src/components/inline_action_button/index.tsx create mode 100644 webapp/channels/src/components/inline_action_button/inline_action_button.scss 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/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/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/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( + +