Cloud Storage recently stopped handling the RFC 3986 URI encoding by it self about java-storage HOT 26 CLOSED

This was broken by the following commit:
googleapis/google-http-java-client@7d4a048

The problem disappeared when I changed GenericUrl.java as it was before:

result.add(verbatim ? sub : CharEscapers.decodeUriPath(sub));
=>
result.add(verbatim ? sub : CharEscapers.decodeUri(sub));

from java-storage.

I do confirm: it works with the libraries-bom 3.2.0, but no longer works with 3.3.0.
I will investigate the root couse.

from java-storage.

Workaround:

            <dependency>
                <groupId>com.google.http-client</groupId>
                <artifactId>google-http-client</artifactId>
                <version>1.33.0</version>
            </dependency>

from java-storage.

Thanks for the detailed report. I think you've nailed the root cause. We're going to have to dig into this one and figure out exactly how to fix it. The old http java client behavior was a common but incorrect handling of plus signs that caused other problems. Something in the GCS stack--I'm not sure what yet--was depending on the buggy behavior. If we're lucky, we can fix it here. If not, we might have to fix this somewhere on the server.

from java-storage.

googleapis/google-http-java-client#398

from java-storage.

Encoding spaces as "%20" not as "+" if met in the path component of URL should probably help.

from java-storage.

Yes, that's exactly what we should be doing. The question is what piece of code in which project is doing this encoding. It probably isn't in this repo.

from java-storage.

The easiest way to fix:

com.google.api.client.util.escape.PercentEscaper

private static final char[] URI_ESCAPED_SPACE = {'+'};
=>
private static final char[] URI_ESCAPED_SPACE = "%20".toCharArray();

It fixes problem with the storage. But it may have some side effects.

from java-storage.

Where do you propose making that change?

from java-storage.

I guess the source of the com.google.api.client.util.escape.PercentEscaper class exists only here:
https://github.com/googleapis/google-http-java-client/blob/master/google-http-client/src/main/java/com/google/api/client/util/escape/PercentEscaper.java

from java-storage.

I don't think we're going to change http-java-client. 1.33.0 had a bug here and 1.34.0 fixed it. The problem is some other code somewhere has a complementary bug.

from java-storage.

We should dig into the actual HTTP calls that are happening here to understand what the server is sending us. That might narrow this down. See

https://cloud.google.com/storage/docs/xml-api/overview

and

https://cloud.google.com/storage/docs/json_api/

I'm not sure which of these two the client library is wrap[ping.

from java-storage.

https://cloud.google.com/storage/docs/request-endpoints#encoding

from java-storage.

Per the GCS API docs %20 is correct and + is wrong. This is consistent with the HTTP spec as well. Something is not following the docs, and I now think it might actually be in this repo. let me look

from java-storage.

As I can see, the storage passes the names of bucket and object to the http client as is. The client is responsible for URL encoding/decoding. It encodes spaces as '+', but since 1.33.0 '+' is no longer decoded as space. Encoding as '%20' allows to decode it properly.

from java-storage.

The client should not encode spaces as +. Just need to figure out exactly where that happens so I can fix it.

from java-storage.

This might be it:

https://github.com/googleapis/java-storage/blob/master/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java#L669

from java-storage.

This works only to obtain a signed URL

from java-storage.

This is stack trace from the point where the storage parameters (bucket name, object name) are starting to be encoded:

	  at com.google.api.client.http.UriTemplate$CompositeOutput.getEncodedValue(UriTemplate.java:160)
	  at com.google.api.client.http.UriTemplate.getSimpleValue(UriTemplate.java:330)
	  at com.google.api.client.http.UriTemplate.expand(UriTemplate.java:314)
	  at com.google.api.client.http.UriTemplate.expand(UriTemplate.java:229)
	  at com.google.api.services.storage.Storage$Objects$Get.buildHttpRequestUrl(Storage.java:7014)
	  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.buildHttpRequest(AbstractGoogleClientRequest.java:422)
	  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:541)
	  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:474)
	  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeMedia(AbstractGoogleClientRequest.java:502)
	  at com.google.api.services.storage.Storage$Objects$Get.executeMedia(Storage.java:7006)
	  at com.google.cloud.storage.spi.v1.HttpStorageRpc.load(HttpStorageRpc.java:631)
	  at com.google.cloud.storage.StorageImpl$16.call(StorageImpl.java:589)
	  at com.google.cloud.storage.StorageImpl$16.call(StorageImpl.java:586)
	  at com.google.api.gax.retrying.DirectRetryingExecutor.submit(DirectRetryingExecutor.java:105)
	  at com.google.cloud.RetryHelper.run(RetryHelper.java:76)
	  at com.google.cloud.RetryHelper.runWithRetries(RetryHelper.java:50)
	  at com.google.cloud.storage.StorageImpl.readAllBytes(StorageImpl.java:585)
	  at com.google.cloud.storage.StorageImpl.readAllBytes(StorageImpl.java:577)
	  at storage.Generic.main(Generic.java:36)

from java-storage.

This is quite a huge bug, preventing anyone from reading files containing spaces in their name, which is quite frequent.

Apparently, it exists in the latest version 1.103.0, but not in version 1.102.0. Wouldn't it be possible to release a new version identical to 1.102.0 for those upgrading to the latest version?

I had the luck to have caught this issue by a test before going to production, but I guess this could break a lot of applications.

from java-storage.

This might be it:

https://github.com/googleapis/java-storage/blob/master/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java#L669

This only applies to signing objects. The code path is not used when reading objects.

from java-storage.

This was broken by the following commit:
googleapis/google-http-java-client@7d4a048

The problem disappeared when I changed GenericUrl.java as it was before:

result.add(verbatim ? sub : CharEscapers.decodeUriPath(sub));
=>
result.add(verbatim ? sub : CharEscapers.decodeUri(sub));

I can confirm this finding.

from java-storage.

@elharo Have you considered update com.google.api.client.util.escape.PercentEscaper

private static final char[] URI_ESCAPED_SPACE = {'+'};
=>
private static final char[] URI_ESCAPED_SPACE = "%20".toCharArray();

from java-storage.

closing this issue as its duplicate of #121.

from java-storage.

@athakor I don't know why you choose to close this old bug and keep the new one which it seems has less priority as well, but any way we are still waiting for a fix, we can just use the work around and make sure google-http-client stays with none problematic version, but we use many Google client java libraries, not sure we want to take the risk of updating them but keeping one of their transitive dependencies.

from java-storage.

@montss this issue has been fixed. wait until next version of google-http-client.

from java-storage.