Tools: Preview was using main (still) #143
Conversation
|
📚 Documentation Preview ✅ A preview of the documentation changes in this PR is available for maintainers at: Last updated: 2025-01-24 03:19 UTC |
codeallthethingz
added
infrastructure
.github/workflows/docs_preview.yml
Outdated
| ref: ${{ github.event.pull_request.merge_commit_sha }} | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| sparse-checkout-cone-mode: false | ||
| sparse-checkout: | |
There was a problem hiding this comment.
issue (non-blocking): I don't think the size of tbp.monty is large enough to matter whether we check out all of it or some of it. And if it is just as quick to checkout the whole repository, then we could get by without having another place to manage a file list.
| - name: Deploy docs | ||
| working-directory: tbp.monty | ||
| run: | | ||
| export PATH="$HOME/miniconda/bin:$PATH" | ||
| source activate tbp.monty | ||
| export README_API_KEY=${{ secrets.README_API_KEY }} | ||
| export IMAGE_PATH=${{ vars.IMAGE_PATH }} | ||
| python -m tools.github_readme_sync.cli upload docs "${{ steps.preview_info.outputs.monty_version }}-${{ steps.preview_info.outputs.branch_name }}" | ||
|
|
||
| python -m tools.github_readme_sync.cli upload ../pr_docs/docs "${{ steps.preview_info.outputs.monty_version }}-${{ steps.preview_info.outputs.branch_name }}" |
There was a problem hiding this comment.
question: I don't understand where ../pr_docs/docs comes from. Why does this work?
There was a problem hiding this comment.
it's in the sparse checkout from above.
The flow is
- check out the main branch using checkout action into
~/tbp.monty/ - check out the PR's branch using the head reference into
~/pr_docs/(was using sparse checkout but could be the entire project as you suggest) - run the tool from the main branch code, but upload the docs from the PR branch.
../pr_docs/docs
Thinking about it last night, this does put us at some risk of markdown/regex attacks as the PR could introduce some documentation that our regex parsing/markdown lib doesn't deal with very well and explodes. https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS for example.
There was a problem hiding this comment.
actually, ignore that, the code has gone... Lemme find it.
There was a problem hiding this comment.
ok! That was embarrassing. The code is now back @tristanls
There was a problem hiding this comment.
note: Now I see what's going on 🙂!
There was a problem hiding this comment.
Looks good to me.
note: As you pointed out, there's likely security nuance with checking out a pull request branch and having README_API_KEY secret available. At first glance, it appears that per your indication ReDoS is possible, followed by some wider escape depending on github_readme_sync tool behavior as well as if anything fancy happens on checkout of a PR. Lastly, it could be an attack vector on Maintainers who open the preview in the browser.
codeallthethingz
merged commit 607505a
into
thousandbrainsproject:main
Turns out
github.event.pull_request.merge_commit_shawas not uploading the docs from the new branch, so I tried another method of only checking out the docs folder on the branch of the PR using sparse checkout.