13
Right, I'll be adding to this answer later on, but for a kick-off, here's a few quick remarks/considerations:
On the hidden input fields
As Martijn said in his comment: most bots will probably parse the DOM, and filter out those fields that are marked as required (looking for labels with a * text node in them), and they'll simply ignore hidden fields. You'...
10
You don't post the code that interacts with database, there is the main security concern. I hope you use PDO prepared statements, which will keep you safe from SQL Injections.
As for non-security-related problems, there are quite some of them.
Consider creating multiple functions for form submit processing, for example:
function process_form_submit($...
10
Ok, moving down the list...
I would separate the content of the includes folder into 2 semantically named folders: includes and classes - or if you want to eventually turn this into an MVC, maybe views and classes would be more appropriate.
It's not uncommon to put database credentials in constants, but truth is you should never have more than one ...
9
Commented out Code
You should remove code that is commented out. If you think that you might need it in the future, think about using version control.
set_session_id()
This is only called in your commented out code. Does the user have to call it manually? If they don't, get_session_id() will return a wrong result. Maybe rewrite it like this (you don't ...
8
First up, that's a pretty good effort for a couple days of experience, you have obviously done some reading yourself.
To be honest it is, what I call spaghetti code, echo html statements in between logic. While it works fine (and is how most people learn), it becomes a pain to maintain in the long term.
Rather then straighten out the spaghetti, I have ...
8
At a quick look:
Your code is vulnerable to SQL Injection: assume the user wants to hurt you, so always parse superglobals $_GET and $_POST
https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php
To check if variable have values:
// good practice
if (isset($userName, $userPass))
// bad practice
if ($userName && $userPass )
...
8
This is indeed quite messy. It's really hard to see which else ends which if.
Just invert the if and return early:
if( !isset($_SESSION["access_token"], $_SESSION["user_email"], $_SESSION["username"]) ){
return false; // mean doesn't have account. Register user and send verification to email
}
if(!$GoogleAccess->does_account_exist){
return "...
7
First thing to do is use proper indentation for your class.
Remove indentation on the code after your second if statement in your login function.
you probably don't need to start the session in the logout function
public function logout(){
session_start();
unset($_SESSION['id']);
unset($this->id);
session_destroy();
$logged_in = ...
7
This is pretty simple code, with not a lot of room for improvement.
You can make the session variable final, and it's good to do so:
private final ConcurrentHashMap<String, ConcurrentHashMap<String, String>> session = new ConcurrentHashMap<String, ConcurrentHashMap<String, String>>();
You can simplify createSession a bit by ...
6
MVC
If you're trying to make a CMS, then there's a possibility that this CMS will become bigger than you first though. More pages, more modules, more code. You might want to consider doing an MVP approach to splitting code (rather than MVC).
A good reference framework for this is CodeIgniter. Presenter (CodeIgninter calls it the Controller) receives a ...
6
Let's see the code:
Your Session class does too many things:
Session management
Login
Data validation
Logout
Authentication
Inclusion management
Your class has no clean way to destroy a session.
You don't verify if a session is active.
Your constructor seems a little off. Sessions are a singleton. Use a single static final class.
You don't need to repeat ...
6
Security
I am not sure if I have done the same thing as explained there
Is my code a good way to secure remember me cookie ?
No and no.
Non-Random Token: Login as anyone
The only data you store in your cookie is the username*. But usernames are generally not considered secret, and are relatively easy to obtain.
Because of this, an attacker ...
5
I agree with @janos's assessment of your code:
the singleton enum is done right
the sessions should be final
comments are 'off'
I would suggest you change the basic interface for your sessions though. Your session service should be a more 'opaque' one: your session 'client' has to first create a session, then get it. Also, you create a new ...
5
In general functionality is fulfilled but I'd improve it a bit.
Below you will find incremental (further ones will contain previous improvements) list of improvements.
Split get to two methods, get and peek, the former one would delete the returned flash message and the latter only return it. Optional parameters that affect method logic suggests that the ...
5
Some Notes:
Your use of private variables to hide the inner workings of your class is a very good idea. This is one of the most founding principles of OOP, and you understand that part very well it seems. :)
The Session class seems responsible for more than just a session, though with the small size of this programme it's hard to differentiate it without ...
5
When looking over your code I find several things I would change. It should be said that I am no expert in security, so I will forward you to places where you can get better guidance than I can about the specific issue. So without further ado, let's go!
The first thing which pops into my mind when reading your code is the singleton. The singleton pattern is ...
5
Security
You are escaping user input, which prevents SQL injection. It's good enough, but really not the recommended way to do this as it's too error-prone (whenever a project does this, it's basically guaranteed that you can find injections). Use prepared statements instead. It's not only more secure, but also leads to nicer code.
Guard Clauses
You have ...
5
isset() accepts multiple parameters, you can simplify:
if (isset($parameters["inserted_id"]) && isset($parameters["inserted_password"])) {
To
if (isset($parameters["inserted_id"], $parameters["inserted_password"])) {
When ever I read anyone's code or database schema and I see any mention of id, I immediately assume that I am dealing with a ...
4
It seems to me like it would probably make sense for the members of Auth to be static, though this may be for some reason undesirable to you. Change it as you see fit.
Your style contrasts with the industry standard in that you are putting braces on their own lines for control structures (i.e. if). If this is purely a personal project then it's your choice, ...
4
The code is understandable, but it can be simplified.
Instead of:
public function unset_session()
{
if (isset($this->session))
{
$this->session = null;
}
}
You can simply:
public function unset_session()
{
$this->session = null;
}
Instead of:
public function get_session()
{
if (isset($this->session))
{
...
4
Checking an "constant" IP address is an bad idea because of ISP with load balancing. IP address should change to less busy proxy.
In most of cases IP address belongs to the same block. It is called CIDR matching:
function cidr_match($ip, $range)
{
list ($subnet, $bits) = explode('/', $range);
$ip = ip2long($ip);
$subnet = ip2long($subnet);
...
4
I like this question because I think it has a lot of answering potential. I'm going to touch on a few things, but my answer shouldn't be considered complete!
Security of custom sessions
There's one thing that's most important here: don't roll out a security system that's been built with only one pair of hands. It's asking for a disaster! I won't do the ...
4
I have been using php for over 10 years and never found a need for a session wrapper, but maybe you find it easier/better.
There are a few improvements that could be made with your code
If we are checking to see if the session has a key, a simpler test is
public static function has($Key)
{
// return (bool)(isset($_SESSION[$Key])) ? $_SESSION[$Key] : ...
4
Security
This code doesn't look secure at all.
SQL injection
SELECT * FROM bc_users
WHERE user_id = "'.$_COOKIE['bc_qanda_user_id'].'"
As anyone can set the cookie value to what they want, this is open to SQL injection (from valid users only, but still).
$user_q = $mysql->query('
SELECT * FROM bc_users
WHERE username = "'.$...
4
To prevent brute forcing of the PHP Session ID you should use configuration rather than rolling your own security.
From OWASP Session Management Cheat Sheet - Session ID Length:
The session ID must be long enough to prevent brute force attacks, where an attacker can go through the whole range of ID values and verify the existence of valid sessions.
The ...
4
Your session logic seems good to me.
Prepared Statements
Use prepared statements.
It's unclear where $_SESSION['useracctid'] comes from, but it is placed directly inside the query, which is a bad idea. It might have been cleaned previously (currently, it's cleaned indirectly, via substr and clean on $useracctid, but you didn't give that function), or it ...
4
First, all of your indentation is half indentation: you two-spaces instead of four.
Most IDEs have proper spacing if you press TAB.
Naming:
You have variables need re-naming:
$_SESSION['log']: a log is a regular or systematic record of incidents or observations, not whether you've logged in, which has entirely different meaning, also the same applies to ...
4
Some notes:
@session: A method called find_or_create_session should not change the state of instance variables.
@sessions.find_by facebook_id: fbid is written twice. Use variables to avoid repetition. Also, the consensus in the Ruby community is to always write parens (except for DSL-like code).
Time.now - @session.last_exchange: You can perform that check ...
4
I've never used these libraries so I only have surface-level comments, but I'm answering anyway because I feel bad you've been ignored. Luckily, I think the lack of answers is at least partly because your code look totally fine. It's much easier to critique worse code.
In particular, though you've "never really programmed with generics before", you're ...
4
Please don't use regexes
Regular expressions are power tools. You can use them for many situations, but they tend to be inherently fragile. They're especially fragile for parsing HTML, in part because of how lenient most browsers will be. I'd recommend using a dedicated parser for it. If you decided that since this is a page you control you don't need to ...
Only top voted, non community-wiki answers of a minimum length are eligible