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

doc(storage): updated the ContentType documentation for the signedUrl in the storage package #11306

Closed
wants to merge 3 commits into from
Closed

doc(storage): updated the ContentType documentation for the signedUrl in the storage package #11306

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffbcbfb
fix(storage): updated the ContentType documentation for the signedUrl…
blagoySimandov Dec 18, 2024
7d56e5e
Merge branch 'main' into change-signedurl-docs
blagoySimandov Dec 18, 2024
15ca8e9
Merge branch 'main' into change-signedurl-docs
blagoySimandov Dec 18, 2024
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
2 changes: 1 addition & 1 deletion storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ type SignedURLOptions struct {

// ContentType is the content type header the client must provide
// to use the generated signed URL.
// Optional.
// Required.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not really accurate. It's valid to generate a URL without a Content-Type header (e.g. for a GET request), so the field is definitely not required in SignedURLOptions.

Moreover from the docs (https://cloud.google.com/storage/docs/authentication/canonical-requests#about-headers) it sounds like content-type is not a required canonical header, so you should be able to add it to the request after signing.

Maybe there is a server side issue here? Let me know if you think I'm mistaken, or you can file an issue with support to share details about the error.

Copy link
Author

Choose a reason for hiding this comment

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

I just believe it's misleading... I imagined that omitting the ContentType would make it so it accepts any kind of content type, this in fact was not the case and the only accepted content type was that of an empty string... Not to mention the fact that in a client-side/browser setting if you omit the content-type from the header the browser will automatically put in a "guess" for the content-type of the file that you are uploading, which would then be rejected since the only accepted content type would be ""... Results in a really nasty bug..

Copy link
Author

Choose a reason for hiding this comment

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

Here is something i wrote quite a while ago that might help you understand my frustration. blog

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so I investigated this further and it looks like you are using v2 signing, while the docs I linked to are for v4.

We recommend v4 signing generally and it does something more like what you want (at least according to the docs). Can you try that out by setting https://pkg.go.dev/cloud.google.com/go/storage#SignedURLOptions.Scheme ?

I think we could improve the docs here for v2, but I would definitely not want to mark this field as required in any case since it is not required for most signed URLs.

ContentType string

// Headers is a list of extension headers the client must provide
Expand Down