fixed in WICKET-4256 -igor
On Mon, Nov 21, 2011 at 10:36 AM, Igor Vaynberg <[email protected]> wrote: > Jira this up so we don't lose it... > > -igor > > On Nov 20, 2011 5:48 AM, "Carl-Eric Menzel" <[email protected]> > wrote: >> >> Hi, >> >> I ran into an odd problem this week. A model fed to a ListView was >> calling service methods the current user wasn't allowed to use, and I >> was wondering how that could happen. A panel far above this ListView in >> the hierarchy had been secured (using Shiro annotations, but that turns >> out to not matter at all) and was not supposed to be rendered for this >> user. From this I had expected the ListView not to be rendered either, >> but here it was trying to assemble itself in onBeforeRender and thus >> calling the "forbidden" service methods. >> >> I investigated Component and friends for a bit, and have found a >> potential problem. >> >> internalBeforeRender() checks determineVisibility() before doing >> anything. So far so good. determineVisibility() then checks >> isRenderAllowed() so the application's IAuthorizationStrategy can block >> certain components. This is where it goes wrong though: >> isRenderAllowed() only looks at FLAG_IS_RENDER_ALLOWED for performance >> reasons, and that flag hasn't been set yet! internalPrepareForRender() >> only calls setRenderAllowed() *after* beforeRender(). >> >> Due to this, the supposedly secure panel was going through its own >> beforeRender and thus calling that ListView's beforeRender. >> >> I think this can be a serious problem, such as in my case described >> above. I'd expect that if isActionAuthorized(RENDER) is false, the >> secured component should basically never get to the beforeRender phase. >> My questions are now: >> >> - Is this intentional? If yes, please explain the reasoning behind it, >> because it isn't obvious to me. >> >> - If not, can we fix it? My intuitive suggestion would be to simply >> move the call to setRenderAllowed() from the end of >> internalBeforeRender() (prepareForRender in 1.4) to the beginning of >> that method, so beforeRender() can reliably look at that flag. >> >> - If we can fix it, when and where do we fix it? This hit me in 1.4, >> and looking at the code it's still there in 1.5. I'd *really* like it >> fixed in the last 1.4 release, and certainly in 1.5, given that this >> has the potential to impact security. >> >> It's not an API break, but I'm not sure whether the implications for >> application behavior are tolerable for all existing applications. On >> the other hand, it seems to be a real security problem, so maybe the >> change is justified. I'd like some core dev opinions please :-) >> >> If this is in fact a bug, I'm of course willing to provide a ticket and >> a patch :-) >> >> Thanks! >> >> Carl-Eric >> www.wicketbuch.de >
