Issues with reading commands from file

[ad8-bdl]

keeper can read commands from a file in at least three different ways (where cmds is a file containing keeper commands):

1. keeper run-batch cmds
2. keeper cmds
3. keeper - < cmds (requires a previously established persistent login session keeper this-device persistent-login on)

The second approach is

• not well documented (passing reference at https://docs.keeper.io/secrets-manager/commander-cli/using-commander/logging-in#batch-mode-through-stdin)
• missing from keeper --help
• has a different implementation, and subtly different behaviour, to run-batch:
  • run-batch strips out comment lines:

    Commander/keepercommander/commands/scripting.py

    Line 85 in fb612e8

    commands = [c.strip() for c in lines if not c.startswith('#')]

  • from args does not:

    Commander/keepercommander/__main__.py

    Line 240 in ada80f4

    params.commands.extend([x.strip() for x in lines])

I suggest just deleting the option 2 (and 3) in its entirety. Executing commands from a file/stdin should be explicit.

[ad8-bdl]

Further to the problems with the 'read commands from file given as argument', it is actually is a local file inclusion vulnerability - should the user execute keeper in a working directory that happens to have a file of the same name as a keeper command, e.g. get, then keeper will execute commands from that file instead of the intended command.

% cat list
this-device
% keeper list
                     Device Name: Commander CLI on macOS
                Data Key Present: YES
                 IP Auto Approve: ON
                Persistent Login: ON
           Device Logout Timeout: 1 hour
                     Is SSO User: False

Available sub-commands:  rename, register, persistent-login, ip-auto-approve, timeout

Observe that Keeper executed the this-device​ command, not the list​ command.

Implicitly reading commands from a file (or stdin for that matter) is a misfeature at best, security flaw at worst, and should be removed, reverting to using the explicit run-batch option instead.

[sk-keeper]

Yes, it was not a good idea to have both keeper run-batch cmds and keeper cmds commands,
though removing the latter command will break the customer's scripts relying on it.

[ad8-bdl]

It's good that commander uses semantic versioning; the faulty behaviour can be deprecated now and removed in the next major release. (Though it seems the commander versioning is somehow tied to the broader keeper release numbering - which seems very odd - they're quite different codebases; sure version x.y of commander may be dependent on version n.m of the keeper API, but x.y and n.m are totally orthogonal), Anyway, leave that to you.