Unsets Persisted Credentials #587
Conversation
|
CC: @Pike |
Pike
left a comment
There was a problem hiding this comment.
Looks good to me.
It's a bit unfortunate to dive so deep under the hood, but I don't have a better idea right now.
|
Yeah I don't feel good about this change, but I'm not sure what the best way is to manage user expectations here. There's a thread about this here which may reveal some better ideas: actions/checkout#162 It seems like it's possible to use credential manager and Git will always prefer it over the |
|
I found the same conversation ;-) It also mentions that the credential manager would be brittle. Which may or may not be an issue for us. The other idea I have in my head is to only use a worktree if we're re-using auth for in-repo pushes, and to set up a vanilla git repo in all other cases. And to then config in the temporary repo instead of inside the workspace. That would also help workflows that don't check out a top-level repo at all. At this point, that might be more something for post-v4? |
|
Yeah post v4 for sure, for now I think this change is fine and should handle 99% of use cases. I think I'm going to add an integration test to handle this case so we can keep track of this to ensure that the unsetting doesn't break in later versions of action/checkout. I'll update the PR before merging. |
Codecov Report
@@ Coverage Diff @@
## dev-v4 #587 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 208 218 +10
Branches 52 55 +3
=========================================
+ Hits 208 218 +10
Continue to review full report at Codecov.
|
* Stop checking out workspace (#515) * Stop checking out base branch before deployment, drop option. * Don't check out default branch, as we don't check out base branch, drop option. * Don't stash/unstash as we don't update the workdir, drop preserve option. * Don't init the workspace * Only fetch the remote branch if it exists, only with depth 1. * Rely on previous checkouts to have handled lfs files correctly, drop option. * Update README, action.yml, integration tests * Set up eslint for test files. (#517) * Add DRY_RUN option, passing --dry-run to git push. (#526) See #499 for the proposal. * Simplifies Token Setup (#530) * Token simplification * Access Token / Github Token -> Token * Oops * Typos * Update README.md * Update README.md * Update action.yml Co-authored-by: Axel Hecht <axel@pike.org> * Update README.md Co-authored-by: Axel Hecht <axel@pike.org> * Update README.md Co-authored-by: Axel Hecht <axel@pike.org> * Adjust codeql action to latest recommendations (#540) Also, add the dev and release branches, and drop master. * Add workflow to update build and node_modules on release branches (#541) * Stores username/email in secrets * Removing stale bot integration * Test current code base as an integration test for PRs and pushes (#505) * Add a build step to create lib and node_modules artifact * Run integration test with built dist and current SHA as base For pull requests, the github.sha is the sha of the merge to the target branch, not the head of the PR. Special case that. * Use v2 checkout, and DRY_RUN for the integration test. I also made the branches more generic, as there are now more of them. * Fix #536, don't push at all on dryRun Also add tests for dryRun and singleCommit and generateBranch code flows. * Try to fix dryRun on new remote branches, refactor fetch * Try to fix dryRun, only fetch if origin branch exists * Refactor worktree setup to include branch generation and setup for singleCommit This is a continuation of the no-checkout work, and sadly suggested pretty intensive changes. * Set up git config to fix tests, also make debugging easier * Add matrix for existing and non-existing branch * Add matrix for singleCommit and not * Drop GITHUB_TOKEN, add DRY_RUN to action.yml * When deploying existing branch, add a modifcation and deploy again * Force branch checkout to work in redeployment scenarios * Make singleCommit easier to see in job descriptions * Review comments * Add a test-only property to action to test code paths with remote branch. * Introduce TestFlag enum to signal different test scenarios to unit tests * Fix util.test.ts * Update worktree.ts * Fix a few nits in tests and automation. Don't try to wordcount ls-rem… (#546) * Fix a few nits in tests and automation. Don't try to wordcount ls-remote. Nits in tests are around undoing changes made to the environment, and to not modify the checkout. * Describe suite with empty SHA * Lowercase Inputs (#547) * Lowercases inputs * Adjusts workflow tests and deployment_status * Use multi-line string for clean-exclude patterns. (#553) As this change is subtle, I'm taking the opportunity to change the underscore for the hyphen, which makes it less likely that users of this action will just pass in an old json array. * Hyphenate inputs and outputs, add step output, fix #558 (#559) * Hyphenate inputs and outputs, add step output, fix #558 I've also tried to make the clean docs a bit clearer, and consistent about clean being on my default. Still not totally happy with the intro of the docs there, though. * Add testing of step outputs to build integration tests * Security Docs * Integration tests * Revert "Integration tests" This reverts commit 639ff53. * Native SSH Key Support (#569) * SSH Key Support 🔑 * Update ssh.ts * Update src/ssh.ts Co-authored-by: Axel Hecht <axel@pike.org> * README fixes/etc * Unit Tests & README * ssh key * Update README.md * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update integration.yml Co-authored-by: Axel Hecht <axel@pike.org> * Deployment Issues (#583) * Update git.ts * Tests * Update git.ts * Formatting * Update src/git.ts Co-authored-by: Axel Hecht <axel@pike.org> * TestFlag * Logging * Update git.ts Co-authored-by: Axel Hecht <axel@pike.org> * Codespace Support (#584) * Add files via upload * Update README.md * Add files via upload * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md * SSH Issues (#588) * Unsets Persisted Credentials (#587) * Persist * Config Setup/Tests * Assets * Update git.ts * Spacing * Update integration.yml * Update README.md Co-authored-by: Axel Hecht <axel@pike.org>
Description
This PR resolves an issue with persisted-credentials from the
actions/checkoutstep. Before we were instructing users to always setpersist-credentials: falsein another action, which was quite confusing to a lot of users. This was necessary because it bakes the authentication into the git configuration.This issue resolves #507 #506
With this change we check to ensure the user is running in a CI setting and not using SSH. If so, we strip the key that is set by
actions/checkout,http.https://github.com/.extraheader.Testing Instructions
persist-credentialstofalseinactions/checkout. You'll get an error saying it does not have permission because it defaults to the configuration used in the Git config which will attempt to use the GitHub token and not the PAT.releases/v4-beta, don't setpersist-credentials, it will deploy without issue.Additional Notes
The last issue which needs finishing before v4 can go live is #586 which I'm struggling to debug. The art assets are also not finalized, and will be done tonight.