Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# CODEOWNERS - Define mandatory reviewers for security-sensitive code
#
# Security-critical paths require review from the @microsoft/brainsmith team
# to prevent unauthorized access to self-hosted infrastructure and secrets.
#
# See: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

# GitHub Actions workflows - can access secrets and self-hosted runners
/.github/workflows/ @microsoft/brainsmith
/.github/actions/ @microsoft/brainsmith

# Docker and container configuration - runs on self-hosted infrastructure
/Dockerfile @microsoft/brainsmith
/docker/ @microsoft/brainsmith
/ctl-docker.sh @microsoft/brainsmith

# Shell scripts - can execute arbitrary commands on runners
*.sh @microsoft/brainsmith

# Dependency definitions - control what code is downloaded during setup
/brainsmith/_internal/io/dependencies.py @microsoft/brainsmith

# Security and repository configuration
/.github/CODEOWNERS @microsoft/brainsmith
/SECURITY.md @microsoft/brainsmith
109 changes: 109 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
## Description

<!-- Provide a clear and concise description of your changes -->

## Type of Change

<!-- Mark the relevant option with an 'x' -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation update
- [ ] CI/CD or infrastructure change
- [ ] Refactoring (no functional changes)

## Related Issues

<!-- Link to related issues using #issue_number -->

Fixes #
Related to #

## Changes Made

<!-- Provide a bulleted list of the changes made in this PR -->

-
-
-

## Testing

<!-- Describe the tests you ran and how to reproduce -->

### Test Environment
- Python version:
- Poetry version:
- Operating System:

### Tests Performed
- [ ] I have tested this locally
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] I have run `poetry run pytest -m 'unit or integration'` successfully

### Test Coverage
<!-- Describe which parts of the code are covered by tests -->

## Documentation

- [ ] I have updated the documentation to reflect my changes
- [ ] I have updated docstrings for modified functions/classes
- [ ] I have added/updated examples if applicable
- [ ] Documentation builds successfully (`mkdocs build`)

## Code Quality

- [ ] My code follows the style guidelines of this project (`ruff check .` passes)
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] My changes generate no new warnings
- [ ] I have removed any debugging code (print statements, breakpoints, etc.)

## Breaking Changes

<!-- If this PR introduces breaking changes, describe them here and provide migration instructions -->

**Does this PR introduce breaking changes?**
- [ ] Yes
- [ ] No

<!-- If yes, describe the breaking changes and migration path: -->

## Security Considerations

<!-- If this PR modifies security-sensitive files, explain why the changes are safe -->

- [ ] This PR does **not** modify `.github/workflows/`, `.github/actions/`, Docker configs, or shell scripts
- [ ] OR: I have explained why these changes are necessary and safe (see below)

<!-- If you checked the second option, explain here: -->

---

## For Maintainers

**Before adding `safe-to-test` label:**

### Security Review
- [ ] Code review completed (especially security-sensitive files)
- [ ] No suspicious network calls or data exfiltration attempts
- [ ] No attempts to access secrets or credentials
- [ ] No hardcoded sensitive information (API keys, passwords, IPs)
- [ ] Docker/workflow changes reviewed by @microsoft/brainsmith team (if applicable)

### Code Review
- [ ] Code follows project conventions and style
- [ ] Tests are adequate and meaningful
- [ ] Documentation is clear and accurate
- [ ] No unnecessary dependencies added
- [ ] Error handling is appropriate

### CI/CD
- [ ] Ready to trigger CI on self-hosted runners
- [ ] Expected test duration is reasonable
- [ ] Artifacts and cleanup are properly configured

**Notes for reviewers:**
<!-- Add any additional notes for reviewers -->
14 changes: 7 additions & 7 deletions .github/actions/run-test-with-artifacts/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,26 @@ runs:
steps:
- name: Execute test
id: test-execution
uses: ./.github/actions/docker-exec
uses: ./.github/actions/docker-exec # codeql[action-injection] - Safe: manual review gate via safe-to-test label
with:
command: ${{ inputs.command }}
timeout-minutes: ${{ inputs.timeout-minutes }}

- name: Collect artifacts
id: collect-artifacts
if: |
always() &&
(inputs.collect-on == 'always' ||
always() &&
(inputs.collect-on == 'always' ||
(inputs.collect-on == 'failure' && steps.test-execution.outcome == 'failure'))
uses: ./.github/actions/collect-artifacts
uses: ./.github/actions/collect-artifacts # codeql[action-injection] - Safe: manual review gate
with:
artifact-directory: ${{ inputs.artifact-name }}

- name: Upload artifacts
id: upload-artifacts
if: |
always() &&
(inputs.collect-on == 'always' ||
always() &&
(inputs.collect-on == 'always' ||
(inputs.collect-on == 'failure' && steps.test-execution.outcome == 'failure'))
uses: actions/upload-artifact@v4
with:
Expand All @@ -67,4 +67,4 @@ runs:

- name: Final cleanup
if: always()
uses: ./.github/actions/docker-cleanup
uses: ./.github/actions/docker-cleanup # codeql[action-injection] - Safe: manual review gate
6 changes: 3 additions & 3 deletions .github/actions/workflow-setup/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ runs:
using: 'composite'
steps:
- name: Check disk space
uses: ./.github/actions/check-disk
uses: ./.github/actions/check-disk # codeql[action-injection] - Safe: manual review gate via safe-to-test label
with:
threshold-gb: ${{ inputs.disk-threshold-gb }}

- name: Clean Docker resources
uses: ./.github/actions/docker-cleanup
uses: ./.github/actions/docker-cleanup # codeql[action-injection] - Safe: manual review gate

- name: Build Docker image
uses: ./.github/actions/build-docker
uses: ./.github/actions/build-docker # codeql[action-injection] - Safe: manual review gate
9 changes: 7 additions & 2 deletions .github/workflows/docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ on:
branches:
- main
- develop
pull_request:
pull_request_target: # Changed from pull_request for fork security
branches:
- main
- develop
Expand All @@ -16,16 +16,21 @@ permissions:

jobs:
deploy:
# SECURITY: Only deploy on push to main/develop, not PRs
# PRs can validate docs build but cannot deploy
if: github.event_name != 'pull_request_target' || github.actor == github.repository_owner
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
# SECURITY: Check out PR head for validation, not base
ref: ${{ github.event.pull_request.head.sha || github.sha }}
fetch-depth: 0 # Needed for git-revision-date-localized and mike

- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: '3.10'
python-version: '3.11'

- name: Configure Git
run: |
Expand Down
90 changes: 87 additions & 3 deletions .github/workflows/pr-validation.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
name: PR Validation

on:
pull_request:
branches: [ develop ]
pull_request_target: # Changed from pull_request for fork security
branches: [ develop, main ]
types: [labeled, synchronize] # Only on label or new commits
push:
branches: [ develop ]
branches: [ develop, main ]
workflow_dispatch:

env:
DOCKER_BUILDKIT: 1
Expand All @@ -20,19 +22,68 @@ env:

permissions:
contents: read
pull-requests: write # Needed to remove labels after workflow completion

jobs:
pytest-validation:
name: Pytest Test Suite
# SECURITY: Only run on trusted contexts
if: |
github.event_name == 'push' ||
github.event_name == 'workflow_dispatch' ||
(github.event_name == 'pull_request_target' &&
contains(github.event.pull_request.labels.*.name, 'safe-to-test') &&
github.event.action == 'labeled')
runs-on: pre-release
timeout-minutes: 15 # Fast test suite with Docker overhead
steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
# SECURITY: Check out PR head, not base
ref: ${{ github.event.pull_request.head.sha || github.sha }}
submodules: recursive
fetch-depth: 0

# SECURITY NOTE: Composite actions below run from PR checkout after manual review.
# This is safe because:
# 1. Maintainer manually reviews ALL changes (including .github/actions/) before adding safe-to-test label
# 2. Label-based gate prevents automatic execution of untrusted code
# 3. This is the standard pattern for manual-approval CI workflows
# CodeQL alert acknowledged: Risk accepted with human review gate.

# SECURITY: Detect new dependencies for manual review
- name: Check for dependency changes
if: github.event_name == 'pull_request_target'
run: |
git fetch origin develop
# Check for changes in dependency files
DEP_CHANGES=$(git diff origin/develop...HEAD -- pyproject.toml poetry.lock)
if [ -n "$DEP_CHANGES" ]; then
echo "⚠️ SECURITY WARNING: Dependency changes detected"
echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"

# Show added dependencies
NEW_DEPS=$(echo "$DEP_CHANGES" | grep -E '^\+[^+].*=' | grep -v '^\+\+\+' || true)
if [ -n "$NEW_DEPS" ]; then
echo "📦 New/Modified dependencies:"
echo "$NEW_DEPS"
echo ""
echo "⚠️ MAINTAINERS: Before approving, verify each package:"
echo " 1. Check package on PyPI (age, downloads, maintainer)"
echo " 2. Verify no typosquatting (e.g., 'requets' vs 'requests')"
echo " 3. Review transitive dependencies: poetry show <package>"
echo " 4. Understand why this dependency is needed"
echo ""
echo "❌ Blocking workflow until dependencies are explicitly reviewed"
exit 1
fi

# Version changes are less risky, just warn
echo "ℹ️ Dependency versions updated (review recommended)"
fi
echo "✅ No new dependencies detected"

- name: Setup workflow
uses: ./.github/actions/workflow-setup
with:
Expand Down Expand Up @@ -66,15 +117,26 @@ jobs:
bert-quicktest:
name: BERT Quicktest
needs: pytest-validation # Only run if pytest passes
# SECURITY: Inherit same conditions as pytest-validation
if: |
github.event_name == 'push' ||
github.event_name == 'workflow_dispatch' ||
(github.event_name == 'pull_request_target' &&
contains(github.event.pull_request.labels.*.name, 'safe-to-test') &&
github.event.action == 'labeled')
runs-on: pre-release
timeout-minutes: 300 # 5 hours total (4 for test + 1 for setup/cleanup)
steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
# SECURITY: Check out PR head, not base
ref: ${{ github.event.pull_request.head.sha || github.sha }}
submodules: recursive
fetch-depth: 0

# SECURITY NOTE: Composite actions run from PR checkout after manual review (see pytest-validation job comment above)

- name: Setup workflow
uses: ./.github/actions/workflow-setup
with:
Expand All @@ -88,3 +150,25 @@ jobs:
artifact-name: "pr-failure-artifacts"
collect-on: "failure"
retention-days: 7

# SECURITY: Remove label after workflow completion to force re-review on updates
- name: Remove safe-to-test label
if: always() && github.event_name == 'pull_request_target'
uses: actions/github-script@v7
with:
script: |
try {
await github.rest.issues.removeLabel({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
name: 'safe-to-test'
});
console.log('✓ Removed safe-to-test label');
} catch (error) {
if (error.status === 404) {
console.log('Label already removed');
} else {
throw error;
}
}
1 change: 0 additions & 1 deletion CODEOWNERS

This file was deleted.

Loading