diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f67003..6565704 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/checks.rs b/src/checks.rs new file mode 100644 index 0000000..dfd2d85 --- /dev/null +++ b/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), +} diff --git a/src/errors.rs b/src/errors.rs index 2c49051..7855e75 100644 --- a/src/errors.rs +++ b/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) { 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( diff --git a/src/main.rs b/src/main.rs index 6ce43d6..aad573a 100644 --- a/src/main.rs +++ b/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) -> libcnb::Result { + // 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. diff --git a/tests/checks_test.rs b/tests/checks_test.rs new file mode 100644 index 0000000..66ca13f --- /dev/null +++ b/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. + "} + ); + }); +} diff --git a/tests/mod.rs b/tests/mod.rs index d428c08..909cfdc 100644 --- a/tests/mod.rs +++ b/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) -> 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