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 ...
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
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
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
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
Your code looks very good. I like the way you are using Dependency Injection. This code is a good example for other people to follow.
Here are just a few points, seeing as no-one else has answered.
1. How can I improve the existing code?
Move the work that you do while constructing the object out of the constructor. Miško Hevery says it better than ...
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
On principle the code is sound except for one issue: you do not check if the language is "valid". This raises some questions:
How will your localization code behave if I added a lang parameter with the value "foobar"?
How will it behave if I try to do an SQL injection using the parameter value?
In both cases, the sane outcome would be to default to your "...
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
It was not easy, but the first step for improving your code is to split the layout from the logic:
layout.php
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">
<head>
<title>Welcome to albsocial</...
5
Docstrings
This:
def open_connections(sessions):
""" open_connections(sessions)
Opens ssh connections and forks them to the background. Passwords are sent
from the user via STDIN to each process. If correctly identified, the
process is successfully created and added to the Session object.
"""
for server in sessions:
# ...
...
5
usr = raw_input("Enter username for " + server.address + ": ")
pwd = getpass.unix_getpass("Enter passphrase for " + server.address + ": ")
con = paramiko.SSHClient()
I'd avoid using abbreviations. Call them username and password and connection. You don't gain anything using abbreviations and it reduces readability.
for session in sessions:
for ...
5
I've seen many attempts to abstract particular ORM from application, and all of them at a certain stage of maturity had to break this abstraction. One of the reasons for that is when you need to optimize certain use cases (like eager-loading related entities, or combining several round trips to server into one) you'll need to use ORM specifics that you're ...
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
I agree with MECU about not storing everything in the session. Caching is probably the best way to go. Sessions are typically used for continuity between page loads. Meaning you can store information like the character ID or login status, but the rest should be done differently. So, even though I'm about to start explaining how to better do what you are ...
4
In addition to losing the hungarian notation there are a few things worth mentioning:
Don't use try-catch construction with empty 'catch' clause. It is almost always a bad idea to hide an exception if it was thrown.
It would be better to check explicitly if the key is in the collection using appropriate methods. BTW exception handling is quite expensive ...
4
You said in a comment that you're storing half of the encryption key alongside the data the data you're encrypting, and the other half in the code itself. What's the point of that? If they have access to the PHP files, they probably have access to the session files, too.
There's basically no point in calling srand() with no arguments. It doesn't make its ...
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
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
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))
{
...
Only top voted, non community-wiki answers of a minimum length are eligible
