Fix #6960 - LFS OID urls uses unusual content-type header #7005
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7005 +/- ##
=========================================
+ Coverage 41.45% 41.5% +0.04%
=========================================
Files 441 440 -1
Lines 59699 59450 -249
=========================================
- Hits 24747 24673 -74
+ Misses 31727 31560 -167
+ Partials 3225 3217 -8
Continue to review full report at Codecov.
|
GiteaBot
added
the
lgtm/need 2
lafriks
added
the
type/refactoring
| @@ -415,7 +413,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 | |||
There was a problem hiding this comment.
why does it checks not equal now? imho it should still be:
| return mt != metaMediaType | |
| return mt == metaMediaType |
There was a problem hiding this comment.
It was previously checking if mt == "application/vnd.git-lfs"
now it checks if mt != "application/vnd.git-lfs+json"
There was a problem hiding this comment.
Is this really correct then as any invalid type would also match?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm interestingly that breaks the media endpoint...
There was a problem hiding this comment.
Reported git-lfs/git-lfs#3662 to git-lfs so they start to send Accept: application/vnd.git-lfs+json for /verify URL.
There was a problem hiding this comment.
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.
|
I think I know how to fix this mess.
|
also ensure header map is unique for verify
This PR drops the unusual content-type requirement of "application/vnd.git-lfs" for repo.git/info/lfs/:oid, and just relies on the content-type not being "application/vnd.git-lfs+json".
Fixes #6960
Of interest, I have been unable to find a definite use for the meta version of this url.