Skip to content

Support %bazel_workspace% interpolation in --repo_env. Attempt to sup… #3

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support %bazel_workspace% interpolation in --repo_env. Attempt to sup… #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80b1401
Support %bazel_workspace% interpolation in --repo_env. Attempt to sup…
visz11 Apr 15, 2025
baa8d8e
Print PATH in test
visz11 Apr 15, 2025
8cb1a07
Use Windows separator and Update starlark_repository_test.sh
visz11 Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ public class CommandEnvironment {
private final DelegatingDownloader delegatingDownloader;
private final RemoteAnalysisCachingEventListener remoteAnalysisCachingEventListener;
private final ImmutableList.Builder<IdleTask> idleTasks = ImmutableList.builder();
private final ResourceManager resourceManager;

Copy link

Choose a reason for hiding this comment

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

Unnecessary Field Removal

The resourceManager field was removed but its usage pattern was changed to a singleton access. This creates an inconsistent pattern where some dependencies are injected while others use global state, reducing maintainability.

Standards
  • Clean-Code-Consistency
  • Maintainability-Quality-Dependency-Management
  • SOLID-DIP

private boolean mergedAnalysisAndExecution;

Expand Down Expand Up @@ -234,8 +233,7 @@ public void exit(AbruptExitException exception) {
CommandExtensionReporter commandExtensionReporter,
int attemptNumber,
@Nullable String buildRequestIdOverride,
ConfigFlagDefinitions configFlagDefinitions,
ResourceManager resourceManager) {
ConfigFlagDefinitions configFlagDefinitions) {
Copy link

Choose a reason for hiding this comment

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

Constructor Parameter Removal

The resourceManager parameter was removed from the constructor but its usage was changed to a singleton access pattern. This creates inconsistent dependency management where some components are injected while others use global state.

Standards
  • SOLID-DIP
  • Clean-Code-Dependency-Management
  • Design-Pattern-Dependency-Injection

checkArgument(attemptNumber >= 1);

this.runtime = runtime;
Expand All @@ -256,7 +254,6 @@ public void exit(AbruptExitException exception) {
this.timestampGranularityMonitor = new TimestampGranularityMonitor(runtime.getClock());
this.attemptNumber = attemptNumber;
this.configFlagDefinitions = configFlagDefinitions;
this.resourceManager = resourceManager;

// Record the command's starting time again, for use by
// TimestampGranularityMonitor.waitForTimestampGranularity().
Expand Down Expand Up @@ -358,6 +355,9 @@ public void exit(AbruptExitException exception) {
value = clientEnv.get(name);
}
if (value != null) {
if (workspace.getWorkspace() != null) {
value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
}
Comment on lines +358 to +360
Copy link

Choose a reason for hiding this comment

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

Path Injection Risk

Direct string replacement of %bazel_workspace% with workspace path without validation creates path injection vulnerability. Malicious repository rules could exploit this to manipulate paths and potentially access files outside intended boundaries. This could lead to unauthorized file access or path traversal attacks.

        if (workspace.getWorkspace() != null) {
          Path workspacePath = workspace.getWorkspace();
          // Normalize path to prevent path traversal attacks
          String normalizedPath = workspacePath.normalize().toAbsolutePath().toString();
          value = value.replace("%bazel_workspace%", normalizedPath);
        } else {
          logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
        }
Commitable Suggestion
Suggested change
if (workspace.getWorkspace() != null) {
value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
}
if (workspace.getWorkspace() != null) {
Path workspacePath = workspace.getWorkspace();
// Normalize path to prevent path traversal attacks
String normalizedPath = workspacePath.normalize().toAbsolutePath().toString();
value = value.replace("%bazel_workspace%", normalizedPath);
} else {
logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
}
Standards
  • CWE-22
  • CWE-73
  • OWASP-A01

Comment on lines +358 to +360
Copy link

Choose a reason for hiding this comment

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

Null Workspace Check

The workspace null check protects against NPE, but there's no handling for the case when workspace is null. Without fallback behavior, repository rules using %bazel_workspace% will receive uninterpolated values, causing potential runtime failures.

        if (workspace.getWorkspace() != null) {
          value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
        } else {
          logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
          // Leave the placeholder unchanged to avoid silent failures
        }
Commitable Suggestion
Suggested change
if (workspace.getWorkspace() != null) {
value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
}
if (workspace.getWorkspace() != null) {
value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
} else {
logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
// Leave the placeholder unchanged to avoid silent failures
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • SRE-Error-Handling

Comment on lines +358 to +360
Copy link

Choose a reason for hiding this comment

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

String Replacement Logic

String replacement is performed unconditionally without checking if the placeholder exists in the value. This could lead to unnecessary string operations for every environment variable, impacting performance with large environment sets.

Standards
  • Clean-Code-Performance
  • Maintainability-Quality-Optimization

repoEnv.put(name, value);
repoEnvFromOptions.put(name, value);
}
Expand Down Expand Up @@ -715,7 +715,7 @@ public WorkspaceInfoFromDiff getWorkspaceInfoFromDiff() {
}

public ResourceManager getLocalResourceManager() {
return resourceManager;
return ResourceManager.instance();
Comment on lines 717 to +718
Copy link

Choose a reason for hiding this comment

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

ResourceManager Reference Change

Method implementation changed from returning stored resourceManager field to ResourceManager.instance(). This changes from instance-specific to global singleton access pattern, potentially affecting resource isolation and reliability in multi-environment scenarios.

Standards
  • ISO-IEC-25010-Reliability-Maturity
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • DbC-Resource-Mgmt

Copy link

Choose a reason for hiding this comment

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

Resource Manager Reference

The constructor parameter resourceManager was removed but the getLocalResourceManager method now returns a global singleton instead of the instance-specific reference. This changes the logical behavior from using a command-specific resource manager to a global one, potentially affecting resource management isolation.

Standards
  • Business-Rule-State-Consistency
  • Logic-Verification-Data-Flow

}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,9 @@ public String getTypeDescription() {
"Specifies additional environment variables to be available only for repository rules."
+ " Note that repository rules see the full environment anyway, but in this way"
+ " configuration information can be passed to repositories through options without"
+ " invalidating the action graph.")
+ " invalidating the action graph. The string <code>%bazel_workspace%</code> in a"
+ " value will be replaced with the absolute path of the workspace as printed by"
+ " <code>bazel info workspace</code>.")
public List<Map.Entry<String, String>> repositoryEnvironment;

@Option(
Expand Down
43 changes: 43 additions & 0 deletions src/test/shell/bazel/starlark_repository_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,49 @@ EOF
|| fail "Expected unrelated action to not be rerun"
}

function test_repo_env_workspace_interpolation() {
setup_starlark_repository

cat > test.bzl <<'EOF'
def _impl(ctx):
result = ctx.execute(["my_tool"])
if result.return_code != 0:
fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr))
Comment on lines +888 to +890
Copy link

Choose a reason for hiding this comment

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

medium

Consider including the actual PATH variable in the error message for better debugging:

fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr))
Suggested change
result = ctx.execute(["my_tool"])
if result.return_code != 0:
fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr))
fail "my_tool failed ($result.return_code, PATH = ${ctx.os.environ["PATH"]}): $result.stderr"

ctx.file("out.txt", result.stdout)
ctx.file("BUILD", 'exports_files(["out.txt"])')

repo = repository_rule(
implementation = _impl,
)
EOF
cat > BUILD <<'EOF'
genrule(
name = "repoenv",
outs = ["repoenv.txt"],
srcs = ["@foo//:out.txt"],
cmd = "cp $< $@",
)
EOF
if "$is_windows"; then
local repo_env_path="%bazel_workspace%/repo_tools;$PATH"
else
local repo_env_path="%bazel_workspace%/repo_tools:$PATH"
fi
cat > .bazelrc <<EOF
common --repo_env=PATH="$repo_env_path"
EOF

mkdir -p repo_tools
cat > repo_tools/my_tool.bat <<'EOF'
echo Hello from my_tool
EOF
cp repo_tools/my_tool.bat repo_tools/my_tool
chmod +x repo_tools/my_tool

bazel build //:repoenv &> $TEST_log || fail "Failed to build"
assert_contains "Hello from my_tool" `bazel info bazel-bin 2>/dev/null`/repoenv.txt
}

function test_repo_env_inverse() {
# This test makes sure that a repository rule that has no dependencies on
# environment variables does _not_ get refetched when --repo_env changes.
Expand Down