repl: make sure historyPath is trimmed by evanlucas · Pull Request #4539 · nodejs/node

evanlucas · 2016-01-05T12:29:42Z

If one were to set NODE_REPL_HISTORY to a string that contains only a
space (" "), then the history file would be created with that name
which can cause problems are certain systems.

Related: #4522

R= @Fishrock123?

thefourtheye · 2016-01-05T14:07:43Z

lib/internal/repl.js

If we are anyway going to use the actual value as it is, then why not just

if (opts.terminal && typeof env.NODE_REPL_HISTORY === 'string' && env.NODE_REPL_HISTORY.trim() !== '') { ... }

Because then we would have to trim the string more than once before it is passed to setupHistory

@evanlucas How so? We are anyway passing env.NODE_REPL_HISTORY as it is to setupHistory.

Oh wow. Not what I meant to do. Will fix shortly. I was meaning to pass the trimmed historyPath to setupHistory

thefourtheye · 2016-01-05T14:21:45Z

Hmmm, I still feel that the type checking would be better here. Because, users can tamper env programmatically and we will fail at runtime if trim is not defined on that object.

evanlucas · 2016-01-05T14:23:44Z

process.env gets coerced to a string when set though.

> process.env.test = 1
1
> process.env.test
'1'
> process.env.test = { 'name': 'test' }
{ name: 'test' }
> process.env.test
'[object Object]'
> process.env.PATH
'/Users/evan/dev/code/depot_tools:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/MacGPG2/bin'

thefourtheye · 2016-01-05T14:28:46Z

TIL :-) Thanks :P

In that case, we can simply do

const historyPath = env.NODE_REPL_HISTORY.trim();
if (historyPath) {
  return setupHistory(...);
}

Right?

evanlucas · 2016-01-05T14:29:41Z

We can't do that because the key may not exist at all:

> process.env.biscuits.trim()
TypeError: Cannot read property 'trim' of undefined
    at repl:1:21
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)
>

thefourtheye · 2016-01-05T14:32:27Z

Ah, right. Thanks :) Change LGTM. Let's see what @Fishrock123 thinks. Apart from this, should we warn the user that the file name is an empty string?

cjihrig · 2016-01-05T14:41:17Z

lib/internal/repl.js

All of this logic, including the check for empty string might make more sense inside of setupHistory().

cjihrig · 2016-01-05T14:42:21Z

This probably warrants a tiny documentation update to mention that whitespace is stripped. Right now it only says:

Setting this value to "" will disable persistent REPL history.

After this change, that would only be partially correct.

jasnell · 2016-01-05T18:44:15Z

LGTM. What do y'all think about landing this in v4?

evanlucas · 2016-01-06T14:17:15Z

@cjihrig nits addressed. PTAL.

@jasnell I think it should definitely be backported to LTS. Especially considering the problems it could cause on windows as stated in #4522 (comment)

thefourtheye · 2016-01-06T14:22:31Z

lib/internal/repl.js

This condition is not necessary. If the control reaches this point, then historyPath is not an empty string.

It could be undefined though

cjihrig · 2016-01-06T14:52:30Z

One comment, then LGTM.

evanlucas · 2016-01-07T15:31:48Z

Ok, updated. PTAL @cjihrig

cjihrig · 2016-01-07T15:42:00Z

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

Fishrock123 · 2016-01-07T15:56:44Z

Looking, sorry for the delay.

Fishrock123 · 2016-01-07T15:58:17Z

lib/internal/repl.js

Wait what else would it be? We don't export setupHistory().

if process.env.NODE_REPL_HISTORY is undefined, then it will be undefined

Oh right, carry on.

jasnell · 2016-01-07T23:46:11Z

LGTM once @Fishrock123 is happy :-)

Fishrock123 · 2016-01-08T23:36:41Z

Seems fine to me.

evanlucas · 2016-01-14T12:46:12Z

Rebased after the eslint changes. @Fishrock123 @jasnell @cjihrig @thefourtheye LGTY?

cjihrig · 2016-01-14T13:30:41Z

Yep, still LGTM.

Fishrock123 · 2016-01-16T19:01:55Z

LGTM

If one were to set NODE_REPL_HISTORY to a string that contains only a space (" "), then the history file would be created with that name which can cause problems are certain systems. PR-URL: nodejs#4539 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

evanlucas · 2016-01-16T19:26:20Z

Landed in da550aa. Thanks!

If one were to set NODE_REPL_HISTORY to a string that contains only a space (" "), then the history file would be created with that name which can cause problems are certain systems. PR-URL: #4539 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

If one were to set NODE_REPL_HISTORY to a string that contains only a space (" "), then the history file would be created with that name which can cause problems are certain systems. PR-URL: nodejs#4539 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

evanlucas added the repl Issues and PRs related to the REPL subsystem. label Jan 5, 2016

thefourtheye reviewed Jan 5, 2016
View reviewed changes

evanlucas force-pushed the replspace branch from 4460446 to 2ffeaab Compare January 5, 2016 14:18

cjihrig reviewed Jan 5, 2016
View reviewed changes

evanlucas force-pushed the replspace branch from 2ffeaab to e75fce5 Compare January 6, 2016 14:15

thefourtheye reviewed Jan 6, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Jan 6, 2016

evanlucas force-pushed the replspace branch from e75fce5 to 5f8d47e Compare January 7, 2016 15:31

Fishrock123 reviewed Jan 7, 2016
View reviewed changes

evanlucas force-pushed the replspace branch from 5f8d47e to 8e724d1 Compare January 14, 2016 12:44

evanlucas force-pushed the replspace branch from 8e724d1 to da550aa Compare January 16, 2016 19:25

evanlucas closed this Jan 16, 2016

evanlucas deleted the replspace branch January 16, 2016 19:25

evanlucas merged commit da550aa into nodejs:master Jan 16, 2016

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 28, 2016

MylesBorins mentioned this pull request Feb 11, 2016

V4.3.1 proposal #5200

Merged

Uh oh!

Conversation

evanlucas commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Jan 5, 2016

Uh oh!

evanlucas commented Jan 5, 2016

Uh oh!

thefourtheye commented Jan 5, 2016

Uh oh!

evanlucas commented Jan 5, 2016

Uh oh!

thefourtheye commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jan 5, 2016

Uh oh!

jasnell commented Jan 5, 2016

Uh oh!

evanlucas commented Jan 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jan 6, 2016

Uh oh!

evanlucas commented Jan 7, 2016

Uh oh!

cjihrig commented Jan 7, 2016

Uh oh!

Fishrock123 commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 7, 2016

Uh oh!

Fishrock123 commented Jan 8, 2016

Uh oh!

evanlucas commented Jan 14, 2016

Uh oh!

cjihrig commented Jan 14, 2016

Uh oh!

Fishrock123 commented Jan 16, 2016

Uh oh!

evanlucas commented Jan 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants