0

While getting my code reviewed by my senior I got a comment to not use Reflection because it is a security vulnerability in companies and also it makes code very slow. Now when I execute it on my IDE then I can't see the difference because it runs immediately. I searched a lot but I didn't get any clear explanation anywhere why this is so.

I had a list of objects containing a lot of fields from which I have to use just a few and the rest needed to stay blank. This object is coming from an external API. We have a spring-boot project.

public void setNullExceptGivenFields(List<String> fieldsToExcludeWhileSettingNull) {
    try {
        Field[] fields = this.getClass().getDeclaredFields();
        for (Field field : fields) {
            field.setAccessible(true);
            if (!field.getClass().isPrimitive() && !fieldsToExcludeWhileSettingNull.contains(field.getName())
                    && field.get(this) != null) {
                field.set(this, null);
            }
        }
    } catch (IllegalArgumentException | IllegalAccessException e) {
        LOGGER.info("Exception occured while setting fields null " + e.getMessage());
    } catch (Exception e) {
        LOGGER.info("Unhandled exception occured");
    }
}

Any help will be appreciated.

Ismaili Mohamedi
  • 906
  • 7
  • 15
Manish Kumar
  • 11
  • 4
  • 4
    Without reading the question: don't use reflection if you don't really understand the implications. You asking the question already tells me you don't have that understanding - so don't use reflection. :) – Thomas Jan 17 '23 at 09:09
  • @Thomas can you atleast give one reason to help me understand why it should not be used. – Manish Kumar Jan 17 '23 at 09:13
  • Reflection _is_ slow and it can cause a lot of problems including security issues. Thus your senior is right. However, it's hard to suggest a better approach without knowing more details, e.g. how many fields are "a lot", why do you need to set them to null in the first place - wouldn't it make sense to create another class that just contains the fields you _want_ and use a mapping library such as mapstruct to copy them over? – Thomas Jan 17 '23 at 09:13
  • 7
    To me, this looks like a bad class design, breaking encapsulation. You allow your callers (could be anywhere because of `public`) to set to null whatever fields they like. To make some fields survive, the caller needs to know the internals of your class, i.e. the field names. And by swallowing the exceptions, you don't inform your caller about the failure, you just write it as INFO (not ERROR) into some log, hoping that someone will eventually read it. – Ralf Kleberhoff Jan 17 '23 at 09:14
  • 1
    "when i execute it on my IDE then i cant see the difference coz it runs immediately" - sure, it is still quite fast when compared to human time and only running on a few elements. However, it is slow compared to proper calls and if you do comparisons with a lot of data you'll see a notable difference. Here's a question that might help you get an idea on the performance differences between mappers using reflection and those using non-reflection methods: https://stackoverflow.com/questions/22078156/orika-vs-jmapper-how-it-works-and-a-speed-difference-why – Thomas Jan 17 '23 at 09:19
  • @Thomas the performace totally depends on the size of data set and that is ok but how does using reflection raises security question isn't this getting used in other frameworks as well, testing frameworks etc. – Manish Kumar Jan 17 '23 at 09:26
  • Yes, reflection is used a lot but if you look at the comparisons of mapper frameworks that use reflection vs. those that don't you often see reflection being 10-100 times slower - of course that depends on how much reflection is being used. – Thomas Jan 17 '23 at 10:08
  • @Thomas why reflection is a security concern though?? – Manish Kumar Jan 17 '23 at 10:26
  • Result of a quick search - might explain it better than I can: https://stackoverflow.com/questions/3002904/what-is-the-security-risk-of-object-reflection - Also have a look here: https://owasp.org/www-community/vulnerabilities/Unsafe_use_of_Reflection#:~:text=This%20vulnerability%20is%20caused%20by,limited%20form%20of%20code%20injection. – Thomas Jan 17 '23 at 10:29
  • 2
    There are lots of things wrong here. `field.setAccessible(true)` is unnecessary as you’re accessing your own fields. But the fact that you are accessing your own fields contradicts your statement “*This object is coming from an external API*”. Besides that, `field.getClass().isPrimitive()` does not do what you think it does. Finally, as Ralf Kleberhoff already said, this allows callers to set arbitrary fields to `null` which should already explain why this is a security vulnerability. Besides that, I can’t imagine any real world use case for such an operation. It simply makes no sense at all. – Holger Jan 17 '23 at 13:48

0 Answers0