Add PYTEST_TMPDIR_FILE_MASK environment variable #10786

BeyondEvil · 2023-03-03T00:02:36Z

nicoddemus

Other than my comments, why did you decide on using an environment variable rather than a configuration option? I suspect because one would use this only when running external code in containers, so it makes sense for it to be configured in the environment, just wanted to make sure.

changelog/10738.trivial.rst

doc/en/how-to/tmp_path.rst

BeyondEvil · 2023-03-03T14:04:27Z

Thanks @BeyondEvil!

Other than my comments, why did you decide on using an environment variable rather than a configuration option? I suspect because one would use this only when running external code in containers, so it makes sense for it to be configured in the environment, just wanted to make sure.

The original fix of setting mode to 700 wasn't necessarily the longterm fix that we wanted/needed, so after discussion with @RonnyPfannschmidt it was decided this was the path of least resistance. And yes, having it as an environment variable simplifies usage in containerized situations.

BeyondEvil · 2023-03-03T14:22:54Z

I think I've adressed all your comments @nicoddemus, thanks for reviewing! 🙇

Github Actions were having issues yesterday, so I expect tests to pass today.

nicoddemus

Thanks @BeyondEvil!

nicoddemus · 2023-03-03T14:52:32Z

The test failure will be fixed by #10788.

nicoddemus · 2023-03-03T15:26:58Z

Took the liberty of rebasing on main.

BeyondEvil · 2023-03-03T15:31:59Z

Took the liberty of rebasing on main.

Thanks, appreciate it! 🙏

Anything else needed from me at this time?

nicoddemus · 2023-03-03T15:36:13Z

Anything else needed from at this time?

Not from my POV, let's see if other maintainers want to chime in before getting this in.

bluetech

No comment on whether this is a good idea or not (unfortunately don't have time to think it through ATM), but a couple of smaller comments.

bluetech · 2023-03-03T15:41:06Z

src/_pytest/pathlib.py

@@ -51,6 +51,8 @@
    1921,  # ERROR_CANT_RESOLVE_FILENAME - fix for broken symlink pointing to itself
 )

+TMPDIR_FILE_MODE = int(os.getenv("PYTEST_TMPDIR_FILE_MODE", "0o700"), 8)


If you specify base 8, then you can drop the 0o prefix.

I can, but should I?

I felt it was clearer this way. But happy to remove it if you think it's better.

To me the 0o means that the field doesn't specify a base, i.e. 123 is decimal, 0x123 would work, etc. I think the base 8 is good, but then we should drop the 0o

bluetech · 2023-03-03T15:42:21Z

src/_pytest/pathlib.py

@@ -51,6 +51,8 @@
    1921,  # ERROR_CANT_RESOLVE_FILENAME - fix for broken symlink pointing to itself
 )

+TMPDIR_FILE_MODE = int(os.getenv("PYTEST_TMPDIR_FILE_MODE", "0o700"), 8)


You've chosen to grab the envvar once during import. Other alternatives are to grab it during initialization, and to grab it each time. Any particular reason you chose this way?

No particular reason. Preferably I would want to grab it in one place and then share it.

But I'm not versed enough in core to know where and how to do that.

Happy to take some guidance! 🙏

IMO (although again, I didn't think it through)

each-time > init > import

init > import because import is not great for this getenv, since then the user doesn't really get a chance to setenv from within python (when using pytest.main etc).

each-time > init is more debatable, but each-time allows a user to setenv whenever and it would take effect -- is there any reason to disallow that?

each-time > init is more debatable, but each-time allows a user to setenv whenever and it would take effect -- is there any reason to disallow that?

Not really, I think maximum flexibility is allowed here. I don't think it's our business to protect users from themselves. Especially since the default is more or less the most restrictive.

So would you recommend a small static method somewhere to get the user defined file mode? If so, where should I put such (static) method?

Something like:

def file_mode() -> int: return int(os.getenv("PYTEST_TMPDIR_FILE_MODE", "700"), 8)

Just a note that if we change the default (and remove the 0o), we should update the docs too. 👍

So would you recommend a small static method somewhere to get the user defined file mode

Yes. Though I'd call it tmpdir_file_mode.

If so, where should I put such (static) method?

I'd say best place for it is src/_pytest/pathlib.py.

Updated accordingly @bluetech 🙇

doc/en/reference/reference.rst

nicoddemus · 2023-03-10T10:34:30Z

@bluetech would you like to take a final review?

Zac-HD · 2023-03-11T17:05:45Z

I'm sorry to raise this so late in the process, but I'm quite strongly against this change.

File permissions for the temporary directories were restricted for security reasons, and allowing that to be adjusted by an environment variable weakens those defenses considerably. Instead, I'd want to use an approach which is clearly visible in the source code, such as by adding a config option, or an explicit fixture for world_accessible_tmpdir (which might take the tmpdir fixture and chown() the result).

nicoddemus · 2023-03-27T11:13:10Z

@BeyondEvil would you like to argue in favor of this change with @Zac-HD?

BeyondEvil · 2023-03-27T14:19:37Z

@BeyondEvil would you like to argue in favor of this change with @Zac-HD?

No, lol.
Jokes aside, sorry, I misinterpreted my role here. I made the assumption this was a discussion for core devs. Apologies for that.

I'm fine with changing the implementation, but I think it's sort of semantics or security by obscurity.

If I have access to modifying the environment variables, it's likely that I can change the test code as well. Meaning I can work around the "restriction" anyway:

        # Begin workaround
        # See: https://github.com/pytest-dev/pytest/issues/10738
        path.chmod(0o755)
        for parent in path.parents:
            try:
                os.chmod(parent, 0o755)
            except PermissionError:
                continue
        # End workaround

That said, I have a limited understanding of all the use cases of pytest and in what environments it's running. Hence, I'm happy to implement a fixture instead.

In my particular use case tho, I don't think a "chown" will work (as evident by the mask above I have to use to make it work).

So I would suggest a file mode fixture, one that potentially could warn on too loose permissions.

nicoddemus · 2023-04-04T13:15:31Z

Let's close this then. Thanks a lot @BeyondEvil for the PR, sorry it did not ended up being included! 👍

BeyondEvil · 2023-04-04T13:27:30Z

Sorry, I don't follow the logic of closing this. 🤔

nicoddemus · 2023-04-04T14:49:44Z

From:

That said, I have a limited understanding of all the use cases of pytest and in what environments it's running. Hence, I'm happy to implement a fixture instead.

I understand the environment variable implementation will be dropped, and potentially a new PR will be opened?

BeyondEvil · 2023-04-04T15:08:01Z

From:

That said, I have a limited understanding of all the use cases of pytest and in what environments it's running. Hence, I'm happy to implement a fixture instead.

I understand the environment variable implementation will be dropped, and potentially a new PR will be opened?

Ah, I get it.

sorry it did not ended up being included

That confused me a little. But yes, I'll open a new PR with a fixture-based solution instead.

nicoddemus reviewed Mar 3, 2023

View reviewed changes

changelog/10738.trivial.rst Outdated Show resolved Hide resolved

doc/en/how-to/tmp_path.rst Outdated Show resolved Hide resolved

nicoddemus closed this Mar 3, 2023

nicoddemus reopened this Mar 3, 2023

BeyondEvil force-pushed the beyondevil/tmpdir-file-mask branch from 7ef00db to 5067e75 Compare March 3, 2023 14:21

nicoddemus approved these changes Mar 3, 2023

View reviewed changes

Add PYTEST_TMPDIR_FILE_MASK environment variable

65fb513

nicoddemus force-pushed the beyondevil/tmpdir-file-mask branch from 5067e75 to 65fb513 Compare March 3, 2023 15:26

bluetech reviewed Mar 3, 2023

View reviewed changes

missed one

c788ace

nicoddemus reviewed Mar 3, 2023

View reviewed changes

doc/en/reference/reference.rst Outdated Show resolved Hide resolved

nicoddemus and others added 2 commits March 3, 2023 12:56

Update doc/en/reference/reference.rst

7237926

changes based on PR comments

c83aafe

BeyondEvil force-pushed the beyondevil/tmpdir-file-mask branch from fab22c9 to c83aafe Compare March 3, 2023 19:59

Merge branch 'main' into beyondevil/tmpdir-file-mask

77acf52

This comment was marked as spam.

Sign in to view

nicoddemus closed this Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PYTEST_TMPDIR_FILE_MASK environment variable #10786

Add PYTEST_TMPDIR_FILE_MASK environment variable #10786

BeyondEvil commented Mar 3, 2023

nicoddemus left a comment

BeyondEvil commented Mar 3, 2023

BeyondEvil commented Mar 3, 2023

nicoddemus left a comment

nicoddemus commented Mar 3, 2023

nicoddemus commented Mar 3, 2023

BeyondEvil commented Mar 3, 2023 •

edited

nicoddemus commented Mar 3, 2023

bluetech left a comment

bluetech Mar 3, 2023

BeyondEvil Mar 3, 2023

bluetech Mar 3, 2023

bluetech Mar 3, 2023

BeyondEvil Mar 3, 2023

bluetech Mar 3, 2023

BeyondEvil Mar 3, 2023

nicoddemus Mar 3, 2023

bluetech Mar 3, 2023

BeyondEvil Mar 3, 2023

nicoddemus commented Mar 10, 2023

This comment was marked as spam.

Zac-HD commented Mar 11, 2023

nicoddemus commented Mar 27, 2023

BeyondEvil commented Mar 27, 2023 •

edited

nicoddemus commented Apr 4, 2023

BeyondEvil commented Apr 4, 2023

nicoddemus commented Apr 4, 2023

BeyondEvil commented Apr 4, 2023

Add PYTEST_TMPDIR_FILE_MASK environment variable #10786

Add PYTEST_TMPDIR_FILE_MASK environment variable #10786

Conversation

BeyondEvil commented Mar 3, 2023

nicoddemus left a comment

Choose a reason for hiding this comment

BeyondEvil commented Mar 3, 2023

BeyondEvil commented Mar 3, 2023

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Mar 3, 2023

nicoddemus commented Mar 3, 2023

BeyondEvil commented Mar 3, 2023 • edited

nicoddemus commented Mar 3, 2023

bluetech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Mar 10, 2023

This comment was marked as spam.

Zac-HD commented Mar 11, 2023

nicoddemus commented Mar 27, 2023

BeyondEvil commented Mar 27, 2023 • edited

nicoddemus commented Apr 4, 2023

BeyondEvil commented Apr 4, 2023

nicoddemus commented Apr 4, 2023

BeyondEvil commented Apr 4, 2023

BeyondEvil commented Mar 3, 2023 •

edited

BeyondEvil commented Mar 27, 2023 •

edited