Get started #1

AnshulMalik · 2017-08-15T02:15:02Z

…ics upload

AnshulMalik · 2017-08-15T02:16:04Z

.idea/vcs.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


We should probably add a gitignore file

AnshulMalik · 2017-08-15T02:18:10Z

app.js

+    }
+});
+app.get('/speaker' , function (req , res) { 
+	res.sendFile(__dirname + '/www/speaker.html');


How about we use Express's static feature to serve static files? It'll be much cleaner.

https://expressjs.com/en/starter/static-files.html

AnshulMalik · 2017-08-15T02:19:36Z

app.js

+    var post = req.body;
+    var username  = post.user;
+    var password = post.password;
+    var sql = "SELECT * FROM login WHERE username='"+username+"'";


We should play safe here, you must have heard of SQL Injection. @rewanth1997 knows better.
Let's sanitize user input before sending it to mysql.

AnshulMalik · 2017-08-15T02:19:59Z

app.js

+    var name  = post.speakername;
+    var topic = post.topic;
+    var description = post.description;
+    var sql = "INSERT INTO speaker(name, topic, description, pic_url) values('"+name+"','"+topic+"','"+description+"','/images/"+post.images+"')";


Same sanitization logic applies here.

AnshulMalik · 2017-08-15T02:21:55Z

app.js

+    });
+}
+// Starting Server
+server.listen(3000, function(){


We will need this port to be configurable from environment variables, so can use

const port = process.env.PORT || 3000; server.listen(port, () => ...)

AnshulMalik · 2017-08-15T02:23:51Z

app.js

+    var name  = post.speakername;
+    var topic = post.topic;
+    var description = post.description;
+    var sql = "INSERT INTO speaker(name, topic, description, pic_url) values('"+name+"','"+topic+"','"+description+"','/images/"+post.images+"')";


Template literals make the code cleaner, we can use those here.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals

AnshulMalik · 2017-08-15T02:24:31Z

package.json

+  "description": "backend for tedx nit k",
+  "main": "app.js",
+  "scripts": {
+    "test": "node app.js"


Shouldn't this command be start instead of test?

AnshulMalik · 2017-08-15T02:25:43Z

www/admin.html

+    });
+</script>
+</body>
+</html>


Every file must contain an empty line at the end :)

…jection, env ports add , image upload done

AnshulMalik · 2017-08-15T15:13:25Z

app.js

@@ -9,6 +9,8 @@ var bodyParser = require('body-parser');
 var session= require('express-session');
 var multer = require('multer');
 var mysql = require('mysql');
+var sanitizeHtml = require('sanitize-html');


This is cool, we'd also like to prevent sql injections

AnshulMalik · 2017-08-15T15:15:16Z

app.js

+    });
+}
+function insertVideo(req , res ) {
+    var post = req.body;


Let's use let and const instead of var

rewanthtammana

This is nice start @nkkumawat.

Here are a few tips:

Make sure you follow the same style of coding in all your files.
Assign appropriate names to the API endpoints and other variables.
Use appropriate commit messages, so that it will be easy for us to review your code.

Do not push multiple parts of code into a single commit message like this, "sanitizition before sql injection, env ports add , image upload done". This is a very bad way to do commit messages.

Instead prefer using single commit message for each change, it makes easy for us to review the code. Before merging them into the master branch we can squash the commits to make the code look more elegant.

rewanthtammana · 2017-08-15T15:20:33Z

app.js

+    resave: true,
+    saveUninitialized: true
+}));
+var Storage = multer.diskStorage({


Why is this in variable starting with Caps letter? Any special reason, if not use small case.

not for any special reason.

rewanthtammana · 2017-08-15T15:23:42Z

app.js

+    });
+});
+app.post("/videoinsert", function(req, res) {
+    insertVideo(req , res);


Prefer not to use create functions if you are not reusing the code. Insert the code here itself.

You used the same style of coding everywhere. Change it wherever required.

rewanthtammana · 2017-08-15T15:26:49Z

app.js

+    insertVideo(req , res);
+});
+function login(req,res){
+    var post = req.body;


Use appropriate names is wherever required. Its the standard way of coding.

const body = req.body

rewanthtammana · 2017-08-15T15:31:01Z

app.js

+    var url = post.url;
+    title = sanitizeHtml(title);
+    description = sanitizeHtml(description);
+    var sql = "INSERT INTO videos(title, description, video_url) values('"+title+"','"+description+"','"+url+"')";


url variable is not sanitized. This type of SQL query is easily prone to attacks. Make sure to sanitize each and every parameter.

rewanthtammana · 2017-08-15T15:32:09Z

package.json

+    "path": "^0.12.7",
+    "sanitize-html": "^1.14.1"
+  }
+}


Add new lines at EOF, asap.

rewanthtammana · 2017-08-15T15:45:47Z

www/css/main.css

+    color: white;
+    font-family: hBold;
+    border: 1px solid rgba(0, 0, 0, 0.1);
+}


This is a common mistake I observed in most of your code. If you plan to use \n after each block, follow same style of coding in all your files. Here there is no newline after the block of code but at lines 34, 38, etc.. you have used new line. Better use newline after each and every block of code.

Apply this fix to all the other files.

rewanthtammana · 2017-08-15T15:52:01Z

www/login.html

+    var pass = $("#pass");
+    var error = $("#error");
+    $(".btn").click(function(){
+        console.log(pass.val());


Make sure to delete these kind of console logs after testing, which expose sensitive data to the end user.

rewanthtammana · 2017-08-15T15:56:34Z

www/login.html

+    var error = $("#error");
+    $(".btn").click(function(){
+        console.log(pass.val());
+        var data = {"user": ""+user.val()+"" , "password":""+ pass.val()+""};


Why is this inline code so complex? Instead you can use an elegant code like this,

const data = { "user" : user.val(), "password" : pass.val() }

Try to make your code as simple as possible.

rewanthtammana · 2017-08-15T16:00:01Z

app.js

+app.get('/speaker' , function (req , res) { 
+	res.sendFile(__dirname + '/www/speaker.html');
+});
+app.post('/speaker' , function (req , res) { 


Since you are returning the list of all speakers, the endpoint must be '/speakers' instead of '/speaker'.

If you are returning details of only one speaker then you can use '/speaker'.

rewanthtammana · 2017-08-15T16:05:12Z

app.js

+    res.redirect("/login");
+});
+app.post('/getuser' , function (req , res) {
+    res.send({"username" : req.session.user});


If req.session.user returns only username then change req.session.user to req.session.username.
If req.session.user returns any extra data apart from username then change "username" to "user".

Make sure you keep the naming conventions appropriate.

username 👍 )

AnshulMalik · 2017-08-18T17:47:09Z

app.js

-    console.log(req.session.user);
-    if(!req.session.user) {
-        console.log("Admin");
+    if(!req.session.username) {
        res.redirect('/login');


We can put all routes in single file, since they'll be reused everywhere.

AnshulMalik · 2017-08-18T17:49:29Z

app.js

 	res.sendFile(__dirname + '/www/speaker.html');
 });
-app.post('/speaker' , function (req , res) { 
+app.post('/speakers' , function (req , res) {


This should be get method, since we are getting list of speakers.

Let's not merge API and static pages, we can put all api requests under /api/* route.

AnshulMalik · 2017-08-18T17:50:06Z

app.js

    });
 });
-app.post("/speakerinsert", function(req, res) {
+app.post("/speaker-insert", function(req, res) {


This should be a POST request to /speakers.

AnshulMalik · 2017-08-18T17:51:43Z

app.js

-    res.send({"username" : req.session.user});
+    res.send({
+        "username" : req.session.username
+    });
 });
 app.get('/blog' , function (req , res) { 


Static pages should not be handled by api, if required, we can have a separate file for serving static files.

AnshulMalik · 2017-08-18T17:56:00Z

app.js

-    description = sanitizeHtml(description);
-    var sql = "INSERT INTO videos(title, description, video_url) values('"+title+"','"+description+"','"+url+"')";
+});
+app.post("/delete-video", function(req, res) {


What if we make this DELETE instead of POST and URL to be /video/<id>.

AnshulMalik · 2017-08-18T17:59:44Z

app.js

+app.post("/delete-video", function(req, res) {
+    const post = req.body;
+    const videoTitle  = sanitizeHtml(post.videoTitle);
+    var sql = "Delete from videos where title = '" + videoTitle +"'" ;


We would probably want to add some auth here in future.

AnshulMalik · 2017-08-18T18:01:15Z

app.js

+app.post("/delete-speaker", function(req, res) {
+    const post = req.body;
+    const speakerName  = sanitizeHtml(post.speakerName);
+    var sql = "Delete from speaker where name = '" + speakerName +"'" ;
    con.query(sql, function (err, result, fields) {
        res.redirect('/admin');


Stick with one convention, either double or single quotes.

A linter will be really helpful.

nkkumawat added 2 commits August 14, 2017 11:13

initial prototype of backend andd admin apnnel

0ee6df3

login page admin page created , speaker and video details insertion p…

e453857

…ics upload

AnshulMalik commented Aug 15, 2017

View reviewed changes

fix some issues which are from ansul sir , sanitizition before sql in…

b450342

…jection, env ports add , image upload done

AnshulMalik commented Aug 15, 2017

View reviewed changes

rewanthtammana reviewed Aug 15, 2017

View reviewed changes

Added delete speaker and video features

4d5bffb

AnshulMalik commented Aug 18, 2017

View reviewed changes

Get started #1

Are you sure you want to change the base?

Get started #1

Conversation

AnshulMalik commented Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnshulMalik Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewanthtammana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewanthtammana Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewanthtammana Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewanthtammana Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewanthtammana Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnshulMalik Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnshulMalik Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnshulMalik commented Aug 15, 2017 •

edited

Loading

AnshulMalik Aug 15, 2017 •

edited

Loading

rewanthtammana Aug 15, 2017 •

edited

Loading

rewanthtammana Aug 15, 2017 •

edited

Loading

rewanthtammana Aug 15, 2017 •

edited

Loading

rewanthtammana Aug 15, 2017 •

edited

Loading

AnshulMalik Aug 18, 2017 •

edited

Loading

AnshulMalik Aug 18, 2017 •

edited

Loading