Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Secret created in Upstream Repo instead of Current Repo #4688

Open
brwilkinson opened this issue Nov 5, 2021 · 14 comments · May be fixed by #9083
Open

Secret created in Upstream Repo instead of Current Repo #4688

brwilkinson opened this issue Nov 5, 2021 · 14 comments · May be fixed by #9083
Labels
bug Something isn't working core This issue is not accepting PRs from outside contributors gh-secret relating to the gh secret command p1 Affects a large population and inhibits work

Comments

@brwilkinson
Copy link

CLI Feedback

When creating secrets, they were unexpectantly created in an Upstream repo.

gh repo view
myrepo123/MyApp123

$Secret | gh secret set $SecretName

gh secret list

> AEU2_MY_Secret123  Updated 2021-09-15

gh secret list -R myrepo123/MyApp123

> 

gh secret list  -R upstream123/MyApp123

> AEU2_MY_Secret123  Updated 2021-11-05

So this means that when creating a secret I have to do the following ?

$repo = git config --get remote.origin.url
$Secret | gh secret set $SecretName -R $repo
gh secret list -R $repo

> AEU2_MY_Secret123  Updated 2021-11-05

I would prefer it the secret was created in the Repo that I am currently in?!
@jojorgeluisrdz198

This comment was marked as spam.

Sign in to view
@jojorgeluisrdz198

This comment was marked as spam.

Sign in to view
@brwilkinson brwilkinson changed the title Secrets created in Upstream Repo instead of Current Repo Secret created in Upstream Repo instead of Current Repo Nov 9, 2021
@mislav
Copy link
Contributor

mislav commented Nov 9, 2021

@brwilkinson Sorry that this wasn't more clear. GitHub CLI has a concept of a "base repository" where most gh operations should be targeted to, and by default the base repository is the parent of the fork. This isn't always desirable and the way to override it is using -R, like you've discovered, but this can be tedious.

Like you pointed out, we should just default to the origin remote when setting secrets to remove this magic behavior which makes more sense for querying issues or pull requests.

Ref. #3304

@brwilkinson
Copy link
Author

Thanks @mislav

Like you pointed out, we should just default to the origin remote when setting secrets to remove this magic behavior which makes more sense for querying issues or pull requests.

Yes that makes the most sense, I haven't tested it very much, however I may not have access to upstream on a fork that I am working on anyway, I guess the secret creation may fail in that case?

Anyway, if this is a duplicate feel free to close as such. In the meantime, I will continue to use the workaround.

@TimoKramer
Copy link

This is bad and should be resolved. It makes my CI create releases on the upstream repo where I don't want it because I am only testing it on my personal fork. This should not happen. It should default to origin I think and be an explicit option and documented on the help.

TimoKramer pushed a commit to TimoKramer/github-cli-orb that referenced this issue Nov 29, 2021
Bugfix for CircleCI-Public#12 Base is chosen implicitely
2ffd842 
This fix uses the hidden --release flag of the github cli tool to
create a release on the repository on which the pipeline is running
and not the one that it was forked from. The github cli tool asks
in interactive mode where to create the release but defaults to
the upstream which can lead to an unwanted official release during
testing as it happened to me.

With this fix this tool will default to origin which I assume is the only
sensible approach in a pipeline because origin presumably is the
one that the pipeline is running on.

- cli/cli#4688
- Closes CircleCI-Public#12
@TimoKramer TimoKramer mentioned this issue Nov 29, 2021
Merged
@jdubois
Copy link

jdubois commented Dec 2, 2021

This is super confusing, and could cause serious security issues. I was creating secrets in my fork (on my personal fork), and the secrets got uploaded to the original repo (which belongs to my company).
As I'm using those secrets to deploy to Azure, it means that my company repo started to deploy automatically to my personal Azure account, which is bad in so many ways!

TimoKramer pushed a commit to TimoKramer/github-cli-orb that referenced this issue Dec 2, 2021
Use the current repo as base
4a111d5 
This fix uses the hidden --release flag of the github cli tool to
create a release on the repository on which the pipeline is running
and not the one that it was forked from. The github cli tool asks
in interactive mode where to create the release but defaults to
the upstream which can lead to an unwanted official release during
testing as it happened to me.

With this fix this tool will default to origin which I assume is the only
sensible approach in a pipeline because origin presumably is the
one that the pipeline is running on.

- cli/cli#4688
- Closes CircleCI-Public#12
@mislav
Copy link
Contributor

mislav commented Dec 2, 2021 •
edited

Thanks for the feedback, everyone. Since this is tripping up a lot of folks, I will re-label this as a bug so we can handle it accordingly.

To clarify: if you were operating from the context of a GitHub fork (e.g. either there is a single origin remote pointing to your fork or there are upstream and origin git remotes pointing to the parent repo and your fork, respectively), should the default for setting secrets and creating releases always be origin, unconditionally? Or should gh interactively ask you to choose once or each time? Finally, if you wanted to be able to pass a flag to switch between git remotes, how would you imagine the interface for this to be? thank you 🙇

Note that right now you can use gh secret set -R owner/repo ... as a workaround.

@mislav mislav added bug Something isn't working discuss Feature changes that require discussion primarily among the GitHub CLI team p1 Affects a large population and inhibits work and removed feedback labels Dec 2, 2021
jdubois added a commit to jdubois/NubesGen-2 that referenced this issue Dec 2, 2021
@jdubois
Correctly set the GitHub repository where secrets are configured
b6c7284 
See cli/cli#4688
@jdubois
Copy link

jdubois commented Dec 2, 2021

Thank you @mislav - for me this should always be the origin by default (see my commit just above on my own project).
Then you can keep the -R option for people who need some customisation: I would basically keep this the way it is, and just change that the default is now the origin.

@bchadwic bchadwic mentioned this issue Dec 6, 2021
Merged
KyleTryon pushed a commit to CircleCI-Public/github-cli-orb that referenced this issue Dec 15, 2021
@TimoKramer
[semver:patch] Use the current repo as base (#15)
75caf8f 
This fix uses the hidden --release flag of the github cli tool to
create a release on the repository on which the pipeline is running
and not the one that it was forked from. The github cli tool asks
in interactive mode where to create the release but defaults to
the upstream which can lead to an unwanted official release during
testing as it happened to me.

With this fix this tool will default to origin which I assume is the only
sensible approach in a pipeline because origin presumably is the
one that the pipeline is running on.

- cli/cli#4688
- Closes #12

Co-authored-by: Timo Kramer <info@lambdaforge.io>
@darcyclarke darcyclarke removed the discuss Feature changes that require discussion primarily among the GitHub CLI team label Jan 19, 2022
@mislav
Copy link
Contributor

mislav commented Jan 19, 2022

Thank you for everyone's patience while we discuss this. We decided to remove the "magic" repo resolution from gh secret set of commands in favor of the following behavior:

  • The local repo has exactly 1 git remote: use the repo named by the git remote;
  • The local repo has more than 1 git remote: error out, list the available repos, and force the user to use the --repo flag to specify exactly which repo they want to operate on.

We believe that such approach would eliminate ambiguity and make it hardly possible for a user to accidentally send secrets to the wrong repository.

@mislav mislav added the core This issue is not accepting PRs from outside contributors label Jan 19, 2022
@mislav mislav removed their assignment Jun 9, 2023
@andyfeller andyfeller added the gh-secret relating to the gh secret command label Oct 2, 2023
@samcoe samcoe mentioned this issue Nov 7, 2023
Closed
@wingleung
Copy link

@samcoe @andyfeller is this open for contributions?

@andyfeller
Copy link
Contributor

@wingleung : open to revisiting this with @williammartin given that all the former GitHub CLI maintainers involved have left GitHub and moved onto other pursuits.

Where is the actual bug?

Looking at #4688 (comment), the following logic for resolving the base repository is blindly picking the first remote:

cli/pkg/cmd/secret/set/set.go

Lines 168 to 197 in 4b077da

func setRun(opts *SetOptions) error {
secrets, err := getSecretsFromOptions(opts)
if err != nil {
return err
}
c, err := opts.HttpClient()
if err != nil {
return fmt.Errorf("could not create http client: %w", err)
}
client := api.NewClientFromHTTP(c)
orgName := opts.OrgName
envName := opts.EnvName
var host string
var baseRepo ghrepo.Interface
if orgName == "" && !opts.UserSecrets {
baseRepo, err = opts.BaseRepo()
if err != nil {
return err
}
host = baseRepo.RepoHost()
} else {
cfg, err := opts.Config()
if err != nil {
return err
}
host, _ = cfg.Authentication().DefaultHost()
}

cli/pkg/cmd/factory/default.go

Lines 47 to 55 in 4b077da

func BaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) {
return func() (ghrepo.Interface, error) {
remotes, err := f.Remotes()
if err != nil {
return nil, err
}
return remotes[0], nil
}
}

Rather than a change in BaseRepoFunc logic, this issue would need to check the number of remotes in both gh variable set and gh secret set as described.

@wingleung wingleung linked a pull request May 14, 2024 that will close this issue
Open
@wingleung
Copy link

another alternative could be to get the remote from the current branch

➜ git rev-parse --abbrev-ref --symbolic-full-name @{u}
origin/add-remote-check-to-secret-and-variable

☝️ we now know the local branch is tracking origin

still kind of seems like magic so I'm leaning towards being more verbose and forcing the user to provide a --repo flag

@andyfeller
Copy link
Contributor

another alternative could be to get the remote from the current branch

➜ git rev-parse --abbrev-ref --symbolic-full-name @{u}
origin/add-remote-check-to-secret-and-variable
☝️ we now know the local branch is tracking origin

still kind of seems like magic so I'm leaning towards being more verbose and forcing the user to provide a --repo flag

I like the thinking outside of the box while recognizing its magical nature might clash with security concerns of setting secrets in the appropriate place ❤

@wingleung
Copy link

wingleung commented May 15, 2024 •
edited

🙏 come to think of it, handling secrets and variables is a repo specific thing, not a branch specific one. So depending on branches might not be a good idea here. Especially for cases where the user creates new local branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working core This issue is not accepting PRs from outside contributors gh-secret relating to the gh secret command p1 Affects a large population and inhibits work
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP add remote check to secret and variable commands wingleung/cli
8 participants
@mislav @jdubois @darcyclarke @wingleung @andyfeller @brwilkinson @TimoKramer @jojorgeluisrdz198