fix(ci): use repository token for docs deploy - #48
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the docs deployment workflow to use the repository-scoped GITHUB_TOKEN with minimal permissions and adds a workflow contract test to prevent regressions back to a personal token. Flow diagram for updated docs deployment workflow authenticationflowchart LR
A[Docs workflow on main] --> B[build job]
B --> C[permissions contents: write]
B --> D[peaceiris/actions-gh-pages]
D --> E[Use secrets.GITHUB_TOKEN]
E --> F[Push to gh-pages branch]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
4 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The workflow contract test for docs deployment is quite brittle because it relies on the exact step name 'Deploy to GitHub Pages'; consider keying off the
uses: peaceiris/actions-gh-pages@v4action (and maybe anid) instead so future copy changes to the name don't break the test. - In
test_docs_deployment_uses_scoped_github_token, accessingbuild['permissions']['contents']and the deploy step fields with direct indexing will raise KeyError/StopIteration with a less-clear traceback; wrapping these in helper functions or using.getplus explicit assertion messages would make failures easier to interpret when the workflow changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow contract test for docs deployment is quite brittle because it relies on the exact step name 'Deploy to GitHub Pages'; consider keying off the `uses: peaceiris/actions-gh-pages@v4` action (and maybe an `id`) instead so future copy changes to the name don't break the test.
- In `test_docs_deployment_uses_scoped_github_token`, accessing `build['permissions']['contents']` and the deploy step fields with direct indexing will raise KeyError/StopIteration with a less-clear traceback; wrapping these in helper functions or using `.get` plus explicit assertion messages would make failures easier to interpret when the workflow changes.
## Individual Comments
### Comment 1
<location path=".github/workflows/docs.yml" line_range="17-18" />
<code_context>
build:
needs: quality
runs-on: ubuntu-latest
+ permissions:
+ contents: write
steps:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider tightening permissions scope to the minimum required for the publish step.
Currently the entire `build` job has `contents: write`. Since only the gh-pages publish step needs write access, consider setting `permissions: contents: read` at the workflow level and granting `contents: write` only to that publish step or a dedicated publish job to limit the impact of a compromised step.
Suggested implementation:
```
build:
needs: quality
runs-on: ubuntu-latest
permissions:
contents: read
steps:
- name: Publish docs to GitHub Pages
if: github.ref == 'refs/heads/main'
uses: peaceiris/actions-gh-pages@v4
with:
github_token: ${{ secrets.GH_PAGES_TOKEN }}
publish_dir: ./docs/_build/html
publish_branch: gh-pages
user_name: "hydroroll-bot"
```
1. At the workflow root (top of `.github/workflows/docs.yml`), set default permissions to read-only:
```yaml
permissions:
contents: read
```
so all jobs default to read unless explicitly overridden.
2. Create and configure a fine-grained PAT (e.g., `GH_PAGES_TOKEN`) with the minimum required repository permissions to push to the `gh-pages` branch, and add it as a repository secret.
3. Ensure other jobs in this workflow do not rely on `GH_PAGES_TOKEN`; they should continue to use the default read-only `GITHUB_TOKEN`, keeping write access effectively limited to the publish step.
</issue_to_address>
### Comment 2
<location path="tests/test_quality_gate.py" line_range="66-77" />
<code_context>
self.assertIn("quality", job_needs(changelog["jobs"]["verify"]))
self.assertIn("verify", job_needs(changelog["jobs"]["publish"]))
+ def test_docs_deployment_uses_scoped_github_token(self):
+ workflow = load_workflow("docs.yml")
+ build = workflow["jobs"]["build"]
+ self.assertEqual(build["permissions"]["contents"], "write")
+
+ deploy = next(
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the contract to assert that only the docs deploy job has `contents: write` permissions
The PR description states that only the deploy job should have `contents: write`. This test currently checks that the docs job has `contents: write` but doesn’t ensure that no other jobs in `docs.yml` have this permission. To align the test with the contract, iterate over `workflow["jobs"]` and assert that all non-deploy jobs either omit `permissions.contents` or use a more restrictive value, so future accidental grants of `contents: write` are caught.
```suggestion
def test_docs_deployment_uses_scoped_github_token(self):
workflow = load_workflow("docs.yml")
jobs = workflow["jobs"]
# Only the docs deploy job should have contents: write
for job_name, job in jobs.items():
contents_permission = job.get("permissions", {}).get("contents")
if job_name == "build":
self.assertEqual(
contents_permission,
"write",
f"build job is expected to have contents: write, got {contents_permission!r}",
)
else:
# Non-deploy jobs must not have contents: write
self.assertIn(
contents_permission,
(None, "read"),
f"job {job_name} must not have contents: write, got {contents_permission!r}",
)
build = jobs["build"]
deploy = next(
step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages"
)
self.assertEqual(
deploy["with"]["github_token"],
"${{ secrets.GITHUB_TOKEN }}",
)
```
</issue_to_address>
### Comment 3
<location path="tests/test_quality_gate.py" line_range="71-76" />
<code_context>
+ build = workflow["jobs"]["build"]
+ self.assertEqual(build["permissions"]["contents"], "write")
+
+ deploy = next(
+ step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages"
+ )
+ self.assertEqual(
+ deploy["with"]["github_token"],
+ "${{ secrets.GITHUB_TOKEN }}",
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative assertion that the legacy personal token (e.g. `BOT_TOKEN`) is not referenced anywhere in `docs.yml`
The current test verifies use of `secrets.GITHUB_TOKEN`, but the workflow contract also requires avoiding the legacy personal token. Consider adding an assertion that the workflow definition does not reference `BOT_TOKEN` (or other personal-token secrets) anywhere, e.g. by checking that `"BOT_TOKEN"` is absent from all `env`, `with`, and `secrets` sections. This would strengthen the test as a guardrail against regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+18
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
🚨 suggestion (security): Consider tightening permissions scope to the minimum required for the publish step.
Currently the entire build job has contents: write. Since only the gh-pages publish step needs write access, consider setting permissions: contents: read at the workflow level and granting contents: write only to that publish step or a dedicated publish job to limit the impact of a compromised step.
Suggested implementation:
build:
needs: quality
runs-on: ubuntu-latest
permissions:
contents: read
steps:
- name: Publish docs to GitHub Pages
if: github.ref == 'refs/heads/main'
uses: peaceiris/actions-gh-pages@v4
with:
github_token: ${{ secrets.GH_PAGES_TOKEN }}
publish_dir: ./docs/_build/html
publish_branch: gh-pages
user_name: "hydroroll-bot"
- At the workflow root (top of
.github/workflows/docs.yml), set default permissions to read-only:so all jobs default to read unless explicitly overridden.permissions: contents: read
- Create and configure a fine-grained PAT (e.g.,
GH_PAGES_TOKEN) with the minimum required repository permissions to push to thegh-pagesbranch, and add it as a repository secret. - Ensure other jobs in this workflow do not rely on
GH_PAGES_TOKEN; they should continue to use the default read-onlyGITHUB_TOKEN, keeping write access effectively limited to the publish step.
Comment on lines
+66
to
+77
| def test_docs_deployment_uses_scoped_github_token(self): | ||
| workflow = load_workflow("docs.yml") | ||
| build = workflow["jobs"]["build"] | ||
| self.assertEqual(build["permissions"]["contents"], "write") | ||
|
|
||
| deploy = next( | ||
| step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages" | ||
| ) | ||
| self.assertEqual( | ||
| deploy["with"]["github_token"], | ||
| "${{ secrets.GITHUB_TOKEN }}", | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Strengthen the contract to assert that only the docs deploy job has contents: write permissions
The PR description states that only the deploy job should have contents: write. This test currently checks that the docs job has contents: write but doesn’t ensure that no other jobs in docs.yml have this permission. To align the test with the contract, iterate over workflow["jobs"] and assert that all non-deploy jobs either omit permissions.contents or use a more restrictive value, so future accidental grants of contents: write are caught.
Suggested change
| def test_docs_deployment_uses_scoped_github_token(self): | |
| workflow = load_workflow("docs.yml") | |
| build = workflow["jobs"]["build"] | |
| self.assertEqual(build["permissions"]["contents"], "write") | |
| deploy = next( | |
| step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages" | |
| ) | |
| self.assertEqual( | |
| deploy["with"]["github_token"], | |
| "${{ secrets.GITHUB_TOKEN }}", | |
| ) | |
| def test_docs_deployment_uses_scoped_github_token(self): | |
| workflow = load_workflow("docs.yml") | |
| jobs = workflow["jobs"] | |
| # Only the docs deploy job should have contents: write | |
| for job_name, job in jobs.items(): | |
| contents_permission = job.get("permissions", {}).get("contents") | |
| if job_name == "build": | |
| self.assertEqual( | |
| contents_permission, | |
| "write", | |
| f"build job is expected to have contents: write, got {contents_permission!r}", | |
| ) | |
| else: | |
| # Non-deploy jobs must not have contents: write | |
| self.assertIn( | |
| contents_permission, | |
| (None, "read"), | |
| f"job {job_name} must not have contents: write, got {contents_permission!r}", | |
| ) | |
| build = jobs["build"] | |
| deploy = next( | |
| step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages" | |
| ) | |
| self.assertEqual( | |
| deploy["with"]["github_token"], | |
| "${{ secrets.GITHUB_TOKEN }}", | |
| ) |
Comment on lines
+71
to
+76
| deploy = next( | ||
| step for step in build["steps"] if step["name"] == "Deploy to GitHub Pages" | ||
| ) | ||
| self.assertEqual( | ||
| deploy["with"]["github_token"], | ||
| "${{ secrets.GITHUB_TOKEN }}", |
There was a problem hiding this comment.
suggestion (testing): Add a negative assertion that the legacy personal token (e.g. BOT_TOKEN) is not referenced anywhere in docs.yml
The current test verifies use of secrets.GITHUB_TOKEN, but the workflow contract also requires avoiding the legacy personal token. Consider adding an assertion that the workflow definition does not reference BOT_TOKEN (or other personal-token secrets) anywhere, e.g. by checking that "BOT_TOKEN" is absent from all env, with, and secrets sections. This would strengthen the test as a guardrail against regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BOT_TOKENin docs deployment with the repository-scopedGITHUB_TOKENcontents: writeonly to the deploy jobRoot cause
The v1.3.5 docs build succeeded, but the personal token push to
gh-pageswas rejected because its owning account has no verified email.Validation
Related to #34.
Summary by Sourcery
Use the repository-scoped GitHub token with minimal permissions for docs deployment and enforce this via workflow tests.
New Features:
Enhancements:
CI: