From 7526844c5052bc7c3fe252a158eb5ff037813974 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Wed, 15 Apr 2026 20:02:14 +0530 Subject: [PATCH] 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 --- server/channels/app/integration_action.go | 29 +-- .../channels/app/integration_action_test.go | 185 ++++++++++++++++++ 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/server/channels/app/integration_action.go b/server/channels/app/integration_action.go index d466ede39e..d9308a8645 100644 --- a/server/channels/app/integration_action.go +++ b/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 - 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) - } + httpClient := a.getPostActionClient(c, inURL, req) 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) diff --git a/server/channels/app/integration_action_test.go b/server/channels/app/integration_action_test.go index ab648ab49b..a3da46770e 100644 --- a/server/channels/app/integration_action_test.go +++ b/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() + } + } + }) + } + }) }