Skip to content

feature/INT-1751 - Security updates - #257

Merged
david-ruiz-cko merged 3 commits into
mainfrom
feature/INT-1751
Oct 26, 2026
Merged

david-ruiz-cko merged 3 commits into
mainfrom
feature/INT-1751

Conversation

@david-ruiz-cko

@david-ruiz-cko david-ruiz-cko commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

This pull request improves the security and reliability of dependency installation in CI workflows, refactors requirements files to separate CI and development dependencies, and strengthens the handling of TLS certificate verification when obtaining OAuth tokens. It also adds comprehensive tests to ensure that server certificate verification is always enforced.

CI/CD and Dependency Management Improvements:

  • Updated all GitHub Actions workflows (build-main.yml, build-pull-request.yml, build-release.yml) to install dependencies using --only-binary :all: and to use a new requirements-ci.txt file, ensuring only pre-built wheels are installed and preventing execution of untrusted setup.py scripts in CI jobs that may contain secrets. [1] [2] [3]
  • Refactored requirements files: moved common CI dependencies into requirements-ci.txt and updated requirements-dev.txt to include requirements-ci.txt and pre-commit, reducing duplication and isolating pre-commit-related dependencies from CI. [1] [2]

Testing Improvements:

  • Added new tests in tests/oauth_credentials_test.py to verify that server certificate verification is always enabled when requesting OAuth tokens and that the HTTP client's custom CA bundle configuration is respected.

Files:

  • The uploaded file is now closed after the request; previously its handle stayed open until garbage collection.

Security and Code Quality Enhancements:

  • Removed explicit disabling of TLS certificate verification (verify=False) in OAuthSdkCredentials.get_access_token, ensuring the SDK relies on the HTTP client's default certificate verification behavior.
  • Removed unused import of urllib3 from checkout_sdk/oauth_credentials.py as it is no longer needed.

Security: ⚠️

  • OAuth: the token request now verifies the server's TLS certificate. Previously verification was turned off for that request and the warning was silenced, which exposed the client id and secret to interception.
    Behaviour change: If your network sits behind a proxy that inspects TLS, the OAuth token request can now fail with CheckoutAuthorizationException until your system trusts the proxy's CA. Add the proxy's CA to your trust store, or set it on the HTTP client you pass to the SDK (for example session.verify = "/path/to/ca-bundle.pem"), or set the REQUESTS_CA_BUNDLE environment variable. The SDK now honours all of these. Turning verification off is not supported.

@david-ruiz-cko
david-ruiz-cko requested a review from a team October 26, 2026 11:04
@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:.github/workflows/build-main.yml
  • security_sensitive_path:.github/workflows/build-pull-request.yml
  • security_sensitive_path:.github/workflows/build-release.yml
  • security_sensitive_path:checkout_sdk/oauth_credentials.py
  • security_sensitive_path:tests/oauth_credentials_test.py

Operational gates

  • ✅ jira_ticket (INT-1751)
  • ✅ independent_review

Files analysed: 7


wall-e 2026.06.19-02 · policy 6b4ce2b3b45a…

@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_path — .github/workflows/build-main.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-pull-request.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-release.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — checkout_sdk/oauth_credentials.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — tests/oauth_credentials_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

🟢 Advisory review: Looks good to me

This PR still needs a human approval — wall-e cannot auto-approve it. For what it's worth, I read the diff and found nothing I'd block on.

Removes verify=False from the OAuth token request (a real security fix), pins CI pip installs to wheels-only, refactors requirements files, adds a file-handle close in a finally block, and backs all of it with new tests. The diff matches every claim in the description and I found no blocking issues.

What I checked

  • The verify=False removal in oauth_credentials.py is the core security fix; the new tests in oauth_credentials_test.py correctly stub at the HTTPAdapter.send level so the real Session merges its verify setting, making the assertions trustworthy.
  • The finally block in api_client.py initialises files = None before the try, so the guard if files is not None is safe even if _prepare_request_payload raises before assigning files.
  • The file-close loop for _, file, _ in files.values() assumes the dict values are always 3-tuples; this matches the (name, file_obj, content_type) shape produced by get_file_request, and the new tests exercise all three exit paths (success, HTTP error, connection error).
  • Pinning pip to 25.0.1 for pypy-3.8 and 26.2.2 elsewhere is reasonable; these are real released versions and the comment explains the pypy constraint.
  • The requirements-ci.txt uses >= lower bounds rather than exact pins, which is intentional for the project dependencies (as opposed to the tooling) and consistent with the existing style.
  • The requirements-dev.txt newline fix (missing newline at EOF was present before) is a harmless cleanup.

This is not an approval. wall-e cannot auto-approve this PR — it is an opinion to help whoever does. Advisory review · us.anthropic.claude-sonnet-4-6 · wall-e 2026.06.19-02

Comment thread .github/workflows/build-main.yml Fixed
Comment thread .github/workflows/build-main.yml Fixed
Comment thread .github/workflows/build-pull-request.yml Fixed
Comment thread .github/workflows/build-pull-request.yml Fixed
Comment thread .github/workflows/build-release.yml Fixed
Comment thread .github/workflows/build-release.yml Fixed
@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:.github/workflows/build-main.yml
  • security_sensitive_path:.github/workflows/build-pull-request.yml
  • security_sensitive_path:.github/workflows/build-release.yml
  • security_sensitive_path:checkout_sdk/oauth_credentials.py
  • security_sensitive_path:tests/oauth_credentials_test.py

Operational gates

  • ✅ jira_ticket (INT-1751)
  • ✅ independent_review

Files analysed: 7


wall-e 2026.06.19-02 · policy 6b4ce2b3b45a…

@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_path — .github/workflows/build-main.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-pull-request.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-release.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — checkout_sdk/oauth_credentials.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — tests/oauth_credentials_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:.github/workflows/build-main.yml
  • security_sensitive_path:.github/workflows/build-pull-request.yml
  • security_sensitive_path:.github/workflows/build-release.yml
  • security_sensitive_path:checkout_sdk/oauth_credentials.py
  • security_sensitive_path:tests/oauth_credentials_test.py

Operational gates

  • ✅ jira_ticket (INT-1751)
  • ✅ independent_review

Files analysed: 9


wall-e 2026.06.19-02 · policy 6b4ce2b3b45a…

@agent-wall-e

agent-wall-e Bot commented Oct 26, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_path — .github/workflows/build-main.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-pull-request.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — .github/workflows/build-release.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — checkout_sdk/oauth_credentials.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path — tests/oauth_credentials_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@sonarqubecloud

Copy link
Copy Markdown

@david-ruiz-cko
david-ruiz-cko merged commit ecba983 into main Oct 26, 2026
4 checks passed
@david-ruiz-cko
david-ruiz-cko deleted the feature/INT-1751 branch October 26, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants