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

WW-5525 Prevent NPE when using new in OGNL #1200

Closed

Conversation

FBibonne
Copy link

No description provided.

@FBibonne FBibonne marked this pull request as draft January 31, 2025 21:20
@FBibonne FBibonne marked this pull request as ready for review February 1, 2025 16:17
@FBibonne
Copy link
Author

FBibonne commented Feb 1, 2025

Failing tests about the issue pass after the fix. It only stays DynAttribsTest which does not pass (DynAttribsTest.setUp:69 unable to find struts-tags.tld error) but it is yet the case for main branch

@@ -141,6 +141,10 @@ public void restore(Map context, Object target, Member member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member, String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property: {}]", target, member, propertyName);

if (member == null){
Copy link
Author

Choose a reason for hiding this comment

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

If member is null, it can cause others NPE (for example at line 151) and according the contract of the method a null member is a non sense

@kusalk
Copy link
Member

kusalk commented Feb 6, 2025

Hi @FBibonne

Thanks for reporting this and contributing a PR, including proper test coverage. However, I did have a simpler fix in mind and have copied across your test case - feel free to review here: #1214

cc @lukaszlenart

@FBibonne
Copy link
Author

FBibonne commented Feb 7, 2025

Thank you @kusalk for fixing this NPE

Your PR 1214 looks very fine to my point of view

@FBibonne FBibonne closed this Feb 7, 2025
@FBibonne FBibonne deleted the fix/WW-5525-NPE-using-new-in-OGNL branch February 7, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants