Am I going about this correctly? [Script]

[Liam]

Well, I am coding a small script, something that I thought would somewhat come in handy for my website, just so I don't have people directly viewing the source of my CSS files, etc.. It's just something for fun I guess, not too big of a deal. And just to improve my PHP skills, slightly (I guess....)

Here is the index.php (the file which does most of the work):

You must be registered to see links

And here is the requested file:

You must be registered to see links

The file being requested, I would replace the ECHO with the file I want to somewhat 'protect' - for example, CSS or Javascript. And of course add the appropriate header for what I'm using, IE: CSS, JS, TEXT

Now, I am not a pro at PHP, I am still learning, per se. I would LOVE all feedback, to help improve my skills, and this little project I'm making. What can be done better? What NEEDS to be done better? Am I going about this right?

I am also a bit confused on how I should LAYOUT my PHP, to make it look simply neat, so if someone wants to show me a demonstration on how it should be properly laid out, that would also be a huge help!

Cheers, Liam. :fanny:

 

[AngraMainyu]

1. Nothing will prevent people from viewing your CSS, JavaScript or HTML. You have to send these to anyone who requests them, so I really don't understand why you're trying to do this. I mean, okay, they won't find the .css file on your server, but they will find it when they go to any other page that requires it. Moreover, if you want to protect files in this way, it's much easier to configure your webserver to block direct requests to certain file types rather than doing it from PHP. Just be careful you don't block access to those files outright.

2. You're using GET parameters as paths onto your hard drive. You've tacked on ".php" and restricted it by IP, which is great because otherwise it would be horribly insecure. As in, without that ".php" extension, a hacker with the ability to make http requests from your server could query your hard drive for any file. .htaccess, passwords, anything. Like I said, your code is reasonably secure thanks to that forced ".php" but I'd avoid using unchecked GET params like that in the future.

3. You can turn those identical if blocks in index.php into an if/else block.

4. Everything else should be okay as far as I can tell, but you might run into problems with 'echo' getting called before 'header' does, in which case the header call just gets ignored.

5. This will kill performance (more than regular PHP.) Static files like CSS and JS can be cached by your server, but as soon as you echo them from PHP you lose that ability. This won't come into play until you're trying to handle decent numbers of concurrent requests, but it means the trade-off between hiding original file names and having to echo everything through PHP makes it so you wouldn't want to do this in a production environment.

 

[Liam]

1. Nothing will prevent people from viewing your CSS, JavaScript or HTML. You have to send these to anyone who requests them, so I really don't understand why you're trying to do this. I mean, okay, they won't find the .css file on your server, but they will find it when they go to any other page that requires it. Moreover, if you want to protect files in this way, it's much easier to configure your webserver to block direct requests to certain file types rather than doing it from PHP. Just be careful you don't block access to those files outright.

2. You're using GET parameters as paths onto your hard drive. You've tacked on ".php" and restricted it by IP, which is great because otherwise it would be horribly insecure. As in, without that ".php" extension, a hacker with the ability to make http requests from your server could query your hard drive for any file. .htaccess, passwords, anything. Like I said, your code is reasonably secure thanks to that forced ".php" but I'd avoid using unchecked GET params like that in the future.

3. You can turn those identical if blocks in index.php into an if/else block.

4. Everything else should be okay as far as I can tell, but you might run into problems with 'echo' getting called before 'header' does, in which case the header call just gets ignored.

5. This will kill performance (more than regular PHP.) Static files like CSS and JS can be cached by your server, but as soon as you echo them from PHP you lose that ability. This won't come into play until you're trying to handle decent numbers of concurrent requests, but it means the trade-off between hiding original file names and having to echo everything through PHP makes it so you wouldn't want to do this in a production environment.

1. Well, to be honest as I said I only indent it to be for learning purposes.. And I am fully aware that people can view the elements in Google Chrome, and C&P the CSS, etc.. But I guess this is to just make things that little bit harder! And those thoughts were considered, that I could just block people from viewing the file(s) through the web-server, but like I said it's to learn PHP

2. Yes, my first intentions were to create a filter() function to filter the GET parameters, but I thought it wouldn't be really needed hence it's going to be a server-side running script (pretty much).. And I manually added the .PHP since it's only going to be reading from a PHP file, which somewhat makes it more secure. And I am fully aware about sanitizing parameters :!

3. Yes I just noticed this after posting my thread, I've since updated the code.

4. is this somewhat better? (

You must be registered to see links

) - If not please elaborate on what you mean!

5. And yes, I am fully aware about it killing site performance.. I took caching into consideration, but noticed there's no way to possible catch anything (to my knowledge)

 

[AngraMainyu]

4. is this somewhat better? (

You must be registered to see links

) - If not please elaborate on what you mean!

Require before echo should work. From the PHP docs:

Remember that header() must be called before any actual output is sent, either by normal HTML tags, blank lines in a file, or from PHP. It is a very common error to read code with include, or require, functions, or another file access function, and have spaces or empty lines that are output before header() is called. The same problem exists when using a single PHP/HTML file.

 

[Ron]

HTML/JS/CSS is parsed by the browser. There is no way to block people from viewing this, otherwise your website will not render correctly.

 

[TimeBomb]

HTML/JS/CSS is parsed by the browser. There is no way to block people from viewing this, otherwise your website will not render correctly.

Indeed. Anything that needs to legitimately be protected should go in the backend. Any major website only minifies+obfuscates+bundles the JS, nothing more.

 

[AngraMainyu]

He's not trying to hide the code itself, just the source file it's from, and only as a proof of concept.

 

[Mucski]

He's not trying to hide the code itself, just the source file it's from, and only as a proof of concept.

And how would that help him? I mean you can always get and view JS/CSS in Chrome, no matter how well you hide it. And I don't need the source, when I can just copy paste it all in my own file. And even obfuscators doesn't work because it can be easily be rewersed. So all in all messing with JS/CSS just to hide, is a waste of time. He needs to focus on backend security not frontend.

 

[AngraMainyu]

And how would that help him? I mean you can always get and view JS/CSS in Chrome, no matter how well you hide it. And I don't need the source, when I can just copy paste it all in my own file. And even obfuscators doesn't work because it can be easily be rewersed. So all in all messing with JS/CSS just to hide, is a waste of time. He needs to focus on backend security not frontend.

You must be registered to see links

You must be registered to see links

You must be registered to see links

 

[Liam]

@TimeBomb & Mucski -

I fully understand what you's both mean, and I've obviously taken things like that into consideration. As per my previous comments, I'm aware that you can't protect files such as CSS in such a way it's 'impossible' to obtain, but it's just a concept, per se. Let's think of it as not protecting any files, let's take the security aspect of the code; being the checks to see if the user has sufficient privileges.. If they do, then we'll run the rest of the code. That in itself will teach me quite a bit, how to correctly go about the security. And then as for the rest of the code (processing, and displaying the CSS/JS files) it teaches me other things, aswell. Which is why I ask for feedback regarding the code itself, not the type of code I'm using it for, since it's my way of easily learning. :

 

[TimeBomb]

TimeBomb & Mucski -

I fully understand what you's both mean, and I've obviously taken things like that into consideration. As per my previous comments, I'm aware that you can't protect files such as CSS in such a way it's 'impossible' to obtain, but it's just a concept, per se. Let's think of it as not protecting any files, let's take the security aspect of the code; being the checks to see if the user has sufficient privileges.. If they do, then we'll run the rest of the code. That in itself will teach me quite a bit, how to correctly go about the security. And then as for the rest of the code (processing, and displaying the CSS/JS files) it teaches me other things, aswell. Which is why I ask for feedback regarding the code itself, not the type of code I'm using it for, since it's my way of easily learning. :

Great, glad to hear that! Sounds like we're all on the same page. In regards to your code:



Here's your code currently (formatted in a more readable manner):

<?php
// Start the check
if(isset($reqFile) && isset($reqExt))
{
	if(!file_exists('./output/' . $reqFile . '.' . $reqExt . '.php')) {
		header($_SERVER["SERVER_PROTOCOL"]." 404 Not Found");
		die('No valid file requested. ');
	}

	if(file_exists('./output/' . $reqFile . '.' . $reqExt . '.php')) {
		if(!in_array($_SERVER['REMOTE_ADDR'], $allowed)) {
			header($_SERVER["SERVER_PROTOCOL"]." 403.8 Access Denied");
			die('You do not have access to this file. '); 
		} else {
			// Success!!
			require('./output/' . $reqFile . '.' . $reqExt . '.php');
			echo ($header);
			// Success!!
		}
	}
}

This looks alright in its current form, though it may benefit from the following:
- You can improve readability and remove comments by setting the conditional expression to a variable, than evaluating that in the if statement.

- Rather than outputting the content in a nested if conditional, let's tell the code to output the content later if that expression (the success expression) is true.

- We can use double quotes " instead of single quotes ' in order to parse variables in the string. You may not want to do this in all cases, but with the page path, it definitely improves readability.

- Always use descriptive variable names. Do this well and you'll almost never need to use comments. Yes, this means spell things out - don't abbreviate.

With this feedback implemented, your code may look something like this:

$pageIsSet = isset($reqFile) && isset($reqExt);
$success = false;
if($pageIsSet)
{
	$pagePath = "./output/$reqFile.$reqExt.php";
	$pageExists = file_exists($pagePath);
	
	if(!$pageExists) {
		header($_SERVER["SERVER_PROTOCOL"]." 404 Not Found");
		die('No valid file requested.');
	} else {
		$allowedToView = !in_array($_SERVER['REMOTE_ADDR'], $allowed);
		if(!$allowedToView) {
			header($_SERVER["SERVER_PROTOCOL"]." 403.8 Access Denied");
			die('You do not have access to this file.'); 
		} else {
			$success = true;
		}
	}

	if ($success) {
		require($pagePath);
		echo ($header);
	}
}

There, isn't that much more readable? You want that! You want to make sure people can read and understand exactly what your code is trying to do. Hell, you want to make sure YOU can read and understand what you were doing when you look back on your code in a few months.

Now, there are always ways to improve. Moving the error strings elsewhere and having a prettier alternative to dieing, e.g. moving the die out of all the if conditionals, would be useful as well. It would also be beneficial to check if the page is not set, and then fail, rather than what's being done now, wherein we nest a whole lot of code in an if conditional only if the page is set. On top of that, we're still nesting if conditionals far too much; this page could definitely benefit from more refactors.

All being said and done, good job. Keep up the good work, and question everything .