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

google-cloud-storage: Path segment of signed URLs not correct for objects starting with a forward slash. #38

Closed
houglum opened this issue Dec 13, 2019 · 3 comments
Assignees
Labels
api: storage Issues related to the googleapis/java-storage API. duplicate This issue or pull request already exists 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@houglum
Copy link
Contributor

houglum commented Dec 13, 2019

google-cloud-storage v0.120

The storage library doesn't create signed URLs correctly for objects starting with a forward slash. (I kept this behavior in a recent refactor I did, but recently found that I should have fixed it instead.) See this block of code:
https://github.com/googleapis/google-cloud-java/blob/afe98d2fb3535d5236b4d6377a5026a1977a9ce3/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java#L729

where the logic also existed before it was shuffled around in a recent refactor:
https://github.com/googleapis/google-cloud-java/blob/6de998cde1ab542d95af740e310ae58cb7c317a0/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java#L666

All slashes should be preserved; the library currently checks for one and removes it, although I can't determine why. I saw the original issue (See googleapis/google-cloud-java#1008 and googleapis/google-cloud-java#1006), but the comments there were very vague and didn't seem to outline an example URL for which this was valid behavior. I'm guessing it made sense for the way the library was written at the time, but today, it prevents users from correctly forming a signed URL for objects whose names start with a forward slash.


Current (wrong) behavior:

The object "/foo" in the bucket "bucket" ends up having a signed URL that starts with one of these strings:

https://storage.googleapis.com/bucket/foo
https://bucket.storage.googleapis.com/foo

Correct behavior:

The object "/foo" in the bucket "bucket" should have a signed URL that starts with one of these strings (notice the preserved forward slashes in the resource name):

https://storage.googleapis.com/bucket//foo
https://bucket.storage.googleapis.com//foo
@chingor13 chingor13 transferred this issue from googleapis/google-cloud-java Jan 7, 2020
@chingor13 chingor13 added the type: question Request for information or clarification. Not an issue. label Jan 7, 2020
@dmitry-fa dmitry-fa self-assigned this Jan 28, 2020
@dmitry-fa dmitry-fa added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Jan 28, 2020
@dmitry-fa
Copy link
Contributor

I think this is a bug in java code. Signing URLs with gsutil works fine:

#> gsutil cat gs://bucket_generation_signed/some.txt
Si sapis, sis apis

#> gsutil cat gs://bucket_generation_signed//some.txt
Qui ventum seminat, turbinem metet.

#> gsutil signurl -d 10m my.json gs://bucket_generation_signed/some.txt
URL    HTTP Method    Expiration    Signed URL
gs://bucket_generation_signed/some.txt    GET    2020-01-28 15:52:17    https://storage.googleapis.com/bucket_generation_signed/some.txt?x-goog-signature=...

#> gsutil signurl -d 10m my.json gs://bucket_generation_signed//some.txt
URL    HTTP Method    Expiration    Signed URL
gs://bucket_generation_signed//some.txt    GET    2020-01-28 15:52:07    https://storage.googleapis.com/bucket_generation_signed//some.txt?x-goog-signature=...

Signing URLs with java doesn't work as expected:

        String bucketName = "bucket_generation_signed";
        String blobName = "some.txt";

        Blob blob1 = storage.get(BlobId.of(bucketName, blobName));
        URL url1 = blob1.signUrl(20, TimeUnit.MINUTES, Storage.SignUrlOption.withV4Signature());

        Blob blob2 = storage.get(BlobId.of(bucketName, "/" + blobName));
        URL url2 = blob2.signUrl(20, TimeUnit.MINUTES, Storage.SignUrlOption.withV4Signature());

        System.out.println(blob1.getName() + ":  " + url1);
        System.out.println(blob2.getName() + ": " + url2);

output:

some.txt:  https://storage.googleapis.com/bucket_generation_signed/some.txt?X-Goog-Algorithm=...
/some.txt: https://storage.googleapis.com/bucket_generation_signed/some.txt?X-Goog-Algorithm=...

@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Jan 28, 2020
@frankyn
Copy link
Member

frankyn commented Jan 29, 2020

Deduping against inconsistency FR (#84) that's prioritized right now.

Storage Conformance tests now include a test for this case.

@frankyn frankyn closed this as completed Jan 29, 2020
@frankyn frankyn added duplicate This issue or pull request already exists and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Jan 29, 2020
@dmitry-fa
Copy link
Contributor

@frankyn FR (#84) sounds good and storage conformance tests are extremely good. One note: the current problem is reproducible with both V2 and V4, when #84 states only about V4.

@google-cloud-label-sync google-cloud-label-sync bot added the api: storage Issues related to the googleapis/java-storage API. label Jan 31, 2020
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. duplicate This issue or pull request already exists 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants