Fixed URL validation for integration actions (#35857) (#36108)

* Fixed URL validation for integratioon actions

* SImplified check to avoid subpath incompatibility

* minor tweak

* refactored for better tests

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/app/integration_action.go

@@ -331,16 +331,7 @@ func (a *App) DoActionRequest(c request.CTX, rawURL string, body []byte) (*http.
 	req.Header.Set("Content-Type", "application/json")
 	req.Header.Set("Accept", "application/json")
 
-	// Allow access to plugin routes for action buttons
+	httpClient := a.getPostActionClient(c, inURL, req)
-	var httpClient *http.Client
-	subpath, _ := utils.GetSubpathFromConfig(a.Config())
-	siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL)
-	if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(inURL.Path, path.Join(subpath, "plugins")) {
-		req.Header.Set(model.HeaderAuth, "Bearer "+c.Session().Token)
-		httpClient = a.HTTPService().MakeClient(true)
-	} else {
-		httpClient = a.HTTPService().MakeClient(false)
-	}
 
 	resp, httpErr := httpClient.Do(req)
 	if httpErr != nil {
@@ -354,6 +345,20 @@ func (a *App) DoActionRequest(c request.CTX, rawURL string, body []byte) (*http.
 	return resp, nil
 }
 
+func (a *App) getPostActionClient(rctx request.CTX, inURL *url.URL, req *http.Request) *http.Client {
+	// Allow access to plugin routes for action buttons
+	var httpClient *http.Client
+	subpath, _ := utils.GetSubpathFromConfig(a.Config())
+	siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL)
+	if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(path.Clean(inURL.Path), path.Join(subpath, "plugins")) {
+		req.Header.Set(model.HeaderAuth, "Bearer "+rctx.Session().Token)
+		httpClient = a.HTTPService().MakeClient(true)
+	} else {
+		httpClient = a.HTTPService().MakeClient(false)
+	}
+	return httpClient
+}
+
 type LocalResponseWriter struct {
 	data    []byte
 	headers http.Header
@@ -387,13 +392,15 @@ func (ch *Channels) doPluginRequest(c request.CTX, method, rawURL string, values
 	if err != nil {
 		return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 	}
-	result := strings.Split(inURL.Path, "/")
+	result := strings.Split(path.Clean(inURL.Path), "/")
 	if len(result) < 2 {
 		return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=Unable to find pluginId", http.StatusBadRequest)
 	}
+
 	if result[0] != "plugins" {
 		return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=plugins not in path", http.StatusBadRequest)
 	}
+
 	pluginID := result[1]
 
 	path := strings.TrimPrefix(inURL.Path, "plugins/"+pluginID)

server/channels/app/integration_action_test.go

@@ -1198,6 +1198,105 @@ func TestPostActionRelativePluginURL(t *testing.T) {
 	})
 }
 
+func TestGetPostActionClient(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic()
+
+	tests := []struct {
+		name       string
+		siteURL    string
+		subpath    string
+		requestURL string
+		expectAuth bool
+	}{
+		{
+			name:       "same host with plugin path gets auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/plugins/myplugin/action",
+			expectAuth: true,
+		},
+		{
+			name:       "same host with non-plugin path does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/api/v4/posts",
+			expectAuth: false,
+		},
+		{
+			name:       "different host with plugin path does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://evil.com/plugins/myplugin/action",
+			expectAuth: false,
+		},
+		{
+			name:       "different host same port does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://attacker.com:8065/plugins/myplugin/action",
+			expectAuth: false,
+		},
+		{
+			name:       "path traversal to reach plugins does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/api/../../plugins/myplugin",
+			expectAuth: true, // path.Clean normalizes to /plugins/myplugin
+		},
+		{
+			name:       "path traversal escaping plugins does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/plugins/../api/v4/posts",
+			expectAuth: false, // path.Clean normalizes to /api/v4/posts
+		},
+		{
+			name:       "subpath with plugin path gets auth",
+			siteURL:    "http://localhost:8065/mattermost",
+			subpath:    "/mattermost",
+			requestURL: "http://localhost:8065/mattermost/plugins/myplugin/action",
+			expectAuth: true,
+		},
+		{
+			name:       "subpath without subpath prefix does not get auth",
+			siteURL:    "http://localhost:8065/mattermost",
+			subpath:    "/mattermost",
+			requestURL: "http://localhost:8065/plugins/myplugin/action",
+			expectAuth: false, // plugins path doesn't include subpath
+		},
+		{
+			name:       "empty path does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/",
+			expectAuth: false,
+		},
+		{
+			name:       "plugins as query param does not get auth",
+			siteURL:    "http://localhost:8065",
+			requestURL: "http://localhost:8065/api?path=plugins/myplugin",
+			expectAuth: false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				*cfg.ServiceSettings.SiteURL = tc.siteURL
+			})
+
+			inURL, err := url.Parse(tc.requestURL)
+			require.NoError(t, err)
+
+			req, err := http.NewRequest("POST", tc.requestURL, nil)
+			require.NoError(t, err)
+
+			_ = th.App.getPostActionClient(th.Context, inURL, req)
+
+			if tc.expectAuth {
+				assert.NotEmpty(t, req.Header.Get(model.HeaderAuth), "expected auth header to be set")
+				assert.Contains(t, req.Header.Get(model.HeaderAuth), "Bearer ")
+			} else {
+				assert.Empty(t, req.Header.Get(model.HeaderAuth), "expected no auth header")
+			}
+		})
+	}
+}
+
 func TestDoPluginRequest(t *testing.T) {
 	mainHelper.Parallel(t)
 	th := Setup(t)
@@ -1303,4 +1402,90 @@ func TestDoPluginRequest(t *testing.T) {
 	require.NotNil(t, resp)
 	body, _ = io.ReadAll(resp.Body)
 	assert.Equal(t, "param multiple not correct", string(body))
+
+	t.Run("should handle URLs with path traversals", func(t *testing.T) {
+		tests := []struct {
+			name      string
+			rawURL    string
+			expectErr bool
+			errDetail string
+		}{
+			{
+				name:      "path traversal to escape plugins directory",
+				rawURL:    "/plugins/../../../etc/passwd",
+				expectErr: true,
+				errDetail: "plugins not in path",
+			},
+			{
+				name:      "path traversal with encoded slashes",
+				rawURL:    "/plugins/..%2F..%2F..%2Fetc%2Fpasswd",
+				expectErr: true, // url.Parse decodes %2F, path.Clean normalizes traversal
+				errDetail: "plugins not in path",
+			},
+			{
+				name:      "double dot in plugin path",
+				rawURL:    "/plugins/../plugins/myplugin/action",
+				expectErr: false, // path.Clean normalizes this back to plugins/myplugin/action
+			},
+			{
+				name:      "path traversal without leading slash",
+				rawURL:    "plugins/../../../etc/passwd",
+				expectErr: true,
+				errDetail: "plugins not in path",
+			},
+			{
+				name:      "only plugins with no plugin ID",
+				rawURL:    "/plugins/",
+				expectErr: true,
+				errDetail: "Unable to find pluginId",
+			},
+			{
+				name:      "just plugins no trailing slash",
+				rawURL:    "/plugins",
+				expectErr: true,
+				errDetail: "Unable to find pluginId",
+			},
+			{
+				name:      "non-plugins path",
+				rawURL:    "/api/v4/users",
+				expectErr: true,
+				errDetail: "plugins not in path",
+			},
+			{
+				name:      "path traversal via dot segments after plugin ID",
+				rawURL:    "/plugins/myplugin/../../etc/passwd",
+				expectErr: true,
+				errDetail: "plugins not in path",
+			},
+			{
+				name:      "backslash traversal attempt",
+				rawURL:    "/plugins/myplugin/..\\..\\etc\\passwd",
+				expectErr: false, // backslashes are not path separators in URL paths; treated as literal
+			},
+			{
+				name:      "null byte injection attempt",
+				rawURL:    "/plugins/myplugin\x00/action",
+				expectErr: true, // url.Parse rejects URLs with null bytes
+			},
+		}
+
+		for _, tc := range tests {
+			t.Run(tc.name, func(t *testing.T) {
+				resp, appErr := th.App.doPluginRequest(th.Context, "GET", tc.rawURL, nil, nil)
+				if tc.expectErr {
+					require.NotNil(t, appErr, "expected error for URL: %s", tc.rawURL)
+					if tc.errDetail != "" {
+						assert.Contains(t, appErr.DetailedError, tc.errDetail)
+					}
+				} else {
+					// Should not return an app error from path validation;
+					// may still get a 404 if the plugin doesn't exist, which is fine.
+					assert.Nil(t, appErr, "unexpected error for URL: %s - %v", tc.rawURL, appErr)
+					if resp != nil {
+						resp.Body.Close()
+					}
+				}
+			})
+		}
+	})
 }