From 2fcede9052a306ee1c61af39226d84c97f17e0b6 Mon Sep 17 00:00:00 2001 From: sai5880 Date: Fri, 23 Oct 2026 00:54:39 +0530 Subject: [PATCH] Validate integrate URL prefixes on URL boundaries Match allowed integrate prefixes on URL boundaries instead of raw string prefixes. Reject empty prefixes and enforce scheme, host, port and path boundaries. --- .../copybara/git/GitHubPrIntegrateLabel.java | 2 +- java/com/google/copybara/git/GitModule.java | 12 +- .../google/copybara/git/IntegrateLabel.java | 122 +++++++++++- .../git/GitHubPrIntegrateLabelTest.java | 174 ++++++++++++++++++ 4 files changed, 307 insertions(+), 3 deletions(-) diff --git a/java/com/google/copybara/git/GitHubPrIntegrateLabel.java b/java/com/google/copybara/git/GitHubPrIntegrateLabel.java index d4739b511..2330d6309 100644 --- a/java/com/google/copybara/git/GitHubPrIntegrateLabel.java +++ b/java/com/google/copybara/git/GitHubPrIntegrateLabel.java @@ -123,7 +123,7 @@ static Optional maybeParse( if (label == null) { return Optional.empty(); } - if (allowedRefPrefixes.stream().anyMatch(label.gitHubIdentifier.getUrl()::startsWith) + if (IntegrateUrlPrefixes.matchesAny(label.gitHubIdentifier.getUrl(), allowedRefPrefixes) || label.matchesDestinationUrl(repoUrl)) { return Optional.of(label); } diff --git a/java/com/google/copybara/git/GitModule.java b/java/com/google/copybara/git/GitModule.java index c95f1dd74..a6e7c5951 100644 --- a/java/com/google/copybara/git/GitModule.java +++ b/java/com/google/copybara/git/GitModule.java @@ -543,6 +543,16 @@ public GitIntegrateChanges integrate( Boolean allowProtocolRef, Sequence allowedRefPrefixes) throws EvalException { + ImmutableList prefixes = + ImmutableList.copyOf( + Sequence.cast(allowedRefPrefixes, String.class, "allowed_ref_prefixes")); + for (String prefix : prefixes) { + try { + IntegrateUrlPrefixes.validateAllowedRefPrefix(prefix); + } catch (ValidationException e) { + throw new EvalException(e.getMessage(), e); + } + } return new GitIntegrateChanges( label, stringToEnum("strategy", strategy, Strategy.class), @@ -550,7 +560,7 @@ public GitIntegrateChanges integrate( allowUnrelatedHistory != null && allowUnrelatedHistory, new LabelTemplate(mergeCommitMessage), allowProtocolRef != null && allowProtocolRef, - Sequence.cast(allowedRefPrefixes, String.class, "allowed_ref_prefixes")); + prefixes); } @SuppressWarnings("unused") diff --git a/java/com/google/copybara/git/IntegrateLabel.java b/java/com/google/copybara/git/IntegrateLabel.java index 8ff80e77b..1f19abda2 100644 --- a/java/com/google/copybara/git/IntegrateLabel.java +++ b/java/com/google/copybara/git/IntegrateLabel.java @@ -16,6 +16,7 @@ package com.google.copybara.git; +import com.google.common.base.Ascii; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.copybara.ChangeMessage; @@ -23,6 +24,8 @@ import com.google.copybara.LabelFinder; import com.google.copybara.exception.RepoException; import com.google.copybara.exception.ValidationException; +import java.net.URI; +import java.net.URISyntaxException; import java.util.List; import java.util.Optional; @@ -67,7 +70,7 @@ static IntegrateLabel genericGitRevision( rawLabel, repoUrl)); } ValidationException.checkCondition( - allowedRefPrefixes.stream().anyMatch(labelUrl::startsWith), + IntegrateUrlPrefixes.matchesAny(labelUrl, allowedRefPrefixes), "Integrate URL in '%s' does not match destination repository URL '%s' or any allowed" + " prefix (%s).", rawLabel, @@ -120,3 +123,120 @@ static String withLabels(String msg, ImmutableList labelsToAdd) return result.toString(); } } + +/** + * Matches an integrate label URL against the URL prefixes allowed by the configuration. + * + *

Matching is done on URL boundaries instead of plain string prefixes, so a prefix of {@code + * https://github.com} does not allow {@code https://github.com.evil.example}, and a prefix of + * {@code https://github.com/google} does not allow {@code https://github.com/google-evil}. + */ +final class IntegrateUrlPrefixes { + + private IntegrateUrlPrefixes() {} + + /** Returns true if {@code url} is contained in at least one of {@code allowedRefPrefixes}. */ + static boolean matchesAny(String url, Iterable allowedRefPrefixes) + throws ValidationException { + for (String prefix : allowedRefPrefixes) { + if (matches(url, prefix)) { + return true; + } + } + return false; + } + + /** + * Returns true if {@code url} is contained in {@code prefix} on a URL boundary. Both values are + * parsed as URIs, and the scheme, host, effective port and path are compared. + */ + static boolean matches(String url, String prefix) throws ValidationException { + URI urlUri = parseUrl(url); + URI prefixUri = parseAllowedRefPrefix(prefix); + return equalsIgnoreCase(urlUri.getScheme(), prefixUri.getScheme()) + && equalsIgnoreCase(urlUri.getHost(), prefixUri.getHost()) + && effectivePort(urlUri) == effectivePort(prefixUri) + && pathMatches(urlUri.getPath(), prefixUri.getPath()); + } + + /** + * Validates an allowed prefix used in the configuration. + * + *

User information, queries and fragments are rejected because they are not part of a + * repository identity and would make prefix matching ambiguous. + */ + static void validateAllowedRefPrefix(String prefix) throws ValidationException { + parseAllowedRefPrefix(prefix); + } + + private static URI parseAllowedRefPrefix(String prefix) throws ValidationException { + ValidationException.checkCondition( + prefix != null && !prefix.trim().isEmpty(), + "Allowed integrate URL prefix cannot be empty."); + URI uri; + try { + uri = new URI(prefix.trim()); + } catch (URISyntaxException e) { + throw new ValidationException( + String.format("Invalid allowed integrate URL prefix '%s': %s", prefix, e.getMessage()), + e); + } + ValidationException.checkCondition( + uri.isAbsolute() && !uri.isOpaque(), + "Allowed integrate URL prefix '%s' must be an absolute hierarchical URL.", + prefix); + ValidationException.checkCondition( + uri.getRawUserInfo() == null, + "Allowed integrate URL prefix '%s' must not contain user information.", + prefix); + ValidationException.checkCondition( + uri.getRawQuery() == null && uri.getRawFragment() == null, + "Allowed integrate URL prefix '%s' must not contain a query or fragment.", + prefix); + return uri; + } + + private static URI parseUrl(String url) throws ValidationException { + try { + return new URI(url); + } catch (URISyntaxException e) { + throw new ValidationException( + String.format("Invalid integrate URL '%s': %s", url, e.getMessage()), e); + } + } + + private static boolean pathMatches(String urlPath, String prefixPath) { + String normalizedUrlPath = urlPath == null ? "" : urlPath; + String normalizedPrefixPath = prefixPath == null ? "" : prefixPath; + if (normalizedUrlPath.equals(normalizedPrefixPath)) { + return true; + } + if (!normalizedUrlPath.startsWith(normalizedPrefixPath)) { + return false; + } + return normalizedPrefixPath.endsWith("/") + || normalizedUrlPath.charAt(normalizedPrefixPath.length()) == '/'; + } + + private static boolean equalsIgnoreCase(String left, String right) { + if (left == null || right == null) { + return left == right; + } + return Ascii.equalsIgnoreCase(left, right); + } + + private static int effectivePort(URI uri) { + if (uri.getPort() != -1) { + return uri.getPort(); + } + String scheme = uri.getScheme(); + if (scheme == null) { + return -1; + } + return switch (Ascii.toLowerCase(scheme)) { + case "http" -> 80; + case "https" -> 443; + default -> -1; + }; + } +} diff --git a/javatests/com/google/copybara/git/GitHubPrIntegrateLabelTest.java b/javatests/com/google/copybara/git/GitHubPrIntegrateLabelTest.java index a86f28333..dd1701b66 100644 --- a/javatests/com/google/copybara/git/GitHubPrIntegrateLabelTest.java +++ b/javatests/com/google/copybara/git/GitHubPrIntegrateLabelTest.java @@ -205,6 +205,50 @@ public void maybeParse_nonGitHubDestinationUrl_allowedWithAllowedRefPrefixes() t assertThat(parsedMatchingDest.get().toString()).isEqualTo(rawLabel); } + @Test + public void maybeParse_hostSuffixSpoof_rejected() throws Exception { + GeneralOptions options = + new GeneralOptions(ImmutableMap.of(), FileSystems.getDefault(), console); + String rawLabel = "https://github.com.evil.example/foo/bar/pull/18 from copybarrista:main"; + + assertThrows( + ValidationException.class, + () -> + GitHubPrIntegrateLabel.maybeParse( + rawLabel, repo, "rpc://foo", options, ImmutableList.of("https://github.com"))); + } + + @Test + public void maybeParse_orgSuffixSpoof_rejected() throws Exception { + GeneralOptions options = + new GeneralOptions(ImmutableMap.of(), FileSystems.getDefault(), console); + String rawLabel = "https://github.com/foo-evil/bar/pull/18 from copybarrista:main"; + + assertThrows( + ValidationException.class, + () -> + GitHubPrIntegrateLabel.maybeParse( + rawLabel, + repo, + "rpc://foo", + options, + ImmutableList.of("https://github.com/foo"))); + } + + @Test + public void maybeParse_allowedOrgPrefix() throws Exception { + GeneralOptions options = + new GeneralOptions(ImmutableMap.of(), FileSystems.getDefault(), console); + String rawLabel = "https://github.com/foo/bar/pull/18 from copybarrista:main"; + + Optional parsed = + GitHubPrIntegrateLabel.maybeParse( + rawLabel, repo, "rpc://foo", options, ImmutableList.of("https://github.com/foo")); + + assertThat(parsed).isPresent(); + assertThat(parsed.get().toString()).isEqualTo(rawLabel); + } + @Test public void parseLabel_hyphenatedUser() throws Exception { GeneralOptions options = @@ -232,4 +276,134 @@ public void parseLabel_sha256() throws Exception { options)) .isNotNull(); } + + @Test + public void matches_hostSuffixSpoof_rejected() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com.evil.example/google/repo", "https://github.com")) + .isFalse(); + } + + @Test + public void matches_orgSuffixSpoof_rejected() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google-evil/repo", "https://github.com/google")) + .isFalse(); + } + + @Test + public void matches_schemeMismatch_rejected() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "http://github.com/google")) + .isFalse(); + assertThat( + IntegrateUrlPrefixes.matches( + "http://github.com/google/repo", "https://github.com/google")) + .isFalse(); + } + + @Test + public void matches_hostMismatch_rejected() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://gitlab.com/google")) + .isFalse(); + } + + @Test + public void matches_emptyPrefix_throws() { + ValidationException thrown = + assertThrows( + ValidationException.class, + () -> IntegrateUrlPrefixes.matches("https://github.com/google/repo", "")); + + assertThat(thrown).hasMessageThat().contains("Allowed integrate URL prefix cannot be empty"); + } + + @Test + public void matches_pathPrefixWithoutSlashBoundary_rejected() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com/goog")) + .isFalse(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com/google/repo-extra")) + .isFalse(); + } + + @Test + public void matches_exactRepo_allowed() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com/google/repo")) + .isTrue(); + } + + @Test + public void matches_allowedOrgPrefix_allowed() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com/google")) + .isTrue(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com/google/")) + .isTrue(); + assertThat(IntegrateUrlPrefixes.matches("https://github.com/google/repo", "https://github.com")) + .isTrue(); + } + + @Test + public void matches_hostIsCaseInsensitive() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://GitHub.com/google/repo", "https://github.com/google")) + .isTrue(); + } + + @Test + public void matches_defaultPortsAreNormalized() throws Exception { + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com:443/google/repo", "https://github.com/google")) + .isTrue(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com:8443/google/repo", "https://github.com/google")) + .isFalse(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com/google/repo", "https://github.com:8443/google")) + .isFalse(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com:8443/google/repo", "https://github.com:8443/google")) + .isTrue(); + assertThat( + IntegrateUrlPrefixes.matches( + "https://github.com:443/google/repo", "https://github.com:443/google")) + .isTrue(); + } + + @Test + public void matches_invalidAllowedPrefixes_throw( + @TestParameter({ + "https://github.com/google?owner=evil", + "https://github.com/google#fragment", + "https://user:pass@github.com/google", + "github.com/google", + "relative/path" + }) + String prefix) { + ValidationException thrown = + assertThrows( + ValidationException.class, + () -> IntegrateUrlPrefixes.matches("https://github.com/google/repo", prefix)); + + assertThat(thrown).hasMessageThat().contains("Allowed integrate URL prefix"); + } }