? ? Pending

User tests: Successful: 0 Unsuccessful: 0

avatar zero-24
zero-24
1 May 2018

Summary of Changes

If the passed parameter is not a valid form our system is broken at all.

Testing Instructions

  • Enable the profile plugin
  • confirm that the profile fields are added to the frontend band backend views.
  • enable the language code plugin
  • confirm that the language code field is placed in the language code plugin

Expected result

Still works as expected

Actual result

There is a check if there is a valid form. But if at that place is somehow not a valid form the site is broken anyway.

Documentation Changes Required

none.

avatar zero-24 zero-24 - open - 1 May 2018
avatar zero-24 zero-24 - change - 1 May 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 May 2018
Category Front End Plugins
avatar brianteeman
brianteeman - comment - 1 May 2018

before we accept this i think we need to go back to when this code was first merged to see why it is there - the must have been a reason

avatar mbabker
mbabker - comment - 1 May 2018

8851fd3#diff-23e0a7ef508c9ce6eac98c42767a6077

Considering the commit makes no references to a tracker item and the lines in question are part of a much larger change, I'd suggest good luck figuring out the why. At this point I think you're best off going through the code between the form being created and the event being dispatched to see if there's any way the variable would NOT be a JForm object.

avatar Bakual
Bakual - comment - 1 May 2018

I'd say this looks just like a regular sanity check on the passed data.
Which actually makes sense to do since you should never just blindly trust passed arguments.

avatar mbabker
mbabker - comment - 1 May 2018
  1. Typehint the form argument if you really want to force validation
  2. Surely there's no reason for it to be setting an error in the dispatcher even if you do want to do that type of sanity check
  3. My previous point still stands, how in core is the event being dispatched without a valid JForm instance, and if it is indeed possible, why are we not doing this same level of sanity checking against every argument in every event?
avatar zero-24 zero-24 - change - 1 May 2018
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 1 May 2018
Category Front End Plugins Administration com_cpanel com_messages Language & Strings Front End com_contact com_content com_mailto Libraries JavaScript External Library Plugins Unit Tests
avatar mbabker
mbabker - comment - 1 May 2018

@zero-24 I think you broke something here.

avatar zero-24 zero-24 - change - 1 May 2018
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 1 May 2018
Category Front End Plugins Administration com_cpanel com_messages Language & Strings com_contact com_content com_mailto Libraries JavaScript External Library Unit Tests Front End Plugins
avatar zero-24
zero-24 - comment - 1 May 2018

@zero-24 I think you broke something here.

Yes thanks should be fixed now.

avatar Bakual
Bakual - comment - 1 May 2018

It sure can be improved but I don't think removing is the best solution.
Imho setting an error isn't needed. The plugin should just silently refuse the work (return false) instead, maybe log something if you really want.
Typehint means it generates an error, which imho would be even worse.

The only way I could think when that condition is triggered is if a 3rd party calls the event without a valid form. Of course we can question if we want to take measurements for that case.
My understanding is that each code should make sure the data it works with is the expected data. Especially if the data is coming from "outside". So the check itself doesn't seem wrong to me.

avatar brianteeman
brianteeman - comment - 1 May 2018

Considering the commit makes no references to a tracker item
which might have meant it was part of a security fix?

avatar mbabker
mbabker - comment - 1 May 2018

Considering the commit makes no references to a tracker item

which might have meant it was part of a security fix?

Doubtful. The absolute worst thing that check could've fixed was a path disclosure when a PHP warning/error was triggered.

If we really need this level of checking on plugin arguments, then why is ONLY the form argument in these two listeners checked and not every other argument in every other listener?

Typehint means it generates an error, which imho would be even worse.

If the event listener requires a JForm object and someone doesn't pass a JForm object, then rightfully it should fail. By that argument (an error will be triggered on invalid argument type) we should have duck typing for our entire API and no typehinting at all.

avatar Bakual
Bakual - comment - 1 May 2018

If the event listener requires a JForm object and someone doesn't pass a JForm object, then rightfully it should fail.

Agreed. But is it really the job of a random plugin to do that? I always understood plugins in a way that they should not break the request cycle and be more or less transparent to the caller. Checking the argument so I can avoid possible errors by just returning empty would be one of those ways. But maybe my understanding is wrong.

avatar brianteeman
brianteeman - comment - 1 May 2018

@zero-24 if this is approved then the same changes will have to be made to the privacy plugin (if it ever gets merged) as it uses the same code

avatar zero-24
zero-24 - comment - 2 May 2018

Sure I can do a PR than. Lets get some tests and merge than we can extend that to this plugin too.

avatar carlitorweb
carlitorweb - comment - 2 May 2018

I dropped it also in this PR #20275 . So far the tests I did doing this PR work ok.

avatar wilsonge
wilsonge - comment - 2 May 2018

I think this code just predates the time where we were using typechecking across the board. The fields plugins have been using this for a while now and no one has screamed so I'm pretty sure normalising our code on this is just fine

avatar zero-24 zero-24 - change - 17 May 2018
Status Pending Ready to Commit
Labels
avatar mbabker mbabker - close - 23 May 2018
avatar mbabker mbabker - merge - 23 May 2018
avatar mbabker mbabker - change - 23 May 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-05-23 23:53:38
Closed_By mbabker
Labels Added: ?
Removed: ? ?

Add a Comment

Login with GitHub to post a comment