MM-54219 - Fix: Improve limits on redirectLocationDataCache (#24429)

* ignore redirect locations over certain length

* maybe the link isn't necessary

* remove unrelated debugging line

* PR comments
This commit is contained in:
Christopher Poile
2023-09-05 14:04:14 -04:00
committed by GitHub
parent 1fb8772ef9
commit b6f2c8205e
2 changed files with 44 additions and 5 deletions

View File

@@ -26,9 +26,11 @@ import (
)
const (
RedirectLocationCacheSize = 10000
DefaultServerBusySeconds = 3600
MaxServerBusySeconds = 86400
RedirectLocationCacheSize = 10000
RedirectLocationMaximumLength = 2100
RedirectLocationCacheExpiry = 1 * time.Hour
DefaultServerBusySeconds = 3600
MaxServerBusySeconds = 86400
)
var redirectLocationDataCache = cache.NewLRU(cache.LRUOptions{
@@ -585,7 +587,7 @@ func getRedirectLocation(c *Context, w http.ResponseWriter, r *http.Request) {
res, err := client.Head(url)
if err != nil {
// Cache failures to prevent retries.
redirectLocationDataCache.SetWithExpiry(url, "", 1*time.Hour)
redirectLocationDataCache.SetWithExpiry(url, "", RedirectLocationCacheExpiry)
// Always return a success status and a JSON string to limit information returned to client.
w.Write([]byte(model.MapToJSON(m)))
return
@@ -596,7 +598,17 @@ func getRedirectLocation(c *Context, w http.ResponseWriter, r *http.Request) {
}()
location = res.Header.Get("Location")
redirectLocationDataCache.SetWithExpiry(url, location, 1*time.Hour)
// If the location length is > 2100, we can probably ignore. Fixes https://mattermost.atlassian.net/browse/MM-54219
if len(location) > RedirectLocationMaximumLength {
// Treating as a "failure". Cache failures to prevent retries.
redirectLocationDataCache.SetWithExpiry(url, "", RedirectLocationCacheExpiry)
// Always return a success status and a JSON string to limit information returned to client.
w.Write([]byte(model.MapToJSON(m)))
return
}
redirectLocationDataCache.SetWithExpiry(url, location, RedirectLocationCacheExpiry)
m["location"] = location
w.Write([]byte(model.MapToJSON(m)))

View File

@@ -673,6 +673,33 @@ func TestRedirectLocation(t *testing.T) {
_, resp, err = client.GetRedirectLocation(context.Background(), "", "")
require.Error(t, err)
CheckUnauthorizedStatus(t, resp)
// Check that too-long redirect locations are ignored
*th.App.Config().ServiceSettings.EnableLinkPreviews = true
urlPrefix := "https://example.co"
almostTooLongUrl := urlPrefix + strings.Repeat("a", 2100-len(urlPrefix))
testServer2 := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
res.Header().Set("Location", almostTooLongUrl)
res.WriteHeader(http.StatusFound)
res.Write([]byte("body"))
}))
defer func() { testServer2.Close() }()
actual, _, err = th.SystemAdminClient.GetRedirectLocation(context.Background(), testServer2.URL, "")
require.NoError(t, err)
assert.Equal(t, almostTooLongUrl, actual)
tooLongUrl := urlPrefix + strings.Repeat("a", 2101-len(urlPrefix))
testServer3 := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
res.Header().Set("Location", tooLongUrl)
res.WriteHeader(http.StatusFound)
res.Write([]byte("body"))
}))
defer func() { testServer3.Close() }()
actual, _, err = th.SystemAdminClient.GetRedirectLocation(context.Background(), testServer3.URL, "")
require.NoError(t, err)
assert.Equal(t, "", actual)
}
func TestSetServerBusy(t *testing.T) {