Skip to content

Integrate URL allow-list uses raw string prefixes and can be spoofed #358

Open

Description

@sai5880

Summary

git.integrate validates the repository URL taken from migration metadata against the configured allowed_ref_prefixes using raw String.startsWith, with no scheme, host, port, or path boundary. The destination check correctly requires exact equality or a repoUrl + "/" prefix, but the allow-list check only requires that the attacker-controlled URL start with one of the configured strings. As a result a crafted integrate URL can pass the allow-list check while pointing at a different host, a sibling repository, or any URL at all when an empty prefix is configured.

Location

  • java/com/google/copybara/git/IntegrateLabel.java:59-76 (generic git integrate label check)
  • java/com/google/copybara/git/GitHubPrIntegrateLabel.java:126-127 (GitHub PR integrate label, same pattern)
  • java/com/google/copybara/git/GitModule.java:493-553 (allowed_ref_prefixes parameter definition and construction)

Details

Current code at HEAD 16e5b15 (IntegrateLabel.java:58-76):

String repoUrlPrefix = repoUrl.endsWith("/") ? repoUrl : repoUrl + "/";
boolean matchesDestination = labelUrl.equals(repoUrl) || labelUrl.startsWith(repoUrlPrefix);
if (!matchesDestination) {
  if (allowedRefPrefixes.isEmpty()) {
    throw new ValidationException(...);
  }
  ValidationException.checkCondition(
      allowedRefPrefixes.stream().anyMatch(labelUrl::startsWith),
      "Integrate URL in '%s' does not match destination repository URL '%s' or any allowed"
          + " prefix (%s).",
      rawLabel, repoUrl, allowedRefPrefixes);
}

The destination branch builds repoUrlPrefix with a trailing slash and requires either exact equality or a slash-terminated prefix, so it has a path boundary. The allow-list branch does not: it calls labelUrl::startsWith directly with each raw configured string.

The GitHub PR variant has the same weakness (GitHubPrIntegrateLabel.java:126-127):

if (allowedRefPrefixes.stream().anyMatch(label.gitHubIdentifier.getUrl()::startsWith)
    || label.matchesDestinationUrl(repoUrl)) {
  return Optional.of(label);
}

GitModule.java:493-553 defines allowed_ref_prefixes as a Sequence of strings and only runs Sequence.cast(allowedRefPrefixes, String.class, ...) in integrate(). There is no normalization, no URL parsing, and no rejection of empty or whitespace-only entries.

Three bypass examples, assuming an operator configures the prefix as shown:

  1. Prefix https://github.com also matches https://github.com.evil.example/owner/repo.
  2. Prefix https://github.com/google also matches https://github.com/google-evil/repo (and https://github.com/googleanything).
  3. Prefix "" (empty string) matches every URL via startsWith(""), so the restriction is silently removed.

The integrate label value comes from migration metadata (for example the COPYBARA_INTEGRATE_REVIEW label carried in the change message), so the author of the upstream change can influence the value that is checked. This is a static code review observation, not a runtime exploit, and no proof-of-concept execution was performed.

Impact

If an operator relies on allowed_ref_prefixes to limit which repositories git.integrate may fetch from, a crafted integrate URL can satisfy the check while resolving to a different host or a sibling repository. The exact practical impact depends on the configured prefix and on what credentials the Copybara process has available when it fetches the URL:

  • A host-suffix prefix such as https://github.com permits look-alike hosts (example 1).
  • A repository prefix such as https://github.com/google permits similarly named repositories (example 2).
  • An empty prefix disables the allow-list entirely (example 3).

Severity therefore depends on configuration, and is highest for deployments that configure broad or empty prefixes and run with credentials that can reach unintended repositories.

Suggested fix

Parse each configured prefix once at config load and validate/normalize it:

  • Reject empty or whitespace-only prefixes in GitModule.integrate / GitIntegrateChanges and fail config loading with a clear ValidationException or EvalException.
  • Parse each prefix as a URL, require an absolute URL with a scheme and host, and reject user-info, query, and fragment components.
  • Compare the candidate URL against each prefix by scheme, host, and port for exact equality, and compare the path on a slash boundary (equal, or prefix path followed by /).
  • Apply the same helper to both IntegrateLabel.genericGitRevision and GitHubPrIntegrateLabel.maybeParse so the two paths cannot diverge.
  • Optionally, add a trailing-slash requirement for prefixes whose path is non-empty, mirroring the existing destination logic.

Verification

Static review only:

  • Confirmed HEAD is 16e5b15a3a14191812537c9221e6b95890773f88 and read the code paths above directly from the clone.
  • Confirmed allowed_ref_prefixes is only type-cast (Sequence.cast) and never validated or normalized before use.
  • Reasoned through the three bypass examples against String.startsWith semantics; no code was executed and no live repository was contacted.

A regression test could call IntegrateLabel.genericGitRevision (or a small extracted URL-matching helper) with prefixes and labels from the examples above and assert that all three crafted values are rejected once the fix is in place.

Activity

  1. hsudhof commented on Oct 21, 2026

    @hsudhof
    Collaborator

    As the allowlist is configured, this would require the owner of the migration to set a prefix that allows integrating a different repository. More importantly, the integrate feature is specifically meant to merge in pending changes - it is up to the owner to review integrate commands before allowing them to be run. The normal rountrip is PullRequest -> Pending change in user repo via ChangeRequest -> Push to external repo with integrate.
    Thus, normally the integrate information is set by copybara in the import to the internal repository and is thus known to be good. An issue could happen where users do not know about integrate and accidentally allow the feature to be active in a setup without round-trips. Ultimately, in a setup as scriptable as copybara, this is a user issue; it is also entirely possible to add additional safeguards in starlark or to disable integrates altogether.

  2. sai5880 commented on Oct 22, 2026

    @sai5880
    Author

    Thanks for the context. I agree the integrate flow assumes a reviewed pending change and that a deliberately broad prefix is an operator decision. The part I would still like to harden in PR #359 is the mismatch between what a prefix appears to mean and what it actually matches:

    • https://github.com also matches https://github.com.evil.example
    • https://github.com/google also matches https://github.com/google-evil
    • allowed_ref_prefixes = [""] matches every URL

    The destination check already uses a slash boundary (repoUrl + "/"), so the allow-list path is the inconsistent one. PR #359 keeps legitimate prefixes working and only rejects empty entries and matches on URL boundaries. It does not change the review or trust model.

    Would you be open to the PR as a hardening change? If you think the trust model means it should not be merged, I can close it and instead document that allowed_ref_prefixes is a raw string prefix with no normalization, so operators know to include a trailing slash. I can also split out just the empty-prefix validation if you prefer a narrower change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions