Wicket
  1. Wicket
  2. WICKET-4256

onBeforeRender() is called on components that are not allowed to render

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3
    • Fix Version/s: 1.5.4, 6.0.0-beta1
    • Component/s: None
    • Labels:
      None

      Description

      pasted from the list:

      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

        Issue Links

          Activity

          Igor Vaynberg created issue -
          Igor Vaynberg made changes -
          Field Original Value New Value
          Fix Version/s 1.5.4 [ 12319051 ]
          Fix Version/s 6.0.0 [ 12315431 ]
          Affects Version/s 1.5.3 [ 12318550 ]
          Igor Vaynberg made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Martin Grigorov made changes -
          Link This issue breaks WICKET-4340 [ WICKET-4340 ]

            People

            • Assignee:
              Igor Vaynberg
              Reporter:
              Igor Vaynberg
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development