[pull] master from mattermost:master

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');
+    });
 });

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,

server/.go-version

@@ -1 +1 @@
-1.26.2
+1.26.3

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

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

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,32 +21,33 @@ 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 {
 		return
 	}
 
 	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)

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)
+	})
+}