Secure folder wiping #220

Closed
HugoGiraudel opened this Issue Sep 25, 2014 · 9 comments

Comments

Projects
None yet
4 participants
@HugoGiraudel
Member

HugoGiraudel commented Sep 25, 2014

In #219, someone came to us with an issue: his project folder has been wiped out by SassDoc after running the regular command. This is no surprise.

There is a warning against this in the docs.
There is a warning against this in the README.

Needless to say running a command that compiles things on your root folder is definitely a bad idea, especially if you're not familiar with the tool you're using.

That being said, I think we could add some safeguards to avoid such a situation to happen again. At this point, we can think of a couple of different things.

Prompting

When running sassdoc, prompt the user if the destination folder is not empty.

Are you sure you want to wipe out <dest>? [y/n]

Also add a --no-prompt option (or whatever) to skip this step.

Folder testing

We could test whether the destination folder is parent of the source folder, in case we abort immediately. For instance:

sassdoc a/b/c a/b

The destination folder (<dest>) seems to be a parent folder from the source folder (<src>). We don't want you to lose your data, so let's abort the whole thing, shall we?

Creating a folder

Instead of wiping the destination folder, we could create a sassdoc folder (configurable) in the destination folder. This would prevent data from being erased. It would probably involve some extra logic that I don't really like.

Move to trash instead of deleting

One solution would be to use trash rather than rm -rf.


What do you think? My opinion: either implement the first two simultaneously, or the fourth.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Sep 25, 2014

Member

I'll be in favor of implementing the first and second solution.
The fourth would be a pain.
I also like the trash module, but might also be a pain, especially while testing/developing.

So basically run 2 test:
if dest is not empty --> warn and prompt
if dest is a parent of source --> warn (bad idea bro!), prompt ?

We then definitely need a new option --clean --erase --replace --sweep or something.
--no-prompts why not.

Member

pascalduez commented Sep 25, 2014

I'll be in favor of implementing the first and second solution.
The fourth would be a pain.
I also like the trash module, but might also be a pain, especially while testing/developing.

So basically run 2 test:
if dest is not empty --> warn and prompt
if dest is a parent of source --> warn (bad idea bro!), prompt ?

We then definitely need a new option --clean --erase --replace --sweep or something.
--no-prompts why not.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 25, 2014

Member

What I like with --no-prompt is that we can use it for possible other things needing to prompt the user.

Member

HugoGiraudel commented Sep 25, 2014

What I like with --no-prompt is that we can use it for possible other things needing to prompt the user.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 25, 2014

Member

+1 to @pascalduez's comment.

Member

valeriangalliat commented Sep 25, 2014

+1 to @pascalduez's comment.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 25, 2014

Member

Let's do this.

If dest is not empty, prompt the user:

Destination folder will be wiped out. Are you sure you want to proceed? [y/n]

Depending on the answer:

  • y: proceed;
  • n: abort.

If dest is a parent of src, warn:

Source folder seems to be contained by destination folder. Let's not wipe everything out.

Then abort. No prompt. So basically a throw.

Member

HugoGiraudel commented Sep 25, 2014

Let's do this.

If dest is not empty, prompt the user:

Destination folder will be wiped out. Are you sure you want to proceed? [y/n]

Depending on the answer:

  • y: proceed;
  • n: abort.

If dest is a parent of src, warn:

Source folder seems to be contained by destination folder. Let's not wipe everything out.

Then abort. No prompt. So basically a throw.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 25, 2014

Member

Who takes it?

Member

HugoGiraudel commented Sep 25, 2014

Who takes it?

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Sep 25, 2014

Member

I would like to tackle this if no one else interested.

Member

pascalduez commented Sep 25, 2014

I would like to tackle this if no one else interested.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Sep 25, 2014

Member

👍 I am fine with that.

Member

FWeinb commented Sep 25, 2014

👍 I am fine with that.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 25, 2014

Member

Alright @pascalduez, your move! Scheduled for 1.9. :)

Member

HugoGiraudel commented Sep 25, 2014

Alright @pascalduez, your move! Scheduled for 1.9. :)

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Sep 27, 2014

Member

This is the last things for 1.9

Member

FWeinb commented Sep 27, 2014

This is the last things for 1.9

@pascalduez pascalduez closed this Oct 4, 2014

@pascalduez pascalduez referenced this issue in SassDoc/gulp-sassdoc Dec 31, 2014

Closed

Safe wiping should be off by default #8

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