Re: [Edbrowse-dev] Disabling local echo for password fields

[Dominique Martinet <asmadeus@codewreck.org>]

Karl Dahlke wrote on Mon, Jul 03, 2017:
> 4. Finally, call this function, on and off, when grabbing the password
> for http 401 authentication - piece of cake.

Up to here was easy, done and pushed again on github
( https://github.com/CMB/edbrowse/pull/29 should probably update )

> 5. [...]
> Line 240 fetches the line with mode 1, which strips all the hidden tags out, just the printable version,
> so by line 255 I have no idea what characters come from what html fields.
> You would need some earlier software, fetch the line with mode -1,
> that has all the codes in it, then step through and look for
> InternalCodeChar number < password text >
> then check tagList[number],
> is its action TAGACT_INPUT and is its itype TAGACT_PASSWORD,
> and if yes then overwrite the characters in the fetched line, the one
> that is fetched with mode 1, with stars.

That seems rather annoying: you have to parse the line twice, then try
and match the first parse with the second parse to check every input
fields.

I'm looking at the code with naive eyes and I think removeHiddenNumbers
would be a good place for this: the function is called with both the raw
buffer and writes the new buffers only used for display (so we can muck
with the output on the fly), and does the checks for InternalCodeChar
already so we just have to handle '<' slightly differently from the
other codes if the tag is INP_PW

Now, removeHiddenNumbers is called multiple times at different places;
it might be best to have two variants of the function (or an option) and
in displayLine call fetchLine with 0 (copy the buffer but does not run
removeHiddenNumber) then call our variant instead.

I've pushed a first version that just hijacks the function as ugly proof
of concept, and because it takes care of 'et' and 'w' commands as well
without any extra code.

> 6. That all seems doable except I treat PASSWORD just like TEXT.

turns out INP_PW is already a 'major' itype, so this worked without this
part.

> 8. Now for input i2=password, which I have no way to anticipate until
> you have already typed it in with echo.
> [...]
> It would set a flag and disable echo.
> buffers.c line 385 inputLine() would get the line as usual, but turn
> echo on after it got the line.
> A one time silent read.
> Question - how would this work in readline mode, when I'm calling
> readline() rather than fgets()?
> I don't know anything about readline; never use it.

I'd rather not use readline/regular read function but call gets within
the same command, just like http auth handles its own reads, maybe using
the same prompt_and_read function.
readline will likely remember the password in its own history and might
mess with echo by itself (it's really not meant for password input)

> 9. The noecho command should probably print "echo off" or some such so
> you know you're ok to enter your password.
> This should be an international message of course.

A good place to discuss both the actual command and the message.
I'd use ipass and MSG_Password, as it likely will be used for passwords
(and I can be lazy and not add a new message)
I think 'priv' or 'private' is not obvious enough that it talks about
input, hence the leading 'i'
The function could be used for non-password fields and set itype to
INP_PW on the fly if it is not already.

> 11. Any of these changes should be documented in the users guide, and
> I'm happy to do that.
> It's written in raw html and not many people want to muck with that.
> This is of course the last step, but an important one; and also
> mention the new feature(s) in CHANGES.

I actually do not mind editing html, will do once we all agree on the
interface details.


> 12. I can't comment on the curl ssl authentication discussion, I'm not
> sure why the change was made, or if it was getting around a bug in an
> old version of curl, a bug that isn't there any more,
> or what we should really do here; I'll leave that to Chris and Adam.

Still waiting on this bit; if the old change (making libcurl not use
NEGOTIATE) is no longer needed it would certainly be easier to just
enable it back.
I'm happy with an option though, and will update it to reuse unused
values for messages instead of adding new ones if we stay with that.

-- 
Dominique