Skip to content

Fix some bugs with encoding #900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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