Overblown config.inc.php

Hi,

while looking at the code I asked myself why there’s so much stuff done in config.inc.php. Wouldn’t it be a cleaner way to only have variable settings here instead of this create session function and base-path mangling? Maybe there are some reasons I didn’t see...

Agreed.

Great question and I’ve not the foggiest idea why it’s that way?

Quote from: netnoise at Aug 12, 2006, 05:05 AM

Hi,

while looking at the code I asked myself why there’s so much stuff done in config.inc.php. Wouldn’t it be a cleaner way to only have variable settings here instead of this create session function and base-path mangling? Maybe there are some reasons I didn’t see...

Well, the create session function is important to any request, so I don’t see a problem with defining it there. And the base_path mangling as you call it is important to make your site portable without manual intervention (i.e. changing paths in a file or database table).

Quote from: OpenGeek at Aug 12, 2006, 11:25 AM

And the base_path mangling as you call it is important to make your site portable without manual intervention (i.e. changing paths in a file or database table).

That’s the point. It works automatically. I think of a config.inc.php where only user defined (and site specific) things are put instead of any deeper php code. I think this is quite common in other php applications. If an unexperienced user ports his site from e.g. sandbox to production server, he may become confused by all that php in config file even he has only to change his database settings.

I speak of "mangling" because it’s always a hard job to get path/url detection to work under every imaginable os/server. I didn’t want to be rude and this is not meant as a judgement of the code, of course
Anyway I have to mention, $base_url/$site_url doesn’t work as expected, because the method relies on PHP_SELF. This could have some impact on security, but I can’t think of any exploit yet, so it’s most likely a minor problem. (FS#429)

Quote from: netnoise at Aug 13, 2006, 08:57 AM

That’s the point. It works automatically. I think of a config.inc.php where only user defined (and site specific) things are put instead of any deeper php code. I think this is quite common in other php applications. If an unexperienced user ports his site from e.g. sandbox to production server, he may become confused by all that php in config file even he has only to change his database settings.

I speak of "mangling" because it’s always a hard job to get path/url detection to work under every imaginable os/server. I didn’t want to be rude and this is not meant as a judgement of the code, of course
Anyway I have to mention, $base_url/$site_url doesn’t work as expected, because the method relies on PHP_SELF. This could have some impact on security, but I can’t think of any exploit yet, so it’s most likely a minor problem. (FS#429)

I didn’t take it as rude; and I’m not protective of the implementation, just the feature of having it auto-determined. What you say makes perfect sense. In fact, in my 1.0 branch, all I have left in config is the db variables and the path stuff, and after some thought about this, I have an idea to offload the path stuff back to the initialization of the main modX class, allowing users to override the auto path determination routine with system settings stored in the db. And in addition, since it uses PDO, we can offload the db config to a named DSN which can be stored in an ini setting. Or, for those without native PDO support, a way to specify a file outside the web root where these important details are stored.

Also, since the session, path, and possibly even the db config code would be out of the config file and become functions of a class, it would reduce the ability of hackers to access and/or modify important global variables like you suggest they can with $base_path now.

Quote from: OpenGeek at Aug 13, 2006, 11:30 AM

[...] and I’m not protective of the implementation, just the feature of having it auto-determined

This is indeed a really good feature - many other apps missing it.

Using a DSN or config outside webroot is a great idea. I’m really excited to see ModX 1.0

I see your point netnoise but I guess it’s has to do with the way of one’s thinking. The "config" file only contains the session start function and the path detection code which allows an extenal system to gain access to the MODx base path and database without requiring the MODx parser.

IMO a chain is strongest at it’s weakest link. If users can read/write to your config file then there’s nothing that you can do to prevent them from hacking your database.

Jared ran into something today that deals with this too. If there’s a blank config file in your manager folder (proactive developer preparing for a fresh install), the installer link won’t kick in, because the start session stuff is missing... dies with a nasty error. We need a bit of double-checking here I think.

Quote from: xwisdom at Aug 14, 2006, 09:48 AM

I see your point netnoise but I guess it’s has to do with the way of one’s thinking. The "config" file only contains the session start function and the path detection code which allows an extenal system to gain access to the MODx base path and database without requiring the MODx parser.

That’s why the parser should be separated from the API, as should request handling and response handling. Encapsulation, easier maintenance, and so you only load the things you need on each usage of it. In MODx 1.0, these will be all part of the initialization routine of a Context, but will be within the modX main class. In this way, no globals are available for hackers to access or modify since these things occur within the scope of a class, you can access various specific contexts of the site definition (again only loading what is necessary for that context); the parser is only called if content needs to be parsed. Same with the modDataManager: only called if tables need to be created or a structure modified. And same with modResponse, only called if modX is responsible for the output. This modular design will allow us to take the config file problem out of the equation, by allowing it to be placed anywhere that seems most secure for the environment, and potentially, by allowing named DSN connection strings to be configured in the PHP ini, so no database connection information is accessible at all outside of the PHP interpreter.

@Ryan, the installer has it’s own session stuff, though are you saying on upgrades it tries to load the old one and use that function?