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

WIP: Set permissions for atomic open_file() #1382

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ Unreleased
- Remove unused compat shim for ``bytes``. (`#1195`_)
- Expand unit testing around termui, especially getchar (Windows). (`#1116`_)
- Fix output on Windows Python 2.7 built with MSVC 14. #1342
- ``open_file`` with ``atomic=True`` retains permissions of existing files and
respects the current umask for new files. (`#1376`_)

.. _#1167: https://github.com/pallets/click/pull/1167
.. _#1151: https://github.com/pallets/click/pull/1151
.. _#1185: https://github.com/pallets/click/issues/1185
.. _#1195: https://github.com/pallets/click/pull/1195
.. _#1116: https://github.com/pallets/click/issues/1116
.. _#1376: https://github.com/pallets/click/issues/1376

Version 7.0
-----------
Expand Down
26 changes: 24 additions & 2 deletions click/_compat.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import re
import io
import os
import stat
import sys
import codecs
from weakref import WeakKeyDictionary
Expand Down Expand Up @@ -33,6 +34,12 @@ def _make_text_stream(stream, encoding, errors,
force_readable=force_readable,
force_writable=force_writable)

def _get_current_umask():
"""Get current umask."""
umask = os.umask(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is, unfortunately, a potential security problem in the presence of multiple threads. (See https://bugs.python.org/issue21082.)

I’m not sure what to suggest instead, other than avoiding tempfile and writing the loop to safely create a fresh filename manually.

os.umask(umask)
return umask


def is_ascii_encoding(encoding):
"""Checks if a given encoding is ascii."""
Expand Down Expand Up @@ -512,7 +519,19 @@ def open_stream(filename, mode='r', encoding=None, errors='strict',
else:
f = os.fdopen(fd, mode)

return _AtomicFile(f, tmp_filename, os.path.realpath(filename)), True
real_filename = os.path.realpath(filename)

# Get permissions to set on the target file. If the target file already
# exists, retain its current permissions. If the target file will be
# created, respect the permissions from the current umask.
permissions = None
if not WIN:
try:
permissions = stat.S_IMODE(os.stat(real_filename).st_mode)
except OSError:
msmolens marked this conversation as resolved.
Show resolved Hide resolved
permissions = 0o666 & ~_get_current_umask()
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO for the fallback it's better to set here a default permission, like 0o666 or even more paranoidal 0o600. As @andersk right said it's not thread-safe.

Copy link
Contributor

@atugushev atugushev Sep 2, 2019

Choose a reason for hiding this comment

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

Or even better, just pass and let the permissions be None.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default permissions if you didn’t specify atomic=True are implicitly 0o666 & ~umask, because that’s the default behavior of open() (no explicit umask computation is required). So it’s unexpected for atomic=True to result in different permissions. permissions = 0o666 is definitely too permissive, as chmod is not implicitly masked by umask, while permissions = 0o600 (or pass, which has the same effect) is too restrictive.

The underlying problem is that tempfile simply has the wrong permissions behavior for this use case. It’s designed for temporary files that will later be deleted, not for creating files with which to atomically replace other files.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, anyways at least on save an existing file there will be correct permissions.


return _AtomicFile(f, tmp_filename, real_filename, permissions), True


# Used in a destructor call, needs extra protection from interpreter cleanup.
Expand All @@ -526,10 +545,11 @@ def open_stream(filename, mode='r', encoding=None, errors='strict',

class _AtomicFile(object):

def __init__(self, f, tmp_filename, real_filename):
def __init__(self, f, tmp_filename, real_filename, permissions):
self._f = f
self._tmp_filename = tmp_filename
self._real_filename = real_filename
self._permissions = permissions
self.closed = False

@property
Expand All @@ -546,6 +566,8 @@ def close(self, delete=False):
except OSError:
pass
_replace(self._tmp_filename, self._real_filename)
if self._permissions is not None:
os.chmod(self._real_filename, self._permissions)
self.closed = True

def __getattr__(self, name):
Expand Down
40 changes: 39 additions & 1 deletion tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import os
import stat
import sys

import pytest

import click
import click.utils
import click._termui_impl
from click._compat import WIN, PY2
from click._compat import WIN, PY2, _get_current_umask


def test_echo(runner):
Expand Down Expand Up @@ -285,6 +286,43 @@ def cli(filename):
assert result.output == 'foobar\nmeep\n'


@pytest.mark.skipif(WIN, reason='os.chmod() is not fully supported on Windows.')
@pytest.mark.parametrize('permissions', [
0o400,
0o444,
0o600,
0o644,
])
def test_open_file_atomic_permissions_existing_file(runner, permissions):
with runner.isolated_filesystem():
with open('existing.txt', 'w') as f:
f.write('content')
os.chmod('existing.txt', permissions)

@click.command()
@click.argument('filename')
def cli(filename):
click.open_file(filename, 'w', atomic=True).close()

result = runner.invoke(cli, ['existing.txt'])
assert result.exception is None
assert stat.S_IMODE(os.stat('existing.txt').st_mode) == permissions


@pytest.mark.skipif(WIN, reason='os.chmod() is not fully supported on Windows.')
def test_open_file_atomic_permissions_new_file(runner):
with runner.isolated_filesystem():
@click.command()
@click.argument('filename')
def cli(filename):
click.open_file(filename, 'w', atomic=True).close()

umask = _get_current_umask()
result = runner.invoke(cli, ['new.txt'])
assert result.exception is None
assert stat.S_IMODE(os.stat('new.txt').st_mode) == (0o666 & ~umask)


@pytest.mark.xfail(WIN and not PY2, reason='God knows ...')
def test_iter_keepopenfile(tmpdir):
expected = list(map(str, range(10)))
Expand Down