chore(ci): harden stainless-builds workflow for pull_request_target #4597
Conversation
dgellow
requested review from
ashwinb,
bbrowning,
cdoern,
ehhuang,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
meta-cla
bot
added
the
CLA Signed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
It might be better to split the workflow into two? https://openssf.org/blog/2024/08/12/mitigating-attack-vectors-in-github-workflows/ |
dgellow
force-pushed
the
sam/python-caching-issue
branch
from
c8fd5d8 to
eae255c
Compare
dgellow
force-pushed
the
sam/python-caching-issue
branch
from
eae255c to
8b08f0e
Compare
| run-replay-mode-tests: | ||
| needs: generate-matrix | ||
| # Always run even if generate-matrix was skipped (when matrix_json is provided) | ||
| if: ${{ !cancelled() }} | ||
| runs-on: ubuntu-latest | ||
| # When disable_cache is true, set UV_NO_CACHE to prevent uv from using cached packages. | ||
| # This is a security measure for pull_request_target contexts to prevent cache poisoning. | ||
| env: | ||
| UV_NO_CACHE: ${{ inputs.disable_cache == true }} | ||
| name: ${{ format('Integration Tests ({0}, {1}, {2}, client={3}, {4})', matrix.client, matrix.config.setup, matrix.python-version, matrix.client-version, matrix.config.suite) }} | ||
|
|
||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| client: [library, docker, server] | ||
| # Use Python 3.13 only on nightly schedule (daily latest client test), otherwise use 3.12 | ||
| python-version: ${{ github.event.schedule == '0 0 * * *' && fromJSON('["3.12", "3.13"]') || fromJSON('["3.12"]') }} | ||
| node-version: [22] | ||
| client-version: ${{ (github.event.schedule == '0 0 * * *' || github.event.inputs.test-all-client-versions == 'true' || inputs.test-all-client-versions == true) && fromJSON('["published", "latest"]') || fromJSON('["latest"]') }} | ||
| # Test configurations: Generated from CI_MATRIX in tests/integration/ci_matrix.json | ||
| # See scripts/generate_ci_matrix.py for generation logic | ||
| config: ${{ fromJSON(needs.generate-matrix.outputs.matrix).include }} | ||
| # Test configurations: Either from matrix_json input or generated from ci_matrix.json | ||
| config: ${{ fromJSON(inputs.matrix_json || needs.generate-matrix.outputs.matrix).include }} | ||
|
|
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
There was a problem hiding this comment.
I think that's orthogonal to security concerns from this specific PR, and I would prefer to not expand its scope
- Add matrix_json input to skip generate_ci_matrix.py execution - Add disable_cache input to prevent cache poisoning - Change composite action refs to use full repo paths (@main) - Set UV_NO_CACHE env var when disable_cache is true - Disable npm caching when disable_cache is true - Update stainless-builds.yml to pass hardcoded matrix and disable_cache
dgellow
force-pushed
the
sam/python-caching-issue
branch
from
8b08f0e to
0215954
Compare
| if: ${{ matrix.client == 'server' }} | ||
| id: setup-ts-client | ||
| uses: ./.github/actions/setup-typescript-client | ||
| uses: llamastack/llama-stack/.github/actions/setup-typescript-client@306e43f882fdfbaf877f989f0c1ea900c6348055 |
There was a problem hiding this comment.
Ideally we would use @main here, but that's forbidden via pre-commit checks. That sha will need to be updated whenever the .github/actions/setup-typescript-client is updated.
| - name: Run tests | ||
| if: ${{ matrix.config.allowed_clients == null || contains(matrix.config.allowed_clients, matrix.client) }} | ||
| uses: ./.github/actions/run-and-record-tests | ||
| uses: llamastack/llama-stack/.github/actions/run-and-record-tests@306e43f882fdfbaf877f989f0c1ea900c6348055 |
There was a problem hiding this comment.
Ideally we would use @main here, but that's forbidden via pre-commit checks. That sha will need to be updated whenever the .github/actions/run-and-record-tests is updated.
| - name: Setup test environment | ||
| if: ${{ matrix.config.allowed_clients == null || contains(matrix.config.allowed_clients, matrix.client) }} | ||
| uses: ./.github/actions/setup-test-environment | ||
| uses: llamastack/llama-stack/.github/actions/setup-test-environment@306e43f882fdfbaf877f989f0c1ea900c6348055 |
There was a problem hiding this comment.
Ideally we would use @main here, but that's forbidden via pre-commit checks. That sha will need to be updated whenever the .github/actions/setup-test-environment is updated.
cdoern
left a comment
cdoern
left a comment
There was a problem hiding this comment.
nothing glaring here, this looks like the right design! will do a more in depth review when I am back from PTO (monday or maybe on Sunday)
3 participants
Summary
Harden the
stainless-builds.ymlworkflow for safe use withpull_request_target. This workflow runs in a privileged context even for fork PRs, so we need to ensure untrusted PR code cannot be exploited.Changes to
integration-tests.ymlmatrix_jsoninput: when provided, skipsgenerate_ci_matrix.pyexecution entirelydisable_cacheinput: disables all caching mechanisms when trueUV_NO_CACHE=1environment variable whendisable_cacheis true (prevents uv from reading/writing package cache)package-manager-cache: falsewhendisable_cacheis true./.github/actions/*tollamastack/llama-stack/.github/actions/*@mainso they're loaded from the trusted main branch, not from PR checkoutChanges to
stainless-builds.ymlmatrix_jsonwith hardcoded stainless test config (matchesci_matrix.json"stainless" key)disable_cache: trueto enable all cache poisoning mitigationsSecurity Model
generate_ci_matrix.pycode executionmatrix_jsoninput@mainUV_NO_CACHE=1env varpackage-manager-cache: falseBackwards Compatibility
These changes only affect behavior when
matrix_jsonordisable_cacheinputs are explicitly passed. Normalpull_request,push, andworkflow_dispatchtriggers are unaffected.Test plan
We would need to merge and test when the workflows are updated in
main