From 9d48fdc32c56c10f3d77cdac386fcb5b5e4e978a Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Tue, 12 Jul 2016 13:05:13 +0200 Subject: [PATCH] Restrict the set of allowed characters in plugin IDs We will want to put plugin id in a control id, which is sent to an app and then to GUI. When we get a control request from GUI, we will want to extract the plugin ID from the control name. To do it unambiguously we need some separator made of chars that are not allowed in a plugin name. This is to avoid the situation when there are two plugins: "Plugin" and "PluginFoo". "Plugin" exposes a control named "FooControl" and "PluginFoo" exposes a control named "Control". Faking the control names which will be sent to the app would result in two "PluginFooControl". One possible option for plugin ID and control name separator would be "/", but that won't work, since the request sent from GUI to the app to // would actually be /// and as such wouldn't match the URL template in RegisterControlRoutes(). --- probe/plugins/registry.go | 21 +++++++--- probe/plugins/registry_internal_test.go | 52 +++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/probe/plugins/registry.go b/probe/plugins/registry.go index 7dfbf2d1b..7f0e2f8c9 100644 --- a/probe/plugins/registry.go +++ b/probe/plugins/registry.go @@ -5,6 +5,7 @@ import ( "net/http" "net/url" "path/filepath" + "regexp" "sort" "strings" "sync" @@ -27,6 +28,7 @@ var ( transport = makeUnixRoundTripper maxResponseBytes int64 = 50 * 1024 * 1024 errResponseTooLarge = fmt.Errorf("response must be shorter than 50MB") + validPluginName = regexp.MustCompile("^[A-Za-z0-9]+([-][A-Za-z0-9]+)*$") ) const ( @@ -103,7 +105,12 @@ func (r *Registry) scan() error { continue } client := &http.Client{Transport: tr, Timeout: pluginTimeout} - plugins[path] = NewPlugin(r.context, path, client, r.apiVersion, r.handshakeMetadata) + plugin, err := NewPlugin(r.context, path, client, r.apiVersion, r.handshakeMetadata) + if err != nil { + log.Warningf("plugins: error loading plugin %s: %v", path, err) + continue + } + plugins[path] = plugin log.Infof("plugins: added plugin %s", path) } // remove plugins which weren't found @@ -216,16 +223,19 @@ type Plugin struct { // NewPlugin loads and initializes a new plugin. If client is nil, // http.DefaultClient will be used. -func NewPlugin(ctx context.Context, socket string, client *http.Client, expectedAPIVersion string, handshakeMetadata map[string]string) *Plugin { +func NewPlugin(ctx context.Context, socket string, client *http.Client, expectedAPIVersion string, handshakeMetadata map[string]string) (*Plugin, error) { + id := strings.TrimSuffix(filepath.Base(socket), filepath.Ext(socket)) + if !validPluginName.MatchString(id) { + return nil, fmt.Errorf("invalid plugin id %q", id) + } + params := url.Values{} for k, v := range handshakeMetadata { params.Add(k, v) } - id := strings.TrimSuffix(filepath.Base(socket), filepath.Ext(socket)) - ctx, cancel := context.WithCancel(ctx) - return &Plugin{ + plugin := &Plugin{ PluginSpec: xfer.PluginSpec{ID: id, Label: id}, context: ctx, socket: socket, @@ -234,6 +244,7 @@ func NewPlugin(ctx context.Context, socket string, client *http.Client, expected client: client, cancel: cancel, } + return plugin, nil } // Report gets the latest report from the plugin diff --git a/probe/plugins/registry_internal_test.go b/probe/plugins/registry_internal_test.go index 64dbeed1e..cba906965 100644 --- a/probe/plugins/registry_internal_test.go +++ b/probe/plugins/registry_internal_test.go @@ -518,3 +518,55 @@ func TestRegistryRejectsPluginResponsesWhichAreTooLarge(t *testing.T) { {ID: "foo", Label: "foo", Status: `error: response must be shorter than 50MB`}, }) } + +func TestRegistryChecksForValidPluginIDs(t *testing.T) { + setup( + t, + mockPlugin{ + t: t, + Name: "testPlugin", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "P-L-U-G-I-N", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"P-L-U-G-I-N","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "another-testPlugin", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"another-testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "testPlugin!", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin!","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "test~plugin", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"test~plugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "testPlugin-", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin-","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + mockPlugin{ + t: t, + Name: "-testPlugin", + Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"-testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`), + }.file(), + ) + defer restore(t) + + root := "/plugins" + r, err := NewRegistry(root, "1", nil) + if err != nil { + t.Fatal(err) + } + defer r.Close() + + r.Report() + checkLoadedPluginIDs(t, r.ForEach, []string{"P-L-U-G-I-N", "another-testPlugin", "testPlugin"}) +}