RocHack / bb

Command line Blackboard client
MIT License
72 stars 8 forks source link

Reading from .netrc only gets the first word of a multi-word password #23

Closed AstralSorcerer closed 9 years ago

AstralSorcerer commented 9 years ago

For example, if my password is "aaa bbb ccc", it will be read as "aaa". The same is true of user names, but I've never seen a bb user name of more than one word. Note that prompting for the user name/password does not have this issue. Example

AstralSorcerer commented 9 years ago

Maybe we should quote usernames/passwords? Example

clehner commented 9 years ago

It seems that some implementations of netrc will handle quoted strings: https://stackoverflow.com/questions/12674888/can-netrc-handle-passphrases-with-spaces See also man 5 netrc Other ways to handle this:

I think handling single-/double- quoted strings would be best. It will just require more complicated parsing to unquote literal quotes and backslashes.

joeljk13 commented 9 years ago

I think we could do this with sed: s/.*password \("\(.*\)"\|\([^ ]*\)\)/\2\3/p.

This won't allow \" in the string though, so it's better but still limited.

AstralSorcerer commented 9 years ago

Sounds good. Should it also have support for escaped quotes (\")?

AstralSorcerer commented 9 years ago

So it looks like my implementation of ftp(1) supports double quoting and backslash escaping. Is this what we should support for bb?

joeljk13 commented 9 years ago

I guess it doesn't really matter in this case. I had thought it would be necessary if a password had spaces and a ", but something like password "123456" "654321" will correctly have the password be 123456" "654321 because of greedy matching.

AstralSorcerer commented 9 years ago

True, but that conflicts with the behavior of ftp(1). s/.*password \("\(\([^"\]\|\\.\)*\)"\|\(\([^ \]\|\\.\)*\)\).*/\2\4/p handles escaped characters both in and out of double quotes (yes I know it's ugly), and as far as I can tell mimics ftp's behavior. It does leave the backslashes in, but those can be removed by ${password/\\/}

AstralSorcerer commented 9 years ago

RocHack/fix-password-spaces@1bc7446: Does this look good?

joeljk13 commented 9 years ago

We can't remove the backslashes in general, since the password could have a backslash in it. It's probably easiest to either

  1. give up on regexes and use more sophisticated parsing, or
  2. go with a non-conforming approach that will work for pretty much all practical purposes.

My regex (I believe) works for 2.

Unless there's a way to modify yours to deal with backslashes properly, but I don't think there is a simple way.

AstralSorcerer commented 9 years ago

Good point. Since the regex leaves backslashes, how about just something that would remove single backslashes and convert double backslashes to single backslashes?

joeljk13 commented 9 years ago

I think that might work, but I'm not sure. I'll write a quick test script to try out a bunch of passwords.

joeljk13 commented 9 years ago

Also, on a related note, I'm planning on changing read ... to read -r ... so that the prompt doesn't make backslashes special. Since read can handle spaces and such, I can't think of anything you'd need to escape.

AstralSorcerer commented 9 years ago
#! /usr/bin/python3

import sys

def remove_backslashes (str):
    prev=False
    ret = ""
    for i in str:
        if i == "\\" and not prev:
            prev = True
        else:
            ret += i
            prev = False

    return ret

for i in sys.argv[1:]:
    print(remove_backslashes(i))

This seems to work for me

joeljk13 commented 9 years ago

Also note that bb uses tabs for indentation - you did it right for a8595374daf5f07db68b5d608bb6049d325987a3 but not for 1bc74460cae133aced9aabfd5996ee0a744b9044.

AstralSorcerer commented 9 years ago

Sorry, I have my Emacs set to not indent with tabs. I didn't even notice, I'll fix that.

Edit: fixed as of 9ba64e2

joeljk13 commented 9 years ago

sed 's/\\[^\]//g;s/\\$//g;s/\\\\/\\/g' should work too.

joeljk13 commented 9 years ago

And yeah I've messed up the spaces vs. tabs thing a bunch on bb, so now I have a script that will auto-set to tabs if that's what the file uses.

AstralSorcerer commented 9 years ago

sed 's/\\\([^\\]\)/\1/g;s/\\$//g;s/\\\\/\\/g' Need to make sure we grab the character after the backslash so it gets left in.

AstralSorcerer commented 9 years ago
colin@colins-computer:~/Documents/programming/bb$ password=$(sed 's/.*password \("\(\([^"\]\|\\.\)*\)"\|\(\([^ \]\|\\.\)*\)\).*/\2\4/;s/\\\([^\\]\)/\1/g;s/\\$//g;s/\\\\/\\/g' <<< 'machine my.rochester.edu login cpronovo password "aaa\\\\\\\\\\ \"\bbb\\" machine')
colin@colins-computer:~/Documents/programming/bb$ echo $password 
aaa\\\\\ "bbb\

This seems to work, but I had to remove the -n option from sed and the p on the regex. Is there a particular reason those were there?

Edit: It also doesn't seem to like the backquote form of command substitution, I had to use the $() form.

AstralSorcerer commented 9 years ago

Also, since newlines are removed as a side-effect of word splitting following command substitution, is the s/\\$//g term necessary?

Edit: It seems that this is more likely caused durring the variable assignment, as var=$(< file) also removes newlines

joeljk13 commented 9 years ago

Oops, yes, thanks for that correction.

Don't know, but it's probably for consistency with other regexes in bb. Removing them shouldn't break anything.

Perhaps not, but it won't hurt, and I'd rather be safe than sorry. If the regex gets used on a plain string like hello\ then it'll be necessary.

AstralSorcerer commented 9 years ago

Is it OK if I use the $() form of command substitution? The backquote form seems to break the sed expression, probably due to bash interpreting the backslashes specially in that form.

Example:

colin@colins-computer:~/Documents/programming/bb$ echo `echo \\\\`
\
colin@colins-computer:~/Documents/programming/bb$ echo $(echo \\\\)
\\

It seems as if backslash escapes are being processed twice in the backquote form.

clehner commented 9 years ago

Yes, $() is good.

Consider factoring out the complicated part of the login/password regexes into a shell variable, to make things more DRY.

AstralSorcerer commented 9 years ago

Double-escaping each backslash (find-and-replace from \ to ) works as well. I think I'll leave it like that for consistancy. And I'll make the pattern a variable as well.

joeljk13 commented 9 years ago

Consistency is good. For the future, we may want to switch bb over to $() - as you've already seen, it nests better, and there's even been discussion about backticks being deprecated, although it technically haven't been (http://unix.stackexchange.com/questions/126927/have-backticks-i-e-cmd-in-sh-shells-been-deprecated).

AstralSorcerer commented 9 years ago

I agree, we should switch to $(). Also, making the regex a variable removes the need for double backslashes.

joeljk13 commented 9 years ago

I've created a password-tests branch for the test script and passwords that I used. It seems to work well. Feel free to add any other password tests you want - it's just the input password followed by the expected password on the next line. I've made it an orphan branch because it's testing the regex, not bb, and it helps keep master clean.

AstralSorcerer commented 9 years ago

I've changed the regexs in bb to those in the password-test script, which worked for my test password as well.

AstralSorcerer commented 9 years ago

I think this is ready for commit.

joeljk13 commented 9 years ago

I've updated the test branch to match how bb's parse_netrc() works. 2 tests currently fail, but they're edge cases and I'm not going to bother fixing them.

AstralSorcerer commented 9 years ago

I fixed one test; the test password had unescaped spaces. The other looks to be escaping the newline/space/whatever that comes after the password, which I agree is a edge case.