heroku / cli

Heroku CLI
https://devcenter.heroku.com/articles/heroku-cli
ISC License
854 stars 224 forks source link

testing for current shell uses incorrect environment variable #2014

Open masukomi opened 2 years ago

masukomi commented 2 years ago

What is the current behavior?

regardless of what your current shell is, heroku autocomplete --refresh-cache fails if your default shell is not supported.

for example:

What is the expected behavior?

when a user is running a supported shell they should never get an error about some other shell not being supported.

To put it another way, the function that test for supported shells should be testing the current shell not the default shell.

Please mention your heroku and OS version.

Please state if you are behind an HTTP proxy or company firewall as well.

yes, but that's not relevant.

BUG SOURCE

I believe the bug originates in the _shell() function in config.js which is using the wrong environment variable to test the current shell.

    _shell() {
        let shellPath;
        /// BUG HERE  vvvvvvvvvvvvvvvvvvvvv
        const { SHELL, COMSPEC } = process.env;
        /// BUG THERE ^^^^^^^^^^^^^^^^^
        if (SHELL) {
            shellPath = SHELL.split('/');
        }
        else if (this.windows && COMSPEC) {
            shellPath = COMSPEC.split(/\\|\//);
        }
        else {
            shellPath = ['unknown'];
        }
        return shellPath[shellPath.length - 1];
    }

Specifically, the problem is that the SHELL environment variable returns the default shell not the current shell. If you want the current shell you should ask for the 0 (zero) environment variable. In bash and zsh this returns bash or zsh respectively. in fish it returns nothing.

[edit: after further testing I don't think that that function is the source of my problem (at least not with autocomplete) but it is definitely going to be the source of problems and both instances of that function need to be corrected.

example in use

❯ bash # switching to bash
➜ echo $0
bash
➜ zsh # switching to zsh
% echo $0
zsh

according to this line

plugin-autocomplete/lib/base.js
18:        if (!['bash', 'zsh'].includes(shell)) {

those are you only two supported shells, so the 0 environment variable should be sufficient for your tests. However... it doesn't appear that process.env["0"] returns anything so... you may have to get at that info in a different way. More info on this problem can be found in this stack overflow answer but i haven't figured out what the idiomatic node way of obtaining this information is.

Regardless, SHELL is absolutely not the correct way to access the information you're looking for.

eablack commented 7 months ago

Confirmed. This is an issue within oclif. We'll look into remediations for our own cli.

eablack commented 6 months ago

Internal Work Item