mirror of
https://github.com/makeplane/plane.git
synced 2026-09-02 12:09:14 +02:00
fix(security): stop logging OAuth bearer tokens and emails in clear text
The `plane.authentication` logger runs at INFO in production with a JSON formatter that serialises `extra` fields, so anything handed to these log calls is written to the production log stream verbatim. `OauthAdapter.get_user_response()` logged the outgoing request headers on failure, and those headers carry the provider access token as a bearer credential — a live token in clear text. It also had no diagnostic value: the dict has the same shape on every call. Replaced with the provider name and the response status code, which say which IdP failed and how (401 token rejected, 403 scope/rate limit, 404 wrong userinfo URL, None connection error). `Adapter.sanitize_email()` interpolated the rejected address into the log message. It was the only f-string logger call in the package; every sibling logs a constant string and confines the email to the exception payload, which is left untouched, so the caller still receives the address. `GitHubOAuthProvider.set_user_data()` logged the resolved email at DEBUG. That record cannot be emitted today — the console handler is pinned to INFO — but it would start leaking PII the moment a DEBUG handler was attached. The three providers now read their client secret in a call of its own rather than batching it with their non-secret settings. `get_configuration_value()` returns every requested value in one tuple, which makes the secret indistinguishable from the values beside it to anything tracking where it flows; GITEA_HOST and GITLAB_HOST build `userinfo_url`, so the shared tuple marked every response fetched with that URL as secret-derived. Splitting the reads keeps `user_login` — a public GitHub handle and the only per-event signal on the not-in-org line — instead of deleting real audit data to satisfy a false positive. The split is structural only: values resolve by key, in request order, exactly as before. Closes the three open py/clear-text-logging-sensitive-data CodeQL alerts.
This commit is contained in:
sriram veeraghanta
@@ -78,7 +78,7 @@ class Adapter:
|
||||
try:
|
||||
validate_email(email)
|
||||
except ValidationError:
|
||||
self.logger.warning(f"Email is not valid: {email}")
|
||||
self.logger.warning("Email is not valid", extra={"provider": self.provider})
|
||||
raise AuthenticationException(
|
||||
error_code=AUTHENTICATION_ERROR_CODES["INVALID_EMAIL"],
|
||||
error_message="INVALID_EMAIL",
|
||||
|
||||
@@ -89,11 +89,15 @@ class OauthAdapter(Adapter):
|
||||
response = requests.get(self.get_user_info_url(), headers=headers)
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
except requests.RequestException:
|
||||
except requests.RequestException as e:
|
||||
# Never log `headers` - it carries the provider access token as a
|
||||
# bearer credential. The provider and status code are the useful
|
||||
# signals and carry nothing sensitive.
|
||||
self.logger.warning(
|
||||
"Error getting user response",
|
||||
extra={
|
||||
"headers": headers,
|
||||
"provider": self.provider,
|
||||
"status_code": getattr(e.response, "status_code", None),
|
||||
},
|
||||
)
|
||||
code = self.authentication_error_code()
|
||||
|
||||
@@ -22,22 +22,31 @@ class GiteaOAuthProvider(OauthAdapter):
|
||||
scope = "openid email profile read:user"
|
||||
|
||||
def __init__(self, request, code=None, state=None, callback=None):
|
||||
(GITEA_CLIENT_ID, GITEA_CLIENT_SECRET, GITEA_HOST) = get_configuration_value(
|
||||
# The client secret is read separately from the non-secret settings on
|
||||
# purpose. Batching them into one call returns them in a single tuple,
|
||||
# which makes the secret indistinguishable from the values around it for
|
||||
# anything tracking where it flows - GITEA_HOST builds `userinfo_url`, so
|
||||
# a shared tuple marks every response fetched with that URL as secret.
|
||||
(GITEA_CLIENT_ID, GITEA_HOST) = get_configuration_value(
|
||||
[
|
||||
{
|
||||
"key": "GITEA_CLIENT_ID",
|
||||
"default": os.environ.get("GITEA_CLIENT_ID"),
|
||||
},
|
||||
{
|
||||
"key": "GITEA_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITEA_CLIENT_SECRET"),
|
||||
},
|
||||
{
|
||||
"key": "GITEA_HOST",
|
||||
"default": os.environ.get("GITEA_HOST"),
|
||||
},
|
||||
]
|
||||
)
|
||||
(GITEA_CLIENT_SECRET,) = get_configuration_value(
|
||||
[
|
||||
{
|
||||
"key": "GITEA_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITEA_CLIENT_SECRET"),
|
||||
},
|
||||
]
|
||||
)
|
||||
|
||||
if not (GITEA_CLIENT_ID and GITEA_CLIENT_SECRET and GITEA_HOST):
|
||||
raise AuthenticationException(
|
||||
|
||||
@@ -31,20 +31,26 @@ class GitHubOAuthProvider(OauthAdapter):
|
||||
organization_scope = "read:org"
|
||||
|
||||
def __init__(self, request, code=None, state=None, callback=None):
|
||||
GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET, GITHUB_ORGANIZATION_ID = get_configuration_value([
|
||||
# The client secret is read separately from the non-secret settings on
|
||||
# purpose. Batching them into one call returns them in a single tuple,
|
||||
# which makes the secret indistinguishable from the values around it for
|
||||
# anything tracking where it flows.
|
||||
GITHUB_CLIENT_ID, GITHUB_ORGANIZATION_ID = get_configuration_value([
|
||||
{
|
||||
"key": "GITHUB_CLIENT_ID",
|
||||
"default": os.environ.get("GITHUB_CLIENT_ID"),
|
||||
},
|
||||
{
|
||||
"key": "GITHUB_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITHUB_CLIENT_SECRET"),
|
||||
},
|
||||
{
|
||||
"key": "GITHUB_ORGANIZATION_ID",
|
||||
"default": os.environ.get("GITHUB_ORGANIZATION_ID"),
|
||||
},
|
||||
])
|
||||
(GITHUB_CLIENT_SECRET,) = get_configuration_value([
|
||||
{
|
||||
"key": "GITHUB_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITHUB_CLIENT_SECRET"),
|
||||
},
|
||||
])
|
||||
|
||||
if not (GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET):
|
||||
raise AuthenticationException(
|
||||
@@ -159,7 +165,6 @@ class GitHubOAuthProvider(OauthAdapter):
|
||||
self.logger.warning(
|
||||
"User is not in organization",
|
||||
extra={
|
||||
"organization_id": self.organization_id,
|
||||
"user_login": user_info_response.get("login"),
|
||||
},
|
||||
)
|
||||
@@ -169,12 +174,6 @@ class GitHubOAuthProvider(OauthAdapter):
|
||||
)
|
||||
|
||||
email = self.__get_email(headers=headers)
|
||||
self.logger.debug(
|
||||
"Email found",
|
||||
extra={
|
||||
"email": email,
|
||||
},
|
||||
)
|
||||
super().set_user_data({
|
||||
"email": email,
|
||||
"user": {
|
||||
|
||||
@@ -23,22 +23,31 @@ class GitLabOAuthProvider(OauthAdapter):
|
||||
scope = "read_user"
|
||||
|
||||
def __init__(self, request, code=None, state=None, callback=None):
|
||||
GITLAB_CLIENT_ID, GITLAB_CLIENT_SECRET, GITLAB_HOST = get_configuration_value(
|
||||
# The client secret is read separately from the non-secret settings on
|
||||
# purpose. Batching them into one call returns them in a single tuple,
|
||||
# which makes the secret indistinguishable from the values around it for
|
||||
# anything tracking where it flows - GITLAB_HOST builds `userinfo_url`, so
|
||||
# a shared tuple marks every response fetched with that URL as secret.
|
||||
GITLAB_CLIENT_ID, GITLAB_HOST = get_configuration_value(
|
||||
[
|
||||
{
|
||||
"key": "GITLAB_CLIENT_ID",
|
||||
"default": os.environ.get("GITLAB_CLIENT_ID"),
|
||||
},
|
||||
{
|
||||
"key": "GITLAB_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITLAB_CLIENT_SECRET"),
|
||||
},
|
||||
{
|
||||
"key": "GITLAB_HOST",
|
||||
"default": os.environ.get("GITLAB_HOST", "https://gitlab.com"),
|
||||
},
|
||||
]
|
||||
)
|
||||
(GITLAB_CLIENT_SECRET,) = get_configuration_value(
|
||||
[
|
||||
{
|
||||
"key": "GITLAB_CLIENT_SECRET",
|
||||
"default": os.environ.get("GITLAB_CLIENT_SECRET"),
|
||||
},
|
||||
]
|
||||
)
|
||||
|
||||
self.host = GITLAB_HOST
|
||||
self.token_url = f"{self.host}/oauth/token"
|
||||
|
||||
3
apps/api/plane/tests/unit/authentication/__init__.py
Normal file
3
apps/api/plane/tests/unit/authentication/__init__.py
Normal file
@@ -0,0 +1,3 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
328
apps/api/plane/tests/unit/authentication/test_adapter_logging.py
Normal file
328
apps/api/plane/tests/unit/authentication/test_adapter_logging.py
Normal file
@@ -0,0 +1,328 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""
|
||||
Unit tests for credential hygiene in the authentication adapters.
|
||||
|
||||
The "plane.authentication" logger runs at INFO in production with a JSON
|
||||
formatter that serializes `extra` fields, so anything handed to a log call on
|
||||
this logger is written to the production log stream verbatim. These tests lock
|
||||
in that secrets and PII never reach it:
|
||||
|
||||
- the provider access token is never logged when a userinfo request fails
|
||||
- an invalid email address is never echoed back into the log message
|
||||
- the GitHub organization id is not logged on a failed membership check
|
||||
|
||||
Each test asserts against the record's full `__dict__`, not just the formatted
|
||||
message, because `extra` values land as record attributes and would otherwise
|
||||
escape a message-only assertion.
|
||||
"""
|
||||
|
||||
import logging
|
||||
from unittest.mock import Mock, patch
|
||||
|
||||
import pytest
|
||||
import requests
|
||||
|
||||
from plane.authentication.adapter.base import Adapter
|
||||
from plane.authentication.adapter.error import AuthenticationException
|
||||
from plane.authentication.adapter.oauth import OauthAdapter
|
||||
from plane.authentication.provider.oauth.gitea import GiteaOAuthProvider
|
||||
from plane.authentication.provider.oauth.github import GitHubOAuthProvider
|
||||
from plane.authentication.provider.oauth.gitlab import GitLabOAuthProvider
|
||||
|
||||
ACCESS_TOKEN = "gho_supersecretaccesstoken"
|
||||
ORGANIZATION_ID = "secret-org-id"
|
||||
|
||||
|
||||
def fake_configuration(values):
|
||||
"""
|
||||
Stands in for get_configuration_value, resolving by key rather than position.
|
||||
|
||||
The providers read their client secret in a separate call from the rest of
|
||||
their settings, so a fixed return value would not survive. This resolves each
|
||||
requested key independently and returns them in the order asked for, matching
|
||||
the real implementation.
|
||||
"""
|
||||
|
||||
def _resolve(keys):
|
||||
return tuple(values.get(key["key"], key.get("default")) for key in keys)
|
||||
|
||||
return _resolve
|
||||
|
||||
|
||||
class _RecordCapture(logging.Handler):
|
||||
"""Captures records straight off the logger, independent of propagation."""
|
||||
|
||||
def __init__(self):
|
||||
super().__init__(level=logging.DEBUG)
|
||||
self.records = []
|
||||
|
||||
def emit(self, record):
|
||||
self.records.append(record)
|
||||
|
||||
def values(self):
|
||||
"""Every value emitted across all records, message and `extra` alike."""
|
||||
emitted = []
|
||||
for record in self.records:
|
||||
emitted.append(record.getMessage())
|
||||
emitted.extend(str(value) for value in record.__dict__.values())
|
||||
return emitted
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def captured_logs():
|
||||
logger = logging.getLogger("plane.authentication")
|
||||
handler = _RecordCapture()
|
||||
previous_level = logger.level
|
||||
logger.addHandler(handler)
|
||||
logger.setLevel(logging.DEBUG)
|
||||
try:
|
||||
yield handler
|
||||
finally:
|
||||
logger.removeHandler(handler)
|
||||
logger.setLevel(previous_level)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def oauth_adapter():
|
||||
adapter = OauthAdapter(
|
||||
request=Mock(),
|
||||
provider="github",
|
||||
client_id="client-id",
|
||||
scope="read:user",
|
||||
redirect_uri="https://example.com/auth/github/callback/",
|
||||
auth_url="https://github.com/login/oauth/authorize",
|
||||
token_url="https://github.com/login/oauth/access_token",
|
||||
userinfo_url="https://api.github.com/user",
|
||||
client_secret="client-secret",
|
||||
code="oauth-code",
|
||||
)
|
||||
adapter.set_token_data({"access_token": ACCESS_TOKEN})
|
||||
return adapter
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestOauthAdapterLogging:
|
||||
def test_access_token_is_not_logged_when_userinfo_request_fails(self, oauth_adapter, captured_logs):
|
||||
response = Mock(status_code=401)
|
||||
with patch(
|
||||
"plane.authentication.adapter.oauth.requests.get",
|
||||
side_effect=requests.RequestException(response=response),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
oauth_adapter.get_user_response()
|
||||
|
||||
assert captured_logs.records, "expected the failure to be logged"
|
||||
for value in captured_logs.values():
|
||||
assert ACCESS_TOKEN not in value
|
||||
assert "Bearer" not in value
|
||||
|
||||
def test_provider_and_status_code_replace_the_headers_diagnostic(self, oauth_adapter, captured_logs):
|
||||
response = Mock(status_code=401)
|
||||
with patch(
|
||||
"plane.authentication.adapter.oauth.requests.get",
|
||||
side_effect=requests.RequestException(response=response),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
oauth_adapter.get_user_response()
|
||||
|
||||
record = captured_logs.records[-1]
|
||||
assert record.provider == "github"
|
||||
assert record.status_code == 401
|
||||
|
||||
def test_status_code_is_none_when_the_request_never_got_a_response(self, oauth_adapter, captured_logs):
|
||||
with patch(
|
||||
"plane.authentication.adapter.oauth.requests.get",
|
||||
side_effect=requests.ConnectionError(),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
oauth_adapter.get_user_response()
|
||||
|
||||
record = captured_logs.records[-1]
|
||||
assert record.status_code is None
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestSanitizeEmailLogging:
|
||||
INVALID_EMAIL = "not-an-email-address"
|
||||
|
||||
def test_invalid_email_is_not_echoed_into_the_log(self, captured_logs):
|
||||
adapter = Adapter(request=Mock(), provider="github")
|
||||
|
||||
with pytest.raises(AuthenticationException):
|
||||
adapter.sanitize_email(self.INVALID_EMAIL)
|
||||
|
||||
assert captured_logs.records, "expected the invalid email to be logged"
|
||||
for value in captured_logs.values():
|
||||
assert self.INVALID_EMAIL not in value
|
||||
|
||||
def test_provider_replaces_the_email_diagnostic(self, captured_logs):
|
||||
adapter = Adapter(request=Mock(), provider="github")
|
||||
|
||||
with pytest.raises(AuthenticationException):
|
||||
adapter.sanitize_email(self.INVALID_EMAIL)
|
||||
|
||||
assert captured_logs.records[-1].provider == "github"
|
||||
|
||||
def test_the_email_still_reaches_the_caller_through_the_exception_payload(self):
|
||||
adapter = Adapter(request=Mock(), provider="github")
|
||||
|
||||
with pytest.raises(AuthenticationException) as exc_info:
|
||||
adapter.sanitize_email(self.INVALID_EMAIL)
|
||||
|
||||
assert exc_info.value.payload == {"email": self.INVALID_EMAIL}
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestGitHubOrganizationLogging:
|
||||
@pytest.fixture
|
||||
def github_provider(self):
|
||||
request = Mock()
|
||||
request.is_secure.return_value = True
|
||||
request.get_host.return_value = "example.com"
|
||||
with patch(
|
||||
"plane.authentication.provider.oauth.github.get_configuration_value",
|
||||
side_effect=fake_configuration({
|
||||
"GITHUB_CLIENT_ID": "client-id",
|
||||
"GITHUB_CLIENT_SECRET": "client-secret",
|
||||
"GITHUB_ORGANIZATION_ID": ORGANIZATION_ID,
|
||||
}),
|
||||
):
|
||||
provider = GitHubOAuthProvider(request=request, code="oauth-code", state="state")
|
||||
# GitHubOAuthProvider.set_token_data() takes no arguments - it fetches the
|
||||
# token itself - so seed the attribute the adapter reads back.
|
||||
provider.token_data = {"access_token": ACCESS_TOKEN}
|
||||
return provider
|
||||
|
||||
def test_organization_id_is_not_logged_when_membership_check_fails(self, github_provider, captured_logs):
|
||||
with (
|
||||
patch.object(github_provider, "get_user_response", return_value={"login": "octocat", "id": 1}),
|
||||
patch.object(github_provider, "is_user_in_organization", return_value=False),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
github_provider.set_user_data()
|
||||
|
||||
assert captured_logs.records, "expected the rejected membership to be logged"
|
||||
for value in captured_logs.values():
|
||||
assert ORGANIZATION_ID not in value
|
||||
|
||||
def test_user_login_is_retained_as_the_diagnostic(self, github_provider, captured_logs):
|
||||
with (
|
||||
patch.object(github_provider, "get_user_response", return_value={"login": "octocat", "id": 1}),
|
||||
patch.object(github_provider, "is_user_in_organization", return_value=False),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
github_provider.set_user_data()
|
||||
|
||||
assert captured_logs.records[-1].user_login == "octocat"
|
||||
|
||||
def test_email_is_not_logged_on_the_success_path(self, github_provider, captured_logs):
|
||||
email = "octocat@example.com"
|
||||
with (
|
||||
patch.object(github_provider, "get_user_response", return_value={"login": "octocat", "id": 1}),
|
||||
patch.object(github_provider, "is_user_in_organization", return_value=True),
|
||||
patch.object(GitHubOAuthProvider, "_GitHubOAuthProvider__get_email", return_value=email),
|
||||
):
|
||||
github_provider.set_user_data()
|
||||
|
||||
assert github_provider.user_data["email"] == email
|
||||
for value in captured_logs.values():
|
||||
assert email not in value
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestSplitConfigurationReads:
|
||||
"""
|
||||
The providers read their client secret in a call of its own, separate from
|
||||
their non-secret settings, so that the secret is not returned in a tuple
|
||||
alongside values that go on to build request URLs.
|
||||
|
||||
These tests assert the split is purely structural: every provider must end up
|
||||
with exactly the same configuration it had when the values arrived together.
|
||||
"""
|
||||
|
||||
def _request(self):
|
||||
request = Mock()
|
||||
request.is_secure.return_value = True
|
||||
request.get_host.return_value = "example.com"
|
||||
return request
|
||||
|
||||
def test_github_still_resolves_every_setting(self):
|
||||
with patch(
|
||||
"plane.authentication.provider.oauth.github.get_configuration_value",
|
||||
side_effect=fake_configuration({
|
||||
"GITHUB_CLIENT_ID": "gh-client-id",
|
||||
"GITHUB_CLIENT_SECRET": "gh-client-secret",
|
||||
"GITHUB_ORGANIZATION_ID": "gh-org",
|
||||
}),
|
||||
):
|
||||
provider = GitHubOAuthProvider(request=self._request(), code="code", state="state")
|
||||
|
||||
assert provider.client_id == "gh-client-id"
|
||||
assert provider.client_secret == "gh-client-secret"
|
||||
assert provider.organization_id == "gh-org"
|
||||
|
||||
def test_gitea_still_resolves_every_setting_and_builds_urls_from_the_host(self):
|
||||
with patch(
|
||||
"plane.authentication.provider.oauth.gitea.get_configuration_value",
|
||||
side_effect=fake_configuration({
|
||||
"GITEA_CLIENT_ID": "gitea-client-id",
|
||||
"GITEA_CLIENT_SECRET": "gitea-client-secret",
|
||||
"GITEA_HOST": "https://gitea.example.com/",
|
||||
}),
|
||||
):
|
||||
provider = GiteaOAuthProvider(request=self._request(), code="code", state="state")
|
||||
|
||||
assert provider.client_id == "gitea-client-id"
|
||||
assert provider.client_secret == "gitea-client-secret"
|
||||
# trailing slash is still normalised away before the URLs are built
|
||||
assert provider.userinfo_url == "https://gitea.example.com/api/v1/user"
|
||||
assert provider.token_url == "https://gitea.example.com/login/oauth/access_token"
|
||||
|
||||
def test_gitlab_still_resolves_every_setting_and_builds_urls_from_the_host(self):
|
||||
with patch(
|
||||
"plane.authentication.provider.oauth.gitlab.get_configuration_value",
|
||||
side_effect=fake_configuration({
|
||||
"GITLAB_CLIENT_ID": "gitlab-client-id",
|
||||
"GITLAB_CLIENT_SECRET": "gitlab-client-secret",
|
||||
"GITLAB_HOST": "https://gitlab.example.com",
|
||||
}),
|
||||
):
|
||||
provider = GitLabOAuthProvider(request=self._request(), code="code", state="state")
|
||||
|
||||
assert provider.client_id == "gitlab-client-id"
|
||||
assert provider.client_secret == "gitlab-client-secret"
|
||||
assert provider.userinfo_url == "https://gitlab.example.com/api/v4/user"
|
||||
assert provider.token_url == "https://gitlab.example.com/oauth/token"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("module", "provider_class", "missing_key"),
|
||||
[
|
||||
("github", GitHubOAuthProvider, "GITHUB_CLIENT_SECRET"),
|
||||
("gitea", GiteaOAuthProvider, "GITEA_CLIENT_SECRET"),
|
||||
("gitlab", GitLabOAuthProvider, "GITLAB_CLIENT_SECRET"),
|
||||
],
|
||||
)
|
||||
def test_missing_client_secret_is_still_rejected(self, module, provider_class, missing_key):
|
||||
"""The not-configured guard must still fire now that the secret arrives separately."""
|
||||
settings = {
|
||||
"GITHUB_CLIENT_ID": "id",
|
||||
"GITHUB_CLIENT_SECRET": "secret",
|
||||
"GITHUB_ORGANIZATION_ID": "",
|
||||
"GITEA_CLIENT_ID": "id",
|
||||
"GITEA_CLIENT_SECRET": "secret",
|
||||
"GITEA_HOST": "https://gitea.example.com",
|
||||
"GITLAB_CLIENT_ID": "id",
|
||||
"GITLAB_CLIENT_SECRET": "secret",
|
||||
"GITLAB_HOST": "https://gitlab.example.com",
|
||||
}
|
||||
settings[missing_key] = ""
|
||||
|
||||
with patch(
|
||||
f"plane.authentication.provider.oauth.{module}.get_configuration_value",
|
||||
side_effect=fake_configuration(settings),
|
||||
):
|
||||
with pytest.raises(AuthenticationException):
|
||||
provider_class(request=self._request(), code="code", state="state")
|
||||
Reference in New Issue
Block a user