From 329b22a980ad7a0835c53938afe06a111f40ac96 Mon Sep 17 00:00:00 2001 From: gregce Date: Thu, 20 May 2021 07:35:39 -0700 Subject: [PATCH] Restrict bazelrc-set flag aliases to the "build" command. This prevents being able to build with an alias that "$ bazel canonicalize-flags" doesn't understand. This is a safety check for build and CI pipelines that process invocation flags through canonicalize-flags. Example scenario this fixes: $ cat .bazelrc test --flag_alias=myalias=//foo build:myconfig --flag_alias=configalias=//foo build:myconfig --configalias=5 $ bazel test //:mytest --myalias=blah $ bazel canonicalize-flags --for_command=test -- --myalias=blah ERROR: Unrecognized option: --myalias=blah $ bazel build //:mybuild --config=myconfig $ bazel canonicalize-flags -- --configalias=4 ERROR: Unrecognized option: --configalias=4 We could potentially expand CanonicalizeCommand.java to inherit from TestCommand.class instead of BuildCommand.class if there was a use case for loosening this restriction. PiperOrigin-RevId: 374867565 --- .../build/lib/runtime/BlazeOptionHandler.java | 19 +++++- .../starlark_configurations_test.sh | 68 ++++++++++++++++++- 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index ca0ba6753f650f..50e2495f434bec 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.InvocationPolicyEnforcer; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionPriority.PriorityCategory; @@ -428,7 +429,8 @@ static ListMultimap structureRcOptionsAndConfigs( EventHandler eventHandler, List rcFiles, List rawOverrides, - Set validCommands) { + Set validCommands) + throws OptionsParsingException { ListMultimap commandToRcArgs = ArrayListMultimap.create(); String lastRcFile = null; @@ -440,6 +442,21 @@ static ListMultimap structureRcOptionsAndConfigs( continue; } String rcFile = rcFiles.get(override.blazeRc); + // The canonicalize-flags command only inherits bazelrc "build" commands. Not "test", not + // "build:foo". Restrict --flag_alias accordingly to prevent building with flags that + // canonicalize-flags can't recognize. + if ((override.option.startsWith("--" + Converters.BLAZE_ALIASING_FLAG + "=") + || override.option.equals("--" + Converters.BLAZE_ALIASING_FLAG)) + // In production, "build" is always a valid command, but not necessarily in tests. + // Particularly C0Command, which some tests use for low-level options parsing logic. We + // don't want to interfere with those. + && validCommands.contains("build") + && !override.command.equals("build")) { + throw new OptionsParsingException( + String.format( + "%s: \"%s %s\" disallowed. --%s only supports the \"build\" command.", + rcFile, override.command, override.option, Converters.BLAZE_ALIASING_FLAG)); + } String command = override.command; int index = command.indexOf(':'); if (index > 0) { diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh index 0bacffabf77820..ddf298ffdfface 100755 --- a/src/test/shell/integration/starlark_configurations_test.sh +++ b/src/test/shell/integration/starlark_configurations_test.sh @@ -56,7 +56,10 @@ if "$is_windows"; then export MSYS2_ARG_CONV_EXCL="*" fi -add_to_bazelrc "build --package_path=%workspace%" +function set_up() { + write_default_bazelrc + add_to_bazelrc "build --package_path=%workspace%" +} #### HELPER FXNS ####################################################### @@ -635,4 +638,67 @@ function test_rc_flag_alias_canonicalizes() { expect_log "--//$pkg:type=coffee" } +function test_rc_flag_alias_unsupported_under_test_command() { + local -r pkg=$FUNCNAME + mkdir -p $pkg + + add_to_bazelrc "test --flag_alias=drink=//$pkg:type" + write_build_setting_bzl + + bazel canonicalize-flags -- --drink=coffee \ + >& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias=drink=//$pkg:type\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + bazel build //$pkg:my_drink >& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias=drink=//$pkg:type\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + # Post-test cleanup_workspace() calls "bazel clean", which would also fail + # unless we reset the bazelrc. + write_default_bazelrc +} + +function test_rc_flag_alias_unsupported_under_conditional_build_command() { + local -r pkg=$FUNCNAME + mkdir -p $pkg + + add_to_bazelrc "build:foo --flag_alias=drink=//$pkg:type" + write_build_setting_bzl + + bazel canonicalize-flags -- --drink=coffee \ +>& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias=drink=//$pkg:type\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + bazel build //$pkg:my_drink >& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias=drink=//$pkg:type\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + # Post-test cleanup_workspace() calls "bazel clean", which would also fail + # unless we reset the bazelrc. + write_default_bazelrc +} + +function test_rc_flag_alias_unsupported_with_space_assignment_syntax() { + local -r pkg=$FUNCNAME + mkdir -p $pkg + + add_to_bazelrc "test --flag_alias drink=//$pkg:type" + write_build_setting_bzl + + bazel canonicalize-flags -- --drink=coffee \ + >& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + bazel build //$pkg:my_drink >& "$TEST_log" && fail "Expected failure" + expect_log "--flag_alias\" disallowed. --flag_alias only "\ +"supports the \"build\" command." + + # Post-test cleanup_workspace() calls "bazel clean", which would also fail + # unless we reset the bazelrc. + write_default_bazelrc +} + run_suite "${PRODUCT_NAME} starlark configurations tests"