On Wed, Jul 2, 2008 at 4:21 PM, Nathan Bubna <[EMAIL PROTECTED]> wrote:
> On Wed, Jul 2, 2008 at 12:59 PM, Will Glass-Husain > <[EMAIL PROTECTED]> wrote: > > Should Velocity be sanity checking these type of arguments in internal > > functions? I open that question to our other experienced developers. > > Henning has advocated null checking in the past, perhaps this is a > similar > > issue. > > no thanks. what would we do if we spotted such an overflow? throw an > error, but that's already what will happen. we could perhaps put a > more explanatory error message in place, but in most cases described, > the user is doing something very extreme to cause it. i expect it > should be pretty obvious to them that their 2+ million argument method > call or macro or 2+ million node template might be the source of their > problem. i also have never heard of anyone running across these in > all of my many years reading these lists. i don't think it's worth > the effort. the Parser and VelocityCharStream ones are perhaps a bit > more likely to occur, but again, i have not heard of that happening. > i say leave them be. > I concur with Nathan. Given that these exceptions would be triggered on theoretically possible circumstances but realistically insane practice I see no reason to waste anyone's time on it. I have enough information from this discussion to state that case then turn it around on them and make them explain their risk assessment. > > > Regarding WebMacro in the velocity.jar. This is a utility class, intended > to > > help WebMacro users migrate to Velocity. Does your app permit users to > call > > main methods? If not, doesn't seem significant. If your security > concerns > > are strong enough to warrant removal of this method, I recommend creating > a > > simple ant script to do a custom build of the Velocity jar that removes > this > > class. (There are no dependencies on it). > No we don't allow calls to main methods. Removing WebMacro from the jar is easy enough to do. (though not at a security concern, more a client management concern) > > > > > Regarding the use of Random in Math.tools. It's simply a pass through to > > the Java function. No worse or better than Java. Don't like it? Don't > > configure your app to use MathTools. Write your own tool-- it's easy. > Nope, we don't use it. Looks like its not used for anything worrisome from your end so I'm not worried. > > > > > I think it's unlikely we'll remove error messages from the log files. We > > find most users of Velocity find these helpful. If this is an issue, > > comment them out and recompile the code. Or (if you don't want to fork > > the source) create a custom logger that ignores those comments. > > > Not a major deal to me. Hope that these lows didn't come across as being considered a real issue to me > > > One more suggestion to Tom. It seems that the integer overflow issues > are > > probably the most troubling to your auditors. It's unlikely we'll > rapidly > > change these (low priority to the rest of us). However, if you or a > > colleague were to go through these 9 methods and add argument-checking > code, > > then submit in a patch, we'd probably add it in to the base. (Would be > > interesting to get other perspectives on this first). > > i wouldn't veto, but don't expect me to waste any more time on it. :) > I'll save that as a backup plan. Given the discussion above, I don't see it having to come to that. Again, thanks all for taking the time. I appreciate it. Tom
