Skip to content

fix(changeprovider): Reject GitHub change URIs that name another host before calling the API - #1044

Draft
roychying wants to merge 1 commit into
mainfrom
chenghan.ying/changeprovider-github-host
Draft

roychying wants to merge 1 commit into
mainfrom
chenghan.ying/changeprovider-github-host

Conversation

@roychying

Copy link
Copy Markdown
Contributor

Why?

The GitHub change provider ignores the URI host and sends every request to its configured API. A URI naming another GitHub instance therefore fails with a misleading error ("could not resolve to a Repository" or "head SHA changed") instead of the validator's clear host rejection, which never runs because the validate stage calls the change provider first.

What?

  • Params.Host is required, mirroring the GitHub validator's Params.Host. NewProvider errors when it is missing.
  • validateChangeConsistency requires every URI to name that host before any API call, which also rejects a stack spanning hosts (previously only scheme, org and repo were compared).
  • The orchestrator server's github change provider block gains host, defaulting to github.com like the validator's.
  • The demo GitHub profile's GitHub Enterprise hint includes host.

Breaking: NewProvider now returns (changeprovider.ChangeProvider, error), and callers must pass Host.

Test Plan

  • TestValidateChangeConsistency: new cases for a PR on another host, a stack spanning hosts, and a port the provider does not name.
  • TestNewProvider: a missing host errors.
  • TestProvider_Get: a wrong-host URI errors without any request reaching the server.
  • bazel test //submitqueue/extension/changeprovider/... //service/submitqueue/orchestrator/... passes; make lint and make check-gazelle pass.

Issue

This branch has not been deployed

No deployments
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.

1 participant