Quickstart makefile 'make clean' may wipe filesystem

[sadielbartholomew]

Describe the bug [not a 'bug' as such, but some dangerous code that could do with a safety catch]

The default makefile currently generated by sphinx-quickstart has the potential (see below) to run the infamously destructive rm -rf /* command and wipe out someone's filesystem. I discovered this because I ended up executing rm -rf /* via make clean myself, such that my machine started trying to remove my root directory and I would have lost a lot of my drive had I not noticed the quickly-moving STDOUT stream did not look right in time to Ctrl-C to stop the command progressing.

This Issue is quite similar to that raised in #2504, but I would like to alert you to my recent experience nearly wiping my drive that actually the makefile is more dangerous than highlighted there. It seems like that Issue has stalled in getting resolved, so I would urge you to make the relevant code safer before anyone else ends up en-route to destroying their filesystem due to the quickstart makefile.

To Reproduce

(Assume a project has received the default Makefile after running the sphinx-quickstart utility and entering sensible answers.)

It appears that a make clean will end up running the problematic rm -rf /* if the BUILDDIR variable is not defined:

sphinx/sphinx/templates/quickstart/Makefile_t

Lines 51 to 52 in af04b64

	clean:
	rm -rf $(BUILDDIR)/*

Though BUILDDIR should be set to a definite directory by the quickstart utility, if somehow the developer makes edits so that it is not set, for example if the developer had tweaked the makefile so that the BUILDDIR variable gets set from the command-line, as even suggested in the Makefile itself:

sphinx/sphinx/templates/quickstart/Makefile_t

Lines 4 to 9 in af04b64

	# You can set these variables from the command line.
	SPHINXOPTS ?=
	SPHINXBUILD ?= sphinx-build
	PAPER ?=
	SOURCEDIR = {{ rsrcdir }}
	BUILDDIR = {{ rbuilddir }}

I realise that in tweaking the makefile it would strictly be the developer at fault and not the design of the Makefile & Sphinx, but I believe that, given the catastrophic nature of the command that could get run, there should be a safety catch that prevents the command ran by make clean from being executed to cover any potential means that BUILDDIR might not be defined, or defined as empty, for example:

clean:
	$(if $(BUILDDIR),,$(error BUILDDIR to clean must be defined))
	rm -rf $(BUILDDIR)/*

Expected behavior

Assuming a developer has not done something truly contrived or actively dangerous, there should be no way in which a rm -rf /* command could ever be run when working with the Sphinx ecosystem. Running make clean should only feasibly be able to delete files related to the Sphinx documentation, ideally ones that have been built and not one comprising the source content.

Your project

The specific project I ran into this on was NCAS-CMS/cf-python (this is our Makefile). In our case, we had commented out the default line to set BUILDDIR = build since we usually build our docs via a wrapper script that runs make html <build dir> where <build dir> is set in the script, but I hit this problem by running a plain make clean without knowing about the underlying danger.

Environment info

• OS: Linux Mint 19.2 Tina
• Python version: 3.8.5
• Sphinx version: I was using v3.3.1 but see the Makefile remains dangerous in this precise manner at the current version
• Sphinx extensions: n/a
• Extra tools: n/a

[tk0miya]

What do you think about this comment; #2504 (comment)? I think it's a simple and good solution.

[sadielbartholomew]

Hi @tk0miya, thanks for your quick response and indeed for putting up a PR to fix this.

I'm assuming that suggestion works because rm -rf without any argument does nothing? If it does that consistently across the common shell types (and I think rm is standardised under POSIX, so there should be no variation by distro, at least), I agree that it would be a great solution. However, I would be happy with any sensible means to prevent the issue at hand, that suggestion or otherwise.

[tk0miya]

Thank you for comment. Okay, let's go with this :-)