Reject known problematic env vars (#308)

We expose all env vars by default to subprocesses to allow for
customisation of package manager behaviour - such as custom indexes,
authentication and requirements file env var interpolation.

(An allow-list approach wouldn't work with all use-cases, plus
wouldn't help the app at run-time.)

To improve the error UX (particularly during initial buildpack
bootstrap, where failures would otherwise look like a problem with the
buildpack and not the user inputs), the buildpack now rejects known
problematic env vars that may break the build / the app.

The list of env vars was based on the env vars this buildpack sets,
plus an audit of:
- https://docs.python.org/3/using/cmdline.html#environment-variables
- https://pip.pypa.io/en/stable/cli/pip/#general-options
- https://pip.pypa.io/en/stable/cli/pip_install/#options

This also unblocks #265.

GUS-W-17454486.

Author: edmorley

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- The build now fails early if known problematic Python and pip-related env vars have been set by the user or earlier buildpacks. ([#308](https://github.com/heroku/buildpacks-python/pull/308))
 - The `PIP_PYTHON` env var is now only set at build time. ([#307](https://github.com/heroku/buildpacks-python/pull/307))
 
 ### Removed

src/checks.rs

@@ -0,0 +1,40 @@
+use libcnb::Env;
+
+// We expose all env vars by default to subprocesses to allow for customisation of package manager
+// behaviour (such as custom indexes, authentication and requirements file env var interpolation).
+// As such, we have to block known problematic env vars that may break the build / the app.
+// This list was based on the env vars this buildpack sets, plus an audit of:
+// https://docs.python.org/3/using/cmdline.html#environment-variables
+// https://pip.pypa.io/en/stable/cli/pip/#general-options
+// https://pip.pypa.io/en/stable/cli/pip_install/#options
+const FORBIDDEN_ENV_VARS: [&str; 12] = [
+    "PIP_CACHE_DIR",
+    "PIP_PREFIX",
+    "PIP_PYTHON",
+    "PIP_ROOT",
+    "PIP_TARGET",
+    "PIP_USER",
+    "PYTHONHOME",
+    "PYTHONINSPECT",
+    "PYTHONNOUSERSITE",
+    "PYTHONPLATLIBDIR",
+    "PYTHONUSERBASE",
+    "VIRTUAL_ENV",
+];
+
+pub(crate) fn check_environment(env: &Env) -> Result<(), ChecksError> {
+    if let Some(&name) = FORBIDDEN_ENV_VARS
+        .iter()
+        .find(|&name| env.contains_key(name))
+    {
+        return Err(ChecksError::ForbiddenEnvVar(name.to_string()));
+    }
+
+    Ok(())
+}
+
+/// Errors due to one of the environment checks failing.
+#[derive(Debug)]
+pub(crate) enum ChecksError {
+    ForbiddenEnvVar(String),
+}

src/errors.rs

@@ -1,3 +1,4 @@
+use crate::checks::ChecksError;
 use crate::django::DjangoCollectstaticError;
 use crate::layers::pip::PipLayerError;
 use crate::layers::pip_dependencies::PipDependenciesLayerError;
@@ -49,6 +50,7 @@ pub(crate) fn on_error(error: libcnb::Error<BuildpackError>) {
 fn on_buildpack_error(error: BuildpackError) {
     match error {
         BuildpackError::BuildpackDetection(error) => on_buildpack_detection_error(&error),
+        BuildpackError::Checks(error) => on_buildpack_checks_error(error),
         BuildpackError::DeterminePackageManager(error) => on_determine_package_manager_error(error),
         BuildpackError::DjangoCollectstatic(error) => on_django_collectstatic_error(error),
         BuildpackError::DjangoDetection(error) => on_django_detection_error(&error),
@@ -70,6 +72,21 @@ fn on_buildpack_detection_error(error: &io::Error) {
     );
 }
 
+fn on_buildpack_checks_error(error: ChecksError) {
+    match error {
+        ChecksError::ForbiddenEnvVar(name) => log_error(
+            "Unsafe environment variable found",
+            formatdoc! {"
+                The environment variable '{name}' is set, however, it can
+                cause problems with the build so we do not allow using it.
+
+                You must unset that environment variable. If you didn't set it
+                yourself, check that it wasn't set by an earlier buildpack.
+            "},
+        ),
+    };
+}
+
 fn on_determine_package_manager_error(error: DeterminePackageManagerError) {
     match error {
         DeterminePackageManagerError::CheckFileExists(io_error) => log_io_error(

src/main.rs

@@ -1,3 +1,4 @@
+mod checks;
 mod detect;
 mod django;
 mod errors;
@@ -9,6 +10,7 @@ mod python_version_file;
 mod runtime_txt;
 mod utils;
 
+use crate::checks::ChecksError;
 use crate::django::DjangoCollectstaticError;
 use crate::layers::pip::PipLayerError;
 use crate::layers::pip_dependencies::PipDependenciesLayerError;
@@ -51,6 +53,15 @@ impl Buildpack for PythonBuildpack {
     }
 
     fn build(&self, context: BuildContext<Self>) -> libcnb::Result<BuildResult, Self::Error> {
+        // We inherit the current process's env vars, since we want `PATH` and `HOME` from the OS
+        // to be set (so that later commands can find tools like Git in the base image), along
+        // with previous-buildpack or user-provided env vars (so that features like env vars in
+        // in requirements files work). We protect against broken user-provided env vars via the
+        // checks feature and making sure that buildpack env vars take precedence in layers envs.
+        let mut env = Env::from_current();
+
+        checks::check_environment(&env).map_err(BuildpackError::Checks)?;
+
         // We perform all project analysis up front, so the build can fail early if the config is invalid.
         // TODO: Add a "Build config" header and list all config in one place?
         let package_manager = package_manager::determine_package_manager(&context.app_dir)
@@ -80,13 +91,6 @@ impl Buildpack for PythonBuildpack {
             )),
         }
 
-        // We inherit the current process's env vars, since we want `PATH` and `HOME` from the OS
-        // to be set (so that later commands can find tools like Git in the base image), along
-        // with previous-buildpack or user-provided env vars (so that features like env vars in
-        // in requirements files work). We protect against broken user-provided env vars by
-        // making sure that buildpack env vars take precedence in layers envs and command usage.
-        let mut env = Env::from_current();
-
         log_header("Installing Python");
         let python_layer_path = python::install_python(&context, &mut env, &python_version)?;
 
@@ -126,6 +130,8 @@ impl Buildpack for PythonBuildpack {
 pub(crate) enum BuildpackError {
     /// I/O errors when performing buildpack detection.
     BuildpackDetection(io::Error),
+    /// Errors due to one of the environment checks failing.
+    Checks(ChecksError),
     /// Errors determining which Python package manager to use for a project.
     DeterminePackageManager(DeterminePackageManagerError),
     /// Errors running the Django collectstatic command.

tests/checks_test.rs

@@ -0,0 +1,25 @@
+use crate::tests::default_build_config;
+use indoc::indoc;
+use libcnb_test::{assert_contains, PackResult, TestRunner};
+
+#[test]
+#[ignore = "integration test"]
+fn checks_reject_pythonhome_env_var() {
+    let mut config = default_build_config("tests/fixtures/pyproject_toml_only");
+    config.env("PYTHONHOME", "/invalid");
+    config.expected_pack_result(PackResult::Failure);
+
+    TestRunner::default().build(config, |context| {
+        assert_contains!(
+            context.pack_stderr,
+            indoc! {"
+                [Error: Unsafe environment variable found]
+                The environment variable 'PYTHONHOME' is set, however, it can
+                cause problems with the build so we do not allow using it.
+
+                You must unset that environment variable. If you didn't set it
+                yourself, check that it wasn't set by an earlier buildpack.
+            "}
+        );
+    });
+}

tests/mod.rs

@@ -3,6 +3,7 @@
 //! These tests are not run via automatic integration test discovery, but instead are
 //! imported in main.rs so that they have access to private APIs (see comment in main.rs).
 
+mod checks_test;
 mod detect_test;
 mod django_test;
 mod package_manager_test;
@@ -38,13 +39,9 @@ fn default_build_config(fixture_path: impl AsRef<Path>) -> BuildConfig {
         ("LD_LIBRARY_PATH", "/invalid"),
         ("LIBRARY_PATH", "/invalid"),
         ("PATH", "/invalid"),
-        ("PIP_CACHE_DIR", "/invalid"),
         ("PIP_DISABLE_PIP_VERSION_CHECK", "0"),
         ("PKG_CONFIG_PATH", "/invalid"),
-        ("PYTHONHOME", "/invalid"),
         ("PYTHONPATH", "/invalid"),
-        ("PYTHONUSERBASE", "/invalid"),
-        ("VIRTUAL_ENV", "/invalid"),
     ]);
 
     config