Skip to content
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

Removed some vestigial code related to Range bounds checks #20312

Merged
merged 6 commits into from Jul 28, 2022

Conversation

philip-peterson
Copy link
Contributor

@philip-peterson philip-peterson commented Jul 11, 2022

When an LFS pointer file has a size that is too large (compared to the actual blob size), the Git LFS client can make Range requests that are out of bounds. Currently Gitea tries to obey these requests but gets an EOF during the copy and still responds with a 200, so this PR adds a bounds check to error early if that would be about to happen.

Also removed some vestigial code related to the bounds checks.

Update: see comment below

Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Is there a test case where this issue can be reproduced(and ideally can be included into Gitea integration tests.

services/lfs/server.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 11, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Jul 11, 2022

I don't understand the problem. The initial toByte = meta.Size - 1 (meta.Size is the content size, there should be no need to stat it). If the client provides a range, toByte gets capped to this size. How can it be out of bounds?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 11, 2022

It comes from user's HTTP header. https://github.com/go-gitea/gitea/pull/20312/files#diff-95b595c61d15024441878562cf12099f39f68773a995de51ff021f7258b5a695R92-L114

While I think the check should be moved above, otherwise the response header (https://github.com/go-gitea/gitea/pull/20312/files#diff-95b595c61d15024441878562cf12099f39f68773a995de51ff021f7258b5a695R111) is still incorrect.


Update: hmm ... the toByte was already checked above. 😂

Maybe the problem is that the size in meta doesn't match the real file size?

@KN4CK3R
Copy link
Member

KN4CK3R commented Jul 11, 2022

The meta size is checked against the written size, must be correct.

if r.currentSize != r.expectedSize {
return n, ErrSizeMismatch
}

@philip-peterson
Copy link
Contributor Author

Yes, I will have to check to see if this is a problem in latest Gitea. The hashingReader code looks like it might address it, I was working based off an older version.

@philip-peterson philip-peterson changed the title Add out-of-bounds check for upper end of range in LFS request Removed some vestigial code related to Range bounds checks Jul 25, 2022
@philip-peterson
Copy link
Contributor Author

I was able to verify that this issue has been fixed in latest Gitea. Although, there is still some dead code, so I changed this PR to be just about removing the dead code.

Thanks for the help, all.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 25, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 25, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@ae52df6). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #20312   +/-   ##
=======================================
  Coverage        ?   46.94%           
=======================================
  Files           ?      977           
  Lines           ?   135303           
  Branches        ?        0           
=======================================
  Hits            ?    63519           
  Misses          ?    64001           
  Partials        ?     7783           
Impacted Files Coverage Δ
modules/lfs/content_store.go 74.66% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

@wxiaoguang wxiaoguang merged commit 4604048 into go-gitea:main Jul 28, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 28, 2022
* giteaofficial/main:
  Removed some vestigial code related to Range bounds checks (go-gitea#20312)
  Add markdownlint (go-gitea#20512)
  Fix possible panic when repository is empty (go-gitea#20509)
  patch (doc): add heading to ssh flow explanation (go-gitea#20506)
  Show hint to link package to repo when viewing empty repo package list (go-gitea#20504)
  Fix ROOT_URL detection for URLs without trailing slash (go-gitea#20502)
  Add Tar ZSTD support (go-gitea#20493)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants