Skip to content

Fix #6960 - LFS OID urls uses unusual content-type header #7005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix #6960 - LFS OID urls uses unusual content-type header #7005

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab05d5d
Fix #6960 - drop content-type header
zeripath May 20, 2019
b56bbd1
As per slonopotamus
zeripath May 22, 2019
63f21d5
Drop checking content-type on verify
zeripath May 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions modules/lfs/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ import (
)

const (
contentMediaType = "application/vnd.git-lfs"
metaMediaType = contentMediaType + "+json"
metaMediaType = "application/vnd.git-lfs+json"
)

// RequestVars contain variables from the HTTP request. Variables from routing, json body decoding, and
Expand Down Expand Up @@ -101,10 +100,8 @@ func ObjectOidHandler(ctx *context.Context) {
getMetaHandler(ctx)
return
}
if ContentMatcher(ctx.Req) || len(ctx.Params("filename")) > 0 {
getContentHandler(ctx)
return
}
getContentHandler(ctx)
return
} else if ctx.Req.Method == "PUT" && ContentMatcher(ctx.Req) {
PutHandler(ctx)
return
Expand Down Expand Up @@ -348,11 +345,6 @@ func VerifyHandler(ctx *context.Context) {
return
}

if !ContentMatcher(ctx.Req) {
writeStatus(ctx, 400)
return
}

rv := unpack(ctx)

meta, _ := getAuthenticatedRepoAndMeta(ctx, rv, true)
Expand Down Expand Up @@ -385,7 +377,6 @@ func Represent(rv *RequestVars, meta *models.LFSMetaObject, download, upload boo
}

header := make(map[string]string)
header["Accept"] = contentMediaType

if rv.Authorization == "" {
//https://github.com/github/git-lfs/issues/1088
Expand All @@ -404,7 +395,12 @@ func Represent(rv *RequestVars, meta *models.LFSMetaObject, download, upload boo

if upload && !download {
// Force client side verify action while gitea lacks proper server side verification
rep.Actions["verify"] = &link{Href: rv.VerifyLink(), Header: header}
verifyHeader := make(map[string]string)
for k, v := range header {
verifyHeader[k] = v
}
verifyHeader["Accept"] = metaMediaType
rep.Actions["verify"] = &link{Href: rv.VerifyLink(), Header: verifyHeader}
}

return rep
Expand All @@ -415,7 +411,7 @@ func Represent(rv *RequestVars, meta *models.LFSMetaObject, download, upload boo
func ContentMatcher(r macaron.Request) bool {
mediaParts := strings.Split(r.Header.Get("Accept"), ";")
mt := mediaParts[0]
return mt == contentMediaType
return mt != metaMediaType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it checks not equal now? imho it should still be:

Suggested change
return mt != metaMediaType
return mt == metaMediaType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was previously checking if mt == "application/vnd.git-lfs"

now it checks if mt != "application/vnd.git-lfs+json"

Copy link
Member

@lafriks lafriks May 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really correct then as any invalid type would also match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the spec says much at all about this - what it doesn't do is specify that it must be the current content type. @slonopotamus any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe accepting any content type for blob is OK. It allows downloading blobs with just a browser. You already have "filename" parameter that skips content-type check. What worries me is verify url. Does it still have ContentMatcher? If yes, this goes against the spec

Copy link
Contributor

@slonopotamus slonopotamus May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to say that you pass tests without installing any Accept header for verify and having ContentMatcher in it? Oh my. I start to suspect that git-lfs does not install Accept header for verify unless told by server to do so, even though spec says that verify has to be +json.

We may possibly want to wait for reaction from LFS devs to git-lfs/lfs-test-server#85.

Copy link
Contributor Author

@zeripath zeripath May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok I've just pushed changes to the branch as above.

Now it's probably the case that our tests are insufficient. For example in #6999 and #6961 I noticed that SSH locks weren't working despite tests (written by myself I should say - but when I understood LFS even less than I do now) suggesting that they were working. I don't think we attempt an LFS checkout either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interestingly that breaks the media endpoint...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reported git-lfs/git-lfs#3662 to git-lfs so they start to send Accept: application/vnd.git-lfs+json for /verify URL.

Copy link
Contributor

@slonopotamus slonopotamus May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suddenly understood that it is only me using "/verify URL" term. This is because in git-as-svn we use completely different URLs for verification and upload/download, thus avoiding the need to determine what to do based on Accept HTTP header.

}

// MetaMatcher provides a mux.MatcherFunc that only allows requests that contain
Expand Down