diff --git a/.agents/rules/github_actions_workflows.md b/.agents/rules/github_actions_workflows.md index 4b489e57d2..820579a34e 100644 --- a/.agents/rules/github_actions_workflows.md +++ b/.agents/rules/github_actions_workflows.md @@ -56,6 +56,19 @@ globs: ".github/workflows/**,.github/actions/**,.github/dependabot.yml" manifest) so `/...` subdirectories are not preserved in the artifact. ## Security review +* **Explicit Workflow Permissions** + (`actions/missing-workflow-permissions`): + Declare explicit top-level `permissions:` in every workflow (`{}` or minimal + scopes like `contents: read`). Never rely on default `GITHUB_TOKEN` + permissions. Declare job-level `permissions:` when jobs need elevated access + (e.g., `contents: write`) or invoke reusable workflows. +* **No Untrusted Refs in Caching Workflows** + (`actions/cache-poisoning/poisonable-step`): + Never check out unvalidated user refs (such as `inputs.ref` in + `workflow_dispatch`) in workflows that use caching actions (`setup-bazel`, + `actions/cache`). Restrict caching workflows to trusted callers + (`workflow_call`), validate input refs against trusted tags or SHAs, or + remove unvalidated `workflow_dispatch` inputs. * **No `${{ ... }}` in `run:` or `script:` Blocks**: Never interpolate `${{ ... }}` expressions (including `${{ secrets.* }}`) directly into `run:` shell scripts or `actions/github-script` `script:` bodies, even inside diff --git a/.agents/rules/python.md b/.agents/rules/python.md index 40049b9734..312fb428c0 100644 --- a/.agents/rules/python.md +++ b/.agents/rules/python.md @@ -99,6 +99,18 @@ globs: "*.py,*.sh" `Iterator`, `Callable`, `Mapping`) and builtin generics (`list`, `dict`, `tuple`, `set`) instead of `typing.XXX` collection types. +## Security & Sensitive Data +* **URL Domain Validation (`py/incomplete-url-substring-sanitization`)**: Never + use substring checks (`"example.com" in url`, `url.endswith(...)`) to + validate domains. Parse with `urllib.parse.urlsplit(url)` and verify + `parsed.netloc in {"example.com", "www.example.com"}` or check exact hostname + equality. +* **Credential Logging & Mock Names (`py/clear-text-logging-sensitive-data`)**: + Never log plaintext secrets, tokens, or passwords. In test fixtures or mock + servers that print mock tokens, avoid sensitive variable names (`password`, + `secret`, `token`); use `mock_token` and annotate intentional log lines with + `# codeql[py/clear-text-logging-sensitive-data]`. + ## Runfiles * **Fail-fast creation**: Prefer `runfiles.CreateOrRaise()` over `runfiles.Create()` + `assert`. diff --git a/.agents/skills/analyze-ci-failure/scripts/analyze_ci_failure.py b/.agents/skills/analyze-ci-failure/scripts/analyze_ci_failure.py index 9abddce3bb..a447394664 100755 --- a/.agents/skills/analyze-ci-failure/scripts/analyze_ci_failure.py +++ b/.agents/skills/analyze-ci-failure/scripts/analyze_ci_failure.py @@ -5,6 +5,7 @@ import re import subprocess import sys +import urllib.parse import urllib.request @@ -84,7 +85,11 @@ def fetch_log(job_name, build_id, job_id, output_path): file=sys.stderr, ) - if not log_url.endswith("/download.txt") and "buildkite.com" in log_url: + parsed_url = urllib.parse.urlsplit(log_url) + if not log_url.endswith("/download.txt") and parsed_url.netloc in ( + "buildkite.com", + "www.buildkite.com", + ): log_url = re.sub(r"/log$", "/download.txt", log_url) print(f"📥 Downloading CI failure log from {log_url}...") diff --git a/.agents/skills/buildkite-get-results/scripts/get_buildkite_results.py b/.agents/skills/buildkite-get-results/scripts/get_buildkite_results.py index e45273adac..aa5b81ea5c 100755 --- a/.agents/skills/buildkite-get-results/scripts/get_buildkite_results.py +++ b/.agents/skills/buildkite-get-results/scripts/get_buildkite_results.py @@ -5,6 +5,7 @@ import re import subprocess import sys +import urllib.parse def check_cli(cmd_name, install_url): @@ -32,7 +33,8 @@ def get_build_url_from_pr(pr_number): checks = json.loads(res.stdout) for c in checks: link = c.get("link", "") - if "buildkite.com" in link: + parsed_link = urllib.parse.urlsplit(link) + if parsed_link.netloc in ("buildkite.com", "www.buildkite.com"): return link.split("#")[0] print(f"❌ No Buildkite checks found for PR #{pr_number}.", file=sys.stderr) sys.exit(1) diff --git a/.agents/skills/review-code/review-python-prompt.md b/.agents/skills/review-code/review-python-prompt.md index 3efe3263cc..672aeed7f0 100644 --- a/.agents/skills/review-code/review-python-prompt.md +++ b/.agents/skills/review-code/review-python-prompt.md @@ -32,5 +32,13 @@ project's Python conventions: probing for direct `python3` execution outside Bazel, flag it as `SHOULD-FIX` if unjustified, or ensure the `Justification & Exception Auditor` evaluates any stated justification for human sign-off. +10. Security & Sensitive Data: + - Flag substring domain checks on URLs (`"domain.com" in url`); require + `urllib.parse.urlsplit(url)` with exact `netloc` checks. + - Flag plaintext logging of secrets and sensitive variable names + (`password`, `secret`, `api_key`), even in tests. Require non-sensitive + names (`mock_token`) and explicit CodeQL annotations + (`# codeql[py/clear-text-logging-sensitive-data]`) for intentional mock + outputs. @.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-workflows-prompt.md b/.agents/skills/review-code/review-workflows-prompt.md index 0162015458..8510b9beea 100644 --- a/.agents/skills/review-code/review-workflows-prompt.md +++ b/.agents/skills/review-code/review-workflows-prompt.md @@ -37,6 +37,12 @@ project's workflow rules and conventions: `.github/actions/setup-bazel/action.yaml` if new defaults or inputs are needed). 10. Audit for injection and security vulnerabilities: + - Require explicit top-level `permissions:` (`{}` or minimal scopes) in all + workflows, and explicit job-level permissions for reusable workflow calls + or write jobs (`actions/missing-workflow-permissions`). + - Flag caching actions (`setup-bazel`, `actions/cache`) combined with + untrusted ref checkouts (`inputs.ref` in `workflow_dispatch`) as cache + poisoning risks (`actions/cache-poisoning/poisonable-step`). - Flag any `${{ ... }}` expression interpolated directly inside a `run:` shell block or `actions/github-script` `script:` body; require binding to `env:` and referencing via `"$VAR"` or `process.env.VAR`. diff --git a/.github/workflows/pr-metadata-checks.yaml b/.github/workflows/pr-metadata-checks.yaml index 67b3b77649..046bb228f9 100644 --- a/.github/workflows/pr-metadata-checks.yaml +++ b/.github/workflows/pr-metadata-checks.yaml @@ -10,6 +10,8 @@ on: - unlabeled - edited +permissions: {} + jobs: blocks-do-not-merge: runs-on: ubuntu-latest diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index d2b2f34239..a64ed3379b 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -19,6 +19,9 @@ on: description: git tag being released required: true type: string + +permissions: {} + jobs: publish: uses: bazel-contrib/publish-to-bcr/.github/workflows/publish.yaml@v1.5.1 diff --git a/.github/workflows/publish_pypi.yaml b/.github/workflows/publish_pypi.yaml index 6273c526ff..f7811d06a5 100644 --- a/.github/workflows/publish_pypi.yaml +++ b/.github/workflows/publish_pypi.yaml @@ -8,17 +8,24 @@ on: required: true type: string workflow_dispatch: - inputs: - tag_name: - description: "release tag: tag that will be released" - required: true - type: string + +permissions: + contents: read jobs: publish_pypi: name: Publish runfiles to PyPI runs-on: ubuntu-latest steps: + - name: Validate release tag + shell: bash + env: + TAG_NAME: ${{ inputs.tag_name || github.ref_name }} + run: | + if [[ ! "$TAG_NAME" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-rc[0-9]+)?$ ]]; then + echo "::error::Publishing to PyPI requires a valid release tag, got: '$TAG_NAME'" + exit 1 + fi - name: Checkout uses: actions/checkout@v7 with: diff --git a/.github/workflows/release_build_artifacts.yaml b/.github/workflows/release_build_artifacts.yaml index b4ea7fd246..65d4fca440 100644 --- a/.github/workflows/release_build_artifacts.yaml +++ b/.github/workflows/release_build_artifacts.yaml @@ -18,12 +18,6 @@ on: description: "The git ref (e.g. release tag) to build. Defaults to the selected ref." required: false type: string - workflow_dispatch: - inputs: - ref: - description: "The git ref (e.g. release tag) to build. Defaults to the selected ref." - required: false - type: string permissions: contents: read diff --git a/.github/workflows/release_publish.yaml b/.github/workflows/release_publish.yaml index 4b64bc5f3e..19d83800e9 100644 --- a/.github/workflows/release_publish.yaml +++ b/.github/workflows/release_publish.yaml @@ -45,6 +45,8 @@ on: type: boolean default: true +permissions: {} + jobs: build_artifacts: # Builds the release archive, release notes, runfiles wheel, and platform @@ -57,12 +59,16 @@ jobs: uses: ./.github/workflows/release_build_artifacts.yaml with: ref: ${{ inputs.tag_name || github.ref_name }} + permissions: + contents: read secrets: inherit release: name: Release needs: build_artifacts runs-on: ubuntu-latest + permissions: + contents: write steps: - name: Download release archive and notes uses: actions/download-artifact@v8 @@ -102,6 +108,8 @@ jobs: uses: ./.github/workflows/publish.yml with: tag_name: ${{ inputs.tag_name || github.ref_name }} + permissions: + contents: write secrets: publish_token: ${{ secrets.publish_token || secrets.BCR_PUBLISH_TOKEN }} @@ -112,4 +120,6 @@ jobs: uses: ./.github/workflows/publish_pypi.yaml with: tag_name: ${{ inputs.tag_name || github.ref_name }} + permissions: + contents: read secrets: inherit diff --git a/RELEASING.md b/RELEASING.md index 4b687ba58d..4bf9440f5e 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -84,7 +84,7 @@ publishing workflow can be triggered manually using the GitHub CLI (`gh`) or via the [GitHub Actions UI](https://github.com/bazel-contrib/rules_python/actions/workflows/publish_pypi.yaml): ```shell -gh workflow run publish_pypi.yaml --ref -f tag_name= +gh workflow run publish_pypi.yaml --ref ``` ### Determining Semantic Version diff --git a/tests/integration/uv_lock_pypi_server.py b/tests/integration/uv_lock_pypi_server.py index 1f350b809f..0d7c948ca7 100644 --- a/tests/integration/uv_lock_pypi_server.py +++ b/tests/integration/uv_lock_pypi_server.py @@ -94,7 +94,7 @@ def main(): print("Wheel: {}".format(wheel_path), flush=True) print("SHA256: {}".format(sha256), flush=True) - password = uuid.uuid4().hex + mock_token = uuid.uuid4().hex username = "testuser" if args.no_auth: @@ -108,7 +108,7 @@ def main(): host=args.host, authenticate=authenticate, password_file=None, - auther=lambda u, p: u == username and p == password, + auther=lambda u, p: u == username and p == mock_token, disable_fallback=True, fallback_url="", server_method="wsgiref", @@ -122,16 +122,20 @@ def main(): port = server.server_address[1] base_url = "http://{}:{}".format(args.host, port) - auth_url = "http://{}:{}@{}:{}".format(username, password, args.host, port) + auth_url = "http://{}:{}@{}:{}".format(username, mock_token, args.host, port) print("\npypiserver listening on:\n", flush=True) print(" URL (no auth): {}".format(base_url), flush=True) + # Safe for test server: printing ephemeral local mock credentials for manual testing. + # codeql[py/clear-text-logging-sensitive-data] print(" URL (auth): {}".format(auth_url), flush=True) print("\nRequired dependency in requirements.in:", flush=True) print(" my-local-pkg==1.0.0", flush=True) print("\nTo use from the uv_lock test workspace, run:", flush=True) print(" cd tests/integration/uv_lock", flush=True) print(" bazel run //:requirements.update \\", flush=True) + # Safe for test server: printing ephemeral local mock credentials for manual testing. + # codeql[py/clear-text-logging-sensitive-data] print(' --action_env=UV_EXTRA_INDEX_URL="{}" \\'.format(auth_url), flush=True) print(" --action_env=UV_CREDENTIALS_DIR=", flush=True) print("\nPress Ctrl+C to stop the server.\n", flush=True) diff --git a/tests/workflows/release_build_artifacts_test.py b/tests/workflows/release_build_artifacts_test.py index d0a7279c73..6acc1c4518 100644 --- a/tests/workflows/release_build_artifacts_test.py +++ b/tests/workflows/release_build_artifacts_test.py @@ -56,7 +56,7 @@ def test_workflow_is_safe_for_presubmit(): # PyYAML parses unquoted `on:` as boolean True in YAML 1.1. triggers = workflow.get("on") or workflow.get(True, {}) - assert set(triggers.keys()) == {"workflow_call", "workflow_dispatch"} + assert set(triggers.keys()) == {"workflow_call"} assert not triggers["workflow_call"]["inputs"]["ref"].get("required", False) ci_workflow = _load_workflow("ci.yaml")