Merge pull request #3957 from buildkite/comments-on-env-files

internal: Add comments about env files

Author: DrJosh9000

agent/job_runner.go

@@ -492,6 +492,15 @@ BUILDKITE_AGENT_JWKS_KEY_ID`
 		}
 
 		for key, value := range env {
+			// Note that the value below is %q-quoted, and not shell-escaped.
+			// Env files are designed to be validated by the pre-bootstrap hook.
+			// Because the job env may contain untrusted or even dangerous env
+			// vars (suppose a user adds an env var in the "New Build" UI with a
+			// value that exploits a command injection in Bash), the
+			// pre-bootstrap hook should do this validation *without* sourcing
+			// the file. (If the job env was universally safe, then we wouldn't
+			// bother using a file - we'd load it straight into the subprocess
+			// env.)
 			if _, err := fmt.Fprintf(r.envShellFile, "%s=%q\n", key, value); err != nil {
 				return nil, err
 			}
@@ -501,6 +510,8 @@ BUILDKITE_AGENT_JWKS_KEY_ID`
 		}
 	}
 	if r.envJSONFile != nil {
+		// key="value" format can be difficult to parse in some circumstances,
+		// so we also have a JSON formatted env file.
 		if err := json.NewEncoder(r.envJSONFile).Encode(env); err != nil {
 			return nil, err
 		}
@@ -783,7 +794,13 @@ func (r *JobRunner) executePreBootstrapHook(ctx context.Context, hook string) (b
 
 	// This (plus inherited) is the only ENV that should be exposed
 	// to the pre-bootstrap hook.
-	// - Env files are designed to be validated by the pre-bootstrap hook
+	// - Env files are designed to be validated by the pre-bootstrap hook.
+	//   Because the job env may contain untrusted or even dangerous env vars
+	//   (suppose a user adds an env var in the "New Build" UI with a value that
+	//   exploits a command injection in Bash), the pre-bootstrap hook should do
+	//   this validation *without* sourcing the file. (If the job env was
+	//   universally safe to source, then we would just pre-populate this env
+	//   with it.)
 	// - The pre-bootstrap hook may want to create annotations, so it can also
 	//   have a few necessary and global args as env vars.
 	environ := envutil.New()