Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Newest preexec version disables ignorespace and ignoreboth #1844

Closed
cornfeedhobo opened this issue Mar 1, 2021 · 5 comments · Fixed by #2003
Closed

Newest preexec version disables ignorespace and ignoreboth #1844

cornfeedhobo opened this issue Mar 1, 2021 · 5 comments · Fixed by #2003

Comments

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Mar 1, 2021

Expected Behavior

When HISTCONTROL is set to ignorespace or ignoreboth, I expect commands with a preceding space to not be stored in history. This is a common expectation to work around situations where it's just easiest to enter a secure string as an argument to a command, and you don't want it stored in history.

Edit: before this inspires a bunch of comments about how this is bad practice, how the secrets are leaked in ps, etc; none of that is relevant. This is a behavior that the user expects to be supported and because they are unable to opt-out of our use of preexec, this should be addressed.

Current Behavior

The updated preexec library edits HISTCONTROL with the __bp_adjust_histcontrol function.

Possible Solution

  • Remove all references to this function
  • Fork and apply a working version of their PR for munging history

See rcaloras/bash-preexec#115

Steps to Reproduce

  1. set HISTCONTROL
  2. reload
  3. type command that starts with a space
  4. history 5
@cornfeedhobo
Copy link
Member Author

@davidpfarrell @NoahGorny could you take a look at this?

@davidpfarrell
Copy link
Contributor

I left a comment on the linked issue.

For now, I think we need to remove references to the function and claim that we don't support $1 in pre-exec actions

@cornfeedhobo
Copy link
Member Author

@davidpfarrell I'll make a PR while waiting for @NoahGorny's input. Thank you very much for reviewing this!

@cornfeedhobo
Copy link
Member Author

@gaelicWizard I know you've since taken an interest in this issue. I want to finally close it out. I didn't really understand what you were trying to suggest for a solution. FWIW, I'm not looking to get into a drawn out solution, I just want something that works now, and can be improved later.

@NoahGorny This relates to the other issue where I brought up patching vendored libs.

The paths forward that I considered are:

  • manually patch when vendoring, with the applied patch file included in our repo just for tracking
  • we fork things we want to vendor and apply our patches there

Considering the overhead of multiple repos for a small use case, I'm leaning towards the first option. I look forward to reading everyone's opinions.

cc: @davidpfarrell

@gaelicWizard
Copy link
Contributor

gaelicWizard commented Oct 10, 2021

My rcaloras/bash-preexec#119 I think is the closest to "best of both worlds" as it allows all relying parties to get what they want in different use cases. I don't have anything to add beyond what I submitted there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants