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.

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

Jump to bottom

Fix support resumable + fix incorrect handling of paths which start with a / #918

Merged
merged 15 commits into from
Sep 19, 2022
Merged

Fix support resumable + fix incorrect handling of paths which start with a / #918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0b0b083
update support
le0pard Sep 12, 2022
85740a1
update support
le0pard Sep 12, 2022
be16d56
update support
le0pard Sep 12, 2022
434ddfe
fix upload
le0pard Sep 12, 2022
0e5e65f
fix routes order
le0pard Sep 12, 2022
41e8db7
Fix normalizeFileName
le0pard Sep 13, 2022
c62771b
Fix escaping for mux vars
le0pard Sep 13, 2022
2792636
Fix escaping for mux vars
le0pard Sep 13, 2022
6412da7
Fix escaping for mux vars
le0pard Sep 13, 2022
a5991c7
fix check and tests
le0pard Sep 13, 2022
07a0d59
move unescapeMuxVars to server
le0pard Sep 14, 2022
71bb72e
add new test cases
le0pard Sep 19, 2022
8695101
add new test cases
le0pard Sep 19, 2022
d18156b
Update fakestorage/upload.go
le0pard Sep 19, 2022
ed3cf5d
remove bucket escaping
le0pard Sep 19, 2022
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
4 changes: 2 additions & 2 deletions fakestorage/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (s *Server) listBuckets(r *http.Request) jsonResponse {
}

func (s *Server) getBucket(r *http.Request) jsonResponse {
bucketName := mux.Vars(r)["bucketName"]
bucketName := unescapeMuxVars(mux.Vars(r))["bucketName"]
bucket, err := s.backend.GetBucket(bucketName)
if err != nil {
return jsonResponse{status: http.StatusNotFound}
Expand All @@ -114,7 +114,7 @@ func (s *Server) getBucket(r *http.Request) jsonResponse {
}

func (s *Server) deleteBucket(r *http.Request) jsonResponse {
bucketName := mux.Vars(r)["bucketName"]
bucketName := unescapeMuxVars(mux.Vars(r))["bucketName"]
err := s.backend.DeleteBucket(bucketName)
if err == backend.BucketNotFound {
return jsonResponse{status: http.StatusNotFound}
Expand Down
20 changes: 10 additions & 10 deletions fakestorage/object.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ func (s *Server) objectWithGenerationOnValidGeneration(bucketName, objectName, g
}

func (s *Server) listObjects(r *http.Request) jsonResponse {
bucketName := mux.Vars(r)["bucketName"]
bucketName := unescapeMuxVars(mux.Vars(r))["bucketName"]
objs, prefixes, err := s.ListObjectsWithOptions(bucketName, ListOptions{
Prefix: r.URL.Query().Get("prefix"),
Delimiter: r.URL.Query().Get("delimiter"),
Expand All @@ -587,7 +587,7 @@ func (s *Server) getObject(w http.ResponseWriter, r *http.Request) {
}

handler := jsonToHTTPHandler(func(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))

obj, err := s.objectWithGenerationOnValidGeneration(vars["bucketName"], vars["objectName"], r.FormValue("generation"))
// Calling Close before checking err is okay on objects, and the object
Expand Down Expand Up @@ -617,7 +617,7 @@ func (s *Server) getObject(w http.ResponseWriter, r *http.Request) {
}

func (s *Server) deleteObject(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
obj, err := s.GetObjectStreaming(vars["bucketName"], vars["objectName"])
// Calling Close before checking err is okay on objects, and the object
// may need to be closed whether or not there's an error.
Expand All @@ -639,7 +639,7 @@ func (s *Server) deleteObject(r *http.Request) jsonResponse {
}

func (s *Server) listObjectACL(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))

obj, err := s.GetObjectStreaming(vars["bucketName"], vars["objectName"])
if err != nil {
Expand All @@ -651,7 +651,7 @@ func (s *Server) listObjectACL(r *http.Request) jsonResponse {
}

func (s *Server) setObjectACL(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))

obj, err := s.GetObjectStreaming(vars["bucketName"], vars["objectName"])
if err != nil {
Expand Down Expand Up @@ -689,7 +689,7 @@ func (s *Server) setObjectACL(r *http.Request) jsonResponse {
}

func (s *Server) rewriteObject(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
obj, err := s.objectWithGenerationOnValidGeneration(vars["sourceBucket"], vars["sourceObject"], r.FormValue("sourceGeneration"))
// Calling Close before checking err is okay on objects, and the object
// may need to be closed whether or not there's an error.
Expand Down Expand Up @@ -744,7 +744,7 @@ func (s *Server) rewriteObject(r *http.Request) jsonResponse {
}

func (s *Server) downloadObject(w http.ResponseWriter, r *http.Request) {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
obj, err := s.objectWithGenerationOnValidGeneration(vars["bucketName"], vars["objectName"], r.FormValue("generation"))
// Calling Close before checking err is okay on objects, and the object
// may need to be closed whether or not there's an error.
Expand Down Expand Up @@ -901,7 +901,7 @@ func parseRange(rangeHeaderValue string, contentLength int64) (start int64, end
}

func (s *Server) patchObject(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
bucketName := vars["bucketName"]
objectName := vars["objectName"]
var metadata struct {
Expand All @@ -928,7 +928,7 @@ func (s *Server) patchObject(r *http.Request) jsonResponse {
}

func (s *Server) updateObject(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
bucketName := vars["bucketName"]
objectName := vars["objectName"]
var metadata struct {
Expand All @@ -955,7 +955,7 @@ func (s *Server) updateObject(r *http.Request) jsonResponse {
}

func (s *Server) composeObject(r *http.Request) jsonResponse {
vars := mux.Vars(r)
vars := unescapeMuxVars(mux.Vars(r))
bucketName := vars["bucketName"]
destinationObject := vars["destinationObject"]

Expand Down
17 changes: 16 additions & 1 deletion fakestorage/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"net/http/httptest"
"net/http/httputil"
"net/textproto"
"net/url"
"strings"
"sync"

Expand Down Expand Up @@ -211,9 +212,22 @@ func newServer(options Options) (*Server, error) {
return &s, nil
}

func unescapeMuxVars(vars map[string]string) map[string]string {
Copy link
Contributor Author

@le0pard le0pard Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In some places we use r.URL.Query(), which provide already decoded values. But mux.Vars(r) provide values "as is" (without decoding). That is why we need decode objectName and bucketName ourself. This is important, because before accessing to storage we encode values and without this this lead to double encoding.

m := make(map[string]string)
for k, v := range vars {
r, err := url.PathUnescape(v)
if err == nil {
m[k] = r
} else {
m[k] = v
}
}
return m
}

func (s *Server) buildMuxer() {
const apiPrefix = "/storage/v1"
s.mux = mux.NewRouter()
s.mux = mux.NewRouter().SkipClean(true).UseEncodedPath()
Copy link
Contributor Author

@le0pard le0pard Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Reason for this.

UseEncodedPath tells the router to match the encoded original path to the routes. For eg. "/path/foo%2Fbar/to" will match the path "/path/{var}/to".

This fix issue, that if we use names like /file.txt, in this case it will be /download/storage/v1/b/{bucketName}/o/%2Ffile.txt in url path, which mux trying to fix by redirect to /download/storage/v1/b/{bucketName}/o/file.txt, which is incorrect behaviour for GCP

SkipClean - When true, if the route path is "/path//to", it will remain with the double slash. This is helpful if you have a route like: /fetch/http://xkcd.com/534/

We do not fix urls, what defined by sdk or users


// healthcheck
s.mux.Path("/_internal/healthcheck").Methods(http.MethodGet).HandlerFunc(s.healthcheck)
Expand Down Expand Up @@ -251,6 +265,7 @@ func (s *Server) buildMuxer() {
s.mux.Host(bucketHost).Path("/{objectName:.+}").Methods(http.MethodGet, http.MethodHead).HandlerFunc(s.downloadObject)
s.mux.Path("/download/storage/v1/b/{bucketName}/o/{objectName:.+}").Methods(http.MethodGet).HandlerFunc(s.downloadObject)
s.mux.Path("/upload/storage/v1/b/{bucketName}/o").Methods(http.MethodPost).HandlerFunc(jsonToHTTPHandler(s.insertObject))
s.mux.Path("/upload/storage/v1/b/{bucketName}/o").Methods(http.MethodPut).HandlerFunc(jsonToHTTPHandler(s.uploadFileContent))
s.mux.Path("/upload/resumable/{uploadId}").Methods(http.MethodPut, http.MethodPost).HandlerFunc(jsonToHTTPHandler(s.uploadFileContent))

// Batch endpoint
Expand Down
23 changes: 17 additions & 6 deletions fakestorage/upload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"mime"
"mime/multipart"
"net/http"
"net/url"
"strconv"
"strings"

Expand Down Expand Up @@ -46,7 +47,7 @@ type contentRange struct {
}

func (s *Server) insertObject(r *http.Request) jsonResponse {
bucketName := mux.Vars(r)["bucketName"]
bucketName := unescapeMuxVars(mux.Vars(r))["bucketName"]

if _, err := s.backend.GetBucket(bucketName); err != nil {
return jsonResponse{status: http.StatusNotFound}
Expand Down Expand Up @@ -78,7 +79,7 @@ func (s *Server) insertObject(r *http.Request) jsonResponse {
}

func (s *Server) insertFormObject(r *http.Request) xmlResponse {
bucketName := mux.Vars(r)["bucketName"]
bucketName := unescapeMuxVars(mux.Vars(r))["bucketName"]

if err := r.ParseMultipartForm(32 << 20); nil != err {
return xmlResponse{errorMessage: "invalid form", status: http.StatusBadRequest}
Expand Down Expand Up @@ -242,7 +243,7 @@ func (s notImplementedSeeker) Seek(offset int64, whence int) (int64, error) {

func (s *Server) signedUpload(bucketName string, r *http.Request) jsonResponse {
defer r.Body.Close()
name := mux.Vars(r)["objectName"]
name := unescapeMuxVars(mux.Vars(r))["objectName"]
predefinedACL := r.URL.Query().Get("predefinedAcl")
contentEncoding := r.URL.Query().Get("contentEncoding")

Expand Down Expand Up @@ -362,6 +363,9 @@ func (s *Server) multipartUpload(bucketName string, r *http.Request) jsonRespons
}

func (s *Server) resumableUpload(bucketName string, r *http.Request) jsonResponse {
if r.URL.Query().Has("upload_id") {
return s.uploadFileContent(r)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if upload_id came - it mean we have request to upload file.

}
predefinedACL := r.URL.Query().Get("predefinedAcl")
contentEncoding := r.URL.Query().Get("contentEncoding")
metadata := new(multipartMetadata)
Expand Down Expand Up @@ -391,9 +395,16 @@ func (s *Server) resumableUpload(bucketName string, r *http.Request) jsonRespons
}
s.uploads.Store(uploadID, obj)
header := make(http.Header)
header.Set("Location", s.URL()+"/upload/resumable/"+uploadID)
location := fmt.Sprintf(
"%s/upload/storage/v1/b/%s/o?uploadType=resumable&name=%s&upload_id=%s",
s.URL(),
bucketName,
url.PathEscape(objName),
uploadID,
)
header.Set("Location", location)
if r.Header.Get("X-Goog-Upload-Command") == "start" {
header.Set("X-Goog-Upload-URL", s.URL()+"/upload/resumable/"+uploadID)
header.Set("X-Goog-Upload-URL", location)
header.Set("X-Goog-Upload-Status", "active")
}
return jsonResponse{
Expand Down Expand Up @@ -438,7 +449,7 @@ func (s *Server) resumableUpload(bucketName string, r *http.Request) jsonRespons
// then has a status of "200 OK", with a header "X-Http-Status-Code-Override"
// set to "308".
func (s *Server) uploadFileContent(r *http.Request) jsonResponse {
uploadID := mux.Vars(r)["uploadId"]
uploadID := r.URL.Query().Get("upload_id")
rawObj, ok := s.uploads.Load(uploadID)
if !ok {
return jsonResponse{status: http.StatusNotFound}
Expand Down
16 changes: 14 additions & 2 deletions fakestorage/upload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ func TestServerClientObjectWriter(t *testing.T) {
"other/interesting/object.txt",
googleapi.MinUploadChunkSize,
},
{
"file with backslash at beginning",
"other-bucket",
"/some/other/object.txt",
googleapi.DefaultUploadChunkSize,
},
{
"file with backslashes at name",
"other-bucket",
"//some//other//file.txt",
googleapi.MinUploadChunkSize,
},
}

for _, test := range tests {
Expand Down Expand Up @@ -793,8 +805,8 @@ func TestServerGzippedUpload(t *testing.T) {
t.Fatal(err)
}

url := server.URL()
req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/upload/storage/v1/b/%s/o?name=testobj&uploadType=media", url, bucketName), &buf)
serverUrl := server.URL()
req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/upload/storage/v1/b/%s/o?name=testobj&uploadType=media", serverUrl, bucketName), &buf)
if err != nil {
t.Fatal(err)
}
Expand Down