Skip to content

Add API to allow reading & writing history to file #9

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeanbaptistelab
Copy link

Implement bash history-like feature in Slash: commands will be saved on disk and read when calling the new Slash API (slash_init_history_from_file)
There will be a counter-part in CSH that uses this new feature once this PR is merged.

@Lykkeberg
Copy link

This is a neat feature. We have to remember that the CSH can be used in a lot of different contexts at the same time, so if this is going to be extra, super smart, I think we should name the history files with some context information along side the placement of where it has been invoked. But I'm not fully aware of how 🫣

@Lykkeberg
Copy link

These history files could also potentially end up containing sensitive information to some customers, so how do we protect that? If say, a command to VM is being entered with a password, then this would be forever locked into a history file. I'm not sure that is a problem or not, but it is something we need to consider.

Copy link
Author

@jeanbaptistelab jeanbaptistelab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat feature. We have to remember that the CSH can be used in a lot of different contexts at the same time, so if this is going to be extra, super smart, I think we should name the history files with some context information along side the placement of where it has been invoked. But I'm not fully aware of how 🫣

This implementation does not set the location or name of the history file, this is entirely up to users (mainly CSH) to either:

  • not use it ;)
  • use it with their own file location, naming policy.

This change also does not change any current behaviour (apart from checking for NULL when adding to the history) so this change should be fairly safe.

Copy link
Author

@jeanbaptistelab jeanbaptistelab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These history files could also potentially end up containing sensitive information to some customers, so how do we protect that? If say, a command to VM is being entered with a password, then this would be forever locked into a history file. I'm not sure that is a problem or not, but it is something we need to consider.

A few things can be done:

  • write a function that processes the line to be added to the history and guess whether some tokens look like password and replace them with "*"
  • the user (CSH) can (temporarily) disable persisting the history for the next few commands and turn it back on again (the "history_file" pointer is part of the "struct slash", so saving it to a temp variable, setting it to NULL for the next few commands and restoring it is feasable. I'm contemplating extending the already existing built-in "history" command to add this functionality

@jeanbaptistelab jeanbaptistelab force-pushed the feature/SI-4830-persist-csh-command-history-across-session-a-la-b branch from 7247cc9 to 4d7e805 Compare August 2, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants