fourjay / vim-password-store

Vim niceties for password store ("pass" the "standard Unix Password Manager")
Other
4 stars 0 forks source link

Syntax obfuscation breaks without CursorLine (was: Colouring hacks depend on dark background) #2

Closed madduck closed 5 years ago

madduck commented 5 years ago

Hi, it seems that there are a number of assumptions in place that prevent me from using this addon on a light background.

The short-password highlight appears red-on-red. Why not link it with Error?

highlight link password_store_password_short Error

And concealing should probably just set the foreground to the background:

highlight! password_store_password guifg=bg ctermfg=bg
fourjay commented 5 years ago

Thanks for the feedback (I'm surprised anyone has noticed this tiny little work)

concealing should probably just set the foreground to the background

My intent was to create a "redacted" look rather then to make the text disappear, something I'd find confusing. I'd want the idea of text without visibility. That said, it makes sense to leverage the existing foreground/background colors. Would you find the reverse, setting the bg color to the fg color reasonable? This would create a redacted style blockout that would respond more gracefully/robustly to differing backgrounds/scheme. An alternative would be to explore vim's conceal, but that's more work (and has a limit on backward compatibility).

Linking to error is reasonable, but, as my intent was to hide the block when not directly editing, I'm leery of making this choice, as Error was meant to highlight, not conceal. I can imagine that a user might find a 6 character password acceptable, in which case the block-out behavior seems preferable to highlighting in red. Maybe an option to disable the short password warning?

fourjay commented 5 years ago

I've thought a bit about it, and chose to map the colors from the existing loaded colorscheme, using the fg colors of Comment and Error. I found the straight fg color to be a little too garish for my tastes. I hope this helps with your colorscheme. If you have further trouble, let me know what colorscheme you are using

madduck commented 5 years ago

My intent was to create a "redacted" look rather then to make the text disappear,

What is the difference? I think before we take this further, I need to understand what exactly you are trying to do.

For instance, I would find it sensible to have the password invisible unless the cursor is in that line. And when the file opens, have the cursor be on another line, if possible. This would successfully deter shoulder-surfers when all I want is metadata.

What are you trying to achieve?

fourjay commented 5 years ago

Did you try the changes? If so, do they help?

I suspect the issue comes down to cursorline. Do you have this set? In context with your question: "what are you trying to achieve?" here is the behavior I actually see.

1) While editing (i.e. cursor is in password line) the text is visible. This is due to cursorline changing the bgcolor of the working line. This seems a good candidate for trouble, as a) some themes will not define CursorLine highlighting, and others may not have it turned on.

2) When out of the line, text is obscured in a block. This is the reference to "redacted" as it looks like the sensitive documents released in court cases where the copy has certain words covered in black rectangles ( https://i.stack.imgur.com/ytmWe.jpg ). FWIW, my inspiration came from https://github.com/Evidlo/passhole where the display of the password on terminal is a solid red block (still allows manual copying). I do want to see that there is something there, but I want it to be not readable.

Worth noting, I see these settings as "one-shot" that is I expect the editor to be fired up for the (quick) password editing job, and then shutdown immediately after. As such it seems reasonable to not worry too much about interaction with editing jobs outside of pass. As such, I'll add a set cursorline to the syntax file. This is not sufficient in all cases, but it's a reasonable step. I'll also add settings to turn off syntax. I think I'll leave highlighing as default, as the original plugin is still a good choice for a more minimalist plugin.

One final quick note, since my initial goal was just to set a filetype to hang additional behavior off of, this is still open to others. A filetype autocommand or some additional ftplugin is a reasonable choice to add additional behavior (if you have a good idea, I'd welcome a suggestion).

fourjay commented 5 years ago

That ended up being more difficult then I'd expected. Syntax is loaded too early for overriding in normal vimrc. I've split off syntax into two files, the expected pass.vim, which links short passwords to Error and normal passwords to comment and obfuscated which overrides the highlight to create the blockout/redacted effect. I've added a settings array, setting g:password_store_settings.enable_syntax='false' (after initializing the array) will keep obfuscation from being loaded. FWIW, the obfuscation can be manually turned off by setting syntax to just 'pass' (it's normally set to 'pass.obfuscated'). Reasonably happy with this, layering syntax is readable, and allows a pseudo syntax to be loaded without vim autoloading order issues.

I've verified that behavior without cursorline is less friendly (only current character is visible) and have added a setlocal cursorline to the obfuscated syntax.

madduck commented 5 years ago

suspect the issue comes down to cursorline. Do you have this set?

I do not.

I just pulled all your changes. The password is now nicely redacted, but despite cursorline set, I can only ever see the character under the cursor, not the entire line. I can double-click the line to highlight it in my terminal, which will reveal the password. This is fine, but I am curious what I am missing.

Also, if I run :Reveal and then :Conceal, the password isn't redacted again.

fourjay commented 5 years ago

:Conceal and :Reveal are now fixed (changing the syntax strategy broke them, something I missed). FWIW, this pushes the limits of Vader testing (although I'm sure it's possible).

Which colorscheme are you using? What does ::highlight CursorLine show?

I'm using alduin, and my CursorLine highlight is CursorLine xxx term=underline ctermbg=238 guibg=#555555 I do use cursorline and have even tweaked the value some.

I am curious what I am missing.

The bgcolor changes on the current line. It resets bgcolor which makes the text visible In a colorscheme that accounts for cursorline, the color choice is reasonable for all the fgcolors.

Your feedback highlights that the obfuscation strategy is a little fragile (to be fair, it's partly a "Works For Me" project :-) although I'm happy to make it something more).

It'd be possible to manually re-implement the cursorline behavior. One idea would be CursorMoved events. But this seems a heavy solution. I suspect some conceal syntax would be better, but I'd need to work that out.

madduck commented 5 years ago

Which colorscheme are you using? What does ::highlight CursorLine show?

I am using my own colorscheme, and CursorLine is undefined, so:

CursorLine     xxx term=underline cterm=underline guibg=Grey90

If I do

hi CursorLine ctermbg=bg

then I get the desired result. So I don't think you should assume people to have CursorLine syntax highlighting defined, and rather establish a default.

I also think you need to set ctermfg and ctermbg to the extracted values obfuscated.vim, as right now the obfuscation works only in the GUI, not on a coloured terminal.

fourjay commented 5 years ago

With your colorscheme, does hi CursorLine show anything? Or does it generate some sort of default values? I'd like to put a warning alert, but I'm not sure what an empty CursorLine highlight group looks like, vim -u NONE show's values for CursorLine`` for all the colorschemes I've tried.

madduck commented 5 years ago

I showed you the output above…

CursorLine     xxx term=underline cterm=underline guibg=Grey90
fourjay commented 5 years ago

Thanks. I've added cterm support (untested)

fourjay commented 5 years ago

I've added a check for a default CursorLine highlight group, and

1) warn user 2) use a sensible default (albeit blind) of gray

Since there's support for disabling the obfuscation, both as a config statement, and as a Command, and as this is a one-off run, I think this is resolved.

fourjay commented 5 years ago

This is at a reasonable stopping point