Skip to content

Commit

Permalink
Detect and set proper Content-Type for avatars
Browse files Browse the repository at this point in the history 
Detect proper avatar type to return instead of returning `image/*`,
which is not valid.
  • Loading branch information
@paskal @umputun
paskal authored and umputun committed Sep 22, 2024
1 parent 458e57e commit f0b6097
Show file tree
Hide file tree
Showing 6 changed files with 188 additions and 26 deletions.
22 changes: 20 additions & 2 deletions avatar/avatar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
83 changes: 72 additions & 11 deletions avatar/avatar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ 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/*")
w.Header().Set("Content-Type", "image/jpg")
w.Header().Set("Custom-Header", "xyz")
_, err := fmt.Fprint(w, "some picture bin data")
require.NoError(t, err)
Expand All @@ -128,8 +128,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)
Expand All @@ -138,17 +138,17 @@ 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)
assert.Equal(t, http.StatusForbidden, rr.Code)
}

{ // 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)
Expand All @@ -160,16 +160,16 @@ func TestAvatar_Routes(t *testing.T) {
assert.NotNil(t, rr.Header()["Etag"])

bb := bytes.Buffer{}
sz, err := io.Copy(&bb, rr.Body)
assert.NoError(t, err)
sz, e := io.Copy(&bb, rr.Body)
assert.NoError(t, e)
assert.Equal(t, int64(21), sz)
assert.Equal(t, "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

Expand All @@ -180,7 +180,68 @@ 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) {
Expand Down
2 changes: 2 additions & 0 deletions avatar/noop_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -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)
Expand Down
22 changes: 20 additions & 2 deletions v2/avatar/avatar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
83 changes: 72 additions & 11 deletions v2/avatar/avatar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ 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/*")
w.Header().Set("Content-Type", "image/jpg")
w.Header().Set("Custom-Header", "xyz")
_, err := fmt.Fprint(w, "some picture bin data")
require.NoError(t, err)
Expand All @@ -128,8 +128,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)
Expand All @@ -138,17 +138,17 @@ 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)
assert.Equal(t, http.StatusForbidden, rr.Code)
}

{ // 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)
Expand All @@ -160,16 +160,16 @@ func TestAvatar_Routes(t *testing.T) {
assert.NotNil(t, rr.Header()["Etag"])

bb := bytes.Buffer{}
sz, err := io.Copy(&bb, rr.Body)
assert.NoError(t, err)
sz, e := io.Copy(&bb, rr.Body)
assert.NoError(t, e)
assert.Equal(t, int64(21), sz)
assert.Equal(t, "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

Expand All @@ -180,7 +180,68 @@ 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) {
Expand Down
2 changes: 2 additions & 0 deletions v2/avatar/noop_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit f0b6097

Please sign in to comment.