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

Auth: Consider catching IllegalArgumentException in GoogleCredential.fromStream #2095

Closed
Capstan opened this issue Jun 18, 2022 · 4 comments
Closed
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release.

Comments

@Capstan
Copy link

Capstan commented Jun 18, 2022

https://github.com/googleapis/google-api-java-client/blob/main/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredential.java#L231

In #1661, Utils.getDefaultJsonFactory() was changed to use the GsonFactory. Gson, in part because of an underspecification of thrown exceptions googleapis/google-http-java-client#1353, will throw IllegalArgumentException instead of IOException when it gets invalid data (or even valid data that it cannot coerce into the Java class's members; e.g., 34.0 cannot be treated as an int).

Mutating the exception thrown by a JsonFactory is mildly problematic, but auth failures are such a crucial problem domain, having these types of errors escape their handlers seems more significant.

Thus, unless the JsonParser spec is updated and the GsonParser amended to abide by that spec, I would like to suggest a local workaround to reduce the likelihood of problems here by wrapping the call to parseAndClose and catch IllegalArgumentException, rethrowing it as an IOException.

Environment details

  1. API: Auth (or Core)
  2. OS type and version: 5.17.6-1rodete1-amd64
  3. Java version: OpenJDK Runtime Environment (build 11.0.13+8-google-release-451398016) OpenJDK 64-Bit Server VM (build 11.0.13+8-google-release-451398016, mixed mode, sharing)
  4. version(s): ~head (with local patches)

Steps to reproduce

  1. Create a bogus credential InputStream, e.g., from a string like "Invalid JSON"
  2. Call GoogleCredential.fromStream(malformedInputStream) on said stream

Stack trace

Caused by: java.lang.IllegalArgumentException: expected primitive class, but got: class com.google.api.client.json.GenericJson
	at com.google.api.client.util.Data.parsePrimitiveValue(Data.java:467)
	at com.google.api.client.json.JsonParser.parseValue(JsonParser.java:870)
	at com.google.api.client.json.JsonParser.parse(JsonParser.java:361)
	at com.google.api.client.json.JsonParser.parse(JsonParser.java:336)
	at com.google.api.client.json.JsonObjectParser.parseAndClose(JsonObjectParser.java:79)
	at com.google.api.client.json.JsonObjectParser.parseAndClose(JsonObjectParser.java:73)
	at com.google.api.client.googleapis.auth.oauth2.GoogleCredential.fromStream(GoogleCredential.java:250)
	at com.google.api.client.googleapis.auth.oauth2.GoogleCredential.fromStream(GoogleCredential.java:226)

External references such as API reference guides

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 18, 2022
@meltsufin
Copy link
Member

@TimurSadykov Is this something in your wheelhouse? Thanks!

@meltsufin meltsufin added priority: p2 Moderately-important priority. Fix may not be included in next release. triage me I really want to be triaged. and removed triage me I really want to be triaged. labels Jun 21, 2022
@yoshi-automation yoshi-automation removed the triage me I really want to be triaged. label Jun 21, 2022
@diegomarquezp
Copy link
Contributor

Seems like wrapping and rethrowing the exception should do. @TimurSadykov, are you able to take on this? Thanks!

@TimurSadykov
Copy link
Member

TimurSadykov commented Dec 13, 2022

The subject class is deprecated for some time, it is basically a duplicate functionality. Normally we won't fix improvements to it unless a security issue. However, we can review contributions.

@diegomarquezp diegomarquezp added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 19, 2022
@blakeli0
Copy link
Contributor

As @TimurSadykov mentioned, this class is deprecated for some time and we do not plan to fix it, closing as not planned.

@blakeli0 blakeli0 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release.
Projects
None yet
Development

No branches or pull requests

6 participants