feature/INT-1751 - Security updates

.github/workflows/build-main.yml

@@ -23,10 +23,19 @@ jobs:
         uses: actions/setup-python@v2
         with:
           python-version: ${{ matrix.python-version }}
+      # --only-binary :all: installs wheels only, so pip never runs a downloaded package's
+      # setup.py in a job that holds repository secrets. requirements-ci.txt is
+      # requirements-dev.txt without pre-commit, a local git hook tool CI never runs whose
+      # PyYAML dependency ships no PyPy wheels.
       - name: Upgrade pip and virtualenv to latest
         run: |
-          python -m pip install --upgrade pip
-          pip install -r requirements-dev.txt
+          # Exact pins (Sonar S8544). 25.0.1 is the last pip that supports Python 3.8.
+          if [ "${{ matrix.python-version }}" = "pypy-3.8" ]; then
+            python -m pip install --only-binary :all: pip==25.0.1
+          else
+            python -m pip install --only-binary :all: pip==26.2.2
+          fi
+          pip install --only-binary :all: -r requirements-ci.txt
       - name: Lint with flake8
         run: flake8 . --count --max-complexity=10 --max-line-length=127 --statistics
       - name: Run unit tests

.github/workflows/build-pull-request.yml

@@ -22,10 +22,19 @@ jobs:
         uses: actions/setup-python@v2
         with:
           python-version: ${{ matrix.python-version }}
+      # --only-binary :all: installs wheels only, so pip never runs a downloaded package's
+      # setup.py in a job that holds repository secrets. requirements-ci.txt is
+      # requirements-dev.txt without pre-commit, a local git hook tool CI never runs whose
+      # PyYAML dependency ships no PyPy wheels.
       - name: Upgrade pip and virtualenv to latest
         run: |
-          python -m pip install --upgrade pip
-          pip install -r requirements-dev.txt
+          # Exact pins (Sonar S8544). 25.0.1 is the last pip that supports Python 3.8.
+          if [ "${{ matrix.python-version }}" = "pypy-3.8" ]; then
+            python -m pip install --only-binary :all: pip==25.0.1
+          else
+            python -m pip install --only-binary :all: pip==26.2.2
+          fi
+          pip install --only-binary :all: -r requirements-ci.txt
       - name: Lint with flake8
         run: flake8 . --count --max-complexity=10 --max-line-length=127 --statistics
       - name: Run PyLint

.github/workflows/build-release.yml

@@ -16,15 +16,17 @@ jobs:
         uses: actions/setup-python@v2
         with:
           python-version: '3.10'
+      # Wheels only, as in build-main.yml: this job also holds the PyPI credentials.
+      # The tooling is pinned to exact versions (Sonar S8544).
       - id: install-tooling
         run: |
-          python -m pip install --upgrade pip
-          pip install wheel
-          pip install twine
-          pip install build
+          python -m pip install --only-binary :all: pip==26.2.2
+          pip install --only-binary :all: wheel==0.48.0
+          pip install --only-binary :all: twine==7.0.0
+          pip install --only-binary :all: build==1.6.1
       - id: install-project-dependencies
         run: |
-          pip install -r requirements-dev.txt
+          pip install --only-binary :all: -r requirements-ci.txt
       - id: testing
         env:
           CHECKOUT_PROCESSING_CHANNEL_ID: ${{ secrets.IT_CHECKOUT_PROCESSING_CHANNEL_ID }}

checkout_sdk/api_client.py

@@ -102,6 +102,7 @@ def invoke(self,
 
         base_uri = self._base_uri + path
 
+        files = None
         try:
             json_body, params_dict, files = self._prepare_request_payload(
                 request_headers, body, params, file_request, multipart_file)
@@ -122,6 +123,11 @@ def invoke(self,
         except OSError as err:
             error = err.strerror
             raise CheckoutException(error) from err
+        finally:
+            # get_file_request opens the upload; requests reads it but never closes it.
+            if files is not None:
+                for _, file, _ in files.values():
+                    file.close()
 
         http_metadata = map_to_http_metadata(response)
         if response.text:

checkout_sdk/oauth_credentials.py

@@ -4,7 +4,6 @@
 from abc import ABCMeta
 from datetime import datetime, timedelta
 
-import urllib3
 from requests import HTTPError, Session, ConnectionError
 
 from checkout_sdk.authorization_type import AuthorizationType
@@ -69,11 +68,9 @@ def get_access_token(self):
             'scope': str.join(' ', self.__scopes)
         }
 
-        urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
-
         try:
             response = self.__http_client.request(method='POST', url=self.__authorization_uri,
-                                                  data=data, verify=False, allow_redirects=False,
+                                                  data=data, allow_redirects=False,
                                                   auth=(self.__client_id, self.__client_secret))
             response.raise_for_status()
         except HTTPError as err:

requirements-ci.txt

@@ -0,0 +1,7 @@
+requests>=2.27.1
+pytest>=6.1.2
+pytest-mock>=3.6.1
+flake8>=4.0.1
+python-dateutil>=2.8.2
+pylint>=2.12.2
+Deprecated>=1.2.14

requirements-dev.txt

@@ -1,8 +1,2 @@
-requests>=2.27.1
-pytest>=6.1.2
-pytest-mock>=3.6.1
-flake8>=4.0.1
-python-dateutil>=2.8.2
+-r requirements-ci.txt
 pre-commit>=2.17.0
-pylint>=2.12.2
-Deprecated>=1.2.14

tests/api_client_file_upload_test.py

@@ -0,0 +1,73 @@
+from unittest.mock import MagicMock
+
+import pytest
+from requests import ConnectionError, HTTPError
+
+from checkout_sdk.api_client import ApiClient
+from checkout_sdk.exception import CheckoutApiException, CheckoutException
+from checkout_sdk.files.files import FileRequest
+
+
+# Exercises the REAL ApiClient upload path (invoke -> get_file_request -> http_client.request).
+# get_file_request opens the file and hands the open handle to requests, which reads it but never
+# closes it, so invoke has to close it whatever the outcome of the request.
+def _build(mock_sdk_configuration, request_side_effect=None, status=200):
+    api_client = ApiClient(configuration=mock_sdk_configuration,
+                           base_uri=mock_sdk_configuration.environment.base_uri)
+    response = MagicMock()
+    response.status_code = status
+    response.text = '{"id": "file_123"}'
+    response.headers = {'Content-Type': 'application/json'}
+    response.json.return_value = {'id': 'file_123'}
+    if status >= 400:
+        response.raise_for_status.side_effect = HTTPError(response=response)
+    http_client = MagicMock()
+    http_client.request.return_value = response
+    http_client.request.side_effect = request_side_effect
+    api_client._http_client = http_client
+
+    authorization = MagicMock()
+    authorization.get_authorization_header.return_value = 'Bearer test'
+    return api_client, http_client, authorization
+
+
+def _file_request(tmp_path):
+    path = tmp_path / 'evidence.pdf'
+    path.write_bytes(b'%PDF-1.4')
+    file_request = FileRequest()
+    file_request.file = str(path)
+    file_request.purpose = 'dispute_evidence'
+    return file_request
+
+
+def _sent_file(http_client):
+    files = http_client.request.call_args.kwargs['files']
+    return files['file'][1]
+
+
+class TestApiClientFileUpload:
+
+    def test_should_close_the_uploaded_file_after_a_successful_request(self, mock_sdk_configuration, tmp_path):
+        api_client, http_client, authorization = _build(mock_sdk_configuration)
+
+        response = api_client.submit_file('files', authorization, _file_request(tmp_path))
+
+        assert response.id == 'file_123'
+        assert _sent_file(http_client).closed
+
+    def test_should_close_the_uploaded_file_when_the_api_returns_an_error(self, mock_sdk_configuration, tmp_path):
+        api_client, http_client, authorization = _build(mock_sdk_configuration, status=422)
+
+        with pytest.raises(CheckoutApiException):
+            api_client.submit_file('files', authorization, _file_request(tmp_path))
+
+        assert _sent_file(http_client).closed
+
+    def test_should_close_the_uploaded_file_when_the_connection_fails(self, mock_sdk_configuration, tmp_path):
+        api_client, http_client, authorization = _build(mock_sdk_configuration,
+                                                        request_side_effect=ConnectionError('unreachable'))
+
+        with pytest.raises(CheckoutException):
+            api_client.submit_file('files', authorization, _file_request(tmp_path))
+
+        assert _sent_file(http_client).closed

tests/oauth_credentials_test.py

@@ -0,0 +1,58 @@
+import json
+
+import pytest
+from requests import Response, Session
+from requests.adapters import HTTPAdapter
+
+from checkout_sdk.environment import Environment
+from checkout_sdk.oauth_credentials import OAuthSdkCredentials
+
+
+def token_response(request):
+    response = Response()
+    response.status_code = 200
+    response.request = request
+    response.url = request.url
+    response._content = json.dumps(
+        {'access_token': 'access_token', 'token_type': 'Bearer', 'expires_in': 3600}).encode('utf-8')
+    return response
+
+
+class TestOAuthSdkCredentials:
+    """The token request carries the client id and secret, so it must never go out with TLS
+    certificate verification turned off. The SDK leaves verification to the http_client the
+    caller passed in, and to requests' defaults, instead of overriding it per request.
+
+    Only the transport is stubbed: a real Session merges its settings into the request, so the
+    test sees the same verify value the TLS handshake would.
+    """
+
+    @pytest.fixture(autouse=True)
+    def no_ca_bundle_from_environment(self, monkeypatch):
+        # requests reads these when verification is on, which would make the asserted value
+        # depend on the machine running the tests.
+        monkeypatch.delenv('REQUESTS_CA_BUNDLE', raising=False)
+        monkeypatch.delenv('CURL_CA_BUNDLE', raising=False)
+
+    @staticmethod
+    def request_token(mocker, http_client: Session):
+        send = mocker.patch.object(HTTPAdapter, 'send', autospec=True,
+                                   side_effect=lambda adapter, request, **kwargs: token_response(request))
+        credentials = OAuthSdkCredentials(http_client, Environment.sandbox(), 'client_id', 'client_secret',
+                                          None, ['vault'])
+        assert credentials.get_access_token().token == 'access_token'
+        return send
+
+    def test_should_verify_the_server_certificate_when_requesting_the_token(self, mocker):
+        send = self.request_token(mocker, Session())
+
+        assert send.call_count == 1
+        assert send.call_args.kwargs['verify'] is True
+
+    def test_should_use_the_ca_bundle_configured_on_the_http_client(self, mocker):
+        http_client = Session()
+        http_client.verify = '/path/to/ca-bundle.pem'
+
+        send = self.request_token(mocker, http_client)
+
+        assert send.call_args.kwargs['verify'] == '/path/to/ca-bundle.pem'