From cd5d5f832cab21faa6bd589d69c6de3f5a92527c Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 13 Feb 2019 11:30:02 -0400 Subject: [PATCH] MM-14052: fix subpath yet again (#10278) * MM-14052: fix subpath yet again The server now emits a script-src directive that overrides the root.html rewrite. Fix this by emitting the requisite sha-256 hash server-side as well as rewriting root.html. We can't remove the root.html rewrite, since the assets may be on a CDN instead and we use the same code path to rewrite them (on demand). Prior to this change, going from / -> /subpath -> / would leave changes in root.html: the Content-Security-Policy header would still have the sha-256 hash, and the inline script would still override the publicPath but to the default subpath value. To avoid sending down a sha-256 hash server-side when no subpath is required, change this to fully strip out the subpath changes. This is the only unit test change, as the existing coverage proves the algorithm still works. * fix subpath concatenation in test path.Join isn't meant to work with a URL + path, and my test was effectively working with the subpath "/localhost:8065/subpath" instead of just "/subpath". The CI servers presumably caught this due to a different configuration than my local development. --- utils/subpath.go | 63 ++++++++++++++++++++++++------------ utils/subpath_test.go | 9 ++++-- web/handlers.go | 5 ++- web/handlers_test.go | 75 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 23 deletions(-) diff --git a/utils/subpath.go b/utils/subpath.go index 457627af30..9bb75db3de 100644 --- a/utils/subpath.go +++ b/utils/subpath.go @@ -22,6 +22,29 @@ import ( "github.com/mattermost/mattermost-server/utils/fileutils" ) +// getSubpathScript renders the inline script that defines window.publicPath to change how webpack loads assets. +func getSubpathScript(subpath string) string { + if subpath == "" { + subpath = "/" + } + + newPath := path.Join(subpath, "static") + "/" + + return fmt.Sprintf("window.publicPath='%s'", newPath) +} + +// GetSubpathScriptHash computes the script-src addition required for the subpath script to bypass CSP protections. +func GetSubpathScriptHash(subpath string) string { + // No hash is required for the default subpath. + if subpath == "" || subpath == "/" { + return "" + } + + scriptHash := sha256.Sum256([]byte(getSubpathScript(subpath))) + + return fmt.Sprintf(" 'sha256-%s'", base64.StdEncoding.EncodeToString(scriptHash[:])) +} + // UpdateAssetsSubpath rewrites assets in the /client directory to assume the application is hosted // at the given subpath instead of at the root. No changes are written unless necessary. func UpdateAssetsSubpath(subpath string) error { @@ -45,47 +68,47 @@ func UpdateAssetsSubpath(subpath string) error { return errors.Wrap(err, "failed to open root.html") } - pathToReplace := "/static/" - newPath := path.Join(subpath, "static") + "/" + oldSubpath := "/" // Determine if a previous subpath had already been rewritten into the assets. - reWebpackPublicPathScript := regexp.MustCompile("window.publicPath='([^']+)'") + reWebpackPublicPathScript := regexp.MustCompile("window.publicPath='([^']+/)static/'") alreadyRewritten := false if matches := reWebpackPublicPathScript.FindStringSubmatch(string(oldRootHtml)); matches != nil { - pathToReplace = matches[1] + oldSubpath = matches[1] alreadyRewritten = true } - if pathToReplace == newPath { - mlog.Debug("No rewrite required for static assets", mlog.String("path", pathToReplace)) - return nil - } + pathToReplace := path.Join(oldSubpath, "static") + "/" + newPath := path.Join(subpath, "static") + "/" - mlog.Debug("Rewriting static assets", mlog.String("from_path", pathToReplace), mlog.String("to_path", newPath)) + mlog.Debug("Rewriting static assets", mlog.String("from_subpath", oldSubpath), mlog.String("to_subpath", subpath)) newRootHtml := string(oldRootHtml) - // Compute the sha256 hash for the inline script and reference same in the CSP meta tag. - // This allows the inline script defining `window.publicPath` to bypass CSP protections. - script := fmt.Sprintf("window.publicPath='%s'", newPath) - scriptHash := sha256.Sum256([]byte(script)) - reCSP := regexp.MustCompile(``) if results := reCSP.FindAllString(newRootHtml, -1); len(results) == 0 { return fmt.Errorf("failed to find 'Content-Security-Policy' meta tag to rewrite") } newRootHtml = reCSP.ReplaceAllLiteralString(newRootHtml, fmt.Sprintf( - ``, - base64.StdEncoding.EncodeToString(scriptHash[:]), + ``, + GetSubpathScriptHash(subpath), )) - // Rewrite the root.html references to `/static/*` to include the given subpath. This - // potentially includes a previously injected inline script. + // Rewrite the root.html references to `/static/*` to include the given subpath. + // This potentially includes a previously injected inline script that needs to + // be updated (and isn't covered by the cases above). newRootHtml = strings.Replace(newRootHtml, pathToReplace, newPath, -1) - // Inject the script, if needed, to define `window.publicPath`. - if !alreadyRewritten { + if alreadyRewritten && subpath == "/" { + // Remove the injected script since no longer required. Note that the rewrite above + // will have affected the script, so look for the new subpath, not the old one. + oldScript := getSubpathScript(subpath) + newRootHtml = strings.Replace(newRootHtml, fmt.Sprintf("", oldScript), "", 1) + + } else if !alreadyRewritten && subpath != "/" { + // Otherwise, inject the script to define `window.publicPath`. + script := getSubpathScript(subpath) newRootHtml = strings.Replace(newRootHtml, "", fmt.Sprintf("", script), 1) } diff --git a/utils/subpath_test.go b/utils/subpath_test.go index 66641e00ee..aa6fbe7b98 100644 --- a/utils/subpath_test.go +++ b/utils/subpath_test.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -117,7 +118,7 @@ func TestUpdateAssetsSubpath(t *testing.T) { baseManifestJson, "/", nil, - resetRootHtml, + baseRootHtml, baseCss, baseManifestJson, }, @@ -137,7 +138,11 @@ func TestUpdateAssetsSubpath(t *testing.T) { contents, err := ioutil.ReadFile(filepath.Join(tempDir, model.CLIENT_DIR, "root.html")) require.NoError(t, err) - require.Equal(t, testCase.ExpectedRootHTML, string(contents)) + + // Rewrite the expected and contents for simpler diffs when failed. + expectedRootHTML := strings.Replace(testCase.ExpectedRootHTML, ">", ">\n", -1) + contentsStr := strings.Replace(string(contents), ">", ">\n", -1) + require.Equal(t, expectedRootHTML, contentsStr) contents, err = ioutil.ReadFile(filepath.Join(tempDir, model.CLIENT_DIR, "main.css")) require.NoError(t, err) diff --git a/web/handlers.go b/web/handlers.go index dbff4ceaf9..9303119871 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -77,7 +77,10 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Instruct the browser not to display us in an iframe unless is the same origin for anti-clickjacking w.Header().Set("X-Frame-Options", "SAMEORIGIN") // Set content security policy. This is also specified in the root.html of the webapp in a meta tag. - w.Header().Set("Content-Security-Policy", "frame-ancestors 'self'; script-src 'self' cdn.segment.com/analytics.js/") + w.Header().Set("Content-Security-Policy", fmt.Sprintf( + "frame-ancestors 'self'; script-src 'self' cdn.segment.com/analytics.js/%s", + utils.GetSubpathScriptHash(subpath), + )) } else { // All api response bodies will be JSON formatted by default w.Header().Set("Content-Type", "application/json") diff --git a/web/handlers_test.go b/web/handlers_test.go index 0eadb1c840..a6f618b2e2 100644 --- a/web/handlers_test.go +++ b/web/handlers_test.go @@ -197,3 +197,78 @@ func TestHandlerServeCSRFToken(t *testing.T) { t.Errorf("Expected status 200, got %d", response.Code) } } + +func handlerForCSPHeader(c *Context, w http.ResponseWriter, r *http.Request) { +} + +func TestHandlerServeCSPHeader(t *testing.T) { + t.Run("non-static", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + web := New(th.Server, th.Server.AppOptions, th.Server.Router) + + handler := Handler{ + GetGlobalAppOptions: web.GetGlobalAppOptions, + HandleFunc: handlerForCSPHeader, + RequireSession: false, + TrustRequester: false, + RequireMfa: false, + IsStatic: false, + } + + request := httptest.NewRequest("POST", "/api/v4/test", nil) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + assert.Equal(t, 200, response.Code) + assert.Empty(t, response.Header()["Content-Security-Policy"]) + }) + + t.Run("static, without subpath", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + web := New(th.Server, th.Server.AppOptions, th.Server.Router) + + handler := Handler{ + GetGlobalAppOptions: web.GetGlobalAppOptions, + HandleFunc: handlerForCSPHeader, + RequireSession: false, + TrustRequester: false, + RequireMfa: false, + IsStatic: true, + } + + request := httptest.NewRequest("POST", "/", nil) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + assert.Equal(t, 200, response.Code) + assert.Contains(t, response.Header()["Content-Security-Policy"], "frame-ancestors 'self'; script-src 'self' cdn.segment.com/analytics.js/") + }) + + t.Run("static, with subpath", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = *cfg.ServiceSettings.SiteURL + "/subpath" + }) + + web := New(th.Server, th.Server.AppOptions, th.Server.Router) + + handler := Handler{ + GetGlobalAppOptions: web.GetGlobalAppOptions, + HandleFunc: handlerForCSPHeader, + RequireSession: false, + TrustRequester: false, + RequireMfa: false, + IsStatic: true, + } + + request := httptest.NewRequest("POST", "/", nil) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + assert.Equal(t, 200, response.Code) + assert.Contains(t, response.Header()["Content-Security-Policy"], "frame-ancestors 'self'; script-src 'self' cdn.segment.com/analytics.js/ 'sha256-tPOjw+tkVs9axL78ZwGtYl975dtyPHB6LYKAO2R3gR4='") + }) +}