DefaultValidator.safeReadLine() Does not handle CRLF

[aaditriray]

I was trying to use DefaultValidator.safeReadLine() to read a line from an input Stream in order to prevent from DoS. However it seems through "\r" and "\n" are handled as End of Line charter but it does not support CRLF ( \r\n ). As a result I am getting an extra blank line for each new line while using in Windows Environment.

As a workaround I have created my own implementation (after extending DefaultValidator ) to handle the CRLF.

Wondering if there is any specific requirement not to consider CRLF otherwise if it would be better to modify DefaultValidator to handle CRLF as well.

[xeno6696]

My apologies for the late response, we’ve been sitting on a point release and I meant to get to this question later.

I don’t see anything wrong with the current implementation. We’re iterating the String char by char, and only appending chars that aren’t CR or LF. There is no such char “CRLF” in existence.

Feel free to prove me wrong by providing a unit test!

[xeno6696]

Never mind. I see it. We’re reading UNTIL we see a “CR” or “LF” then terminating the line.

However... on the very next call, it should just immediately return on the same check.

[jeremiahjstacey]

I think that the attached DefaultValidatorTest.txt shows the three \r \n \r\n cases with the safeReadLine method.

DefaultValidatorTest.txt

[jeremiahjstacey]

I would like to suggest deprecating this method in DefaultValidator and instead offer an alternate implementation of a Reader. I've attached the premise of the suggestion below. Effectively, the BufferedLineLimitedReader is a verbatim copy of the BufferedReader from java with the addition of a character length limit check prior to reading the data into a buffer, which is provided by the constructor set. It was not possible to extend BufferedReader to do this, since the implementation would require access to parent-level private variables.

More testing should be done on the readLine implementation, I'm sure I'm missing cases!! I did only the minimum work to update the API and behavior to throw the expected Exception in my test cases.

Attachement Deleted

[xeno6696]

@jeremiahjstacey did you check out this "sub test" in ValidatorTest:

        // This sub-test attempts to validate that BufferedReader.readLine() and safeReadLine() are similar in operation
        // for the nominal case
        try {
            s.reset();
            InputStreamReader isr = new InputStreamReader(s);
            BufferedReader br = new BufferedReader(isr);
            String u = br.readLine();
            s.reset();
            String v = instance.safeReadLine(s, 20);
            assertEquals(u, v);
        }
        catch (IOException e) {
            fail();
        }
        catch (ValidationException e) {
            fail();
        }

[jeremiahjstacey]

I had not seen this test. I will note that there is no non-zero/positive checks on the max limit of my previously offered files. Those tests should be added if that course is persued.

testExceptionOnZeroMaxLength() testExceptionOnNegativeMaxLenth()

[xeno6696]

Why do you submit patches instead of PR’s?

[jeremiahjstacey]

If that is an acceptable approach to resolution I will work to get it committed and offer it up. I mostly attached the file to illustrate the intent of the suggested course and proof that it could work. If this is an acceptable way to resolve the issue, then I will move forward with branching and the PR.

If, for some reason DefaultValidator must retain that method, and we need this functionality to work in that context, then there doesn't seem to be much point in me spending time going through the motions. Though I am becoming more familiar with git it still takes me a fair amount of time investment to research and apply the tool's syntax.

In short, because I'm lazy and still don't know git as well as I probably should -- and I don't want to invest time unless it adds value.