Skip to content

fix(google-auth-oauthlib): create saved credentials file with mode 0600 - #18361

Open
Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:oauthlib-tool-credentials-mode
Open

Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:oauthlib-tool-credentials-mode

Conversation

@Samin061

Copy link
Copy Markdown
Contributor

google-oauthlib-tool --save writes credentials.json, which holds the OAuth refresh token and client secret, through a plain open() and creates its config directory with os.makedirs(), so both take the process umask and on a typical host end up 0644 and 0755, readable by every local user. Create the file with mode 0600 and the directory with 0700 inside the tool instead, matching how the mTLS helper in google-auth writes key material, and add a test that checks neither has group or other bits. The test is skipped on Windows, where these mode bits do not apply.

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@Samin061
Samin061 requested a review from a team as a code owner September 14, 2026 08:06

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request restricts the permissions of the saved credentials file and its parent directory to the current user (using modes 0o600 and 0o700 respectively) and adds a corresponding unit test. The feedback suggests using tempfile.TemporaryDirectory instead of tempfile.mkdtemp() in the test to avoid leaking temporary directories.

Comment on lines +140 to +164
credentials_tmpdir = tempfile.mkdtemp()
credentials_path = os.path.join(
credentials_tmpdir, "new-directory", "credentials.json"
)
result = runner.invoke(
cli.main,
[
"--client-secrets",
CLIENT_SECRETS_FILE,
"--scope",
"somescope",
"--credentials",
credentials_path,
"--save",
],
)
local_server_mock.assert_called_with(mock.ANY)
assert not result.exception
assert result.exit_code == 0
# The saved file holds the refresh token and client secret, so neither
# it nor the directory created for it may be group or world accessible.
file_mode = stat.S_IMODE(os.stat(credentials_path).st_mode)
dir_mode = stat.S_IMODE(os.stat(os.path.dirname(credentials_path)).st_mode)
assert file_mode & 0o077 == 0
assert dir_mode & 0o077 == 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The temporary directory created with tempfile.mkdtemp() is never cleaned up, which leads to directory leaks in the test environment. Using tempfile.TemporaryDirectory as a context manager ensures that the directory and its contents are automatically cleaned up after the test completes, even if assertions fail.

        with tempfile.TemporaryDirectory() as credentials_tmpdir:
            credentials_path = os.path.join(
                credentials_tmpdir, "new-directory", "credentials.json"
            )
            result = runner.invoke(
                cli.main,
                [
                    "--client-secrets",
                    CLIENT_SECRETS_FILE,
                    "--scope",
                    "somescope",
                    "--credentials",
                    credentials_path,
                    "--save",
                ],
            )
            local_server_mock.assert_called_with(mock.ANY)
            assert not result.exception
            assert result.exit_code == 0
            # The saved file holds the refresh token and client secret, so neither
            # it nor the directory created for it may be group or world accessible.
            file_mode = stat.S_IMODE(os.stat(credentials_path).st_mode)
            dir_mode = stat.S_IMODE(os.stat(os.path.dirname(credentials_path)).st_mode)
            assert file_mode & 0o077 == 0
            assert dir_mode & 0o077 == 0

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant