Skip to content

Commit

Permalink
Add authorize step to tool_tests.yml (#1480)
Browse files Browse the repository at this point in the history
# Description

By abusing the "pull_request_target" event for triggering workflows, any
GitHub user can create a Pull Request from a Fork against this
repository and the action will execute. There is a step in the workflow
that runs the following code:
``` python
python ./scripts/tool/generate_connection_config.py --tenant_id ${{ secrets.TENANT_ID }} --client_id ${{ secrets.CLIENT_ID }} --client_secret ${{ secrets.CLIENT_SECRET }}
```
This means that an attacker can create a Fork with a modified
generate_connection_config.py that will be executed, and has credentials
passed in as a parameter, making it trivial to dump these credentials
out, so add the authorize step.

The authorize step appears to be used as a preliminary job to determine
the context in which the workflow is being executed, specifically in
terms of the origin of the pull request. This job categorizes pull
requests into two environments: external for forked repository pull
requests and internal for pull requests within the same repository.

# All Promptflow Contribution checklist:
- [ ] **The pull request does not introduce [breaking changes].**
- [ ] **CHANGELOG is updated for new features, bug fixes or other
significant changes.**
- [ ] **I have read the [contribution guidelines](../CONTRIBUTING.md).**
- [ ] **Create an issue and link to the pull request to get dedicated
review from promptflow team. Learn more: [suggested
workflow](../CONTRIBUTING.md#suggested-workflow).**

## General Guidelines and Best Practices
- [ ] Title of the pull request is clear and informative.
- [ ] There are a small number of commits, each of which have an
informative message. This means that previously merged commits do not
appear in the history of the PR. For more information on cleaning up the
commits in your PR, [see this
page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).

### Testing Guidelines
- [ ] Pull request includes test coverage for the included changes.
  • Loading branch information
chenslucky authored Dec 14, 2023
1 parent 79b991d commit 97d213a
Showing 1 changed file with 13 additions and 2 deletions.
15 changes: 13 additions & 2 deletions .github/workflows/tools_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,25 @@ on:
- '**tools_tests.yml'
workflow_dispatch:
jobs:
run_tool_ci_job:
authorize:
environment:
# forked prs from pull_request_target will be run in external environment, domain prs will be run in internal environment
${{ github.event_name == 'pull_request_target' &&
github.event.pull_request.head.repo.full_name != github.repository &&
'external' || 'internal' }}
# The type of runner that the job will run on
runs-on: ubuntu-latest
name: Tool Test
timeout-minutes: 30
steps:
- run: true
build:
needs: authorize
strategy:
fail-fast: false
runs-on: ubuntu-latest
env:
DEPENDENCY_SOURCE_MODE: ${{ secrets.DEPENDENCY_SOURCE_MODE }}

steps:
- name: Check for dockerenv file
run: (ls /.dockerenv && echo Found dockerenv) || (echo No dockerenv)
Expand Down

0 comments on commit 97d213a

Please sign in to comment.