Zikula Community

• Grant29
  
  

  • Rank: Helper
  • Registered: 15.07.03
  • last visit: 11.04.06
  • Posts: 261

  Posted: 01.03.2006, 01:36

  Hi,
  
  I have a module that's failing to load on 0.762. The module works fine on 0.761.
  
  I tracked it down to a change in the pnModLoad function of the includes/pnMod.php function.
  
  I was passing an URL like:
  
  

  Quote

  
  http://gentoo1/websites/mysite/index.php?module=mymodule&type=adminapi&func=savesetup
  

  
  
  and I got this error:
  
  

  Quote

  
  Failed to load module mymodule (at function: "savesetup")
  

  
  
  I tracked the root cause of the error down to 3 new lines in the pnModLoad function:
  
  

  Code

  if (strtolower(substr($type, -3)) == 'api') {
           return false;
       }

  
  
  What is the purpose of adding that piece of code? My module justs submits data to my savesetup function and let's that function store the data in the database.
  
  Commenting out those 3 lines of code lets my module run fine.
  
  Is this a permanent change to the pnModLoad function? I can't figure why it needs to be there.
  
  Thanks,
  Grant
  
  --
  Retail Retreat
  http://www.retailretreat.com

  • 
  •  
• Chestnut
  
  

  • Rank: Steering Committee
  • Registered: 29.08.02
  • last visit: 27.06.09
  • Posts: 1201

  Posted: 01.03.2006, 02:12

  Although I don't exactly know why and when it was added as it wasn't from me, it seems totally logical to me.
  
  An API function should NOT be called via an URL. The purpose of an API function is an execution but calling an execution directly from an URL without any form of validation is really Bad Behavior from your module as it can lead to problems if permissions are not checked the appropriate way.
  
  IMHO... you should change your savesetup function to pnadmin.php wich will call the relevant PHP code that saves the data in your DB via pnModAPIFunc.
  
  
  
  --
  Chestnut !
  Support via Private message won't be answered...
  http://dev.pnconcept.com
  http://www.postnuke-france.org

  • 
  •  
• Grant29
  
  

  • Rank: Helper
  • Registered: 15.07.03
  • last visit: 11.04.06
  • Posts: 261

  Posted: 01.03.2006, 02:34

  That'd be a simple enough change (except that I've done this same sort of thing on many of my modules). I have always included permissions and the authentication key checks when passing forms back to the pnadmin.php and pnadminapi.php files.
  
  Adding the code:
  

  Code

  if (strtolower(substr($type, -3)) == 'api') {
           return false;
       }

  
  
  doesn't make pnadmin more secure than pnadminapi if someone doesn't do the checks. Moving the code from one file to another can be just as insecure if security is no properly implemented.
  
  I can see benefits to both sides of the implemention.
  
  I generally use the pnadmin functions to generate page output, and the pnadminapi functions to retreive/save/delete/edit configuration type data.
  
  Thanks,
  Grant
  
  --
  Retail Retreat
  http://www.retailretreat.com

  • 
  •  
• Chestnut
  
  

  • Rank: Steering Committee
  • Registered: 29.08.02
  • last visit: 27.06.09
  • Posts: 1201

  Posted: 01.03.2006, 08:54

  Grant29

  
  doesn't make pnadmin more secure than pnadminapi if someone doesn't do the checks. Moving the code from one file to another can be just as insecure if security is no properly implemented.

  
  
  True, but at least, it ensures that what is suppose to be an execution only is not called directly via an URL.
  
  As an example in VB or VBA, you can't call the function GetComputerNameA from the Kernel directly... you must declare, load and alias it. (Although not specifically an execution, you can compare pnuserapi and pnadminapi as dlls or libraries).
  
  

  Code

  '-- Declares for pnGetComputerName() ----------
  Declare Function GetComputerName& Lib "kernel32" Alias "GetComputerNameA" (ByVal lpBuffer As String, nSize As Long)
  '------------------------------------------------
  
  public function pnGetComputerName() as String
  
  'DECLARATIONS:
  '-------------
      Dim s$, dl&
     
  'INITIALIZE:
  '----------
      On Error GoTo ErrHandler
  
  'MAIN BODY:
  '---------
      s$ = String$(255, 0)
     
      dl& = GetComputerName(s$, 255)
         
      pnGetComputerName = Left$(s$, InStr(s$, chr$(0)) - 1)
     
  'WRAPUP:
  '------
  WrapUp:
  exit function
  
  'ERROR HANDLER:
  '-------------
  ErrHandler:
      pnGetComputerName = ""
  
      'ErrHandler Code goes here
  Resume WrapUp
  
  End Function

  
  
  
  
  --
  Chestnut !
  Support via Private message won't be answered...
  http://dev.pnconcept.com
  http://www.postnuke-france.org

  • 
  •  
• rgasch
  
  

  • Rank: Professional
  • Registered: 06.01.03
  • last visit: 30.06.09
  • Posts: 603

  Posted: 01.03.2006, 09:22

  Yep, this is a security fix. I ran into the same issue with some of my modules. The easiest solution: rename pnuserapi.php to pnuserform.php and change your calling URL/Form accodringly. This is a solution which will remain valid/compatible with future PN versions ...
  
  Greetings
  R

  • 
  •  
• Grant29
  
  

  • Rank: Helper
  • Registered: 15.07.03
  • last visit: 11.04.06
  • Posts: 261

  Posted: 01.03.2006, 13:21

  Thanks for the input guys, I'll start designing/fixing my modules to fit to the new standard.
  
  Thanks,
  Grant
  
  --
  Retail Retreat
  http://www.retailretreat.com

  • 
  •  
•  
•  

Users online

• Simon,
• Teb,
• Wendell,
• craigh,
• jmvedrine,
• shyra,
• thezikulan

This list is based on the users active over the last 60 minutes.