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.

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

Fix unowned plugin reference #1733

Merged
merged 2 commits into from Aug 3, 2022
Merged

Fix unowned plugin reference #1733

merged 2 commits into from Aug 3, 2022

Conversation

moskyb
Copy link
Contributor

@moskyb moskyb commented Aug 1, 2022

We had a report on hackerone (only available to buildkite staff unfortunately!), reporting that one of our plugin definitions in the plugin tests refers to a org/repo combo that doesn't belong to buildkite.

While this currently poses no security risk as we don't actually clone or run the plugins as part of the tests, it's not out of the question that we might run/clone plugins as part of tests in the future, so we should change this reference to something that we own, just in case.

@moskyb moskyb requested a review from a team August 1, 2022 00:38
`["github.com/buildkite-unofficial/ping#master"]`,
[]*Plugin{&Plugin{
Location: `github.com/buildkite-unofficial/ping`,
`["github.com/buildkite-plugins/fake-plugin#master"]`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the agent have special behaviour for handling plugins in the github.com/buildkite-plugins org vs. plugins in third party GH orgs?

I assume this is OK because none of the tests are checking the special buildkite-plugins form (docker-compose#v3.7.0), but I thought it was worth checking.

Copy link
Member

@sj26 sj26 Aug 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's the purpose of this test — it should not be in buildkite or buildkite-plugins.

Copy link
Contributor Author

@moskyb moskyb Aug 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good call! in that case, could someone with github admin superpowers create a buildkite org that's neither buildkite nor buildkite-plugins, and then we could use that?

edit: see paul's second comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to register another GitHub org for testing this?
Or paramerise some test things so that we can use an existing one that we do already control?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the agent have special behaviour for handling plugins in the github.com/buildkite-plugins org vs. plugins in third party GH orgs?

TIL this happens server-side, not agent-side.

    case paths.length
    when 1
      "github.com/buildkite-plugins/#{add_plugin_suffix(paths.first, uri.fragment)}"
    when 2
      "github.com/#{paths.first}/#{add_plugin_suffix(paths.last, uri.fragment)}"
    else
      source
    end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(So, I think it's fine that we use github.com/buildkite-plugins for testing, unless we're really worried about future-proofing it against future agent-side handling of this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Maybe you could use this demo org for now?

https://github.com/hashikite

Copy link
Contributor

@tessereth tessereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make the executive decision to approve this and leave it up to @moskyb whether you want to change the url based on the comments.

@moskyb
Copy link
Contributor Author

moskyb commented Aug 3, 2022

i might leave it as is for now. if we move the repo-handling agentside, we can update this test.

@moskyb moskyb merged commit 2822430 into main Aug 3, 2022
@moskyb moskyb deleted the fix-unowned-plugin-reference branch August 3, 2022 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants