From 6ffca12c6a9465dbd020225e545eb4dc986334b2 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 21 Sep 2024 02:49:08 +0100 Subject: [PATCH] Detect and set proper Content-Type for avatars Detect proper avatar type to return instead of returning `image/*`, which is not valid. --- avatar/avatar.go | 22 ++++++++- avatar/avatar_test.go | 101 ++++++++++++++++++++++++++++++++------- avatar/noop_test.go | 2 + v2/avatar/avatar.go | 22 ++++++++- v2/avatar/avatar_test.go | 101 ++++++++++++++++++++++++++++++++------- v2/avatar/noop_test.go | 2 + 6 files changed, 214 insertions(+), 36 deletions(-) diff --git a/avatar/avatar.go b/avatar/avatar.go index ed691fa8..cab724ba 100644 --- a/avatar/avatar.go +++ b/avatar/avatar.go @@ -23,6 +23,9 @@ import ( "github.com/go-pkgz/auth/token" ) +// http.sniffLen is 512 bytes which is how much we need to read to detect content type +const sniffLen = 512 + // Proxy provides http handler for avatars from avatar.Store // On user login token will call Put and it will retrieve and save picture locally. type Proxy struct { @@ -100,7 +103,6 @@ func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err err // Handler returns token routes for given provider func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) { - if r.Method != "GET" { w.WriteHeader(http.StatusMethodNotAllowed) } @@ -136,9 +138,25 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) { } }() - w.Header().Set("Content-Type", "image/*") + buf := make([]byte, sniffLen) + n, err := avReader.Read(buf) + if err != nil && err != io.EOF { + p.Logf("[WARN] can't read from avatar reader for %s, %s", avatarID, err) + rest.SendErrorJSON(w, r, p.L, http.StatusInternalServerError, err, "can't read avatar") + return + } w.Header().Set("Content-Length", strconv.Itoa(size)) + contentType := http.DetectContentType(buf) + if contentType == "application/octet-stream" { + contentType = "image/*" + } + w.Header().Set("Content-Type", contentType) w.WriteHeader(http.StatusOK) + if _, err = w.Write(buf[:n]); err != nil { + p.Logf("[WARN] can't write response to %s, %s", r.RemoteAddr, err) + return + } + // write the rest of response size if it's bigger than 512 bytes, or nothing as EOF would be sent right away then if _, err = io.Copy(w, avReader); err != nil { p.Logf("[WARN] can't send response to %s, %s", r.RemoteAddr, err) } diff --git a/avatar/avatar_test.go b/avatar/avatar_test.go index a5e0bc2b..7822ad89 100644 --- a/avatar/avatar_test.go +++ b/avatar/avatar_test.go @@ -107,12 +107,21 @@ func TestAvatar_Routes(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/pic.png" { - w.Header().Set("Content-Type", "image/*") + // set wrong content type, in reality would be image/png based on the first bytes + w.Header().Set("Content-Type", "image/jpg") w.Header().Set("Custom-Header", "xyz") - _, err := fmt.Fprint(w, "some picture bin data") + _, err := fmt.Fprint(w, "\x89PNG\x0D\x0A\x1A\x0A some picture bin data") require.NoError(t, err) return } + if r.URL.Path == "/circles.png" { + http.ServeFile(w, r, "testdata/circles.png") + return + } + if r.URL.Path == "/circles.jpg" { + http.ServeFile(w, r, "testdata/circles.jpg") + return + } http.Error(w, "not found", http.StatusNotFound) })) defer ts.Close() @@ -128,8 +137,8 @@ func TestAvatar_Routes(t *testing.T) { { // status 400 - req, err := http.NewRequest("GET", "/123aa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/123aa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) @@ -138,8 +147,8 @@ func TestAvatar_Routes(t *testing.T) { { // status 403 - req, err := http.NewRequest("GET", "../not-allowed.txt", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "../not-allowed.txt", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) @@ -147,29 +156,29 @@ func TestAvatar_Routes(t *testing.T) { } { // status 200 - req, err := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, []string{"image/*"}, rr.Header()["Content-Type"]) - assert.Equal(t, []string{"21"}, rr.Header()["Content-Length"]) + assert.Equal(t, []string{"image/png"}, rr.Header()["Content-Type"]) + assert.Equal(t, []string{"30"}, rr.Header()["Content-Length"]) assert.Equal(t, []string(nil), rr.Header()["Custom-Header"], "strip all custom headers") assert.NotNil(t, rr.Header()["Etag"]) bb := bytes.Buffer{} - sz, err := io.Copy(&bb, rr.Body) - assert.NoError(t, err) - assert.Equal(t, int64(21), sz) - assert.Equal(t, "some picture bin data", bb.String()) + sz, e := io.Copy(&bb, rr.Body) + assert.NoError(t, e) + assert.Equal(t, int64(30), sz) + assert.Equal(t, "\x89PNG\x0D\x0A\x1A\x0A some picture bin data", bb.String()) } { // status 304 - req, err := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) id := p.Store.ID("b3daa77b4c04a9551b8781d03191fe098f325e67.image") req.Header.Add("If-None-Match", p.Store.ID(id)) // hash of `some_random_name.image` since the file doesn't exist @@ -180,7 +189,67 @@ func TestAvatar_Routes(t *testing.T) { assert.Equal(t, http.StatusNotModified, rr.Code) assert.Equal(t, []string{`"` + id + `"`}, rr.Header()["Etag"]) } +} +func TestAvatar_WithValidPictures(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/circles.png" { + http.ServeFile(w, r, "testdata/circles.png") + return + } + if r.URL.Path == "/circles.jpg" { + http.ServeFile(w, r, "testdata/circles.jpg") + return + } + http.Error(w, "not found", http.StatusNotFound) + })) + defer ts.Close() + + p := Proxy{RoutePath: "/avatar", Store: NewLocalFS("/tmp/avatars.test"), L: logger.Std} + assert.NoError(t, os.MkdirAll("/tmp/avatars.test", 0o700)) + defer os.RemoveAll("/tmp/avatars.test") + client := &http.Client{Timeout: time.Second} + + testCases := []struct { + name string + user token.User + imageFile string + contentType string + }{ + { + name: "PNG Image", + user: token.User{ID: "user2", Name: "user2 name", Picture: ts.URL + "/circles.png"}, + imageFile: "testdata/circles.png", + contentType: "image/png", + }, + { + name: "JPG Image", + user: token.User{ID: "user3", Name: "user3 name", Picture: ts.URL + "/circles.jpg"}, + imageFile: "testdata/circles.jpg", + contentType: "image/jpeg", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + imageURL, err := p.Put(tc.user, client) + assert.NoError(t, err) + t.Logf("%s URL: %s", tc.name, imageURL) + + req, err := http.NewRequest("GET", imageURL, http.NoBody) + require.NoError(t, err) + rr := httptest.NewRecorder() + handler := http.Handler(http.HandlerFunc(p.Handler)) + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, []string{tc.contentType}, rr.Header()["Content-Type"]) + + imageData, err := os.ReadFile(tc.imageFile) + require.NoError(t, err) + assert.Equal(t, imageData, rr.Body.Bytes()) + assert.Equal(t, []string{fmt.Sprintf("%d", len(imageData))}, rr.Header()["Content-Length"]) + }) + } } func TestAvatar_resize(t *testing.T) { diff --git a/avatar/noop_test.go b/avatar/noop_test.go index 378b4cb4..1312ec6c 100644 --- a/avatar/noop_test.go +++ b/avatar/noop_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -39,6 +40,7 @@ func TestNoOp_Get(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) require.Zero(t, resp.ContentLength) + assert.Equal(t, "image/*", resp.Header.Get("Content-Type")) body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Empty(t, body) diff --git a/v2/avatar/avatar.go b/v2/avatar/avatar.go index 3e9b5c2a..a1d749a6 100644 --- a/v2/avatar/avatar.go +++ b/v2/avatar/avatar.go @@ -23,6 +23,9 @@ import ( "github.com/go-pkgz/auth/v2/token" ) +// http.sniffLen is 512 bytes which is how much we need to read to detect content type +const sniffLen = 512 + // Proxy provides http handler for avatars from avatar.Store // On user login token will call Put and it will retrieve and save picture locally. type Proxy struct { @@ -100,7 +103,6 @@ func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err err // Handler returns token routes for given provider func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) { - if r.Method != "GET" { w.WriteHeader(http.StatusMethodNotAllowed) } @@ -136,9 +138,25 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) { } }() - w.Header().Set("Content-Type", "image/*") + buf := make([]byte, sniffLen) + n, err := avReader.Read(buf) + if err != nil && err != io.EOF { + p.Logf("[WARN] can't read from avatar reader for %s, %s", avatarID, err) + rest.SendErrorJSON(w, r, p.L, http.StatusInternalServerError, err, "can't read avatar") + return + } w.Header().Set("Content-Length", strconv.Itoa(size)) + contentType := http.DetectContentType(buf) + if contentType == "application/octet-stream" { + contentType = "image/*" + } + w.Header().Set("Content-Type", contentType) w.WriteHeader(http.StatusOK) + if _, err = w.Write(buf[:n]); err != nil { + p.Logf("[WARN] can't write response to %s, %s", r.RemoteAddr, err) + return + } + // write the rest of response size if it's bigger than 512 bytes, or nothing as EOF would be sent right away then if _, err = io.Copy(w, avReader); err != nil { p.Logf("[WARN] can't send response to %s, %s", r.RemoteAddr, err) } diff --git a/v2/avatar/avatar_test.go b/v2/avatar/avatar_test.go index 937f8866..0b6a6427 100644 --- a/v2/avatar/avatar_test.go +++ b/v2/avatar/avatar_test.go @@ -107,12 +107,21 @@ func TestAvatar_Routes(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/pic.png" { - w.Header().Set("Content-Type", "image/*") + // set wrong content type, in reality would be image/png based on the first bytes + w.Header().Set("Content-Type", "image/jpg") w.Header().Set("Custom-Header", "xyz") - _, err := fmt.Fprint(w, "some picture bin data") + _, err := fmt.Fprint(w, "\x89PNG\x0D\x0A\x1A\x0A some picture bin data") require.NoError(t, err) return } + if r.URL.Path == "/circles.png" { + http.ServeFile(w, r, "testdata/circles.png") + return + } + if r.URL.Path == "/circles.jpg" { + http.ServeFile(w, r, "testdata/circles.jpg") + return + } http.Error(w, "not found", http.StatusNotFound) })) defer ts.Close() @@ -128,8 +137,8 @@ func TestAvatar_Routes(t *testing.T) { { // status 400 - req, err := http.NewRequest("GET", "/123aa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/123aa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) @@ -138,8 +147,8 @@ func TestAvatar_Routes(t *testing.T) { { // status 403 - req, err := http.NewRequest("GET", "../not-allowed.txt", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "../not-allowed.txt", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) @@ -147,29 +156,29 @@ func TestAvatar_Routes(t *testing.T) { } { // status 200 - req, err := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) rr := httptest.NewRecorder() handler := http.Handler(http.HandlerFunc(p.Handler)) handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, []string{"image/*"}, rr.Header()["Content-Type"]) - assert.Equal(t, []string{"21"}, rr.Header()["Content-Length"]) + assert.Equal(t, []string{"image/png"}, rr.Header()["Content-Type"]) + assert.Equal(t, []string{"30"}, rr.Header()["Content-Length"]) assert.Equal(t, []string(nil), rr.Header()["Custom-Header"], "strip all custom headers") assert.NotNil(t, rr.Header()["Etag"]) bb := bytes.Buffer{} - sz, err := io.Copy(&bb, rr.Body) - assert.NoError(t, err) - assert.Equal(t, int64(21), sz) - assert.Equal(t, "some picture bin data", bb.String()) + sz, e := io.Copy(&bb, rr.Body) + assert.NoError(t, e) + assert.Equal(t, int64(30), sz) + assert.Equal(t, "\x89PNG\x0D\x0A\x1A\x0A some picture bin data", bb.String()) } { // status 304 - req, err := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) - require.NoError(t, err) + req, e := http.NewRequest("GET", "/b3daa77b4c04a9551b8781d03191fe098f325e67.image", http.NoBody) + require.NoError(t, e) id := p.Store.ID("b3daa77b4c04a9551b8781d03191fe098f325e67.image") req.Header.Add("If-None-Match", p.Store.ID(id)) // hash of `some_random_name.image` since the file doesn't exist @@ -180,7 +189,67 @@ func TestAvatar_Routes(t *testing.T) { assert.Equal(t, http.StatusNotModified, rr.Code) assert.Equal(t, []string{`"` + id + `"`}, rr.Header()["Etag"]) } +} +func TestAvatar_WithValidPictures(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/circles.png" { + http.ServeFile(w, r, "testdata/circles.png") + return + } + if r.URL.Path == "/circles.jpg" { + http.ServeFile(w, r, "testdata/circles.jpg") + return + } + http.Error(w, "not found", http.StatusNotFound) + })) + defer ts.Close() + + p := Proxy{RoutePath: "/avatar", Store: NewLocalFS("/tmp/avatars.test"), L: logger.Std} + assert.NoError(t, os.MkdirAll("/tmp/avatars.test", 0o700)) + defer os.RemoveAll("/tmp/avatars.test") + client := &http.Client{Timeout: time.Second} + + testCases := []struct { + name string + user token.User + imageFile string + contentType string + }{ + { + name: "PNG Image", + user: token.User{ID: "user2", Name: "user2 name", Picture: ts.URL + "/circles.png"}, + imageFile: "testdata/circles.png", + contentType: "image/png", + }, + { + name: "JPG Image", + user: token.User{ID: "user3", Name: "user3 name", Picture: ts.URL + "/circles.jpg"}, + imageFile: "testdata/circles.jpg", + contentType: "image/jpeg", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + imageURL, err := p.Put(tc.user, client) + assert.NoError(t, err) + t.Logf("%s URL: %s", tc.name, imageURL) + + req, err := http.NewRequest("GET", imageURL, http.NoBody) + require.NoError(t, err) + rr := httptest.NewRecorder() + handler := http.Handler(http.HandlerFunc(p.Handler)) + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, []string{tc.contentType}, rr.Header()["Content-Type"]) + + imageData, err := os.ReadFile(tc.imageFile) + require.NoError(t, err) + assert.Equal(t, imageData, rr.Body.Bytes()) + assert.Equal(t, []string{fmt.Sprintf("%d", len(imageData))}, rr.Header()["Content-Length"]) + }) + } } func TestAvatar_resize(t *testing.T) { diff --git a/v2/avatar/noop_test.go b/v2/avatar/noop_test.go index 378b4cb4..1312ec6c 100644 --- a/v2/avatar/noop_test.go +++ b/v2/avatar/noop_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -39,6 +40,7 @@ func TestNoOp_Get(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) require.Zero(t, resp.ContentLength) + assert.Equal(t, "image/*", resp.Header.Get("Content-Type")) body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Empty(t, body)