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

Add support for reflection access filter #1905

Merged

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Jun 4, 2021

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.

@google-cla google-cla bot added the cla: yes label Jun 4, 2021
@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Aug 12, 2021

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?

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

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.

@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Feb 4, 2022

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?

@@ -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>();
Copy link
Member

@eamonnmcmanus eamonnmcmanus Feb 4, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@Marcono1234 Marcono1234 Feb 5, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

@eamonnmcmanus eamonnmcmanus Apr 17, 2022

Choose a reason for hiding this comment

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

Seems fair.

Copy link
Collaborator Author

@Marcono1234 Marcono1234 Apr 17, 2022

Choose a reason for hiding this comment

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

Thanks! Have created #2104 for that.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Feb 5, 2022

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.

Marcono1234 added 3 commits Feb 5, 2022
For Java < 9, AccessibleObject.canAccess is not available and therefore checks
might pass even if object is not accessible, causing IllegalAccessException
later.
@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Feb 5, 2022

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
Copy link
Collaborator Author

Marcono1234 commented Apr 16, 2022

@eamonnmcmanus, what do you think?

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Sorry, I lost track of this.

@@ -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>();
Copy link
Member

@eamonnmcmanus eamonnmcmanus Apr 17, 2022

Choose a reason for hiding this comment

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

Seems fair.

@eamonnmcmanus eamonnmcmanus merged commit e82637c into google:master Apr 17, 2022
3 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/reflection-access-filter branch Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants