In JSONTokener.nextValue(), accepts erroneous values

[jpfiset]

This change fixes an issue where the JSONTokener mis-behaves on nextValue() on values close to end of stream, or values with spaces in them.

[kushieda-minori]

Do you have some tests or samples that showcase the issue before and after the change?

[jpfiset]

import java.io.StringReader;

import org.json.JSONArray;
import org.json.JSONObject;
import org.json.JSONTokener;

import junit.framework.TestCase;

public class JSONTokenerTest extends TestCase {

	public void testValid() throws Exception {
		checkValid("0",Number.class);
		checkValid(" 0  ",Number.class);
		checkValid("23",Number.class);
		checkValid("23.5",Number.class);
		checkValid(" 23.5  ",Number.class);
		checkValid("null",null);
		checkValid(" null  ",null);
		checkValid("true",Boolean.class);
		checkValid(" true\n",Boolean.class);
		checkValid("false",Boolean.class);
		checkValid("\nfalse  ",Boolean.class);
		checkValid("{}",JSONObject.class);
		checkValid(" {}  ",JSONObject.class);
		checkValid("{\"a\":1}",JSONObject.class);
		checkValid(" {\"a\":1}  ",JSONObject.class);
		checkValid("[]",JSONArray.class);
		checkValid(" []  ",JSONArray.class);
		checkValid("[1,2]",JSONArray.class);
		checkValid("\n\n[1,2]\n\n",JSONArray.class);
	}

	public void testErrors() throws Exception {
		// Check that stream can detect that a value is found after
		// the first one
		checkError(" { \"a\":1 }  4 ");
		checkError("null \"a\"");
		checkError("1 2");
		checkError("{} true");
	}

	private Object checkValid(String testStr, Class<?> aClass) throws Exception {

		try {

			Object result = nextValue(testStr);

			// Check class of object returned
			if( null == aClass ) {
				if( null == result ) {
					// OK
				} else if( JSONObject.NULL == result ) {
					// OK
				} else {
					throw new Exception("Unexpected class: "+result.getClass().getSimpleName());
				}
			} else {
				if( null == result ) {
					throw new Exception("Unexpected null result");
				} else if( aClass.isAssignableFrom(result.getClass()) ) {
					// OK
				} else {
					throw new Exception("Unexpected class: "+result.getClass().getSimpleName());
				}
			}
			
			return result;

		} catch (Exception e) {
			throw new Exception("Error on test: "+testStr, e);
		}
	}
	
	private void checkError(String testStr) throws Exception {

		try {
			nextValue(testStr);
			
			fail("Error should be triggered: "+testStr);

		} catch (Exception e) {
			// OK
		}
	}
	
	/**
	 * Verifies that JSONTokener can read a stream that contains a value. After
	 * the reading is done, check that the stream is left in the correct state
	 * by reading the characters after. All valid cases should reach end of stream.
	 * @param testStr
	 * @return
	 * @throws Exception
	 */
	private Object nextValue(String testStr) throws Exception {
		StringReader sr = new StringReader(testStr);
		JSONTokener tokener = new JSONTokener(sr);

		Object result = tokener.nextValue();

		char c = tokener.nextClean();
		if( 0 != c ) {
			throw new Exception("Unexpected character found at end of JSON stream: "+c+ " ("+tokener+")");
		}

		sr.close();

		return result;
	}
}

[johnjaylward]

Looking at your test cases, I think your expectations for how the Tokener is supposed to work may not be matching the actual use. I'll take a closer look though and try to determine where the differences are. The main issue I see is your use of JSONTokener.nextClean in your nextValue method.

[johnjaylward]

Per the JSON specification, JSON Readers (in this case our tokener) is allowed to be more lax than the JSON spec. For example, it's OK for a JSON Parser to take illegal forms of JSON.

In this case, our Tokener allows both keys and values to be unquoted. That this means is that the following examples are expected to be parsable by our JSONTokener:

• {my key: my value} -> {"my key":"my value"}
• [val 1, val 2, val\t4] -> ["val 1", "val 2", "val\t4"]

Given that, your change appears to break those use-cases. For the full list of test cases see https://github.com/stleary/JSON-Java-unit-test and you can see specific examples of allowing these malformed JSON texts in the CDLTest class.

For your test cases on the failure side, this means that the "1 2" test in the failure section would actually pass and be read as a single string token "1 2" instead of 2 integer tokens [1,2].

However, your check for the eof does not appear to break any existing test cases. I think we could still allow that.

[johnjaylward]

Updates test examples so they pass with only the eof check

    @Test
    public void testValid() {
        checkValid("0",Number.class);
        checkValid(" 0  ",Number.class);
        checkValid("23",Number.class);
        checkValid("23.5",Number.class);
        checkValid(" 23.5  ",Number.class);
        checkValid("null",null);
        checkValid(" null  ",null);
        checkValid("true",Boolean.class);
        checkValid(" true\n",Boolean.class);
        checkValid("false",Boolean.class);
        checkValid("\nfalse  ",Boolean.class);
        checkValid("{}",JSONObject.class);
        checkValid(" {}  ",JSONObject.class);
        checkValid("{\"a\":1}",JSONObject.class);
        checkValid(" {\"a\":1}  ",JSONObject.class);
        checkValid("[]",JSONArray.class);
        checkValid(" []  ",JSONArray.class);
        checkValid("[1,2]",JSONArray.class);
        checkValid("\n\n[1,2]\n\n",JSONArray.class);
        checkValid("1 2", String.class);
    }
    
    @Test
    public void testErrors() {
        // Check that stream can detect that a value is found after
        // the first one
        checkError(" { \"a\":1 }  4 ");
        checkError("null \"a\"");
        checkError("{} true");
    }
    
    private Object checkValid(String testStr, Class<?> aClass)  {
        Object result = nextValue(testStr);

        // Check class of object returned
        if( null == aClass ) {
            if(JSONObject.NULL.equals(result)) {
                // OK
            } else {
                throw new JSONException("Unexpected class: "+result.getClass().getSimpleName());
            }
        } else {
            if( null == result ) {
                throw new JSONException("Unexpected null result");
            } else if(!aClass.isAssignableFrom(result.getClass()) ) {
                throw new JSONException("Unexpected class: "+result.getClass().getSimpleName());
            }
        }
        
        return result;
    }

    private void checkError(String testStr) {
        try {
            nextValue(testStr);
            
            fail("Error should be triggered: (\""+testStr+"\")");
        } catch (JSONException e) {
            // OK
        }
    } 

    /**
     * Verifies that JSONTokener can read a stream that contains a value. After
     * the reading is done, check that the stream is left in the correct state
     * by reading the characters after. All valid cases should reach end of stream.
     * @param testStr
     * @return
     * @throws Exception
     */
    private Object nextValue(String testStr) throws JSONException {
        try(StringReader sr = new StringReader(testStr);){
            JSONTokener tokener = new JSONTokener(sr);
    
            Object result = tokener.nextValue();
    
            if( result == null ) {
                throw new JSONException("Unable to find value token in JSON stream: ("+tokener+"): "+testStr);
            }
            
            char c = tokener.nextClean();
            if( 0 != c ) {
                throw new JSONException("Unexpected character found at end of JSON stream: "+c+ " ("+tokener+"): "+testStr);
            }
    
            return result;
        }

    }

[jpfiset]

OK, sorry about breaking your test cases. Did not realize you had that project.

I would still like to see the fix for eof go into master. How should I proceed to see that change accepted?

[johnjaylward]

you can either update this pull request to just change the EOF, or you can close this one and reopen a new one with just the EOF change. whichever is easier for you.

[stleary]

Tokener changes are not accepted at this time due to the cited reasons. If the EOF change is still needed, please submit a separate pull request.