Skip to content

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jul 17, 2025

I think this should correctly fix the issue with the uploads creating empty files described in #5299.

On #5292, we changed the Location header to an empty string because there was a double slash in the path. However, that was because we were concatenating /api/path/ (with slash) with r.URL.Path which is prefixed with the slash.

@hacdias hacdias requested a review from o1egl as a code owner July 17, 2025 06:55
@allixx
Copy link

allixx commented Jul 17, 2025

Maybe url joining should be done in more robust way instead of naive string concatenation?

Like

package main

import (
	"fmt"
	"net/url"
	"os"
)

func main() {
	u, err := url.JoinPath("/api/tus", "bar/")
	if err != nil {
		os.Exit(1)
	}
	fmt.Println(u)
}

This will give correct results regardless of leading slash presence or absence in joining value.

@hacdias
Copy link
Member Author

hacdias commented Jul 17, 2025

That's true, but I don't see the need since r.URL.Path is guaranteed to be the same as the URL, stripped of the /api/tus, since that's the path matcher for this HTTP route.

@@ -147,8 +147,7 @@ func tusPostHandler() handleFunc {
// Enables the user to utilize the PATCH endpoint for uploading file data
registerUpload(file.RealPath(), uploadLength)

// Signal the frontend to reuse the current request URL
w.Header().Set("Location", "")
w.Header().Set("Location", "/api/tus"+r.URL.Path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
w.Header().Set("Location", "/api/tus"+r.URL.Path)
w.Header().Set("Location", d.server.BaseURL+"/api/tus"+r.URL.Path)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a need to add the server URL? Does the JS library not resolve the absolute URL in relation to the root URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

The frontend will resolve the URL by joining origin (http://localhost:8080) with the location value (/baseurl/api/tus/file.test).

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely right, thanks!

@hacdias hacdias merged commit 607f570 into master Jul 17, 2025
5 checks passed
@hacdias hacdias deleted the fix/location-header branch July 17, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants