MYFACES-4266: Ajax update fails due to invalid characters in response XML (DoS)

[cnsgithub]

Fixes

• https://issues.apache.org/jira/browse/MYFACES-4266

Related to

• Ajax: Escaping XML properly for control characters primefaces/primefaces#3875

@tandraschko Could you please check that and - if accepted - merge it to the other branches as well?

[tandraschko]

thanks, really appreciate new contributors!

i just wonder if we could implement it without always creating a new String? (we could just loop the char array)
In MyFaces we are very performance "sensitiv" ;)

WDYT @pnicolucci @ebreijo ?

[melloware]

@tandraschko I think since Strings are immutable you will have no choice but to create a new String but I could be wrong.

[tandraschko]

Of course - but this methods uses char and char[]:

public void write(int c) throws IOException 
public void write(char[] cbuf, int off, int len) throws IOException 

There is no need to wrap it by a string

[melloware]

Ahh I see what you mean just override those write methods and process each char.

[cnsgithub]

Of course - but this methods uses char and char[]:

public void write(int c) throws IOException 
public void write(char[] cbuf, int off, int len) throws IOException 

There is no need to wrap it by a string

Sorry, I don't see any problem here since xmlEncode has been overloaded three times to fit all write variations, i.e. write(char[],int,in) is using xmlEncode(char[]), write(int) is using xmlEncode(char) and just write(String,off,len) is using xmlEncode(String).

Edit: @tandraschko Okay, now I see, just added another commit. 😉

[tandraschko]

cool :D

I'm a just a bit unsure... The current version will work but maybe it's a bit "unattractive".

1. I think we should overwrite all #write methods, to cover 100% all cases?! Probably also writeText?
2. all #write methods should call the same super.write - currently the write(String) calls super.write(char[]) to avoid confusion (and maybe but unlikely bugs)

JFYI: you can also use a for-each loop when looping over arrays, we just avoid it on ArrayLists, to avoid a iterator instance (ArrayLists are used for component lists e.g. and the component tree is traversed very often)

[cnsgithub]

1. I think we should overwrite all #write methods, to cover 100% all cases?! Probably also writeText?

I think the remaining writeXXX methods operate on a higher abstraction level and internally call the low level methods like the ones that have already been overridden. The provided unit test is already testing writeText to verify this assumption.

2. all #write methods should call the same super.write - currently the write(String) calls super.write(char[]) to avoid confusion (and maybe but unlikely bugs)

I am a bit confused as this statement seems to be in contrast to what you have written in your first review. Afterwards I changed the behavior in c89e67f to fit your requirement.

JFYI: you can also use a for-each loop when looping over arrays, we just avoid it on ArrayLists, to avoid a iterator instance (ArrayLists are used for component lists e.g. and the component tree is traversed very often)

Ok, good to know. However, in this special case it's probably necessary to have a counter since we don't want to copy the array, instead we want to modify the contents of the existing array.

[tandraschko]

1. all right, it would be just cool if we would have some more tests which also covers other #write methods or even writeAttribute - but not required
2. Oh, really sorry - thats my fault because of a too fast review. I thought that you would wrap a simple char with a string now.
   I would just do:

@Override public void write(String str, int off, int len) throws IOException { super.write(new String(xmlEncode(str.toCharArray())), off, len); }

I just wonder if we should replace the invalid char by a blank instead of empty? Not sure...

[cnsgithub]

1. all right, it would be just cool if we would have some more tests which also covers other #write methods or even writeAttribute - but not required

Provided another test for writeAttribute.

2. Oh, really sorry - thats my fault because of a too fast review. I thought that you would wrap a simple char with a string now.
   I would just do:

@Override public void write(String str, int off, int len) throws IOException { super.write(new String(xmlEncode(str.toCharArray())), off, len); }

Done.

I just wonder if we should replace the invalid char by a blank instead of empty? Not sure...

Also considered this. However, it would complicate things since array lengths might change then. When looking at OWASP's encoder you'll find they also replace illegal characters by spaces.