critique my php project

[prolemur]

I posted my project on github today which is pretty much done. I still have some things to do like make more comprehensive variables, formatting code, and create a terms of service & privacy policy pdf, but other than that it's pretty much ready to launch.

 

• cache files
• check x-browser support
• custom 404 errors
• minimize files
• etc

 

 

So if you know anything about php, mysqli, html, css or javascript and have a suggestion or question like "why did you do it like that", It would be greatly appreciated

If you want the commands to create a data base and a user for this site I can supply that code, I'll just need to change the password so you can test it on your local webserver. Note: links rely on mod_rewrite.

 

*The connect.php has not been included in the github repo because it contains user info data and passwords.

[babadoctor]

I posted my project on github today which is pretty much done. I still have some things to do like make more comprehensive variables, formatting code, and create a terms of service & privacy policy pdf, but other than that it's pretty much ready to launch.

 

• cache files
• check x-browser support
• custom 404 errors
• minimize files
• etc

 

So if you know anything about php, mysqli, html, css or javascript and have a suggestion or question like "why did you do it like that", It would be greatly appreciated

If you want the commands to create a data base and a user for this site I can supply that code, I'll just need to change the password so you can test it on your local webserver. Note: links rely on mod_rewrite.

 

*The connect.php has not been included in the github repo because it contains user info data and passwords.

you protect urself from sql injection?

you should do that

or else someone is going to drop all tables

[givingtnt]

 

will you make it hosted ?

[prolemur]

you protect urself from sql injection?

you should do that

or else someone is going to drop all tables

I believe so, all queries that involve user input in any way or any variables uses a prepared statement.

 

@givingtnt Ya I'm going to rent a vps when I receive my credit card in the mail any day now

[babadoctor]

I believe so, all queries that involve user input in any way or any variables uses a prepared statement.

*tries to sql inject project*

*works*

WHOOPS

now just gotta delete all my comments and all of my internet history and 

DELETE EVERYTHING

sudo rm -rf ~

[givingtnt]

I believe so, all queries that involve user input in any way or any variables uses a prepared statement.

 

@givingtnt Ya I'm going to rent a vps when I receive my credit card in the mail any day now

you should check out hostinger.co.uk  they have free hosting and I think they support (for free) sql and php.

 

[vorticalbox]

you should check out hostinger.co.uk  they have free hosting and I think they support (for free) sql and php.

 

they took my text based game offline as "game scripts use to much resources"  the game I made didn't even have any cron jobs.....

[prolemur]

they took my text based game offline as "game scripts use to much resources"  the game I made didn't even have any cron jobs.....

If you want full access to the server and not have that kind of crap happen is to rent a VPS or host something yourself. My internet speed is much too slow and a vps should be exponentially less stressful. Around $60 a year for a little side project is decent, as well as hosting anything else I do and my own website. Good investment in my opinion.

Edited October 31, 2015 by prolemur
... and I'm up at 5am

[N0ps32]

Since you didn't provide the database (???) I can't say for sure but it looks like you've got at least 2 serious XSS issues in your site.

It should be possible to exploit this reflected XSS attack:

<form action = "vote.php?n=<?php echo $_GET[n];?>" method = "post" autocomplete = "off"> 

by submitting this query:

http://yourdomain.com/poll.php?n=%22%3E%3Cscript%3Ealert(1)%3C%2Fscript%3E

some  browsers (chrome) may detect and prevent this attack because it's reflected.

But it should work on Firefox.

 

Also you are not sanitizing your questions/poll options so it should be possible to easily get a stored XSS attack working which browsers can't block.

The application structure is not bad but you should consider looking at the MVC pattern for building web applications.

[givingtnt]

they took my text based game offline as "game scripts use to much resources"  the game I made didn't even have any cron jobs.....

really ? thats surprising

[prolemur]

Since you didn't provide the database (???) I can't say for sure but it looks like you've got at least 2 serious XSS issues in your site.

It should be possible to exploit this reflected XSS attack:

<form action = "vote.php?n=<?php echo $_GET[n];?>" method = "post" autocomplete = "off"> 

by submitting this query:

http://yourdomain.com/poll.php?n=%22%3E%3Cscript%3Ealert(1)%3C%2Fscript%3E

some  browsers (chrome) may detect and prevent this attack because it's reflected.

But it should work on Firefox.

 

Also you are not sanitizing your questions/poll options so it should be possible to easily get a stored XSS attack working which browsers can't block.

The application structure is not bad but you should consider looking at the MVC pattern for building web applications.

Thank you Should parsing that value as an int prevent this or is there a better/more recommended way?

Yes the alert box did appear.

 

Can I sanitize user input with htmlspecialchars function on the question and options when creating the poll (create.php)?

 

Here's my database structure (no frills):

Polls

    id mediumint unsigned primary key auto_increment

    question varchar (154) not null

Options

    poll_id medium int unsigned not null

    id tinyint unsigned not null

    `option` varchar (77) not null

    votes smallint unsigned not null

Votes

    poll_id mediumint unsigned not null

    ipv4 int unsigned not null

 

Thanks for the links as well, this is my first time making anything not-terrible with php

[N0ps32]

Thank you Should parsing that value as an int prevent this or is there a better/more recommended way?

Yes the alert box did appear.

 

Can I sanitize user input with htmlspecialchars function on the question and options when creating the poll (create.php)?

 

Here's my database structure (no frills):

Polls

    id mediumint unsigned primary key auto_increment

    question varchar (154) not null

Options

    poll_id medium int unsigned not null

    id tinyint unsigned not null

    `option` varchar (77) not null

    votes smallint unsigned not null

Votes

    poll_id mediumint unsigned not null

    ipv4 int unsigned not null

 

Thanks for the links as well, this is my first time making anything not-terrible with php

 

Yes htmlspecialchars is enough to prevent XSS attacks.

If the question id is always a number casting to a number is also sufficient to prevent XSS attacks.

I would recommend sanitizing the questions when you display them to the user.

Saving them with HTML characters in the database shouldn't be a problem.

 

Your website is also vulnerable to XSRF attacks.

I would recommend adding a CSRF token to your forms to prevent this.

[prolemur]

Yes htmlspecialchars is enough to prevent XSS attacks.

If the question id is always a number casting to a number is also sufficient to prevent XSS attacks.

I would recommend sanitizing the questions when you display them to the user.

Saving them with HTML characters in the database shouldn't be a problem.

 

Your website is also vulnerable to XSRF attacks.

I would recommend adding a CSRF token to your forms to prevent this.

So I would rather store the exact user input (after trimmed) in my database because there is a limit to the amount of characters they can use and then just convert to htmlspecialchars on display to the user. Changing to special html character codes uses more space than a single character.

 

Yes the id will always be a number.

 

I'll look into this xsrf and adding a token, I think I saw codecourse do a video about this in his php security series which I will definitely watch closely before uploading this site to my vps

 

So many things to protect against when making an online app :mellow: :blink:

[prolemur]

Did another commit with quite a few changes

• external js scripts
• shorthand echo
• intval() of id's put in the url
• made a readme with more info, shows database structure, dictionary lookup
• css optimizations

[Brenz]

 

 

Q: Why not just use coherant variable names instead of having a look-up table?

A: PHP is an interpreted language, therefore a smaller file will take less time to parse.

 

Please don't do this. This isn't a valid reason to not use coherent variable names.

 

This just adds to the development time of anyone else trying to work on or understand your code.

 

EDIT:

 

Also please consider following PSR-2 code standards in your code. In particular:

 

• Tabs should be made up of 4 spaces, not actual tabs.
• All IF statements should include curly brackets
• Code should be indented correctly

[prolemur]

Please don't do this. This isn't a valid reason to not use coherent variable names.

This just adds to the development time of anyone else trying to work on or understand your code.

 

EDIT:

Also please consider following PSR-2 code standards in your code. In particular:

• Tabs should be made up of 4 spaces, not actual tabs.
• All IF statements should include curly brackets
• Code should be indented correctly

I hate spaces as tabs, I'll use my own coding style. Also I get it makes it harder for other people to read it, but at least I included a lookup table which can be used easily since there really aren't that many variables, $m = mysqli, $s = $stmt which are pretty common.

 

I wrote the code mostly for myself and posted here as well as github so others could see it and understand/ciritique as an afterthough / not my main purpose.

 

I am wondering what are the reasons to have inline vs external js?

[Brenz]

I hate spaces as tabs, I'll use my own coding style. Also I get it makes it harder for other people to read it, but at least I included a lookup table which can be used easily since there really aren't that many variables, $m = mysqli, $s = $stmt which are pretty common.

 

I wrote the code mostly for myself and posted here as well as github so others could see it and understand/ciritique as an afterthough / not my main purpose.

 

I am wondering what are the reasons to have inline vs external js?

 

While having your own programming style may seem great if you ever wanted to transfer these skills into an actual programming job you would be expected to code to the defined standards. This isn't just what a few people thought worked best, they are classified as standards and you should try your best to follow them. There are even tools that can help analyse your code and correct any styling issues and learning it now means you will hopefully find it easier to pick up.

 

Mostly its about performance, an external style sheet can be cached by a web browser which is also why you will find things like jQuery and Bootstrap on public CDN's so once a person has that file, any website using the same CDN can use that cached version.

 

When using inline style sheets in PHP files it cannot be cached and will need to be downloaded on every page load which depending on the size can add a considerable amount of loading time, far more than using short variable names.