repl: create history file with mode 0600. #3394

XeCycle · 2015-10-16T08:03:33Z

Test code mostly written by Trott
#3392 (comment).

targos · 2015-10-16T08:25:28Z

lib/internal/repl.js

@@ -88,7 +88,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
  var writing = false;
  var pending = false;
  repl.pause();
-  fs.open(historyPath, 'a+', oninit);
+  fs.open(historyPath, 'a+', /* 0600 */ 384, oninit);


you can use an octal literal: 0o0600

Trott · 2015-10-16T17:11:24Z

LGTM if CI is happy.

Trott · 2015-10-16T17:12:02Z

Fixes: #3392

Trott · 2015-10-16T17:15:44Z

CI: https://ci.nodejs.org/job/node-test-commit/859/

Trott · 2015-10-16T18:04:03Z

Test fails on Windows: https://ci.nodejs.org/job/node-test-binary-windows/14/RUN_SUBSET=0,VS_VERSION=vs2015,label=win10/tapTestReport/test.tap-155/

XeCycle · 2015-10-17T00:14:24Z

Tried it on Windows, and the returned stat is 010666. I guess that's because Windows uses a different permission model; do you think it okay to skip this test on Windows?

rvagg · 2015-10-17T00:15:15Z

I think so but lets ask @nodejs/platform-windows

Trott · 2015-10-17T01:00:02Z

I changed

  assert.strictEqual(mode, '0600', 'REPL history file should be mode 0600');

To just this

  assert.strictEqual(mode, '0600');

So that it would report the actual value. Result is here.

So, Windows is creating the file with mode 0666 rather than 0600.

The question is: Is this a bug in the code that creates the file or is it a limitation of running the Node.js REPL on the Windows operating system?

Trott · 2015-10-17T01:10:59Z

The code uses fs.open() to set the mode on file creation, so I guess if there's stuff that does not completely work there, a Windows libuv person would know the deal?

Trott · 2015-10-17T01:19:17Z

This comment in the libuv source would seem to be more-or-less a smoking gun. I think you can just check common.isWindows() at the top of the test, and if it is Windows, skip the test. You can emulate other tests like this from test-process-remove-all-signal-listeners:

if (common.isWindows) {
  console.log('1..0 # Skipped: Win32 doesn\'t have signals, just a kind of ' +
              'emulation, insufficient for this test to apply.');
  return;
}

XeCycle · 2015-10-17T01:30:42Z

From docs of CreateFile:

The access control lists (ACL) in the default security descriptor for a file or directory are inherited from its parent directory.

So privacy on the history file relies on the user having a fine ACL in his/her home directory (which appers to be like C:\Users\username).

Trott · 2015-10-17T01:36:17Z

Cool. LGTM if CI passes.

CI: https://ci.nodejs.org/job/node-test-commit/865/

Trott · 2015-10-17T02:09:34Z

All test failures are just the unrelated usual suspects and something weird with OS X such that it won't even build or something.

In other words: 👍

Hopefully someone else can give it an LGTM too.

thefourtheye · 2015-10-17T03:11:35Z

test/parallel/test-repl-history-perm.js

+
+const checkResults = common.mustCall(function(err, r) {
+  if (err)
+    throw err;


You can use assert.ifError instead of this if block.

I see there are other tests writing if (err) throw err; e.g. test-fs-write. Does the current style explicitly prefer assert.ifError? If not I think this if block is also okay.

No need to use assert here, this is common practice.

Fishrock123 · 2015-10-19T14:46:05Z

Seems fine to me.

Trott · 2015-10-19T17:26:37Z

@Fishrock123 Does that constitute an LGTM?

Fishrock123 · 2015-10-20T14:52:08Z

test/parallel/test-repl-history-perm.js

+stream.readable = stream.writable = true;
+
+common.refreshTmpDir();
+const replHistoryPath = path.join(common.tmpDir, 'repl_history');


Could you make this consistent with the other test? '.node_repl_history'

XeCycle · 2015-10-21T00:53:38Z

@Fishrock123 file name changed, not rebasing.

XeCycle · 2015-11-03T09:39:28Z

Please check this commit. Discovered we have -i/--interactive, so tried testing with that, but it fails because of another check here and here. Is it intended that a forced interactive session without terminal does not create a history file?

Fishrock123 · 2015-11-09T18:37:37Z

Discovered we have -i/--interactive, so tried testing with that, but it fails because of another check here and here. Is it intended that a forced interactive session without terminal does not create a history file?

Undefined. -i/--interactive should act just like the regular repl as much as possible though imo.

jasnell · 2016-04-22T04:52:04Z

What's the status on this one?

XeCycle · 2016-04-22T04:55:18Z

Seems like maintainers did not agree on the tests. Anyway there are no conflicts, a rebase is easy, so let me know if you want this landed.

jasnell · 2016-04-22T05:00:16Z

@nodejs/collaborators @nodejs/ctc ... does anyone have any further thoughts on this one?
@XeCycle ... if you'd like to rebase I'll see if I can get an updated review.

Fishrock123 · 2016-05-02T17:42:48Z

ci: https://ci.nodejs.org/job/node-test-pull-request/2463/

Fishrock123 · 2016-05-02T18:19:47Z

ci is green, I can't really make an informative opinion about this though

silverwind · 2016-05-02T18:37:51Z

600 is the right mode for history files, it protects the content from other users in case the home directory is readable. The test seems a bit overly complicated, but overall LGTM.

Fishrock123 · 2016-05-02T18:55:08Z

Ok, LGTM then

santigimeno · 2016-05-03T10:53:35Z

LGTM

jasnell · 2016-05-03T13:28:24Z

lib/internal/repl.js

@@ -93,7 +93,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
  var writing = false;
  var pending = false;
  repl.pause();
-  fs.open(historyPath, 'a+', oninit);
+  fs.open(historyPath, 'a+', 0o0600, oninit);


Nit: Would you mind adding a quick code comment with a quick explanation here. Nothing complicated, just enough for someone scanning through to know why the flag was set to this.

jasnell · 2016-05-03T13:36:58Z

Added a couple of comments that I'd like to see addressed before this lands. Otherwise LGTM

jasnell · 2016-05-03T13:37:42Z

What semver-iness would this be? I'm thinking just a patch (because I'd consider this a bug fix) but figured I'd ask.

Fishrock123 · 2016-05-03T16:38:42Z

I don't think it would be major.

Test code mostly written by Trott nodejs#3392 (comment).

XeCycle · 2016-05-04T01:11:12Z

@jasnell

Would you mind adding a quick code comment with a quick explanation here

Added, pointing to this PR and issue.

Personally I prefer the masked version

Me too, changed it to the masked version.

jasnell · 2016-05-04T03:08:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2491/

jasnell · 2016-05-04T03:09:03Z

LGTM if CI is green

Set the mode bits on the history file to 0o600 instead of leaving it unspecified, which resulted in 0o755 on Unices. Test code mostly written by Trott: #3392 (comment). PR-URL: #3394 Fixes: #3392 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

silverwind · 2016-05-04T17:09:30Z

Thanks you! Landed in b72d048.

Set the mode bits on the history file to 0o600 instead of leaving it unspecified, which resulted in 0o755 on Unices. Test code mostly written by Trott: #3392 (comment). PR-URL: #3394 Fixes: #3392 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

XeCycle force-pushed the repl-hist-perm branch from 3d57ee8 to 9ec50f0 Compare October 16, 2015 08:18

targos reviewed Oct 16, 2015
View reviewed changes

XeCycle force-pushed the repl-hist-perm branch from 9ec50f0 to 4e6c697 Compare October 16, 2015 08:29

ChALkeR added the repl Issues and PRs related to the REPL subsystem. label Oct 16, 2015

thefourtheye reviewed Oct 17, 2015
View reviewed changes

Fishrock123 reviewed Oct 20, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

jasnell added the stalled Issues and PRs that are stalled. label Apr 22, 2016

estliberitas force-pushed the master branch from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

jasnell reviewed May 3, 2016
View reviewed changes

jasnell removed the stalled Issues and PRs that are stalled. label May 3, 2016

repl: create history file with mode 0600

80612f3

Test code mostly written by Trott nodejs#3392 (comment).

XeCycle force-pushed the repl-hist-perm branch from ae592ff to 80612f3 Compare May 4, 2016 01:09

silverwind closed this May 4, 2016

silverwind added the lts-watch-v4.x label May 4, 2016

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 1, 2016

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

github-actions bot mentioned this pull request Apr 24, 2024

Adding VersionStream for nodejs-22 wolfi-dev/os#17646

Merged

repl: create history file with mode 0600. #3394

repl: create history file with mode 0600. #3394

Conversation

XeCycle commented Oct 16, 2015

targos Oct 16, 2015

Choose a reason for hiding this comment

Trott commented Oct 16, 2015

Trott commented Oct 16, 2015

Trott commented Oct 16, 2015

Trott commented Oct 16, 2015

XeCycle commented Oct 17, 2015

rvagg commented Oct 17, 2015

Trott commented Oct 17, 2015

Trott commented Oct 17, 2015

Trott commented Oct 17, 2015

XeCycle commented Oct 17, 2015

Trott commented Oct 17, 2015

Trott commented Oct 17, 2015

thefourtheye Oct 17, 2015

Choose a reason for hiding this comment

XeCycle Oct 17, 2015

Choose a reason for hiding this comment

Fishrock123 Oct 19, 2015

Choose a reason for hiding this comment

Fishrock123 commented Oct 19, 2015

Trott commented Oct 19, 2015

Fishrock123 Oct 20, 2015

Choose a reason for hiding this comment

XeCycle commented Oct 21, 2015

XeCycle commented Nov 3, 2015

Fishrock123 commented Nov 9, 2015

jasnell commented Apr 22, 2016

XeCycle commented Apr 22, 2016

jasnell commented Apr 22, 2016

Fishrock123 commented May 2, 2016

Fishrock123 commented May 2, 2016

silverwind commented May 2, 2016

Fishrock123 commented May 2, 2016

santigimeno commented May 3, 2016

jasnell May 3, 2016 • edited Loading

Choose a reason for hiding this comment

jasnell commented May 3, 2016

jasnell commented May 3, 2016

Fishrock123 commented May 3, 2016

XeCycle commented May 4, 2016

jasnell commented May 4, 2016

jasnell commented May 4, 2016

silverwind commented May 4, 2016

jasnell May 3, 2016 •

edited

Loading