Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix #268: Revert "fix #246: remove any double quotes or single quotes… #278

Merged
merged 2 commits into from
Aug 26, 2022
Merged

fix #268: Revert "fix #246: remove any double quotes or single quotes… #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbc65be
fix #268: Revert "fix #246: remove any double quotes or single quotes…
mbargiel Aug 23, 2022
a831713
Adjust #246 regression tests into #268 regression tests
mbargiel Aug 26, 2022
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
25 changes: 5 additions & 20 deletions lib/tmp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ function _assertAndSanitizeOptions(options) {
options.template = _isBlank(options.template) ? undefined : path.relative(options.dir, options.template);

// for completeness' sake only, also keep (multiple) blanks if the user, purportedly sane, requests us to
options.name = _isUndefined(options.name) ? undefined : _sanitizeName(options.name);
options.name = _isUndefined(options.name) ? undefined : options.name;
options.prefix = _isUndefined(options.prefix) ? '' : options.prefix;
options.postfix = _isUndefined(options.postfix) ? '' : options.postfix;
}
Expand All @@ -552,28 +552,13 @@ function _assertAndSanitizeOptions(options) {
* @private
*/
function _resolvePath(name, tmpDir) {
const sanitizedName = _sanitizeName(name);
if (sanitizedName.startsWith(tmpDir)) {
return path.resolve(sanitizedName);
if (name.startsWith(tmpDir)) {
return path.resolve(name);
} else {
return path.resolve(path.join(tmpDir, sanitizedName));
return path.resolve(path.join(tmpDir, name));
}
}

/**
* Sanitize the specified path name by removing all quote characters.
*
* @param name
* @returns {string}
* @private
*/
function _sanitizeName(name) {
if (_isBlank(name)) {
return name;
}
return name.replace(/["']/g, '');
}

/**
* Asserts whether specified name is relative to the specified tmpDir.
*
Expand Down Expand Up @@ -663,7 +648,7 @@ function setGracefulCleanup() {
* @returns {string} the currently configured tmp dir
*/
function _getTmpDir(options) {
return path.resolve(_sanitizeName(options && options.tmpdir || os.tmpdir()));
return path.resolve(options && options.tmpdir || os.tmpdir());
}

// Install process exit listener
Expand Down
15 changes: 8 additions & 7 deletions test/name-sync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
const
assert = require('assert'),
os = require('os'),
path = require('path'),
inbandStandardTests = require('./name-inband-standard'),
tmp = require('../lib/tmp');

Expand Down Expand Up @@ -53,30 +54,30 @@ describe('tmp', function () {
}
});
});
describe('on issue #246', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you leave these tests and make these so that they check for the presence of the single quotes and double quotes instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! I wasn't sure which you would have preferred between simply reverting or keeping the tests but amended to serve as regression tests. I'll do this when I can, either tonight or tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see a831713.

describe('on issue #268', function () {
const origfn = os.tmpdir;
it('must produce correct name on os.tmpdir() returning path that includes double quotes', function () {
it(`should not alter ${isWindows ? 'invalid' : 'valid'} path on os.tmpdir() returning path that includes double quotes`, function () {
const tmpdir = isWindows ? '"C:\\Temp With Spaces"' : '"/tmp with spaces"';
os.tmpdir = function () {
return tmpdir;
};
const name = tmp.tmpNameSync();
const index = name.indexOf(path.sep + tmpdir + path.sep);
try {
assert.ok(name.indexOf('"') === -1);
assert.ok(name.startsWith(tmpdir.replace(/["']/g, '')));
assert.ok(index > 0, `${tmpdir} should have been a subdirectory name in ${name}`);
} finally {
os.tmpdir = origfn;
}
});
it('must produce correct name on os.tmpdir() returning path that includes single quotes', function () {
it('should not alter valid path on os.tmpdir() returning path that includes single quotes', function () {
const tmpdir = isWindows ? '\'C:\\Temp With Spaces\'' : '\'/tmp with spaces\'';
os.tmpdir = function () {
return tmpdir;
};
const name = tmp.tmpNameSync();
const index = name.indexOf(path.sep + tmpdir + path.sep);
try {
assert.ok(name.indexOf('\'') === -1);
assert.ok(name.startsWith(tmpdir.replace(/["']/g, '')));
assert.ok(index > 0, `${tmpdir} should have been a subdirectory name in ${name}`);
} finally {
os.tmpdir = origfn;
}
Expand Down
15 changes: 8 additions & 7 deletions test/name-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
const
assert = require('assert'),
os = require('os'),
path = require('path'),
inbandStandardTests = require('./name-inband-standard'),
tmp = require('../lib/tmp');

Expand Down Expand Up @@ -63,15 +64,15 @@ describe('tmp', function () {
});
});
});
describe('on issue #246', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, tests must stay in place, yet test for the presence of the quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see a831713.

describe('on issue #268', function () {
const origfn = os.tmpdir;
it('must produce correct name on os.tmpdir() returning path that includes double quotes', function (done) {
it(`should not alter ${isWindows ? 'invalid' : 'valid'} path on os.tmpdir() returning path that includes double quotes`, function (done) {
const tmpdir = isWindows ? '"C:\\Temp With Spaces"' : '"/tmp with spaces"';
os.tmpdir = function () { return tmpdir; };
tmp.tmpName(function (err, name) {
const index = name.indexOf(path.sep + tmpdir + path.sep);
try {
assert.ok(name.indexOf('"') === -1);
assert.ok(name.startsWith(tmpdir.replace(/["']/g, '')));
assert.ok(index > 0, `${tmpdir} should have been a subdirectory name in ${name}`);
} catch (err) {
return done(err);
} finally {
Expand All @@ -80,13 +81,13 @@ describe('tmp', function () {
done();
});
});
it('must produce correct name on os.tmpdir() returning path that includes single quotes', function (done) {
it('should not alter valid path on os.tmpdir() returning path that includes single quotes', function (done) {
const tmpdir = isWindows ? '\'C:\\Temp With Spaces\'' : '\'/tmp with spaces\'';
os.tmpdir = function () { return tmpdir; };
tmp.tmpName(function (err, name) {
const index = name.indexOf(path.sep + tmpdir + path.sep);
try {
assert.ok(name.indexOf('\'') === -1);
assert.ok(name.startsWith(tmpdir.replace(/["']/g, '')));
assert.ok(index > 0, `${tmpdir} should have been a subdirectory name in ${name}`);
} catch (err) {
return done(err);
} finally {
Expand Down