Add support for reflection access filter #1905

Marcono1234 · 2021-06-04T23:00:41Z

Adds a ReflectionAccessFilter which allows preventing to use the reflection based type adapter by accident, instead throwing an exception. This is quite useful to avoid depending on Java platform implementation details.

A use case where this would help is stripe/stripe-java#1197, that pull request adds a check whether a returned adapter is com.google.gson.internal.bind.ReflectiveTypeAdapterFactory; the addition of a ReflectionAccessFilter would allow implementing this without having to rely on Gson internal implementation details.

In the future it would probably be good to apply some of these access filters by default since it appears users are often by accident depending on implementation details, or are not aware of the consequences of this. However, the way ReflectionAccessFilter is designed would still allow overwriting this by registering a filter which returns FilterResult.ALLOW for all classes, in case users for some reason want to serialize or deserialize Java platform classes using reflection.
Moshi by default prevents reflective access for platform classes, see internal Util.isPlatformType(Class) and its callers.

Marcono1234 · 2021-08-12T20:26:54Z

Alternatively FilterResult could be a regular class with a nullable String filterName and a FilterResultType type, where FilterResultType would be an enum with ALLOW, INDECISIVE, BLOCK_INACCESSIBLE and BLOCK_ALL.
Then a filter could optionally specify its name (for example "platform type filter") for the filter result. This could help with debugging and would allow including it in the message of the exception thrown when not allowed reflective access occurs.

But I am not sure if this is worth it. What do you think?

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java

eamonnmcmanus

Sorry for taking so long to get to this. I think this is an excellent addition. Accidental reflective access seems to be a major problem with Gson, and this will go some way towards mitigating that.

If you merge to HEAD I will try running this against Google's internal tests. I don't really expect any failures but we are sometimes surprised. Feel free to do just the merge initially and hold off on the documentation changes I suggested, until we know the results. Or go ahead and make them, as you prefer.

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java

gson/src/main/java/com/google/gson/GsonBuilder.java

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java

Marcono1234 · 2022-02-04T20:50:59Z

Have improved the documentation and fixed some issues with the previous implementation. I chose a slightly different wording for the sentences describing the standard platform packages. Please let me know if it is alright like this.

As mentioned in #1905 (comment), should this be extended to support describing which filter returned the final result? Or do you think that is not worth it?

gson/src/main/java/com/google/gson/GsonBuilder.java

eamonnmcmanus · 2022-02-04T21:54:24Z

gson/src/main/java/com/google/gson/GsonBuilder.java

@@ -101,6 +102,7 @@
  private boolean useJdkUnsafe = DEFAULT_USE_JDK_UNSAFE;
  private ToNumberStrategy objectToNumberStrategy = DEFAULT_OBJECT_TO_NUMBER_STRATEGY;
  private ToNumberStrategy numberToNumberStrategy = DEFAULT_NUMBER_TO_NUMBER_STRATEGY;
+  private final LinkedList<ReflectionAccessFilter> reflectionFilters = new LinkedList<ReflectionAccessFilter>();


We're on Java 7 now, right? So we should be able to use new LinkedList<>(), and similarly in other places in the PR.

Good point! I completely forgot that Java 7 added that feature already. Would you mind if I create a separate PR which replaces all redundant type arguments with the diamond operator, and include these changes there?

Seems fair.

Thanks! Have created #2104 for that.

eamonnmcmanus · 2022-02-05T15:55:09Z

Google's internal tests pass with this change, so we can merge it once the conflicts have been resolved and the remaining minor comments addressed.

For Java < 9, AccessibleObject.canAccess is not available and therefore checks might pass even if object is not accessible, causing IllegalAccessException later.

…/reflection-access-filter

Marcono1234 · 2022-02-05T18:09:16Z

Thanks for running this against Google's internal tests! Just to verify: You do not think that supporting specifying which filter returned a result (see #1905 (comment)) is worth it, correct?

I have added one commit improving IllegalAccessException handling. Currently when Gson users use Java < 9, but use FilterResult.BLOCK_INACCESSIBLE, they might run into IllegalAccessException because ReflectionAccessFilterHelper.canAccess(AccessibleObject, Object) won't validate whether the object is accessible. Do you think that is acceptable, or should it try to emulate the access check (but in a simplified way because package-private and protected checks are irrelevant because Gson is in a different package)?

Marcono1234 · 2022-04-16T23:58:54Z

@eamonnmcmanus, what do you think?

eamonnmcmanus

Sorry, I lost track of this.

eamonnmcmanus · 2022-04-17T16:02:39Z

gson/src/main/java/com/google/gson/GsonBuilder.java

@@ -101,6 +102,7 @@
  private boolean useJdkUnsafe = DEFAULT_USE_JDK_UNSAFE;
  private ToNumberStrategy objectToNumberStrategy = DEFAULT_OBJECT_TO_NUMBER_STRATEGY;
  private ToNumberStrategy numberToNumberStrategy = DEFAULT_NUMBER_TO_NUMBER_STRATEGY;
+  private final LinkedList<ReflectionAccessFilter> reflectionFilters = new LinkedList<ReflectionAccessFilter>();


Seems fair.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.code.gson:gson-parent](https://togithub.com/google/gson) | `2.9.0` -> `2.10.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/compatibility-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/confidence-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/) | | [com.google.code.gson:gson](https://togithub.com/google/gson) | `2.9.0` -> `2.10.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/compatibility-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/confidence-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### ⚠ Dependency Lookup Warnings ⚠ Warnings were logged while processing this repo. Please check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>google/gson</summary> ### [`v2.10`](https://togithub.com/google/gson/blob/HEAD/CHANGELOG.md#Version-210) - Support for serializing and deserializing Java records, on Java ≥ 16. ([google/gson#2201) - Add `JsonArray.asList` and `JsonObject.asMap` view methods ([google/gson#2225) - Fix `TypeAdapterRuntimeTypeWrapper` not detecting reflective `TreeTypeAdapter` and `FutureTypeAdapter` ([google/gson#1787) - Improve `JsonReader.skipValue()` ([google/gson#2062) - Perform numeric conversion for primitive numeric type adapters ([google/gson#2158) - Add `Gson.fromJson(..., TypeToken)` overloads ([google/gson#1700) - Fix changes to `GsonBuilder` affecting existing `Gson` instances ([google/gson#1815) - Make `JsonElement` conversion methods more consistent and fix javadoc ([google/gson#2178) - Throw `UnsupportedOperationException` when `JsonWriter.jsonValue` is not supported ([google/gson#1651) - Disallow `JsonObject` `Entry.setValue(null)` ([google/gson#2167) - Fix `TypeAdapter.toJson` throwing AssertionError for custom IOException ([google/gson#2172) - Convert null to JsonNull for `JsonArray.set` ([google/gson#2170) - Fixed nullSafe usage. ([google/gson#1555) - Validate `TypeToken.getParameterized` arguments ([google/gson#2166) - Fix [#1702](https://togithub.com/google/gson/issues/1702): Gson.toJson creates CharSequence which does not implement toString ([google/gson#1703) - Prefer existing adapter for concurrent `Gson.getAdapter` calls ([google/gson#2153) - Improve `ArrayTypeAdapter` for `Object[]` ([google/gson#1716) - Improve `AppendableWriter` performance ([google/gson#1706) ### [`v2.9.1`](https://togithub.com/google/gson/blob/HEAD/CHANGELOG.md#Version-291) - Make `Object` and `JsonElement` deserialization iterative rather than recursi[google/gson#1912) - Added parsing support for enum that has overridden toString() method ([google/gson#1950) - Removed support for building Gson with Gradle ([google/gson#2081) - Removed obsolete `codegen` hierarchy ([google/gson#2099) - Add support for reflection access filter ([google/gson#1905) - Improve `TypeToken` creation validation ([google/gson#2072) - Add explicit support for `float` in `JsonWriter` ([google/gson#2130, [google/gson#2132) - Fail when parsing invalid local date ([google/gson#2134) Also many small improvements to javadoc. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/elide-dev/v3).

Add support for reflection access filter

6786766

google-cla bot added the cla: yes label Jun 4, 2021

Marcono1234 commented Jan 21, 2022

View reviewed changes

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java Outdated Show resolved Hide resolved

Marcono1234 mentioned this pull request Jan 21, 2022

Error reflection JDK 17 and gson #1979

Open

Marcono1234 commented Jan 21, 2022

View reviewed changes

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java Show resolved Hide resolved

eamonnmcmanus requested changes Feb 2, 2022

View reviewed changes

Marcono1234 added 9 commits February 4, 2022 18:11

Merge branch 'master' into marcono1234/reflection-access-filter

153380f

Improve documentation

1dbca2b

Fix compilation errors

37e1ea8

Relax handling for BLOCK_ALL when invoking default constructor

9eb5fa2

Improve handling for inherited fields

bb42ff9

Fix accessible test failing for static fields

988c68c

Simplify ReflectiveTypeAdapterFactory field writing

dd2ba4e

Fix GsonBuilder changes affecting created Gson instances

5298957

Improve documentation

870496e

eamonnmcmanus reviewed Feb 4, 2022

View reviewed changes

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

eamonnmcmanus reviewed Feb 4, 2022

View reviewed changes

Marcono1234 added 3 commits February 5, 2022 18:49

Improve handling for IllegalAccessException

837b21f

For Java < 9, AccessibleObject.canAccess is not available and therefore checks might pass even if object is not accessible, causing IllegalAccessException later.

Merge remote-tracking branch 'remotes/origin/master' into marcono1234…

f4e0ecd

…/reflection-access-filter

Fix incorrect GsonBuilder.addReflectionAccessFilter documentation

8acf2a9

eamonnmcmanus approved these changes Apr 17, 2022

View reviewed changes

eamonnmcmanus merged commit e82637c into google:master Apr 17, 2022

Marcono1234 deleted the marcono1234/reflection-access-filter branch April 17, 2022 20:36

Marcono1234 mentioned this pull request Jul 29, 2022

com.google.gson.internal.reflect.UnsafeReflectionAccessor illegal reflective access operation on jdk12 #1540

Closed

Marcono1234 mentioned this pull request Aug 22, 2022

Add tests checking GsonBuilder not affecting existing Gson instances #1815

Merged

BePPPower mentioned this pull request Jan 29, 2024

[Fix](JDK17) Start Fe with JDK17 apache/doris#30485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reflection access filter #1905

Add support for reflection access filter #1905

Marcono1234 commented Jun 4, 2021

Marcono1234 commented Aug 12, 2021 •

edited

eamonnmcmanus left a comment

Marcono1234 commented Feb 4, 2022

eamonnmcmanus Feb 4, 2022

Marcono1234 Feb 5, 2022 •

edited

eamonnmcmanus Apr 17, 2022

Marcono1234 Apr 17, 2022

eamonnmcmanus commented Feb 5, 2022

Marcono1234 commented Feb 5, 2022

Marcono1234 commented Apr 16, 2022

eamonnmcmanus left a comment

eamonnmcmanus Apr 17, 2022

Add support for reflection access filter #1905

Add support for reflection access filter #1905

Conversation

Marcono1234 commented Jun 4, 2021

Marcono1234 commented Aug 12, 2021 • edited

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 commented Feb 4, 2022

eamonnmcmanus Feb 4, 2022

Choose a reason for hiding this comment

Marcono1234 Feb 5, 2022 • edited

Choose a reason for hiding this comment

eamonnmcmanus Apr 17, 2022

Choose a reason for hiding this comment

Marcono1234 Apr 17, 2022

Choose a reason for hiding this comment

eamonnmcmanus commented Feb 5, 2022

Marcono1234 commented Feb 5, 2022

Marcono1234 commented Apr 16, 2022

eamonnmcmanus left a comment

Choose a reason for hiding this comment

eamonnmcmanus Apr 17, 2022

Choose a reason for hiding this comment

Marcono1234 commented Aug 12, 2021 •

edited

Marcono1234 Feb 5, 2022 •

edited