Repository navigation
Authenticate thread reads with the publication credential - #177
Merged
Merged
Conversation
Thread listing and the pre-publish lookup were calling gh with no token. post, threads, and complete-resolutions now send the --token-env PAT, or one App installation token, as GH_TOKEN on every gh call in that run. Co-authored-by: Oleksiy Akimov <oakimov@users.noreply.github.com>
[Medium impact] The reviewed changes appear safe to merge; both previous findings are addressed. Summary
Reviews (2) · Last reviewed commit: "Cover App completion reads and correct t..." · Reviewed by Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
complete-resolutions on the App path now has to use the installation token when a different GH_TOKEN is set. The guide no longer says post mints that token after the final pull read. Co-authored-by: Oleksiy Akimov <oakimov@users.noreply.github.com>
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
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
threadsand the thread lookuppostdoes before publishing calledghwith no token. On a Cloud Agent VM that is an anonymous request, which hit the rate limit (HTTP 403,postexit 4).post,threads, andcomplete-resolutions.pynow choose one credential at the start of the run and send it asGH_TOKENon everyghcall in that process:--token-env NAME: theghp_orgithub_pat_value in that variable. It wins over ambientGH_TOKENandGITHUB_TOKEN.threadsaccepts--token-env. With neither an App key nor--token-env, the command fails beforegh.bindandstartare unchanged revision lookups.The guide's publication paragraph now matches that order: the credential is selected before the first GitHub lookup.
complete-resolutions.pyon the App path is checked with a different ambientGH_TOKEN.Tests
bun run typecheckandbun run buildpassed.bun test: 3413 pass, 0 fail, 13791expect()calls, 126 files. The fakeghrecords a SHA-256 ofGH_TOKENfor every invocation. The tests check the App path and the--token-envpath forthreads, the pre-publish lookup, andcomplete-resolutions.py, including when a different ambient token is set. Token values are not printed.Hunks to port
Head
416e8bd00f9d9c03865cdade8642c5a127de524f. The script lines below are unchanged from366b10d.scripts/review-publish.py:652:_publication_tokenand_publication_required, next to_hidden_env.789publisher_env: explicit token, else_publication_token(801), else the hidden--token-envvalue. All three win over ambientGH_TOKEN/GITHUB_TOKEN. If a publication run is active and the result is still empty (808), exit 4 withNo GitHub credential is available. GitHub was not called.1080begin_publication: callpublication_tokenonce and store it.1101resolve_threads: reuse_publication_tokeninstead of minting again.1725post:begin_publicationbeforethread_inventoryandchecked_thread.1766post_locked: send that same token. Do not mint after the preparatory reads.1967:--token-envis valid forpostandthreads.2004:threadscallsbegin_publicationbeforethread_inventory. Put--token-envafter the owner/repo/pull positionals; argparse drops those positionals if the option comes first.scripts/complete-resolutions.py:82:begin_publicationbeforereviews_of, replacing the PAT-only preamble.resolve_threadsthen uses the same token.test/review-publish.test.ts: the fakeghappendssha256(GH_TOKEN)ormissingtostate/call-authon every invocation. Assertions cover both credentials and reject an anonymous call.docs/pr-review.md: the publication paragraph selects the credential before the first GitHub lookup, then reads the pull request and the pinned approval again immediately before the send.