Fix plugin /public handling for subpaths (#21886)

* Fix plugin /public handling for subpaths

Plugins support a `public` folder in the bundle automatically being accessible at `<site_url>/plugins/<plugin_id/public/`, but it seems support for a `site_url` with a subpath has been broken for some time.

I tried to figure out when this stopped working, but gave up after a while and just focussed on the requisite changes plus tests.

* simplify comment

* more testing coverage, and simpler diff

* try cleaning up plugins after tests

* skip TestServePluginPublicRequest to isolate build issue

* Revert "skip TestServePluginPublicRequest to isolate build issue"

This reverts commit 62d0e4e427c7cf7b7f9b09202ce10cd5f1a56bca.

* do th.TearDown last by using t.Cleanup vs. defer

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Co-authored-by: Mattermost Build <build@mattermost.com>

app/plugin_requests.go

@@ -5,7 +5,6 @@ package app
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 	"net/http"
 	"path"
@@ -93,7 +92,7 @@ func (ch *Channels) ServePluginPublicRequest(w http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	// Should be in the form of /$PLUGIN_ID/public/{anything} by the time we get here
+	// Should be in the form of /(subpath/)?/plugins/{plugin_id}/public/* by the time we get here
 	vars := mux.Vars(r)
 	pluginID := vars["plugin_id"]
 
@@ -111,8 +110,13 @@ func (ch *Channels) ServePluginPublicRequest(w http.ResponseWriter, r *http.Requ
 		return
 	}
 
+	subpath, err := utils.GetSubpathFromConfig(ch.cfgSvc.Config())
+	if err != nil {
+		http.Error(w, "Internal Server Error", http.StatusInternalServerError)
+	}
+
 	publicFilePath := path.Clean(r.URL.Path)
-	prefix := fmt.Sprintf("/plugins/%s/public/", pluginID)
+	prefix := path.Join(subpath, "plugins", pluginID, "public")
 	if !strings.HasPrefix(publicFilePath, prefix) {
 		http.NotFound(w, r)
 		return

app/plugin_requests_test.go

@@ -4,23 +4,65 @@
 package app
 
 import (
+	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/v6/model"
+	"github.com/mattermost/mattermost-server/v6/utils/fileutils"
 )
 
 func TestServePluginPublicRequest(t *testing.T) {
+	installPlugin := func(t *testing.T, th *TestHelper, pluginID string) {
+		t.Helper()
+
+		path, _ := fileutils.FindDir("tests")
+		fileReader, err := os.Open(filepath.Join(path, fmt.Sprintf("%s.tar.gz", pluginID)))
+		require.NoError(t, err)
+		defer fileReader.Close()
+
+		_, appErr := th.App.WriteFile(fileReader, getBundleStorePath(pluginID))
+		checkNoError(t, appErr)
+
+		appErr = th.App.SyncPlugins()
+		checkNoError(t, appErr)
+
+		env := th.App.GetPluginsEnvironment()
+		require.NotNil(t, env)
+
+		// Check if installed
+		pluginStatus, err := env.Statuses()
+		require.NoError(t, err)
+		found := false
+		for _, pluginStatus := range pluginStatus {
+			if pluginStatus.PluginId == pluginID {
+				found = true
+			}
+		}
+		require.True(t, found, "failed to find plugin %s in plugin statuses", pluginID)
+
+		appErr = th.App.EnablePlugin(pluginID)
+		checkNoError(t, appErr)
+
+		t.Cleanup(func() {
+			appErr = th.App.ch.RemovePlugin(pluginID)
+			checkNoError(t, appErr)
+		})
+	}
+
 	t.Run("returns not found when plugins environment is nil", func(t *testing.T) {
 		th := Setup(t)
-		defer th.TearDown()
+		t.Cleanup(th.TearDown)
 		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.PluginSettings.Enable = true })
 
-		req, err := http.NewRequest("GET", "/plugins", nil)
+		req, err := http.NewRequest("GET", "/plugins/plugin_id/public/file.txt", nil)
 		require.NoError(t, err)
 
 		rr := httptest.NewRecorder()
@@ -29,4 +71,83 @@ func TestServePluginPublicRequest(t *testing.T) {
 
 		assert.Equal(t, http.StatusNotFound, rr.Code)
 	})
+
+	t.Run("resolves path for valid plugin", func(t *testing.T) {
+		th := Setup(t)
+		t.Cleanup(th.TearDown)
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.PluginSettings.Enable = true })
+
+		path, _ := fileutils.FindDir("tests")
+		fileReader, err := os.Open(filepath.Join(path, "testplugin.tar.gz"))
+		require.NoError(t, err)
+		defer fileReader.Close()
+
+		installPlugin(t, th, "testplugin")
+
+		req, err := http.NewRequest("GET", "/plugins/testplugin/public/file.txt", nil)
+		require.NoError(t, err)
+
+		rr := httptest.NewRecorder()
+		th.App.ch.srv.Router.ServeHTTP(rr, req)
+
+		assert.Equal(t, http.StatusOK, rr.Code)
+		body, err := io.ReadAll(rr.Body)
+		require.NoError(t, err)
+		require.Equal(t, "Hello World!", string(body))
+	})
+
+	t.Run("resolves path for valid plugin when subpath configured", func(t *testing.T) {
+		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://localhost:8065/subpath")
+		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
+
+		th := Setup(t)
+		t.Cleanup(th.TearDown)
+
+		installPlugin(t, th, "testplugin")
+
+		req, err := http.NewRequest("GET", "/subpath/plugins/testplugin/public/file.txt", nil)
+		require.NoError(t, err)
+
+		rr := httptest.NewRecorder()
+		th.App.ch.srv.RootRouter.ServeHTTP(rr, req)
+
+		assert.Equal(t, http.StatusOK, rr.Code)
+		body, err := io.ReadAll(rr.Body)
+		require.NoError(t, err)
+		assert.Equal(t, "Hello World!", string(body))
+	})
+
+	t.Run("fails for invalid plugin", func(t *testing.T) {
+		th := Setup(t)
+		t.Cleanup(th.TearDown)
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.PluginSettings.Enable = true })
+
+		req, err := http.NewRequest("GET", "/plugins/invalidplugin/public/file.txt", nil)
+		require.NoError(t, err)
+
+		rr := httptest.NewRecorder()
+		th.App.ch.srv.Router.ServeHTTP(rr, req)
+
+		assert.Equal(t, http.StatusNotFound, rr.Code)
+	})
+
+	t.Run("fails attempting to break out of path", func(t *testing.T) {
+		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://localhost:8065/subpath")
+		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
+
+		th := Setup(t)
+		t.Cleanup(th.TearDown)
+
+		installPlugin(t, th, "testplugin")
+		installPlugin(t, th, "testplugin2")
+
+		req, err := http.NewRequest("GET", "/subpath/plugins/testplugin/public/../../testplugin2/file.txt", nil)
+		require.NoError(t, err)
+
+		rr := httptest.NewRecorder()
+		th.App.ch.srv.RootRouter.ServeHTTP(rr, req)
+
+		require.Equal(t, http.StatusMovedPermanently, rr.Code)
+		assert.Equal(t, "/subpath/plugins/testplugin2/file.txt", rr.Header()["Location"][0])
+	})
 }

tests/README.md

@@ -22,18 +22,19 @@ It is possible to manually test specific sections of any test, instead of using
 There are two test plugins: `testplugin.tar.gz` and `testplugin2.tar.gz`. These are use in some integration tests in the `api4` package. Any changes to the plugin bundles require updating the corresponding signatures.
 
 First, import the public and private development key:
-```
+```sh
-$ gpg --import ./development-public-key.gpg
+gpg --import ./development-public-key.gpg
-$ gpg --import ./development-private-key.asc
+gpg --import ./development-private-key.asc
 ```
 
 This has to be done only once.
 
 Then update the signatures:
+```sh
+gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign testplugin.tar.gz
+gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign --armor testplugin.tar.gz
+gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign testplugin2.tar.gz
+gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign --armor testplugin2.tar.gz
 ```
-$ gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign testplugin.tar.gz
-$ gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign --armor testplugin.tar.gz
-$ gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign testplugin2.tar.gz
-$ gpg -u F3FACE45E0DE642C8BD6A8E64C7C6562C192CC1F --verbose --personal-digest-preferences SHA256 --detach-sign --armor testplugin2.tar.gz
 
 Finally, include the updates bundles and signatures in your commit.

tests/testplugin.tar.gz.asc

@@ -1,14 +1,14 @@
 -----BEGIN PGP SIGNATURE-----
 
-iQGzBAABCAAdFiEE8/rOReDeZCyL1qjmTHxlYsGSzB8FAl4sogwACgkQTHxlYsGS
+iQGzBAABCAAdFiEE8/rOReDeZCyL1qjmTHxlYsGSzB8FAmOaRXoACgkQTHxlYsGS
-zB+CiwwAqNhwq6PQeKCQyJ4F1kZBpSkHrlbaT+V89tcj5BomhxFin30XukW2tiov
+zB8H1Av+MuNxBuQFxNvORGcudExCAAgQZb3ykYNVxPT1CzwVdd16B+VvRyt3+PKz
-+U4cfeKI+NAu9uPUxN6f4r6khQOGQK0bvun3YDemhbVozaPneNoxs+ugkBLMrwvp
+nSsyYyrdvd2xpdaEXFHBA8RxS5ZWCz/hkrdxUhBUWV8O5OUMOHDYvetWc6/9GeuR
-v3Vbi241bTWsi6NxlwJDSM+LEYWkFXZKCjQFjX2UWEM86uocKZnjHHqqke4ZkXWm
+3dd4VElpLsEs6hIpwnejR1EouNr5OhxstpnB2AOz5N7LWlG5lTKhaHs1zN1uLc4f
-Sal1mOvfZtx/R0+8aKt7FEbdUy4s15gRcVfnp017PD9VDwfiXSMVrdaYr3HqD2Q/
+GmdJZ+5+PYm1UUipFj4kolkI+44Ytl6mj+tTyC4VJAj0mwnXQtp/JdFcDmmeRrTI
-WMMmZ9lW4Y0I6qtv+1Ud9YZAXPr8OzsgU13FXU1GcUG+L/W8jSb9XY/4EIFpb4O6
+AwmanJKQlK3yw331FYSd/CXuqCGOh157X7Z5P2Mtr3ZOaNj7qLY0mjweqxjj4fnN
-tQGRBBtjq0EofVq8S9V6/LMPH3/CPgHufK7TWl12mnyGUOac4YmFlGtStkovIHZJ
+YTUu22KhRLGzggEbTg+5huYhtvqa1b87EcH6ukxWoBYQpFK+TyyhuX3ZeT5x6lFi
-+nxcMsV5xU3UhdM+/uBJnC5EH8sH2hQpkJugZIFruswfHNSiNKUpiHjupepfsV7v
+8SP9o/9KcQxB5oD6X5FGMR4v5VDosNnNuqW8G7g4fkcjQKY65tnX75G5Ih156BAP
-jzKCEgh7Rv99QBSSBtZSuBitnzEWAE3X9UsEYx5qCQJvBBVLiugFHFv6MtkePRNd
+dfZ6+nKCQvXgv1XRymF3UrJeddXtOMQzh4aSvYwwy46qNMPYMeDq6PoMXGVNeylP
-ElLLqMat
+bTrjLEtE
-=KHZP
+=OvPw
 -----END PGP SIGNATURE-----