What should gh do if gitcreds_get() returns a password that is not a PAT?

[gaborcsardi]

• Error?
• Warn?

Sending a password that is not a PAT can be a security issue, although we are sending it to the very same host we would send the PAT to, so maybe not a very serious issue?

Also, at the time of writing we don't know any situation when sending a non-PAT password in the Authorization header would do anything useful.

So probably we should error?

[jennybc]

gh's validation around the gh_pat class would catch a lot of non-PAT creds. You probably already thought of that.

[gaborcsardi]

Yeah, AFAICT right now you get a silent "no pat". Should this be an error or a warning instead?

[jennybc]

Although it's part of a user-facing API for putting creds into the store, credentials certainly tries to catch this case:

https://github.com/r-lib/credentials/blob/c86ae4b43771c5096a45265bbe4006f69a30237f/R/github-pat.R#L41

and even goes on to reject that cred (presumably to make sure there's no entry with a real username + a PAT in the store).

[gaborcsardi]

I think it would reasonable to do that in gh and usethis, when you need a GH PAT. So maybe gh should have a set_pat() function?

But the git credential store is actually used for a lot of things already, not just PATs, so I don't think gitcreds should have such restrictions.

[jennybc]

So maybe gh should have a set_pat() function?

Yes, I think so. I think usethis may also need one. I think any package that pulls from the store potentially needs to offer a humane way to put something there as well. You can, of course, recommend a function in another package for this. But with usethis, for example, we generally try to provide lots of scaffolding and make usethis feel like the storefront.

[gaborcsardi]

I think any package that pulls from the store potentially needs to offer a humane way to put something there as well.

Yeah, gitcreds_set() can put anything there. The question is whether to have one specialized to PATs.

[jennybc]

I think our current gh_pat validation would be a good bare minimum for a function in gh and/or usethis that helps a user store a PAT.

gh/R/gh_token.R

Lines 63 to 84 in d41174a

	# gh_pat class: exists in order have a print method that hides info ----
	new_gh_pat <- function(x) {
	if (is.character(x) && length(x) == 1) {
	structure(x, class = "gh_pat")
	} else {
	throw(new_error("A GitHub PAT must be a string"))
	}
	}
	# validates PAT only in a very narrow, technical, and local sense
	validate_gh_pat <- function(x) {
	stopifnot(inherits(x, "gh_pat"))
	if (x == "" || grepl("[[:xdigit:]]{40}", x)) {
	x
	} else {
	throw(new_error("A GitHub PAT must consist of 40 hexadecimal digits"))
	}
	}
	gh_pat <- function(x) {
	validate_gh_pat(new_gh_pat(x))
	}

Which, I think, suggests that gh should offer this and usethis should recommend or wrap that. Because the gh_pat validation stuff won't be exported.

[gaborcsardi]

Closed wrong issue....

[gaborcsardi]

@jennybc Do you remember if we wanted to do anything more about this? Or it is fine as it is now?

[jennybc]

I have definitely heard of people who enter a password when asked for a PAT, then are confused when things don't work. So I'm still a general fan of trying to catch this and emit a message that suggests what might be wrong.

[Sumidu]

I was really confused about this error.
Since github changed the format of the PATs creating a new one does not create a legal PAT for gh. However, updating gh helped. It was confusing for me (and others) because updating usethis and git_creds did not help. Which were the only packages we were actively using.

TLDR: update gh, git_creds, and usethis