pezra / rspec-mode

An RSpec minor mode for Emacs
257 stars 112 forks source link

Set safe function for string defcustom variables #167

Closed carlthuringer closed 6 years ago

carlthuringer commented 6 years ago

Using the following .dir-locals.el file...

;;; Directory Local Variables
;;; For more information see (info "(emacs) Directory Variables")

((nil
  (rspec-use-docker-when-possible . t)
  (rspec-docker-command . "docker-compose exec")
  (rspec-docker-container . "core"))
 )

I was getting the error: (sorry couldn't copy out of emacs... and buffer gone after proceeding) screen shot 2017-10-19 at 4 11 33 pm

My lisp is super rusty, but after some fidgeting around and reading about this dir-locals and error, I learned that there should be a :safe function that identifies safe values. Using stringp solved it for rspec-docker-command, and so I figured it should be applied to the other string variables. I'm not sure why I don't get a warning about core.

I also tried to learn ert and figure out a way to test the dir-locals-read-from-file method but while I got the test passing... I couldn't get it failing, which indicates to me that I wasn't testing what I thought I was.

Anyways, here's a PR for my hack.

dgutov commented 6 years ago

That is not a good idea. The safe variables mechanism is supposed to protect you from somebody making Emacs do something bad by adding a malicious .dir-locals.el to a parent directory.

Allowing though just any string here without user confirmation can do just that.

I was getting the error: (sorry couldn't copy out of emacs... and buffer gone after proceeding)

It's not an error, it's a user prompt.

carlthuringer commented 6 years ago

That is not a good idea. The safe variables mechanism is supposed to protect you from somebody making Emacs do something bad

Yes, I also read this and struggled with what to do, given my admitted inferior elisp capabilities. While the documentation is clear on what this means, it is unclear on how to address such situations, leaving it up to tool authors' interpretation.

As you can see I want to use docker-compose exec instead of run, so I don't generate a lot of myapplication_run_X containers while running tests. This isn't a feasible default for all users because it requires the user to start the containers or docker-compose up elsewhere in order to test, so it makes sense for me to customize in this way.

However, I don't want to have to answer this prompt, and I don't want to disable the safety feature, by some compromise I see a few alternatives.

1) whitelist rspec-docker-command using safe-local-variable-values. 2) change this customization to a choice among docker-compose run and docker-compose exec. 3) Write a safe function that asserts that whatever's written here begins with docker-compose and has at most one more word.

And roll back all the other :safe keys I eagerly added to get feedback rather than just to my specific point of concern.

dgutov commented 6 years ago

However, I don't want to have to answer this prompt

Note that only user has to answer it just once, to whitelist a particular value. Adding it to safe-local-variable-values in their custom-file as a result.

whitelist rspec-docker-command using safe-local-variable-values

I'm really not sure that Lisp programs are supposed to modify this variable.

change this customization to a choice among docker-compose run and docker-compose exec

Does that actually make both values "safe"?

Write a safe function that asserts that whatever's written here begins with docker-compose and has at most one more word

Or simply checks that it equals to either of the two strings. I'd prefer the previous option if it works, though.

carlthuringer commented 6 years ago

Or simply checks that it equals to either of the two strings.

How about this? Excuse my code, I really am poor at lisp.

dgutov commented 6 years ago

How about this?

Looking good. And I think you can remove the quote char before the lambda form.

carlthuringer commented 6 years ago

It looks like you've updated master, @pezra so there's no longer any need for this PR. Thanks for your patience and help. :)

pezra commented 6 years ago

@carlthuringer, this was all @dgutov. As are most things (read: everything) rspec-mode these days.

@dgutov, thanks again for being such a good maintainer of this project.

pedz commented 6 years ago

Agree!!! dgutov is superb!

dgutov commented 6 years ago

Thanks, guys. :-)