Timeline for A very simple router for a PHP application
Current License: CC BY-SA 4.0
19 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Jan 11 at 7:43 | history | edited | Your Common Sense | CC BY-SA 4.0 |
added 34 characters in body
|
| Jan 10 at 21:21 | history | edited | Your Common Sense | CC BY-SA 4.0 |
added 21 characters in body
|
| Jan 10 at 21:13 | history | edited | Your Common Sense | CC BY-SA 4.0 |
added 830 characters in body
|
| Jan 10 at 21:08 | comment | added | Your Common Sense | @MrWhite would you mind taking a look at the revisited code? | |
| Jan 10 at 21:07 | history | edited | Your Common Sense | CC BY-SA 4.0 |
added 830 characters in body
|
| Jan 10 at 20:51 | comment | added | Your Common Sense | @GregBurghardt no, realpath does nothing good or bad, just materializes a relative path into absolute one. I habitually use basename() as it removes all dots and slashes and I consider it safe enough. | |
| Jan 10 at 20:51 | comment | added | MrWhite |
Using basename() sanitizes the input, it doesn't validate it, so you can end up with a many-to-one (duplicate content) issue. eg. <anything>/login returns /login.
|
|
| Jan 10 at 20:41 | comment | added | MrWhite |
A potential issue with the revised code here is that requests to both / and /home return the same (home) content. @BeeH. "the .htaccess will always set that parameter no matter what" - for requests to the root directory the page parameter is not set (the request is essentially rewritten by mod_dir in this case, not the mod_rewrite rule - at least the way it is currently written.)
|
|
| Jan 10 at 20:40 | comment | added | Greg Burghardt |
Ok, it's not just me. It's been many years since I wrote PHP. Something like realpath($_GET['page']) guards against that, right?
|
|
| Jan 10 at 20:39 | comment | added | Your Common Sense | @GregBurghardt thank you for the warning, that was a brainfart on my part. | |
| Jan 10 at 20:38 | comment | added | Greg Burghardt |
Are you sure require "inc/$page.php"; isn't vulnerable to a directory traversal and code injection attack? What if a malicious client requests /?page=../../hack_the_server?
|
|
| Jan 10 at 20:36 | comment | added | Your Common Sense | @BeeH. in theory, this file could be called directly. But you are right, I blindly followed the original code and failed to think one step ahead | |
| Jan 10 at 20:29 | comment | added | Your Common Sense | @Vilx- man I am getting rusty. What a shame. Thank you a ton. | |
| Jan 10 at 20:29 | history | edited | Your Common Sense | CC BY-SA 4.0 |
added 211 characters in body
|
| Jan 10 at 20:03 | comment | added | Bee H. |
I don't think $_GET['page'] ?? 'home' would work here because, if I understand correctly, the .htaccess will always set that parameter no matter what. In the case that it's an empty string, $_GET['page'] ?? 'home' will then equal an empty string. I tested this using php 8.1.2. Understanding this, it might be specifically validate $_GET['page'], like @vilx suggested, and catch an empty value that way.
|
|
| Jan 10 at 19:48 | comment | added | Vilx- |
I'd like to add that it wouldn't hurt to validate $_GET['page'] a bit. At least make sure it doesn't contain a / symbol. Although I myself prefer to provide a whitelist of allowed pages. Yes, you need to change it when a new page is added, but I see that as a good thing (security-wise).
|
|
| Jan 10 at 14:03 | comment | added | Your Common Sense | Well, traditionally templates are included (as sometimes it can be insignificant part) but on the second thought, in this particular case it makes little sense to make it slack, and require indeed is more appropriate | |
| Jan 10 at 13:22 | comment | added | KIKO Software |
Excellent answer. The only thing I would like to mention is that one script is included with require and the next with include. To me this makes little sense. Both are needed, why would the absence of one generate an error and the other only a warning? I guess it can be choice, but to me it just looks weird.
|
|
| Jan 10 at 11:53 | history | answered | Your Common Sense | CC BY-SA 4.0 |