Skip to content

Commit

Permalink
Fix some bugs with encoding
Browse files Browse the repository at this point in the history
* Remove legacy namespace in all routes ... part of OpenUserJS#262 and OpenUserJS#130... officially EOL as per sizzle and confirmed by me... no redirects.
* Remove the patches I put in for this portion
* Create function for encoding similar to GH's... see helper `encoding`
* Using encodeURIComponent from OpenUserJS#795
* Fix bug of Fork History showing "slugged" instead of native/raw
* Fix category reference bug... redirect was set to the Object itself and not one of its properties
* Fix some more missing object identifiers in modelParser.js ... not all are validated yet and some may still be missing for future proofing
* Remove and Add some more watchpoints
* Flip successful "rating updated" for Groups to debug mode and related in `./libs/modelParser.js` ... these are generating too much log traffic OpenUserJS#430

**NOTES**
* [ ] Followup with DB migration to add `fork.utf` to existing forks otherwise they may not show up next to the bullet
* Leaving GH import alone as it still works so far ... however a watchpoint added... GH web UI always `encodeURIComponent` just about everything... will need to reaffirm existing `encodeURI` later
* The term uri is inclusive to urls as previously described in OpenUserJS#819 ... however in this context url means it could be partially encoded whereas uri means it absolutely is encoded... except for the slashes... normally those are encoded but we don't want them encoded... again following GH example. A url containing `ä` may be just that however a uri has `%E4`. You will notice that I renamed some of our identifiers from `url` to `uri`... this is on purpose to indicate that is what we are expecting.
* *express* `redirect()` uses [`Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.30) and [`Content-Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.14) headers. This requires all urls to be at least URI encoded as per specs... e.g. all the browsers are adding in an `escape` for the section symbol *which may be an issue with them*. So hence why the naming of `url` vs `uri`.
* The base issue of readability of the code is still there... but at least this punches out some of the bugs mentioned.
* Compression/optimization hasn't occurred yet
* No DB migration is necessary now... this includes fixing `Re:` linkage
* The smarter encoder isn't finished yet as I may be rearranging this a bit but...

**This should knock out the BLOCKING label I put on**... at least from my perspective.

Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment) among many others.
  • Loading branch information
Martii committed Feb 21, 2016
1 parent ec98e84 commit 9ff620f
Show file tree
Hide file tree
Showing 12 changed files with 291 additions and 106 deletions.
4 changes: 2 additions & 2 deletions controllers/admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ exports.adminUserUpdate = function (aReq, aRes, aNext) {

// Make sure the change is reflected in the session store
updateSessions(aReq, aUser, function (aErr, aSess) {
aRes.redirect(user.userPageUrl);
aRes.redirect(user.userPageUri);
});
});
});
Expand Down Expand Up @@ -497,6 +497,6 @@ exports.authAsUser = function (aReq, aRes, aNext) {

aReq.session.user = user;

aRes.redirect(encodeURI(user.userPageUrl)); // NOTE: Watchpoint
aRes.redirect(user.userPageUri);
});
};
10 changes: 5 additions & 5 deletions controllers/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ exports.callback = function (aReq, aRes, aNext) {
var username = aReq.session.username;
var newstrategy = aReq.session.newstrategy;
var strategyInstance = null;
var doneUrl = aReq.session.user ? '/user/preferences' : '/';
var doneUri = aReq.session.user ? '/user/preferences' : '/';

// The callback was called improperly
if (!strategy || !username) {
Expand Down Expand Up @@ -231,7 +231,7 @@ exports.callback = function (aReq, aRes, aNext) {
console.error(chalk.red('`User` not found'));
}

aRes.redirect(doneUrl + (doneUrl === '/' ? 'register' : '') + '?authfail');
aRes.redirect(doneUri + (doneUri === '/' ? 'register' : '') + '?authfail');
return;
}

Expand Down Expand Up @@ -263,16 +263,16 @@ exports.callback = function (aReq, aRes, aNext) {
addSession(aReq, aUser, function () {
if (newstrategy && newstrategy !== strategy) {
// Allow a user to link to another account
aRes.redirect('/auth/' + newstrategy);
aRes.redirect('/auth/' + newstrategy); // NOTE: Watchpoint... careful with encoding
return;
} else {
// Delete the username that was temporarily stored
delete aReq.session.username;
delete aReq.session.newstrategy;
doneUrl = aReq.session.redirectTo;
doneUri = aReq.session.redirectTo;
delete aReq.session.redirectTo;

aRes.redirect(doneUrl);
aRes.redirect(doneUri);
return;
}
});
Expand Down
12 changes: 8 additions & 4 deletions controllers/discussion.js
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,10 @@ exports.createTopic = function (aReq, aRes, aNext) {
return;
}

aRes.redirect(encodeURI(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
return encodeURIComponent(aStr);
}).join('/')
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : ''));
});
};

Expand Down Expand Up @@ -560,8 +562,10 @@ exports.createComment = function (aReq, aRes, aNext) {
}

postComment(authedUser, aDiscussion, content, false, function (aErr, aDiscussion) {
aRes.redirect(encodeURI(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
return encodeURIComponent(aStr);
}).join('/') +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : ''));
});
});
};
6 changes: 3 additions & 3 deletions controllers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,16 +297,16 @@ exports.register = function (aReq, aRes) {

exports.logout = function (aReq, aRes) {
var authedUser = aReq.session.user;
var redirectUrl = getRedirect(aReq);
var redirectUri = getRedirect(aReq); // NOTE: Watchpoint

if (!authedUser) {
aRes.redirect(redirectUrl);
aRes.redirect(redirectUri);
return;
}

User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
removeSession(aReq, aUser, function () {
aRes.redirect(redirectUrl);
aRes.redirect(redirectUri);
});
});
};
20 changes: 13 additions & 7 deletions controllers/issue.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,12 +380,14 @@ exports.open = function (aReq, aRes, aNext) {
discussionLib.postTopic(authedUser, category.slug, topic, content, true,
function (aDiscussion) {
if (!aDiscussion) {
aRes.redirect('/' + encodeURI(category) + '/open');
aRes.redirect('/' + category.slugUri + '/open');
return;
}

aRes.redirect(encodeURI(aDiscussion.path +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
return encodeURIComponent(aStr);
}).join('/') +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : ''));
}
);
} else {
Expand Down Expand Up @@ -439,8 +441,10 @@ exports.comment = function (aReq, aRes, aNext) {

discussionLib.postComment(authedUser, aIssue, content, false,
function (aErr, aDiscussion) {
aRes.redirect(encodeURI(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
return encodeURIComponent(aStr);
}).join('/') +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : ''));
});
});
});
Expand Down Expand Up @@ -487,8 +491,10 @@ exports.changeStatus = function (aReq, aRes, aNext) {

if (changed) {
aIssue.save(function (aErr, aDiscussion) {
aRes.redirect(encodeURI(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
return encodeURIComponent(aStr);
}).join('/') +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : ''));
});
} else {
aNext();
Expand Down
26 changes: 13 additions & 13 deletions controllers/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,15 +467,15 @@ exports.edit = function (aReq, aRes, aNext) {
if (aReq.body.remove) {
// POST
scriptStorage.deleteScript(aScript.installName, function () {
aRes.redirect(authedUser.userScriptListPageUrl);
aRes.redirect(authedUser.userScriptListPageUri);
});
} else if (typeof aReq.body.about !== 'undefined') {
// POST
aScript.about = aReq.body.about;
scriptGroups = (aReq.body.groups || '');
scriptGroups = scriptGroups.split(/,/);
addScriptToGroups(aScript, scriptGroups, function () {
aRes.redirect(script.scriptPageUrl);
aRes.redirect(script.scriptPageUri);
});
} else {
// GET
Expand All @@ -495,20 +495,20 @@ exports.edit = function (aReq, aRes, aNext) {
// Script voting
exports.vote = function (aReq, aRes, aNext) {
//
var url = aReq._parsedUrl.pathname.split('/');
var uri = aReq._parsedUrl.pathname.split('/');
var vote = aReq.params.vote;
var unvote = false;

var isLib = aReq.params.isLib;
var installNameBase = scriptStorage.getInstallNameBase(aReq);

// ---
if (url.length > 5) {
url.pop();
if (uri.length > 5) {
uri.pop();
}
url.shift();
url.shift();
url = '/' + url.join('/');
uri.shift();
uri.shift();
uri = '/' + uri.join('/');

if (vote === 'up') {
vote = true;
Expand All @@ -517,7 +517,7 @@ exports.vote = function (aReq, aRes, aNext) {
} else if (vote === 'unvote') {
unvote = true;
} else {
aRes.redirect(url);
aRes.redirect(uri);
return;
}

Expand All @@ -530,7 +530,7 @@ exports.vote = function (aReq, aRes, aNext) {

// ---
if (aErr || !aScript) {
aRes.redirect(url);
aRes.redirect(uri);
return;
}

Expand All @@ -543,15 +543,15 @@ exports.vote = function (aReq, aRes, aNext) {
function saveScript() {
if (!flags) {
aScript.save(function (aErr, aScript) {
aRes.redirect(url);
aRes.redirect(uri);
});
return;
}

flagLib.getAuthor(aScript, function (aAuthor) {
flagLib.saveContent(Script, aScript, aAuthor, flags,
function (aFlagged) {
aRes.redirect(url);
aRes.redirect(uri);
});
});
}
Expand All @@ -565,7 +565,7 @@ exports.vote = function (aReq, aRes, aNext) {
}

if (authedUser._id == aScript._authorId || (!aVoteModel && unvote)) {
aRes.redirect(url);
aRes.redirect(uri);
return;
} else if (!aVoteModel) {
aVoteModel = new Vote({
Expand Down
10 changes: 9 additions & 1 deletion controllers/scriptStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var RepoManager = require('../libs/repoManager');

var cleanFilename = require('../libs/helpers').cleanFilename;
var findDeadorAlive = require('../libs/remove').findDeadorAlive;
var encode = require('../libs/helpers').encode;

//--- Configuration inclusions
var userRoles = require('../models/userRoles.json');
Expand Down Expand Up @@ -88,6 +89,9 @@ function getInstallNameBase(aReq, aOptions) {
base = encodeURIComponent(username) + '/' + encodeURIComponent(scriptname);
break;

case 'url':
base = encode(username) + '/' + encode(scriptname);

default:
base = username + '/' + scriptname;
}
Expand Down Expand Up @@ -733,7 +737,11 @@ exports.webhook = function (aReq, aRes) {
payload.commits.forEach(function (aCommit) {
aCommit.modified.forEach(function (aFilename) {
if (aFilename.substr(-8) === '.user.js') {
repo[aFilename] = '/' + encodeURI(aFilename);
console.log([
'Webhook filename:',
' ' + aFilename
].join('\n')); // TODO: After some data collected, reaffirm and remove this
repo[aFilename] = '/' + encodeURI(aFilename); // NOTE: Watchpoint
}
});
});
Expand Down
Loading

0 comments on commit 9ff620f

Please sign in to comment.