internal: Add comments about env files

Author: DrJosh9000

agent/job_runner.go

@@ -491,6 +491,13 @@ 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, 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
 			}
@@ -500,6 +507,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
 		}
@@ -775,7 +784,11 @@ 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,
+	//   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()