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

Advice: Do not set site_dir to / #1161

Closed
mattwelke opened this issue Mar 13, 2017 · 8 comments
Closed

Advice: Do not set site_dir to / #1161

mattwelke opened this issue Mar 13, 2017 · 8 comments
Labels
Bug

Comments

@mattwelke
Copy link

@mattwelke mattwelke commented Mar 13, 2017

Just a warning to anyone who might want to change their project build directory to the root of the project, with a .yml file like this:

site_name: My Docs
site_dir: /
pages:
  - Home: index.md
theme: material

It won't do what you think it will. If you run mkdocs build --clean you will have it recursively delete everything it can from your system root folder until it runs into a file it can't delete without sudo. You will lose the entire contents of your home folder as well as your Ubuntu profile, and do unknown damage to your system.

Time to reformat. :D

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Mar 13, 2017

I think you would need to set it to ~ as / would be the root of the system and thus not your home folder. However, it would be worth a check to make sure users don't use ~ or /.

I'd accept a change that warned users and forced them to type continue or something similar

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Mar 13, 2017

Also, we don't delete any "hidden" (dotfiles), so .profile wouldn't be deleted.

@mattwelke
Copy link
Author

@mattwelke mattwelke commented Mar 13, 2017

A warning would be a good idea is what I'm thinking. It would have saved me from that really dumb, costly mistake.

What I actually wanted to do there was build to the root of the project folder, because our GitHub pages is set up to serve from root of master.

@d0ugal d0ugal added the Bug label Mar 13, 2017
@waylan
Copy link
Member

@waylan waylan commented Mar 13, 2017

In config validation, the first thing we generally do after confirming it is a string, is pass the value to os.path.abspath. It's only after that that we actually check the location. So we wouldn't notice if it was absolute to start.

Although, I'm surprised the site_dir-can't-be-in-docs_dir and docs_dir-can't-be-in-site_dir checks didn't catch this. If one of them is at the system root, then the other must be in a subdir (of some level).

The general use case is that all local paths in the config file are expected to be relative to the location of the config file. I guess we just assume that everyone would always use relative paths. However, there may be a valid usecase for a user to use an absolute path, so we probably should issue a warning (and require explicit verification) before proceeding when an absolute path is provided. Something like:

Local paths in the config are generally expected to be relative to the config file location. 
You provided an absolute path from the root of your local file system.
setting_name: "site_dir"; value: "/"
Are you sure you want to continue? [y/N]:

A warning without the explicit verification is not of much value as the damage will already be started by the time the user sees the warning. Of course, with explicit verification, we would need to also provide a CLI flag to bypass the check for those users who need to automate with an absolute value.

@waylan
Copy link
Member

@waylan waylan commented Mar 13, 2017

The docs_dir-can't-be-in-site_dir check didn't catch it because we do:

if (config['docs_dir'] + os.sep).startswith(config['site_dir'] + os.sep):

The + os.sep breaks things for / (ex: the string /anything/ does not start with //). Normally, os.path.abspath will return a directory without a slash on the end, so we didn't bother to explicitly remove (or check for) a slash as the last character. Of course, the one exception is the path / (os.path.abspath('/foo/') returns '/foo' whereas os.path.abspath('/') returns /).

And while removing the + os.sep from those checks would solve this problem, it would reintroduce #1011, so they need to stay.

Perhaps the fix here would be to just do value.rstrip(os.sep). In the rare event a user actually provides / as the value, the modified path would not exist, which would be an error. It would also be harmless, as there should be no situation where / is actually the correct value.

waylan added a commit to waylan/mkdocs that referenced this issue Mar 14, 2017
As neither setting can point at a child dir of the other, "/" would be an
invalid value for either setting. However, given its unique nature,
os.path.abspath does not follow normal bahavior of returning a string without
an ending slash when passed "/". Therefore, we need to special case it.

Fixes mkdocs#1161.
@waylan waylan closed this in #1162 Mar 14, 2017
waylan added a commit that referenced this issue Mar 14, 2017
As neither setting can point at a child dir of the other, "/" would be an
invalid value for either setting. However, given its unique nature,
os.path.abspath does not follow normal bahavior of returning a string without
an ending slash when passed "/". Therefore, we need to special case it.

Fixes #1161.
@waylan
Copy link
Member

@waylan waylan commented Mar 14, 2017

Given the serious nature of this bug (destructive), I just pushed out a new release (0.16.2) after fixing it.

@welkie sorry for the problems. That never should have passed validation, and in fact it didn't before #1011. In any event, we have specific tests for it now.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Mar 14, 2017

Fantastic, thanks @waylan!

@mattwelke
Copy link
Author

@mattwelke mattwelke commented Mar 14, 2017

Cool. This was no trouble at all. The way I do my work, having my home folder wiped doesn't cause me to lose any work. This wasn't destructive for me at all. In retrospect, I was the perfect person to encounter this bug. Thanks for the patch!

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.