In-snap bash tab completion

[chipaca]

This introduces in-snap bash tab completion.

For a snap app to tab complete, it needs to include a completer key, that points to the bash snippet that would usually live in /usr/share/bash-completion/completions. Completion is run confined, and marshalled out. There's an example snap, used in tests, under tests/lib/snaps/complexion.

Furthermore, at this stage to enable snap completions you need to source /usr/lib/snapd/complete.sh after sourcing /etc/bash_completion (or /usr/share/bash-completion/bash_completion). It adds a default completion handler that overrides and falls back to the usual one.

Note that while the snap-side support is for bash completion, if you want to add, say, zsh completion all you need to do is write something like complete.sh that works for bash. This is the unmarshaller.

Also note that because the completion is confined, if the snap can't reach it it can't complete it. If you need to complete hosts, for example, you'll have to override the usual hosts/known hosts file inside the snap. If you want to complete filenames, the snap needs to be able to access the directory path. This usually is what's wanted, as you don't want to complete things you can then not access.

Lastly, I expect there will be issues with this that need refining. The whole edifice of tab completion is a tower of toothpicks stood on end, and this work is about adding a pipe to the middle of it. In other words, please report bugs about this!

I'll be writinig something up about what's going on here, as it's super useful for writing tested bash completion. gasp.

[pedronis]

2 strange things

[pedronis]

is this indented?

[chipaca]

nope, fixed

[zyga]

We should fix the package build process to have a data/info.in, it's just annoying.

[pedronis]

debugging left over?

[chipaca]

yeah, noticed a few of these. Running tests on a fixed commit.

[chipaca]

these seemingly random changes are so that the diff between the 14.04 and 16.04 snapd.installs are only about what's important

[chipaca]

this is very similar to tests/main/complete/lib.exp0; work for another day to DRY it.

[pedronis]

cp -a might be the simplest thing here, also these bits will conflict with #3027

[zyga]

This is incorrect. The mount dir is different in the outside and on the inside of the snap execution environment. On the inside it is a /snap/... but on the outside it is variable.

[chipaca]

snap-exec always has app.Snap.MountDir() to /snap/..., which is correct because it's always "inside".

[zyga]

Correct, sorry, I didn't notice this is inside the core snap already (snap-exec). Thank you for taking the effort to measure this experimentally.

[chipaca]

I'll be doing manual testing of this on Fedora to check @zyga 's concerns, but probably next week at this point.

[zyga]

Just some comments. I didn't review the tests, I need to wrap my head around the bash completion thing to review it properly.

[zyga]

This link is no longer working.

[zyga]

What is happening here?

[chipaca]

Each completion test tries to complete something by double-tabbing on empty, double-tabbing at the start of a word, single-tabbing something that returns a single match, and double-tabbing something that returns some but not all matches. This same thing is run "directly" (from tests/completion/simple and "indirectly" via snap completion (from tests/completion/indirect). These directories set up the tests, but the tests themselves key off of the variant to source the appropriate .vars (which sets what to complete, and what to expect of that completion), .sh (for test-specific setup; e.g. changing to a particular directory to test particular filename comletions), and .completion (the completion snippet itself).

HTH?

[chipaca]

I've built snap, snapd, and snap-exec and copied them over to a F25 box, and it works :-)

[zyga]

Curious, why is this a separate suite?

[chipaca]

so i could inherit the environ down into the tests (otherwise i wold've had to duplicate it).

Also, they're very different and weird :-)

[zyga]

I didn't know that [ string ] is equivalent to [ -n string ].

[chipaca]

my personal problem with -n is that i always have to look it up as i never remember which is it and which is -z.

[mvo5]

Curious, why is the file called this way? And a small comment in the header what its actually doing would be great.

[chipaca]

complete.sh (sets up tab completion for snap-based things, and) serialises the tab completion "request", and de-serialises the tab completion "response" into the result that's seen by the shell. etelpmoc.sh de-serialises the tab-completion request, performs the tab completion, and serialises its result into the tab completion response. etelpmoc is a weird, backwards, tab completion thing, which is why it's called complete, backwards.

[chipaca]

addressed in the latest commit

[jdstrand]

The design looks good: the complete.sh is running under the user's context and is therefore just a normal bash completion script. If it the thing to complete is in our PATH and in /snap/bin (or /var/lib/snapd/snap/bin), then it calls snap run --complete ... which only executes the snap's completion code via snap-exec, which is called via snap-confine and thus under confinement for the given snap command (ie the same confinement as --shell). As a result of this, a few harmless accesses need to be added to the default template. The unmarshalling happens via etelpmoc.sh, which also runs under the snap's confinement (including new devpts). This is a good design.

It would be nice to show that confinement is working as expected in the spread tests by having an evil completer script try to access files the snap is not allowed access to.

As an attacker, it seems that etelpmoc.sh is the thing to target. Specifically, making it feed bad things back in an effort to affect the shell or that will run outside of confinement (eg, 'foo' results in 'foo bar ; sudo ...'). By the end of etelpmoc.sh you are echo'ing quoted strings still under confinement, but I'd like to see some output sanitizing here. A start might be to use "%q" with "printf" as suggested inline. Perhaps escaping anything that isn't part of '[a-zA-Z0-9_-]' as well.

[jdstrand]

Small nit-pick. It seems like perhaps setting defaultShell := /bin/bash up above and then setting "shell" and the fallback for "complete" to this might be better.

[jdstrand]

Likewise here (set defaultShell somewhere and set fullCmd to it).

[jdstrand]

Can you change this to:

. "$_compscript"

[chipaca]

yep

[jdstrand]

Is xargs expected to be everywhere? It is in the core snap...

[chipaca]

this is run "inside", so yes :-)

[jdstrand]

Is it possible to quote this too (ie, "$_compfunc")? It isn't terribly important from a snap confinement perspective since we are running under confinement at this point, but still.

[chipaca]

the problem is $_compfunc will be one of two things: a function name (from -F) or a command (from -C); quoting the second will break, in most cases.

[jdstrand]

Is there a reason you didn't use printf for all of these? Eg:

printf "%q\n%q\n\n%q\n" "${!_compopts[@]}" "$_bounce" "${COMPREPLY[@]}"

[chipaca]

your example wouldn't work because each of the two array variables would expand to a number of words and printf would take them off one at a time assigning a word to a %, resulting in something quote different to what I think you meant.

I used a plain echo for the keys of the _compopts associative array because they're supposed to be single words so no quoting should be necessary.

I didn't use %q at all because then I'd have to unescape things, and I think that means I'd have to use eval?

[jdstrand]

Actually, reading the bash man page on the builtin printf command, it says: "%q causes printf to output the corresponding argument in a format that can be reused as shell input.". Perhaps that is all you need for this case? Eg:

$ printf "%q\n%q\n\n%q\n" "foo" "bar" "; sudo cat /etc/shadow"
foo
bar

\;\ sudo\ cat\ /etc/shadow

[chipaca]

ignoring the one-single-printf-won't-work-for-this but looking at your request for %q, I'm not sure I'm following your reasoning here. The output of etelpmoc.sh isn't read as shell input, that is, it isn't just sourced from the 'outside'; it's used to set environment variables which the programmable completion tells you to set (or to call compopt). As such, if etelpmoc.sh quotes things, complete.sh needs to unquote them again, otherwise the user won't be offered the right completions.

[jdstrand]

As discussed on IRC, what I'm thinking about here is the transition from confined to unconfined. Ie, everything up until snap run command=complete is fine because it is running within the user's context with only trusted input and code. Everything after that and until etelpmoc.sh exits is ok and no worse than running non-bash-completion snap cli commands in the shell because it is running under the confinement of the snap. etelpmoc.sh then fills various variables for the unconfined shell to interpret based on the completer script in the snap and the unconfined bash displays this to the user-- it is this handoff I am concerned about.

More specifically, I think wrt this transition that there are two things to think about:

1. that a malicious completer script can feed something back to etelpmoc.sh and ultimately bash that causes bash to misbehave. This would arguably be a bug in bash, but perhaps there is output validation from etelpmoc.sh that can be done to put guard rails on potential issues
2. tricking the user into completing something that she didn't want to, but accidentally did

I've been looking at '2' this afternoon (I'll continue to look at '1' later) and found there are a number of ways to trick the user with just ';':

$ mkdir bar
$ cd bar
$ complexion <tab>
  |
   -> $ complexion ;ls ~/.ssh # user is one 'Enter' away from getting pwn'd
      $ ^C
$ touch 1 2
$ complexion <tab>
  | 
   -> $ complexion 
      1;ls ~/.ssh  2;ls ~/.ssh
      $ complexion 1<tab> # after typing '1<tab>, user is one 'Enter' away from getting pwn'd
      $ ^C
$ complexion <tab>
  |
   -> $ complexion
      1            2            3;ls ~/.ssh # note, '3' doesn't exist
      $ ^C

It is easy to imagine less obvious things, especially if doing command completion. Eg:

$ complexion <tab>
  |
   -> $ complexion
      current/1;/usr/bin/baz  current/7/usr/bin/foo   current/9/usr/bin/bar
      $ ^C

I'm sure creative folks could do a lot more. To me this clearly shows we need some sort of output validation in etelpmoc.sh such as piping through grep and disregarding any results that have weird stuff. Ideally this would be a whitelist, but I fear this is going to be problematic for filenames.... At an absolute minimum ';' needs to be removed, but really several things ('*' comes to mind, but likely others) are going to be potentially problematic. :\

[jdstrand]

A couple other things while I was playing with this:

• shell gets very grumpy when etelpmoc.sh is not in the core snap. input and output is all messed up. This should be considered with refresh/rollback/etc
• shell gets pretty grumpy if the completer prints to stdout or stderr. Not sure what to do about this. Might be nice to redirect them somewhere...
• tests/lib/snaps/complexion/bin/complexion should use /usr/bin/printf or bash instead of sh since '%q' is not supported by the dash printf builtin
• tab causes /etc/init.d/ denial. Is this _init_completion?
• cd /tmp ; complexion <tab> doesn't work cause /tmp is in the context of the snap. The only thing for _filedir and friends with this is document that tab completion of files works only within the context of the snap's mount namespace

[jdstrand]

You can simplify these two to:

/etc/bash_completion.d/{,*} r,

[jdstrand]

Are /etc/bash_completion.d/ and /usr/share/bash-completion/bash_completion what are used on all distros?

[chipaca]

AFAIK yes (but @morphis can probably confirm)

[jdstrand]

Thanks for the output validation. That seems to work pretty well in blackbox testing. I also am satisfied with the way the handoff back to unconfined is working: you are validating 'opts', 'bounced', and 'sep' very strictly. Anything after 'sep' is passed through grep into COMPREPLY and that removes results that contain a few potentially scary items that might cause problems for the user running the completion.

Most everything in this review is regarding commenting the code. Maybe it's just me, but I found how bash completion works rather non-intuitive so it took me a bit to understand the flow. Adding the requested comments will go a long way to help future maintenance.

Where will the snap.yaml changes be documented? Wherever it is, I think it is important that it mentions that the completion happens within the context of the snap's runtime, so completion's like _filedir in the system's /tmp won't work .

[jdstrand]

This comment seems slightly off. I suggest rewording the comment as:

# _complete_from_snap performs the tab completion request by calling the
# appropriate 'snap run --command=complete' with serialized args and
# deserializes the response into the usual tab completion result.

[jdstrand]

Perhaps an overview of the flow of control would be good here (or at the top of the file) as well. Eg:

# How snap command completion works is:
# 1. snappy's complete.sh is sourced into the user's shell environment
# 2. user performs '<command> <tab>'. If '<command>' is a snap command,
#    proceed to step '3', otherwise perform normal bash completion
# 3. run 'snap run --command=complete ...', converting bash completion
#    environment into serialized command line arguments
# 4. 'snap run --command=complete ...' exec()s 'etelpmoc.sh' within the snap's
#    runtime environment and confinement
# 5. 'etelpmoc.sh' takes the serialized command line arguments from step '3'
#    and puts them back into the bash completion environment variables
# 6. 'etelpmoc.sh' sources the snap's 'completer' script, performs the bash
#    completion and serializes the resulting completion environment variables
#    by printing to stdout the results in a format that snappy's complete.sh
#    will understand, then exits
# 7. control returns to snappy's 'complete.sh' and it deserializes the output
#    from 'etelpmoc.sh', validates the results and puts the validated results
#    into the bash completion environment variables
# 8. bash displays the results to the user

[jdstrand]

Please add a comment here:

# De-serialize the output of 'snap run command=complete ...' into the format
# bash expects.

[jdstrand]

Please adjust this comment to # only lowercase alpha characters allowed

[jdstrand]

If I'm reading the tests correctly, we only have tests for "" and "export". Would it make sense to add tests for alias, job, variable and "unrecognised bounce"? Please correct me if there are tests for all of these.

[chipaca]

you are correct, more tests could be added

[jdstrand]

Personal preference is to use if [ -n "$sep" ]; then but I won't block on it.

[jdstrand]

I saw some weirdness that I think was related to IFS if the snap's completer script (or modified etelpmoc.sh) did unusual things. I wonder if after 'snap run' we could make sure that IFS is set back to something sane?

[chipaca]

it being local means it shouldn't be able to affect behaviour outside of the function

[jdstrand]

Hmm, it might not have been IFS related and instead terminal related. It was an unfortunate situation. There is likely work to be done in this area when a reproducer is found/a bug comes in.

[jdstrand]

Thanks for this! Note that shellcheck had this to say:

$ shellcheck -s bash ./complete.sh
In /usr/lib/snapd/complete.sh line 45:
            COMPREPLY=( $( \grep -v '[[:cntrl:];?*{}]' ) )
                           ^-- SC1001: This \g will be a regular 'g' in this context.

We use shellcheck for our shell scripts elsewhere and think adding this to run-checks for this and etelpmoc.sh would be a great idea.

[jdstrand]

I meant to also say to please document the arguments to this grep. Eg:

# Ignore any suspicious results that are uncommon in filenames and that
# might be used to trick the user. A whitelist approach would be better
# but is impractical with UTF-8 and common characters like quotes.

All that said, I did the following and think we can probably do better:

$ mkdir foo ; cd foo
$ touch "fooz'ball" 'party "animal"' "disc (1)"
$ complexion <tab>
  |
   -> $ complexion
      disc (1)        fooz'ball       party "animal"
      $ complexion d<tab>
        |
         -> $ complexion disc (1) # tab completion has no escapes! Pressing 'enter' doesn't work as expected
            -bash: syntax error near unexpected token `('
$ ls <tab>
  |
   -> $ ls
      disc (1)        fooz'ball       party "animal"
      $ ls d<tab>
        |
         -> $ ls disc\ \(1\) # tab completion has escapes! Pressing 'enter' works as expected
            disc (1)

This demonstrates a usability bug. I don't think we can get away with not quoting. The trick will be not quoting the listing, but quoting the actual completion, like 'ls' does.

[jdstrand]

Note that shellcheck complained about this:

$ shellcheck -s bash ./complete.sh
In /usr/lib/snapd/complete.sh line 50:
            compopt $(printf " -o %s" "${opts[@]}")
                    ^-- SC2046: Quote this to prevent word splitting.

[jdstrand]

The code in _complete_from_snap() is primarily about deserialization and I'm not terribly familiar with bash completion code, so I think it would be most helpful if you documented the various variables where you are setting them. Eg: opts, bounced, and sep. Then mention why you are calling compopt and how you are manipulating COMPREPLY. I don't think these have to be extensive comments, just something so that when we look at this in the future it is a little more penetrable.

[jdstrand]

Please adjust this comment to include something about how 'snap run --command=complete' calls this and that this script is running under the snap's runtime environment (eg, mount namespace, confinement, etc).

[jdstrand]

Please add a comment:

# De-serialize the command line arguments and populate tab completion environment

[jdstrand]

Please add a comment:

# Source the bash-completion library functions and common completion setup

[jdstrand]

Please add a comment:

# Now source the snap's 'completer' script

[jdstrand]

Can you update the comment to say what the options are for? When I read the comment, I asked myself "options for what?" :)

[jdstrand]

Why? Can you update this comment to describe why you are doing this?

[jdstrand]

Shellcheck has this to say:

$ shellcheck ../foo/core/etelpmoc.sh
In ../foo/core/etelpmoc.sh line 70:
    \compgen "$@"
    ^-- SC1001: This \c will be a regular 'c' in this context.

I feel like your comment is meant to deal with this, but it is unclear to me...

[jdstrand]

Can you add a comment something to the effect of:

# aliasing compgen and setting expand_aliases so we may use our xcompgen instead

That said, why are you aliasing compgen but overriding compopts, below?

[jdstrand]

Can you update this comment to say why you are calling 'complete -p' and putting it into _comp? What is _comp being used for? 'complete'? If so, why aren't all of complete's args not represented (eg, -G, -P, -S, -X, -DE). Perhaps just mention what _comp is and that you are only processing args relevant for deserializing?

[jdstrand]

Shellcheck has this to say:

In ../foo/core/etelpmoc.sh line 149:
                readarray -t COMPREPLY < <( \compgen -W "$OPTARG" -- "${COMP_WORDS[$COMP_CWORD]}" )
                                            ^-- SC1001: This \c will be a regular 'c' in this context.

[jdstrand]

Why? What about -DE?

[chipaca]

-D and -E don't make sense here as they're "default" handlers, you'd never cross into snap-land for those. The rest ... just haven't implemented them yet i guess? ;-)

[jdstrand]

In ../foo/core/etelpmoc.sh line 171:
        readarray -t COMPREPLY < <( \compgen -A "$_compact" -- "${COMP_WORDS[$COMP_CWORD]}" )
                                    ^-- SC1001: This \c will be a regular 'c' in this context.

[jdstrand]

'%q' is not supported by 'dash', which is '/bin/sh' on (at least) Debian and Ubuntu. Please change to /bin/bash or use an absolute path to printf.

[jdstrand]

In addition to my previous comments, I thought about this a little more and realized we need to validate when bouncing and that our completion output is different than say, ls, which is a usability bug (fixing the usability bug would also address my security concerns).

[jdstrand]

Thanks for this! Note that shellcheck had this to say:

$ shellcheck -s bash ./complete.sh
In /usr/lib/snapd/complete.sh line 45:
            COMPREPLY=( $( \grep -v '[[:cntrl:];?*{}]' ) )
                           ^-- SC1001: This \g will be a regular 'g' in this context.

We use shellcheck for our shell scripts elsewhere and think adding this to run-checks for this and etelpmoc.sh would be a great idea.

[jdstrand]

I meant to also say to please document the arguments to this grep. Eg:

# Ignore any suspicious results that are uncommon in filenames and that
# might be used to trick the user. A whitelist approach would be better
# but is impractical with UTF-8 and common characters like quotes.

All that said, I did the following and think we can probably do better:

$ mkdir foo ; cd foo
$ touch "fooz'ball" 'party "animal"' "disc (1)"
$ complexion <tab>
  |
   -> $ complexion
      disc (1)        fooz'ball       party "animal"
      $ complexion d<tab>
        |
         -> $ complexion disc (1) # tab completion has no escapes! Pressing 'enter' doesn't work as expected
            -bash: syntax error near unexpected token `('
$ ls <tab>
  |
   -> $ ls
      disc (1)        fooz'ball       party "animal"
      $ ls d<tab>
        |
         -> $ ls disc\ \(1\) # tab completion has escapes! Pressing 'enter' works as expected
            disc (1)

This demonstrates a usability bug. I don't think we can get away with not quoting. The trick will be not quoting the listing, but quoting the actual completion, like 'ls' does.

[jdstrand]

I think we need to grep/quote here to, no?

[jdstrand]

Ok, so if I change complete.sh to have (there are better ways to do this):

            local reply=( $( cat ) )
            for i in $(seq 0 $((${#reply[@]} - 1))); do
                COMPREPLY[$i]=`printf "%q" "${reply[$i]}"`
            done

Then get:

$ mkdir foo ; cd foo
$ touch "fooz'ball" 'party "animal"' "disc (1)" "norf;qux"
$ complexion <tab>
  |
   -> $ complexion
      disc\ \(1\)        fooz\'ball         norf\;qux          party\ \"animal\"
      $ complexion n<tab>
        |
         -> $ complexion norf\;qux
            Greetings from inside /snap/complexion/x3.
            Arguments given:
            > norf\;qux
            That is all. Have a nice day.

This at least makes it so the actual completion is both safe and usable. However, it still differs from 'ls' output. It also isn't likely going to work right for command completion that might use one of the special characters as part of the command (I think this is what you were getting at initially).

Also, was reading /usr/share/bash-completion/bash_completion _quote_readline_by_ref() and it said that compgen needs its arguments quoted/escaped, so, yes, we need to handle bounced.

One last thing, '&' is just as bad as ';' so it should be added to the grep regex. However, really starting to think we need to remove that and add quoting....

[chipaca]

again, adding a level quoting unilaterally is not going to be the answer. In this case in particular it seems _filedir isn't working properly, somehow the compopt -o filenames isn't making it across. Digging into it.

[chipaca]

in particular the problem here was because at some point i changed compopt to run in a sub-shell, which doesn't work. :-(

[jdstrand]

Regarding quoting unilaterally, I agree: it really depends on what the completer script is doing on whether or not it should be heavily quoted. Your observation about _filedir is keen-- nice! I think that with our grep, we are ok (but see inline comment on the bounce case) and fixing _filedir is a bug fix.

[jdstrand]

Thank you for all the extra code comments, the insight about _filedir and for adding '&'. Leaving as 'request changes' as there is one remaining question surrounding bounce.

[jdstrand]

I feel like this needs the grep treatment. I'm not terribly clear on the program flow of how bounce works wrt etelpmoc.sh. Can you comment?

[chipaca]

I don't mind, but note that the bounced things are all for things the user has defined in the "outside" shell (aliases, variables, jobs); it's not arbitrary strings.

[jdstrand]

Yeah, I don't care about trying to protect the user from herself. I was unclear on how 'bounced' worked and what was in '${COMP_WORDS[$COMP_CWORD]}' and what would be in there. Reading https://github.com/snapcore/snapd/pull/3150/files#diff-96d0d48e1bd095153ade81afdb99f7ebR173 and 'complete.sh' more carefully, I see that '$bounced' can only contain "alias"|"export"|"job"|"variable" (and you are already validating it up above) and that ${COMP_WORDS[$COMP_CWORD]} is not coming from 'etelpmoc.sh'.
As a result, can you add a comment:

# We validated 'bounced' above and '${COMP_WORDS[$COMP_CWORD]}'
# is coming from the user's session, not the snap so skip input
# validation since we aren't trying to protect the user from
# herself.

[jdstrand]

Approving since all that is needed is a final comment regarding 'bounced' in complete.sh. Please fix the issues surrounding quoting and _filedir to address the usability issues identified in this PR either before merging or in a later PR.

[zyga]

Do we need to do anything else special to prevent random stuff from interfering with tab completion?

[chipaca]

I wouldn't be surprised if we did, but I didn't spot anything

[zyga]

Some small comments. I think I'm +1 on this change but I fear we will be fixing some security issues here down the line. It's just too complex (bash is hairy) to be perfect IMO. I don't think I grok bash tab completion as much as I'd like, the arcane syntax doesn't help.

Still, I think we should merge this and iterate. A piece of solid and impressive work @chipaca!

[zyga]

We may need a GPL header here.

[zyga]

nitpick: s/snappy/snapd/

[zyga]

nitpick: s/snappy/snapd/

[zyga]

same

[zyga]

Thank you for documenting this! This goes a long way towards explaining the concept in simple terms!

[zyga]

maybe use the term reverse, I know it is backwards but I think it would read better here.

[zyga]

s/environs/environment/ (unless I misunderstand)

[chipaca]

man 7 environ :-)

[zyga]

This is somewhat tricky as each app can have different confinement. Do we allow plugs for completion scripts?

[jdstrand]

No please. I think it would be quite surprising for a completion script to by hitting the network or doing privileged operations outside of the snap. I strongly prefer the first iteration is 'no plugs allowed' for the completer and only after we have clear use cases do we consider something more.

[jdstrand]

Actually in retrospect, as much as I prefer this, I think it is impractical. _filedir clearly would benefit from plugs not to mention the nmcli command needing to plug network-manager. To specifically answer @zyga's question, "yes, we allow plugs for completion scripts" (the tests even have an example).

[chipaca]

@jdstrand I'm unsure whether this means I have work to do on this or not

[zyga]

I think it is okay. If something has access to $HOME it should also have access to $HOME when tab-completing.

[jdstrand]

@chipaca - no. I was thinking you had work and then in retrospect realized it needs to be the way it is.

[zyga]

This syntax is pretty unusual. I'd leave a comment for the future readers to explain what is going on.

[chipaca]

you mean ${!i}?

[zyga]

Yes

[zyga]

LOL

[jdstrand]

@zyga - "I fear we will be fixing some security issues here down the line"

If you fear security issues we shouldn't be merging it. Are there particular things that you fear (apart from implementation bugs or bugs in bash)? I believe the design is sound because the snap's completer code only runs within confinement. The code in complete.sh and etelpmoc.sh is also careful (but more eyes on this are welcome!). Do you have concerns on the design or the input validation of etelpmoc.sh and complete.sh?

I very much agree that the _filedir quoting should be addressed and this is certainly a 'usability bug' (the grep filters on the most egregious characters so this shouldn't be a 'security bug', unless something was missed (did I mention more eyes?)). Ideally I'd like to see that fixed before merging since this PR missed 2.25 and there should be time to fix that for 2.26-- @chipaca, what is the status of this?

[chipaca]

I very much agree that the _filedir quoting should be addressed and this is certainly a 'usability bug' (the grep filters on the most egregious characters so this shouldn't be a 'security bug', unless something was missed (did I mention more eyes?)). Ideally I'd like to see that fixed before merging since this PR missed 2.25 and there should be time to fix that for 2.26-- @chipaca, what is the status of this?

The work on debugging why _filedir does not result in properly quoted things in some situations has not started; having a long chain of PRs that can only land once the last one is "green" is a recipe for the whole thing taking multiple months, with a lot of busywork resolving conflicts. Given it's taken the team nearly a month (and two releases) so far to have one and a half reviews of this code, if anything further delays this then it's not going to make it for 2.26 either. So I'd rather this work were not put in that situation.

[zyga]

@jdstrand I just think that given the relative obscurity of tab completion implementation we may be missing something. I agree that we should add more tests here, perhaps as malicious as one can think of (trying to exploit bash to do something unwanted).

[jdstrand]

@chipaca

"The work on debugging why _filedir does not result in properly quoted things in some situations has not started; having a long chain of PRs that can only land once the last one is "green" is a recipe for the whole thing taking multiple months, with a lot of busywork resolving conflicts. Given it's taken the team nearly a month (and two releases) so far to have one and a half reviews of this code, if anything further delays this then it's not going to make it for 2.26 either. So I'd rather this work were not put in that situation."

It is definitely a usability bug and nearly a security bug. I'm in favor of getting the quoting right ASAP as a hardening measure for landing in case we missed something in the blacklist (since blacklisting shell metacharacters is almost always a recipe for disaster). That said, in this case getting the quoting right isn't about preventing subverting the system but about tricking the users via wacky completions. IMO we should never introduce a feature that knowingly introduces a security bug and we shouldn't introduce usability bugs when possible. This issue is borderline, so I'll let others decide if it is a blocker.

[jdstrand]

Recent commits fixed most of the problems with _filedir. Specifically, if I do:

$ mkdir foo
$ cd foo
$ touch "fooz'ball" 'party "animal"' "disc (1)" "y&uptime" "z;uptime"
$ ls
disc (1)  fooz'ball  party "animal"  y&uptime  z;uptime
$ complexion <tab>
disc (1)        fooz'ball       party "animal"
$ complexion di<tab>
   |
   -> $ complexion disc\ \(1\)
      Greetings from inside /snap/complexion/x2.
      Arguments given:
      > disc\ \(1\)
      That is all. Have a nice day.

The above shows that "y&uptime" and "z;uptime" were properly filtered out, that the completion list looks pretty (no backslashes) but the command completion is properly escaped. I think this demonstrates _filedir is working reasonably well.

If I try to create a malicious completer that fills COMPREPLY with ';', etc, then these things are still filtered out.