You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by GitBox <gi...@apache.org> on 2020/10/08 03:20:36 UTC
[GitHub] [velocity-tools] JHHAX opened a new pull request #9: Fixed Reflected XSS Vuln
JHHAX opened a new pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9
Velocity Tools has an automatically generated error page, which echoes back the file name unescaped. This commit sanitizes user input and fixes the XSS Vulnerability!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] natechadwick commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
natechadwick commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716284428
This is a shared library so I can see @mkienenb point on compatibility. They may be relying on that exception as it was documented or expected to be thrown from that API. This Is this going to create a security issue for any Velocity Tools users even if we aren't using view / mvc packages but are using Velocity Tools. If it is just an encoding issue in the error message - and that fixes the problem, why not just do that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX edited a comment on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX edited a comment on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705364135
@michael-o
I have now used StringEscapeUtils to patch the XSS. Apologies for the inconvenience
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] mkienenb commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
mkienenb commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716539378
@natechadwick No, this can only cause an issue if you use the VelocityViewServlet.error() method either directly or indirectly. It will not affect you if you are not using the view subpackage, or if you have subclassed error() in such a way that it will not call VelocityViewServlet.error(). However, in that case, you'd want to check your own code to insure that you were not causing the same issue by displaying an unescaped path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] mkienenb commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
mkienenb commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716588542
@michael-o Thanks. Can you either cancel your review or approve the changes so far?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705364135
I have now used StringEscapeUtils to patch the XSS. Apologies for the inconvenience
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716663835
Please squash
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-714055078
> The cheapest is to drop:
>
> https://github.com/apache/velocity-tools/blob/098a993730c9887c03a56a99224f5a0a54b4dca1/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/VelocityViewServlet.java#L240-L242
>
>
> The container will do the rest in displaying HTTP/1.1 500.
Have we decided that this is the best course of action?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705422154
Interesting.....
Im not familiar with what you intend to do with `response#setError()` would it be possible if you were to add the commit?
Kind Regards,
Jackson Henry
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-715655059
Thanks @mkienenb !
I am fine with whatever you all think is best. I would just like this to patched as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705364135
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716550276
>
>
> This is a shared library so I can see @mkienenb point on compatibility. They may be relying on that exception as it was documented or expected to be thrown from that API. This is going to create a security issue for any Velocity Tools users even if we aren't using view / mvc packages but are using Velocity Tools. If it is just an encoding issue in the error message - and that fixes the problem, why not just do that?
I have checked the API. `error()` implements what `HttpServletResponse#sendError()` defines. Server API requires `#sendError()` to generate an HTML page with an error description. Throwing out/reducing `error()` would still comply with the contract for two reasons:
1. If we can properly determine the cause we can populate `#sendError()`
2. If we cannot we throw an exception and the servlet container *must* catch and invoke the HTML page handler
In both cases, an HTML page is emitted.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-721010302
Please squash
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-715345255
@arkanovicz The catch clause.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] ChristopherSchultz commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705591304
How can an attacker affect the file name of the Velocity template? I have never seen an application with a template file named `<script href="https://badsite.com/bad.js"/>.vm` before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX closed pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX closed pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716575664
@mkienenb I accept this approach as intermediate step. For the next at least minor version it'd be best to rework.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716117442
>
>
> @michael-o As I stated privately, removing the catch clause will not fix the issue -- that's not the catch that's triggered, and it'll break backwards compatibility (expected behavior). Nor will it fix the problem for anyone who may be calling error from a subclass.
>
> @JHHAX's simple fix which escapes path is the correct one to use.
I don't share this opinion. With the removal of the code your potential security issue would be gone. I see no benefit exposing Velocity internal information to the user besides saying 404 and the request path is not available. Moreover, HTML is not a guaranteed nor stable interface to provide any backward compat.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX edited a comment on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX edited a comment on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705364135
@michael-o
I have now used StringEscapeUtils to patch the XSS. Apologies for the inconvenience
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o merged pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o merged pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-720907360
Hey guys,
are we going to merge?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] ChristopherSchultz commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705591304
How can an attacker affect the file name of the Velocity template? I have never seen an application with a template file named `<script href="https://badsite.com/bad.js"/>.vm` before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705387417
Looking at the code, it deserves to be removed altogether and replaced witth `response#setError()`. No custom handling.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-714295423
@arkanovicz If you don't mind, I'd throw this out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705387417
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] michael-o commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-705456118
The cheapest is to drop: https://github.com/apache/velocity-tools/blob/098a993730c9887c03a56a99224f5a0a54b4dca1/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/VelocityViewServlet.java#L240-L242
The container will do the rest in displaying HTTP/1.1 500.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-722136523
Done!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] arkanovicz commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-715273301
@michael-o What do you want to throw out? The merge request or the catch clause?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] JHHAX commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
JHHAX commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716844480
Thank you all!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] natechadwick edited a comment on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
natechadwick edited a comment on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716284428
This is a shared library so I can see @mkienenb point on compatibility. They may be relying on that exception as it was documented or expected to be thrown from that API. This is going to create a security issue for any Velocity Tools users even if we aren't using view / mvc packages but are using Velocity Tools. If it is just an encoding issue in the error message - and that fixes the problem, why not just do that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] mkienenb commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
mkienenb commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-716565224
@michael-o If you want to redesign how VelocityViewServlet handles errors in a separate release and PR, that'd be fine.
For a security fix and release, we should be making the change that fixes the problem as widely as possible (every path to the potential XSS) and makes as small of a change as possible so people can read the change, see that it will not impact anything they have done, and drop in the fix without having to make any other changes.
Escaping the path is the best fit for those criteria.
Right now, callers of error() expect to see the internal details of what went wrong. You may not agree that is a good idea, but that's the expected behavior. Changing that is not a minor change in how their application will behave, especially as we have no idea who is calling error() or for what reasons. As you say, they may be doing something insane and parsing the html. A security fix is not the place to be making behavior changes if they can be avoided.
@michael-o
Note that I have no vested interest in the current behavior of error() -- I don't use VelocityViewServlet.error() -- and that I am only looking out for the interests of other velocity end-developers who do use it. It is difficult for me to understand why we are not going with the simple trivially-verifiable fix for this with known minimal impact and making a security release rather than discussing how we should redesign error handling.
Don't force users to pick up behavior changes for a security fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
[GitHub] [velocity-tools] mkienenb commented on pull request #9: Fixed Reflected XSS Vuln
Posted by GitBox <gi...@apache.org>.
mkienenb commented on pull request #9:
URL: https://github.com/apache/velocity-tools/pull/9#issuecomment-715386248
@michael-o As I stated privately, removing the catch clause will not fix the issue -- that's not the catch that's triggered, and it'll break backwards compatibility (expected behavior). Nor will it fix the problem for anyone who may be calling error from a subclass.
@JHHAX's simple fix which escapes path is the correct one to use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org