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

Send commit object in http response header for archives #17923

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
51 changes: 51 additions & 0 deletions integrations/repo_download_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

// based on repo_commits_test.go

package integrations

import (
"net/http"
"path"
"testing"
"encoding/base64"

"github.com/stretchr/testify/assert"
// gitea/integrations/integration_test.go -> NewRequest
)

// gitea/routers/web/repo/repo.go: addCommitObjectResponseHeader
func TestRepoDownloadWithCommitObject(t *testing.T) {
defer prepareTestEnv(t)()

// Request repository commits page
req := NewRequest(t, "GET", "/user2/repo1/commits/branch/master")
resp := MakeRequest(t, req, http.StatusOK)

doc := NewHTMLParser(t, resp.Body)
// Get first commit URL
commitURL, exists := doc.doc.Find("#commits-table tbody tr td.sha a").Attr("href")
assert.True(t, exists)
assert.NotEmpty(t, commitURL)

commitHash := path.Base(commitURL)

q2 := NewRequest(t, "GET", "/user2/repo1/archive/"+commitHash+".tar.gz")
assert.NotNil(t, q2)
q2.Header.Set("X-Commit-Object", "1") // request the commit object
r2 := MakeRequest(t, q2, http.StatusOK)
// decode the commit object
str64 := r2.Header().Get("X-Commit-Object")
bytes, err := base64.StdEncoding.DecodeString(str64)
assert.NoError(t, err)
str := string(bytes)
strExpected := (
"tree 2a2f1d4670728a2e10049e345bd7a276468beab6\n" +
"author user1 <address1@example.com> 1489956479 -0400\n" +
"committer Ethan Koenig <ethantkoenig@gmail.com> 1489956479 -0400\n" +
"\n" +
"Initial commit\n")
assert.Equal(t, str, strExpected)
}
11 changes: 11 additions & 0 deletions modules/git/repo_commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) {
return commits[0], nil
}

// GetCommitObject returns the raw commit object.
func (repo *Repository) GetCommitObject(commitID string) ([]byte, error) {
// we need a bit-exact copy of the commit object for hash calculation
// -p = print
stdout, err := NewCommandContext(repo.Ctx, "cat-file", "-p", commitID).RunInDirBytes(repo.Path)
if err != nil {
return nil, err
}
return stdout, nil
}

func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, error) {
stdout, err := NewCommandContext(repo.Ctx, "log", id.String(), "--skip="+strconv.Itoa((page-1)*pageSize),
"--max-count="+strconv.Itoa(pageSize), prettyLogFormat).RunInDirBytes(repo.Path)
Expand Down
23 changes: 23 additions & 0 deletions routers/web/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"
"strings"
"time"
"encoding/base64"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/db"
Expand Down Expand Up @@ -443,6 +444,7 @@ func download(ctx *context.Context, archiveName string, archiver *repo_model.Rep
//If we have a signed url (S3, object storage), redirect to this directly.
u, err := storage.RepoArchives.URL(rPath, downloadName)
if u != nil && err == nil {
addCommitObjectResponseHeader(ctx, archiver)
milahu marked this conversation as resolved.
Show resolved Hide resolved
ctx.Redirect(u.String())
return
}
Expand All @@ -455,9 +457,30 @@ func download(ctx *context.Context, archiveName string, archiver *repo_model.Rep
return
}
defer fr.Close()
addCommitObjectResponseHeader(ctx, archiver)
ctx.ServeStream(fr, downloadName)
}

// Add X-Commit-Object response header if it was requested.
// The commit object is needed to verify a downloaded archive by commit hash.
// We use base64 to ensure a lossless encoding of binary data.
// test: gitea/integrations/repo_download_test.go
func addCommitObjectResponseHeader(ctx *context.Context, archiver *repo_model.RepoArchiver) {
if ctx.Req.Header.Get("X-Commit-Object") != "1" {
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 7, 2021

Choose a reason for hiding this comment

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

Using X-Commit-Object for different purpose in request and response is not a good idea.

HTTP has Content-Length in request and response, it has the same meaning.

But now the X-Commit-Object is used for different meaning. I think we can make it like this:

request:

X-Request-Extra-Info: commit-id

response:

X-Extra-Commit-ID: .....

If we want to add more options, we can extend: X-Request-Extra-Info: commit-message, commit-author ...

Copy link
Author

Choose a reason for hiding this comment

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

is not a good idea

why not?

If we want to add more options

then we can add more headers

keep it simple : )

Copy link
Contributor

Choose a reason for hiding this comment

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

why not?

As explained, the X-Commit-Object has different meanings, will mislead users and developers.

keep it simple : )

I don't think X-Request-Extra-Info + X-Extra-Commit-ID is more complex than X-Commit-Object + X-Commit-Object.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for your opinion, but youre just wasting my time. bye : )

Copy link
Author

Choose a reason for hiding this comment

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

unresolve

its interesting that bad taste and hostage-taking so often appear together.
probably both are facets of the same personality type (conservative)

in the real world, i would kill the tyrant, and install someone who is "less bad".
in the digital world? lets just pretend this never happened ... "muh ideals!"

Copy link
Contributor

@wxiaoguang wxiaoguang Dec 10, 2021

Choose a reason for hiding this comment

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

unresolve

its interesting that bad taste and hostage-taking so often appear together.
probably both are facets of the same personality type (conservative)

in the real world, i would kill the tyrant, and install someone who is "less bad".
in the digital world? lets just pretend this never happened ... "muh ideals!"

Are you talking about yourself?

For me, I feel fine. I submitted some PRs, approved many PRs and helped some contributors to improve their PRs, like #17152, #17900, #17929, etc.

If you don't like my suggestions, that's your choice. I won't waste time on non-sense communications.

We can keep this review open and let everyone read it.

return
}
gitrepo := ctx.Repo.GitRepo
bytes, err := gitrepo.GetCommitObject(archiver.CommitID)
if err != nil {
// CommitID should be valid here, but still, errors can happen
ctx.Resp.Header().Set("X-Commit-Object", "")
ctx.Resp.Header().Set("X-Commit-Object-Error", "1")
return
}
str64 := base64.StdEncoding.EncodeToString(bytes)
ctx.Resp.Header().Set("X-Commit-Object", str64)
}

// InitiateDownload will enqueue an archival request, as needed. It may submit
// a request that's already in-progress, but the archiver service will just
// kind of drop it on the floor if this is the case.
Expand Down