Skip to content

Fix /verify LFS handler expecting wrong content-type #6958

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.

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 1 commit into from

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented May 15, 2019

According to spec,
/verify requests must have "Accept: application/vnd.git-lfs+json"

Previous code worked just because native git-lfs implementation replaced Accept header that is required by
spec with value given by Gitea ("application/vnd.git-lfs"), however this

  1. Doesn't apply to other clients, at least git-lfs-java appends headers instead of replacing them
  2. Forces client to violate spec and send unexpected Accept header

@slonopotamus
Copy link
Contributor Author

I'm afraid I need someone from dev team to help with this, my Go skills are almost none.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 15, 2019
@lafriks
Copy link
Member

lafriks commented May 15, 2019

Seems that tests fail after this change

@techknowlogick
Copy link
Member

Looking through the logs of CI it seems that the git client is having an error with new header, so perhaps even if we aren't using spec, then neither is git cli.

@slonopotamus
Copy link
Contributor Author

I'm not quite sure what's going on here.

https://github.com/git-lfs/git-lfs/blob/master/lfshttp/client.go#L140 says git-lfs is using "application/vnd.git-lfs+json" =/

@slonopotamus
Copy link
Contributor Author

slonopotamus commented May 15, 2019

Where does "application/vnd.git-lfs" that you expect come from at all? There is no such thing neither in git-lfs nor in LFS specification.

@slonopotamus
Copy link
Contributor Author

https://github.com/slonopotamus/gitea/blob/patch-1/modules/lfs/server.go#L388

this is the problem. you're telling client "hey, client, when you go to /verify, use "Accepts: application/vnd.git-lfs". BUUUT. LFS spec tells client to use "Accept: application/vnd.git-lfs+json". For native git-lfs client, your header overwrites header from spec. But this doesn't happen for java client I am using. Instead, it ends up having two Accept headers and you're only checking the first one.

@slonopotamus slonopotamus force-pushed the patch-1 branch 2 times, most recently from f8a642b to f85b0f4 Compare May 15, 2019 15:52
slonopotamus added a commit to slonopotamus/git-lfs-java that referenced this pull request May 15, 2019
…-lfs behavior

See go-gitea/gitea#6958

While I believe this is supposed to be fixed on Gitea side, we can make changes
in order to become compatible with older Gitea releases.
According to [spec](https://github.com/git-lfs/git-lfs/blob/master/docs/api/basic-transfers.md#verification),
/verify requests must have "Accept: application/vnd.git-lfs+json"

Previous code worked just because native `git-lfs` implementation *replaced* Accept header that is required by
spec with value given by Gitea ("application/vnd.git-lfs"), however this

1. Doesn't apply to other clients, at least `git-lfs-java` *appends* headers instead of replacing them
2. Forces client to violate spec and send unexpected Accept header
@slonopotamus
Copy link
Contributor Author

Aaargh. I'll close this for now and open a bug instead.

@slonopotamus
Copy link
Contributor Author

I've reported this problem in #6960.

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants