Space is encoded in dataset parameter

[eclipse-birt]

Space is encoded in dataset parameter

[SteveSchafer-Innovent]

The change to do the HTML-encode was made back in 2019 as a fix for bug 546816. Since this is a valid fix, we should leave it and decode the parameters just before using them in a prepared query. I'll make the change and submit a pull request for it.

[wimjongman]

Thanks for reporting, Steve. Is there a stack trace that you can attach?

[SteveSchafer-Innovent]

I've been stepping into the code with the eclipse debugger and I've determined that org.eclipse.birt.report.utility.ParameterAccessor.getParameter() unconditionally encodes string parameters using the htmlEncode function of the same class. This happens just before the RunAndRender task is created. If this is appropriate behavior (and I'm not sure it is), then the method that prepares the sql statement needs to decode the strings. Please comment about whether you think this encoding behavior is appropriate.

[wimjongman]

IMO, if parameters are passed in the URL then they need to be encoded and decoded.

What happens when you don't encode?

[SteveSchafer-Innovent]

Well, the parameters are delivered by HttpServletRequest, which automatically decodes them. Then ParameterAccessor re-encodes them, not using URL encoding but HTML encoding. It replaces the space with  , which is an HTML entity. I can't see a reason for this although I imagine there must be one, or why would they have done it? Version 4.6 does not do this. I can remove the encoding, thus making it the same as 4.6, but the question remains why they did it and if it's necessary.

[pipebaum]

Steve, Were there any bug tickets related to this? If not, my guess is that it was to be compatible with their commercial products. Seems to me that decoding and re-encoding is a bit of overkill. Scott

…

On Mon, Mar 22, 2021 at 3:32 PM Steve Schafer ***@***.***> wrote: Well, the parameters are delivered by HttpServletRequest, which automatically decodes them. Then ParameterAccessor re-encodes them, not using URL encoding but HTML encoding. It replaces the space with &#32 <#32>;, which is an HTML entity. I can't see a reason for this although I imagine there must be one, or why would they have done it? Version 4.6 does not do this. I can remove the encoding, thus making it the same as 4.6, but the question remains why they did it and if it's necessary. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKF2BMI4UFHMQ7ZYKVEX6LTE6SNDANCNFSM4ZTJG3NA> .

-- Scott Rosenbaum Innovent Solutions, Inc. 612 220 6006 cell

[pipebaum]

Thanks Steve, yes it looks like it was a security issue so it should stay in.

[SteveSchafer-Innovent]

After digging into the code more I have a better solution. The XSS problem is caused by the format parameter being inserted directly into the HTML in Attributes.jsp. The problem doesn't appear to affect any other parameter. So the best solution is to just use the htmlEncode function to encode the format parameter in Attributes.jsp. The htmlEncode function is already used many other places in the jsp's so I think this was just an oversight.

I have forked the repo and am ready to submit a pull request, but I'd like to run the unit tests first. However when I execute mvn package without -DskipTests, it gets class-not-found errors. Maybe this isn't the correct way to run the unit tests. I'll ask on the newsgroup before creating another issue.

[wimjongman]

Tests are failing ATM. See issue #588

Just create a pull request.

ECA

However, before we can accept PR's you have to jump through some legal hoops.

• Create an Eclipse account
• Your Eclipse account must contain your github id
• You have to sign the eclipse contributor agreement before patches can be accepted.

After that, just create a pull request with commits in the following form:

Space is encoded in dataset parameter #623

meaningful description here after one empty line.

Signed-off-by: Steve Schafer <[email protected]>

If your commits are not like this then the pull request will be rejected.