Skip to content

Commit

Permalink
Merge pull request #265 from orphan-oss/fix/WW-5417-check
Browse files Browse the repository at this point in the history
Cherry-picks "WW-5417 update OgnlRuntime & ObjectPropertyAccessor to do access check for set field value"
  • Loading branch information
lukaszlenart authored Apr 19, 2024
2 parents 4769f68 + 88d6e08 commit 88ab8fc
Show file tree
Hide file tree
Showing 7 changed files with 265 additions and 29 deletions.
18 changes: 3 additions & 15 deletions .github/workflows/maven.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,28 +49,16 @@ jobs:
server-password: MAVEN_PASSWORD
- name: Build with Maven on Java ${{ matrix.java }}
if: matrix.java != '17'
run: mvn -B -V test --no-transfer-progress
run: ./mvnw -B -V test --no-transfer-progress
- name: Code coverage
if: matrix.java == '17'
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
run: mvn -B -V -Pcoverage verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar --no-transfer-progress
run: ./mvnw -B -V -Pcoverage verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar --no-transfer-progress
- name: Deploy SNAPSHOT
if: matrix.java == '8' && github.ref == 'refs/heads/master'
env:
MAVEN_USERNAME: ${{ secrets.OSSRH_USERNAME }}
MAVEN_PASSWORD: ${{ secrets.OSSRH_TOKEN }}
run: mvn -B -V -DskipTests=true deploy --no-transfer-progress
- name: Apply automerge label
if: matrix.java == '17' && github.actor == 'dependabot[bot]'
run: gh pr edit "$PR_URL" --add-label "automerge"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PR_URL: ${{ github.event.pull_request.html_url }}
- name: Automerge
if: matrix.java == '17' && github.actor == 'dependabot[bot]'
run: gh pr merge -m "$PR_URL"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PR_URL: ${{ github.event.pull_request.html_url }}
run: ./mvnw -B -V -DskipTests=true deploy --no-transfer-progress
2 changes: 1 addition & 1 deletion .mvn/wrapper/maven-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1 +1 @@
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.5.2/apache-maven-3.5.2-bin.zip
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.6.3/apache-maven-3.6.3-bin.zip
4 changes: 2 additions & 2 deletions src/main/java/ognl/ObjectPropertyAccessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,12 @@ public Object setPossibleProperty(OgnlContext context, Object target, String nam
Object result = null;
try {
if (!OgnlRuntime.setMethodValue(context, target, name, value, true)) {
result = OgnlRuntime.setFieldValue(context, target, name, value) ? null : OgnlRuntime.NotFound;
result = OgnlRuntime.setFieldValue(context, target, name, value, true) ? null : OgnlRuntime.NotFound;
}

if (result == OgnlRuntime.NotFound) {
Method m = OgnlRuntime.getWriteMethod(target.getClass(), name);
if (m != null) {
if (m != null && context.getMemberAccess().isAccessible(context, target, m, name)) {
result = m.invoke(target, value);
}
}
Expand Down
44 changes: 34 additions & 10 deletions src/main/java/ognl/OgnlRuntime.java
Original file line number Diff line number Diff line change
Expand Up @@ -1648,7 +1648,7 @@ public static Object callConstructor(OgnlContext context, String className, Obje
throw new NoSuchMethodException();
}
}
if (!context.getMemberAccess().isAccessible(context, target, ctor, null)) {
if (!isAccessible(context, target, ctor, null)) {
throw new IllegalAccessException(
"access denied to " + target.getName() + "()");
}
Expand Down Expand Up @@ -1702,7 +1702,7 @@ public static Object getMethodValue(OgnlContext context, Object target, String p
m = getReadMethod((target == null) ? null : target.getClass(), propertyName, null);

if (checkAccessAndExistence) {
if ((m == null) || !context.getMemberAccess().isAccessible(context, target, m, propertyName)) {
if ((m == null) || !isAccessible(context, target, m, propertyName)) {
result = NotFound;
}
}
Expand Down Expand Up @@ -1740,7 +1740,7 @@ public static boolean setMethodValue(OgnlContext context, Object target, String
Method m = getSetMethod(context, (target == null) ? null : target.getClass(), propertyName);

if (checkAccessAndExistence) {
if ((m == null) || !context.getMemberAccess().isAccessible(context, target, m, propertyName)) {
if ((m == null) || !isAccessible(context, target, m, propertyName)) {
result = false;
}
}
Expand Down Expand Up @@ -1863,7 +1863,7 @@ public static Object getFieldValue(OgnlContext context, Object target, String pr
final Field f = getField((target == null) ? null : target.getClass(), propertyName);

if (checkAccessAndExistence) {
if ((f == null) || !context.getMemberAccess().isAccessible(context, target, f, propertyName)) {
if ((f == null) || !isAccessible(context, target, f, propertyName)) {
result = NotFound;
}
}
Expand Down Expand Up @@ -1892,16 +1892,27 @@ public static Object getFieldValue(OgnlContext context, Object target, String pr
return result;
}

public static boolean setFieldValue(OgnlContext context, Object target, String propertyName, Object value)
throws OgnlException {

/**
* Don't use this method as it doesn't check member access rights via {@link MemberAccess} interface
*/
@Deprecated
public static boolean setFieldValue(OgnlContext context, Object target, String propertyName, Object value) throws OgnlException {
return setFieldValue(context, target, propertyName, value, false);
}

public static boolean setFieldValue(OgnlContext context, Object target, String propertyName, Object value,
boolean checkAccessAndExistence)
throws OgnlException
{
boolean result = false;

try {
final Field f = getField((target == null) ? null : target.getClass(), propertyName);

if (f != null) {
final int fModifiers = f.getModifiers();
if (!Modifier.isStatic(fModifiers) && !Modifier.isFinal(fModifiers)) {
if (!Modifier.isStatic(fModifiers) && !Modifier.isFinal(fModifiers) && (!checkAccessAndExistence || isAccessible(context, target, f, propertyName))) {
final Object state = context.getMemberAccess().setup(context, target, f, propertyName);
try {
if (isTypeCompatible(value, f.getType())
Expand All @@ -1925,7 +1936,7 @@ public static boolean isFieldAccessible(OgnlContext context, Object target, Clas
}

public static boolean isFieldAccessible(OgnlContext context, Object target, Field field, String propertyName) {
return context.getMemberAccess().isAccessible(context, target, field, propertyName);
return isAccessible(context, target, field, propertyName);
}

public static boolean hasField(OgnlContext context, Object target, Class<?> inClass, String propertyName) {
Expand Down Expand Up @@ -1977,7 +1988,7 @@ public static Object getStaticField(OgnlContext context, String className, Strin
}

Object result;
if (context.getMemberAccess().isAccessible(context, null, f, null)) {
if (isAccessible(context, null, f, null)) {
final Object state = context.getMemberAccess().setup(context, null, f, null);
try {
result = f.get(null);
Expand Down Expand Up @@ -2134,7 +2145,7 @@ private static Method _getGetMethod(Class<?> targetClass, String propertyName) {
}

public static boolean isMethodAccessible(OgnlContext context, Object target, Method method, String propertyName) {
return (method != null) && context.getMemberAccess().isAccessible(context, target, method, propertyName);
return (method != null) && isAccessible(context, target, method, propertyName);
}

public static boolean hasGetMethod(OgnlContext context, Object target, Class<?> targetClass, String propertyName) {
Expand Down Expand Up @@ -3032,4 +3043,17 @@ public static boolean getUseFirstMatchGetSetLookupValue() {
return _useFirstMatchGetSetLookup;
}

/**
* Returns true if the given member is accessible or can be made accessible
* by this object.
*
* @param context the current execution context.
* @param target the Object to test accessibility for.
* @param member the Member to test accessibility for.
* @param propertyName the property to test accessibility for.
* @return true if the target/member/propertyName is accessible in the context, false otherwise.
*/
private static boolean isAccessible(OgnlContext context, Object target, Member member, String propertyName) {
return context.getMemberAccess().isAccessible(context, target, member, propertyName);
}
}
52 changes: 52 additions & 0 deletions src/test/java/ognl/ExcludedObjectMemberAccess.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2020 OGNL Contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package ognl;

import java.lang.reflect.Member;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

/**
* This class provides simple functionality for mark / unmark an object as inaccessible
*/
public class ExcludedObjectMemberAccess extends DefaultMemberAccess {
private final List<Object> excludedObjects = new ArrayList<>(); // Any field or method in this list will be inaccessible

public ExcludedObjectMemberAccess(boolean allowAllAccess) {
super(allowAllAccess);
}

public ExcludedObjectMemberAccess(boolean allowPrivateAccess, boolean allowProtectedAccess, boolean allowPackageProtectedAccess) {
super(allowPrivateAccess, allowProtectedAccess, allowPackageProtectedAccess);
}

public boolean isAccessible(OgnlContext context, Object target, Member member, String propertyName) {
if (excludedObjects.contains(member)) {
return false;
}

return super.isAccessible(context, target, member, propertyName);
}

public void exclude(Object obj) {
excludedObjects.add(obj);
}

public void removeExclusion(Object obj) {
excludedObjects.remove(obj);
}
}
99 changes: 99 additions & 0 deletions src/test/java/ognl/TestObjectPropertyAccessor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright 2020 OGNL Contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package ognl;

import junit.framework.TestCase;

import java.beans.IntrospectionException;
import java.lang.reflect.Method;
import java.util.Map;

/**
* Tests various methods / functionality of {@link ObjectPropertyAccessor}.
*/
public class TestObjectPropertyAccessor extends TestCase {
private Map context;
private ObjectPropertyAccessor propertyAccessor;

public void setUp() throws Exception {
super.setUp();
context = Ognl.createDefaultContext(null, new ExcludedObjectMemberAccess(false));
propertyAccessor = new ObjectPropertyAccessor();
}

/**
* Public class for "setPossibleProperty" method tests.
*/
public static class SimplePublicClass {
private String gender = "male";
public String email = "test@test.com";
private String name = "name";
private String age = "18";

public void setGender(String gender) {
this.gender = gender;
}

private void setEmail(String email) {
this.email = email;
}

private void setName(String email) {
this.email = email;
}

public void setname(String name) {
this.name = name;
}

private void setAge(String age) {
this.age = age;
}

public void setage(String age) {
this.age = age;
}
}

public void testSetPossibleProperty() throws OgnlException, IntrospectionException {
OgnlContext context = (OgnlContext) this.context;
SimplePublicClass simplePublic = new SimplePublicClass();

// 1. when set method is accessible and set method
assertNotSame(OgnlRuntime.NotFound, propertyAccessor.setPossibleProperty(context, simplePublic, "gender", "female"));
assertEquals("female", simplePublic.gender);

// 2. when set method is NOT accessible and fallback to set field (field is accessible)
assertNotSame(OgnlRuntime.NotFound, propertyAccessor.setPossibleProperty(context, simplePublic, "email", "admin@admin.com"));
assertEquals("admin@admin.com", simplePublic.email);

// 3. when set method is NOT accessible, field is NOT accessible, fallback to write method (write method is accessible)
assertEquals("setName", OgnlRuntime.getSetMethod(context, SimplePublicClass.class, "name").getName());
assertEquals("setname", OgnlRuntime.getWriteMethod(SimplePublicClass.class, "name", null).getName());
assertNotSame(OgnlRuntime.NotFound, propertyAccessor.setPossibleProperty(context, simplePublic, "name", "new name"));
assertEquals("new name", simplePublic.name);

// 4. when set method is NOT accessible, field is NOT accessible, fallback to write method (write method is NOT accessible)
Method ageWriteMethod = OgnlRuntime.getWriteMethod(SimplePublicClass.class, "age", null);
((ExcludedObjectMemberAccess) context.getMemberAccess()).exclude(ageWriteMethod);

assertEquals("setage", ageWriteMethod.getName());
assertFalse(context.getMemberAccess().isAccessible(context, simplePublic, ageWriteMethod, "age"));
assertEquals("setAge", OgnlRuntime.getSetMethod(context, SimplePublicClass.class, "age").getName());
assertEquals(OgnlRuntime.NotFound, propertyAccessor.setPossibleProperty(context, simplePublic, "age", "99"));
assertEquals("18", simplePublic.age);
}
}
Loading

0 comments on commit 88ab8fc

Please sign in to comment.