Skip to content

Commit

Permalink
Fix some bugs with encoding
Browse files Browse the repository at this point in the history
* Remove the patch I put in for this portion
* Create function for encoding like GH's... see helper `encoding`
* Using encodeURIComponent from OpenUserJS#795
* Fix bug of Fork History showing "slugged" instead of native/raw
* Fix a missing model parser item for `category` virtual model
* Fix some more bugs in modelParser.js
* Transform a watchpoint with AuthAs ... see third alternative at http://stackoverflow.com/questions/15825333/how-to-reload-current-page-in-express-js with `aRes.redirect(req.get('referer'));` or *express*@4.x shortcut of `aRes.redirect('back');`... but if referer is missing will default to `/` Ref: http://expressjs.com/en/api.html#res.redirect ... this still appears to be a bug in *express* for quite some time so this is a workaround
* Add another series of watchpoints with *express*@4.x `redirect()` doing `escape` on occasion ... special handling of `redirect`.... this particular bug doesn't appear to make much sense
* Project version bump

**NOTES**
* Followup with DB migration to add `fork.utf` to existing forks otherwise they show up as `/`
* Followup with `discussion.path` fixes.
* If *express* fixes their redirect issue can simplify `encode` to be a smarter encode where it only encodes when needed... which is where it started but had to back that out in this PR
* Leaving GH import alone as it still works so far
* `./app.js` has one `encodeURI` in it... leaving that be for the time being until it can be retested on production... dev isn't https so no way to test here

Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment) among many others.
  • Loading branch information
Martii committed Feb 20, 2016
1 parent 1be5ef5 commit af5f8b8
Show file tree
Hide file tree
Showing 11 changed files with 108 additions and 57 deletions.
2 changes: 1 addition & 1 deletion controllers/admin.js
Original file line number Diff line number Diff line change
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.userPageUrl); // NOTE: Watchpoint
});
};
21 changes: 14 additions & 7 deletions controllers/discussion.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ var modelParser = require('../libs/modelParser');
var modelQuery = require('../libs/modelQuery');

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

var execQueryTask = require('../libs/tasks').execQueryTask;
var statusCodePage = require('../libs/templateHelpers').statusCodePage;
var pageMetadata = require('../libs/templateHelpers').pageMetadata;
Expand Down Expand Up @@ -243,9 +245,12 @@ exports.list = function (aReq, aRes, aNext) {

// Locate a discussion and deal with topic url collisions
function findDiscussion(aCategory, aTopicUrl, aCallback) {

// To prevent collisions we add an incrementing id to the topic url
var topic = /(.+?)(?:_(\d+))?$/.exec(aTopicUrl);
var query = { path: '/' + aCategory + '/' + topic[1] };
var query = { path: '/' + aCategory.split('/').map(function (aStr) {
return encode(aStr);
}).join('/') + '/' + encode(topic[1]) };

// We only need to look for the proper duplicate if there is one
if (topic[2]) {
Expand Down Expand Up @@ -433,8 +438,10 @@ exports.postComment = postComment;
// Does all the work of submitting a new topic and
// resolving topic url collisions
function postTopic(aUser, aCategory, aTopic, aContent, aIssue, aCallback) {
var urlTopic = cleanFilename(aTopic, '').replace(/_\d+$/, '');
var path = '/' + aCategory + '/' + urlTopic;
var urlTopic = encode(cleanFilename(aTopic, '').replace(/_\d+$/, ''));
var path = '/' + aCategory.split('/').map(function (aStr) {
return encode(aStr);
}).join('/') + '/' + urlTopic;
var params = { sort: {} };
params.sort.duplicateId = -1;

Expand Down Expand Up @@ -525,8 +532,8 @@ exports.createTopic = function (aReq, aRes, aNext) {
return;
}

aRes.redirect(encodeURI(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')); // NOTE: Watchpoint
});
};

Expand Down Expand Up @@ -560,8 +567,8 @@ 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
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')); // NOTE: Watchpoint
});
});
};
5 changes: 3 additions & 2 deletions controllers/flag.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var scriptStorage = require('./scriptStorage');
var flagLib = require('../libs/flag');

var statusCodePage = require('../libs/templateHelpers').statusCodePage;
var encode = require('../libs/helpers').encode;

//--- Configuration inclusions

Expand Down Expand Up @@ -103,7 +104,7 @@ exports.flag = function (aReq, aRes, aNext) {

fn(Script, aScript, authedUser, reason, function (aFlagged) {
aRes.redirect((isLib ? '/libs/' : '/scripts/') + scriptStorage.getInstallNameBase(
aReq, { encoding: 'uri' }));
aReq, { encoding: 'url' })); // NOTE: Watchpoint
});

});
Expand All @@ -121,7 +122,7 @@ exports.flag = function (aReq, aRes, aNext) {
}

fn(User, aUser, authedUser, reason, function (aFlagged) {
aRes.redirect('/users/' + encodeURIComponent(username));
aRes.redirect('/users/' + encode(username)); // NOTE: Watchpoint
});

});
Expand Down
14 changes: 7 additions & 7 deletions controllers/issue.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,12 +380,12 @@ 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.slugUrl + '/open'); // NOTE: Watchpoint
return;
}

aRes.redirect(encodeURI(aDiscussion.path +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')));
aRes.redirect(aDiscussion.path +
(aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')); // NOTE: Watchpoint
}
);
} else {
Expand Down Expand Up @@ -439,8 +439,8 @@ 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
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')); // NOTE: Watchpoint
});
});
});
Expand Down Expand Up @@ -487,8 +487,8 @@ 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
+ (aDiscussion.duplicateId ? '_' + aDiscussion.duplicateId : '')); // NOTE: Watchpoint
});
} else {
aNext();
Expand Down
4 changes: 3 additions & 1 deletion controllers/remove.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ var scriptStorage = require('./scriptStorage');
//--- Library inclusions
var removeLib = require('../libs/remove');

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

var destroySessions = require('../libs/modifySessions').destroy;
var statusCodePage = require('../libs/templateHelpers').statusCodePage;

Expand Down Expand Up @@ -92,7 +94,7 @@ exports.rm = function (aReq, aRes, aNext) {
aNext();
return;
}
aRes.redirect('/users/' + encodeURIComponent(username) + '/scripts');
aRes.redirect('/users/' + encode(username) + '/scripts'); // NOTE: Watchpoint
});
});
break;
Expand Down
6 changes: 3 additions & 3 deletions 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 @@ -84,15 +85,14 @@ function getInstallNameBase(aReq, aOptions) {
}

switch (aOptions.encoding) {
case 'uri':
base = encodeURIComponent(username) + '/' + encodeURIComponent(scriptname);
case 'url':
base = encode(username) + '/' + encode(scriptname);
break;

default:
base = username + '/' + scriptname;
}


return base;
}
exports.getInstallNameBase = getInstallNameBase;
Expand Down
44 changes: 28 additions & 16 deletions controllers/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,7 @@ exports.userGitHubImportScriptPage = function (aReq, aRes, aNext) {
} else if (options.javascriptBlob.isJSLibrary) {
if (!hasMissingExcludeAll(aBlobUtf8)) {
scriptName = options.javascriptBlob.path.name;

jsLibraryMeta = scriptName;
scriptStorage.storeScript(authedUser, jsLibraryMeta, aBlobUtf8, onScriptStored);
} else {
Expand All @@ -1194,7 +1195,7 @@ exports.userGitHubImportScriptPage = function (aReq, aRes, aNext) {

script = modelParser.parseScript(options.script);

aRes.redirect(script.scriptPageUrl);
aRes.redirect(script.scriptPageUrl); // NOTE: Watchpoint
});
};

Expand Down Expand Up @@ -1379,8 +1380,6 @@ exports.uploadScript = function (aReq, aRes, aNext) {
User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
var scriptName = aFields.script_name;
var bufferConcat = Buffer.concat(bufs);
var rJS = /\.js$/;
var rUserJS = /\.user\.js$/;

if (isLib) {
if (!hasMissingExcludeAll(bufferConcat)) {
Expand All @@ -1391,8 +1390,12 @@ exports.uploadScript = function (aReq, aRes, aNext) {
return;
}

aRes.redirect('/libs/' + encodeURI(aScript.installName
.replace(rJS, ''))); // TODO: Split handling
aRes.redirect(
'/libs/' +
helpers.encode(helpers.cleanFilename(aScript.author)) +
'/' +
helpers.encode(helpers.cleanFilename(aScript.name))
);
});
} else {
aRes.redirect(failUrl);
Expand All @@ -1408,8 +1411,12 @@ exports.uploadScript = function (aReq, aRes, aNext) {
return;
}

aRes.redirect('/scripts/' + encodeURI(aScript.installName
.replace(rUserJS, ''))); // TODO: Split handling
aRes.redirect(
'/scripts/' +
helpers.encode(helpers.cleanFilename(aScript.author)) +
'/' +
helpers.encode(helpers.cleanFilename(aScript.name))
);
});
});
}
Expand Down Expand Up @@ -1444,14 +1451,14 @@ exports.submitSource = function (aReq, aRes, aNext) {
var url = null;

function storeScript(aMeta, aSource) {
var rUserJS = /\.user\.js$/;
var rJS = /\.js$/;

User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
scriptStorage.storeScript(aUser, aMeta, aSource, function (aScript) {
var redirectUrl = encodeURI(aScript ? (aScript.isLib ? '/libs/'
+ aScript.installName.replace(rJS, '') : '/scripts/'
+ aScript.installName.replace(rUserJS, '')) : decodeURI(aReq.body.url)); // TODO: Split handling
var redirectUrl = aScript
? ((aScript.isLib ? '/libs/' : '/scripts/') +
helpers.encode(helpers.cleanFilename(aScript.author)) +
'/' +
helpers.encode(helpers.cleanFilename(aScript.name)))
: aReq.body.url;

if (!aScript || !aReq.body.original) {
aRes.redirect(redirectUrl);
Expand All @@ -1462,15 +1469,20 @@ exports.submitSource = function (aReq, aRes, aNext) {
function (aErr, aOrigScript) {
var fork = null;

var origInstallNameSlugUrl = helpers.encode(helpers.cleanFilename(aOrigScript.author)) +
'/' +
helpers.encode(helpers.cleanFilename(aOrigScript.name));

if (aErr || !aOrigScript) {
aRes.redirect(redirectUrl);
return;
}

fork = aOrigScript.fork || [];
fork.unshift({
author: aOrigScript.author, url: aOrigScript
.installName.replace(aOrigScript.isLib ? rJS : rUserJS, '') // TODO: Split handling
author: aOrigScript.author,
url: origInstallNameSlugUrl,
utf: aOrigScript.author + '/' + aOrigScript.name
});
aScript.fork = fork;

Expand Down Expand Up @@ -1646,7 +1658,7 @@ exports.editScript = function (aReq, aRes, aNext) {
script.scriptPermalinkInstallPageXUrl = 'https://' + aReq.get('host') +
script.scriptInstallPageXUrl;
script.scriptRawPageUrl = '/src/' + (isLib ? 'libs' : 'scripts') + '/'
+ scriptStorage.getInstallNameBase(aReq, { encoding: 'uri' }) +
+ scriptStorage.getInstallNameBase(aReq, { encoding: 'url' }) +
(isLib ? '.js#' : '.user.js#');

// Page metadata
Expand Down
7 changes: 7 additions & 0 deletions libs/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ exports.cleanFilename = function (aFilename, aDefaultName) {
return cleanName || aDefaultName;
};

// Default encoding like GitHubs
// Future code point for the smart encoder
// pending *express* redirect issue resolution
exports.encode = function (aStr) {
return encodeURIComponent(aStr);
}

exports.limitRange = function (aMin, aX, aMax, aDefault) {
var x = Math.max(Math.min(aX, aMax), aMin);

Expand Down
Loading

0 comments on commit af5f8b8

Please sign in to comment.