MemberAccess support private static field. #58

[hengyunabc]

No description provided.

[lukaszlenart]

Looks good, left some comments

[hengyunabc]

@lukaszlenart Thanks, I updated the commit.

[lukaszlenart]

Now I wonder if we shouldn't throw an exception here, I mean, the field is not accessible but we want to get access to it anyway. Wdyt?

[hengyunabc]

@lukaszlenart Do you mean line 2079?

            Field f = getField(c, fieldName);
            if (f == null) {
                throw new NoSuchFieldException(fieldName);
            }

OgnlRuntime#getField return all fields, contain private fields, so throw NoSuchFieldException is ok.

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

Suggested change

	result = f.get(null);
	throw new IllegalAccessException("Access to " + fieldName + " is forbidden");

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

Suggested change

	result = f.get(null);
	throw new IllegalAccessException("Access to " + fieldName + " is forbidden");

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

Suggested change

	result = f.get(null);
	throw new IllegalAccessException("Access to " + fieldName + " is forbidden");

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

[lukaszlenart]

No, I meant that we check access first and if the field is not accessible we access it anyway. This can lead to a security vulnerability as basically we ignore if the field is accessible or not.

We should throw an exception here instead of f.get(null);

Suggested change

	result = f.get(null);
	throw new IllegalAccessException("Access to " + fieldName + " is forbidden");

[hengyunabc]

@lukaszlenart I got it, I updated the commit.

[lukaszlenart]

LGTM 👍

[lukaszlenart]

Thanks a lot!

[lukaszlenart]

Do you plan to cherry-pick those changes into the master branch?

[hengyunabc]

Do you plan to cherry-pick those changes into the master branch?

OK, I created a new PR: #60.

In addition, may I ask is there any plan to release the new version?

[lukaszlenart]

In few hours, working on it ;-)