Add .env build-time variable substitution for scripts - #26
Conversation
Substitute ${VAR} placeholders in install scripts at build time from a
.env file (auto-detected, or --env-file) merged with SWIFTPKG_* process
variables (.env wins; --no-inherit-env disables the merge). --strict-env
fails the build on any unresolved placeholder; otherwise unresolved names
are warned.
Hardening ported from munki-pkg: 1MB file cap, identifier-shape key
validation (invalid keys skipped with a warning), single-pass replacement
(a substituted value is never re-expanded), and processed scripts written
to a private mode-0700 temp dir that pkgbuild packages instead of the
originals. Values are spliced verbatim and land as plain text in the .pkg,
so this is for build-time config, not secrets (documented in-file).
Note: munki-pkg inherited MUNKIPKG_*; this uses SWIFTPKG_* to match the
tool name — worth confirming upstream.
Tests: EnvLoaderTests (parse/quote/comment, invalid-key skip, oversize
reject, merge precedence + inherit toggle), PlaceholderReplacerTests
(substitute/unresolved, single-pass, verbatim splice), ScriptEnvironmentTests
(0700 perms, substitution, empty-vars no-op). verify-loop.sh builds a
project with a .env and asserts the packaged postinstall was substituted.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughBuild-time environment substitution is added through ChangesEnvironment substitution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLICommand
participant PackageBuildCoordinator
participant ScriptEnvironment
participant ComponentPackageBuilder
CLICommand->>PackageBuildCoordinator: pass environment build configuration
PackageBuildCoordinator->>ScriptEnvironment: process scripts with merged variables
ScriptEnvironment-->>PackageBuildCoordinator: return processed scripts and placeholder reports
PackageBuildCoordinator->>ComponentPackageBuilder: provide effectiveScripts
ComponentPackageBuilder->>ComponentPackageBuilder: pass effectiveScripts to pkgbuild
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 35 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
swiftpkg/EnvLoader.swift (1)
116-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a more explicit name for
ScriptEnvironment.process.
processwrites files to disk and mutates permissions but the name doesn't convey that side effect. Something likewriteSubstitutedScriptsormaterializeScriptswould better signal the mutation.As per coding guidelines, "keep side effects explicit in method names and parameter labels."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@swiftpkg/EnvLoader.swift` around lines 116 - 166, The ScriptEnvironment.process method name does not convey that it writes substituted scripts and changes permissions. Rename process to an explicit name such as writeSubstitutedScripts or materializeScripts, and update all call sites and references while preserving its existing behavior and signature.Source: Coding guidelines
swiftpkgTests/EnvLoaderTests.swift (1)
91-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct unit test for
ScriptEnvironment.unresolvedPlaceholders.It's only exercised indirectly via
process. It's the functionPackageBuilder.applyBuildVariablesrelies on for the strict-mode, no-variables branch — worth a direct test given the guideline on covering script-related behavior changes.As per path instructions, "When behavior changes affect established defaults, version substitution, payload-free packages, BOM synchronization, script permission normalization, or importer rollback, add targeted compatibility tests before changing it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@swiftpkgTests/EnvLoaderTests.swift` around lines 91 - 123, Add a direct unit test for ScriptEnvironment.unresolvedPlaceholders covering the strict no-variables behavior used by PackageBuilder.applyBuildVariables, including unresolved placeholder detection and the expected result for scripts without placeholders. Keep the existing process-based tests unchanged and target the function directly rather than relying on indirect coverage.Source: Path instructions
scripts/verify-loop.sh (1)
74-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a
--strict-envfailure-path check.This block only exercises the happy-path substitution. A quick negative check (script referencing an undefined
${VAR}with--strict-env, expecting a non-zero exit) would catch regressions infailOnUnresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify-loop.sh` around lines 74 - 89, Extend the environment-substitution section in verify-loop.sh with a negative test for --strict-env: create a postinstall script referencing an undefined ${VAR}, run the build with --strict-env, and assert it exits non-zero. Use the existing test workspace and failure-checking conventions, while preserving the current successful substitution test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@swiftpkg/EnvLoader.swift`:
- Around line 160-162: Replace the silently ignored copy operation in the
non-script branch of EnvLoader with throwing error propagation, matching the
substituted script write path. Ensure fileManager.copyItem failures abort the
operation and surface the original error instead of leaving the override scripts
directory incomplete.
---
Nitpick comments:
In `@scripts/verify-loop.sh`:
- Around line 74-89: Extend the environment-substitution section in
verify-loop.sh with a negative test for --strict-env: create a postinstall
script referencing an undefined ${VAR}, run the build with --strict-env, and
assert it exits non-zero. Use the existing test workspace and failure-checking
conventions, while preserving the current successful substitution test.
In `@swiftpkg/EnvLoader.swift`:
- Around line 116-166: The ScriptEnvironment.process method name does not convey
that it writes substituted scripts and changes permissions. Rename process to an
explicit name such as writeSubstitutedScripts or materializeScripts, and update
all call sites and references while preserving its existing behavior and
signature.
In `@swiftpkgTests/EnvLoaderTests.swift`:
- Around line 91-123: Add a direct unit test for
ScriptEnvironment.unresolvedPlaceholders covering the strict no-variables
behavior used by PackageBuilder.applyBuildVariables, including unresolved
placeholder detection and the expected result for scripts without placeholders.
Keep the existing process-based tests unchanged and target the function directly
rather than relying on indirect coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 568bef4d-f45d-4754-8b84-cfba222c9b6f
📒 Files selected for processing (6)
scripts/verify-loop.shswiftpkg/EnvLoader.swiftswiftpkg/PackageBuildOptions.swiftswiftpkg/PackageBuilder.swiftswiftpkgCLI/CLI.swiftswiftpkgTests/EnvLoaderTests.swift
The non-script branch copied with try?, discarding errors, while substituted scripts write with a throwing call. A failed copy left the override scripts directory silently incomplete, so pkgbuild could produce a package missing files with no error. Propagate the copy error instead.
|
Addressed in the latest commit: the non-script copy path now uses a throwing |
${VAR} is ambiguous by construction: it is both swiftpkg's build-time
substitution syntax and ordinary shell expansion. The scanner reported every
unsubstituted ${...} it saw, so a helper like
plist_set() { local key="$1" type="$2" val="$3"; ... }
warned about key, type and val on every build — names the script declares and
resolves itself at install time.
Subtract the names a script assigns (plain and declarator-prefixed assignments,
bare declarations, for-loop variables, read targets) from what gets reported. A
build variable that is genuinely missing is never assigned in the script, so it
still warns, and --strict-env still fails on it. Substitution behaviour is
untouched; only reporting narrows.
"Applied 2 build variable(s) to install scripts" counted the entries in the
merged variable map. It said the same thing whether both names were substituted
or neither appeared in any script, so a typo on either side -- a placeholder
spelled one way and a .env key spelled another -- reported success.
Track the names actually replaced. PlaceholderReplacer.Result gains
`substituted` alongside `unresolved`, and ScriptEnvironment.process returns an
Outcome carrying both per script, so the message can name how many of the
loaded variables reached a script and how many scripts they reached:
Applied 1 of 3 build variable(s) to 1 install script(s)
When nothing matched, say that outright rather than report an application that
did not happen.
|
Two commits added. Do not report shell-owned names as unresolved placeholders. Report the variables a build applied, not the ones it loaded. Suite green (39 tests) and This supersedes rodchristiansen#2, which targeted an earlier base; closing that one. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
swiftpkgTests/EnvLoaderTests.swift (1)
204-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the negative side of
shellOwnedNames.The test only asserts positives, so a pattern edit that over-captures (e.g. treating a bare
${SERVER}reference or a[ "$X" = y ]comparison as a declaration) would still pass while silently suppressing real warnings.♻️ Suggested addition
let owned = ScriptEnvironment.shellOwnedNames(in: script) for name in ["plain", "scoped", "exported", "counted", "frozen", "typed", "declared", "item", "answer"] { `#expect`(owned.contains(name), "expected \(name) to be recognised as shell-owned") } + for name in ["SERVER", "REPORTMATE_API_URL"] { + `#expect`(!owned.contains(name), "expected \(name) to remain a build placeholder") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@swiftpkgTests/EnvLoaderTests.swift` around lines 204 - 222, Add negative assertions to recognisesDeclarationForms for shellOwnedNames, including referenced or compared variables such as SERVER in ${SERVER} and X in [ "$X" = y ], and verify they are not reported as shell-owned. Keep the existing positive declaration coverage unchanged.swiftpkg/EnvLoader.swift (1)
218-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReporting rule now lives in two places.
Line 219 re-implements
reportableUnresolved's filter inline (to avoid a secondreplacepass). Extracting the filter itself keeps one definition of "reportable" for both entry points.♻️ Suggested extraction
/// Unresolved names minus the ones the script declares for itself. static func reportable(_ unresolved: Set<String>, declaredIn content: String) -> Set<String> { unresolved.subtracting(shellOwnedNames(in: content)) } static func reportableUnresolved(in content: String, with variables: [String: String]) -> Set<String> { reportable(PlaceholderReplacer.replace(in: content, with: variables).unresolved, declaredIn: content) }// Substitution is unchanged — only what gets reported narrows. - let reportable = result.unresolved.subtracting(shellOwnedNames(in: text)) - if !reportable.isEmpty { unresolvedByScript[name] = reportable } + let reportableNames = reportable(result.unresolved, declaredIn: text) + if !reportableNames.isEmpty { unresolvedByScript[name] = reportableNames }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@swiftpkg/EnvLoader.swift` around lines 218 - 220, Extract the shared reporting filter into a helper near reportableUnresolved, such as reportable(_:declaredIn:), that subtracts shellOwnedNames(in:) from the unresolved set. Update reportableUnresolved and the unresolvedByScript assignment to call this helper, preserving the existing single replacement pass and reporting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@swiftpkg/EnvLoader.swift`:
- Around line 170-181: Update
unresolvedPlaceholders(in:scriptsDir:given:fileManager:) to surface warnings
whenever the scripts directory cannot be listed or a script cannot be read or
decoded as UTF-8, instead of silently skipping those entries. Preserve the
existing unresolved-placeholder collection for successfully inspected scripts,
and use the project’s established warning/logging mechanism so
PackageBuilder.applyBuildVariables can expose skipped files during strict-env
processing.
---
Nitpick comments:
In `@swiftpkg/EnvLoader.swift`:
- Around line 218-220: Extract the shared reporting filter into a helper near
reportableUnresolved, such as reportable(_:declaredIn:), that subtracts
shellOwnedNames(in:) from the unresolved set. Update reportableUnresolved and
the unresolvedByScript assignment to call this helper, preserving the existing
single replacement pass and reporting behavior.
In `@swiftpkgTests/EnvLoaderTests.swift`:
- Around line 204-222: Add negative assertions to recognisesDeclarationForms for
shellOwnedNames, including referenced or compared variables such as SERVER in
${SERVER} and X in [ "$X" = y ], and verify they are not reported as
shell-owned. Keep the existing positive declaration coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6dada97c-c464-4e7a-b0e8-4e5ea29df7b8
📒 Files selected for processing (3)
swiftpkg/EnvLoader.swiftswiftpkg/PackageBuilder.swiftswiftpkgTests/EnvLoaderTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- swiftpkg/PackageBuilder.swift
| /// Returns placeholder names referenced by any script but not in `variables`. | ||
| public static func unresolvedPlaceholders(in scriptsDir: URL, given variables: [String: String], fileManager: FileManager = .default) -> [String: Set<String>] { | ||
| let contents = (try? fileManager.contentsOfDirectory(atPath: scriptsDir.path)) ?? [] | ||
| var byScript: [String: Set<String>] = [:] | ||
| for name in contents where isScript(name) { | ||
| let path = scriptsDir.appendingPathComponent(name).path | ||
| guard let data = fileManager.contents(atPath: path), let text = String(data: data, encoding: .utf8) else { continue } | ||
| let unresolved = reportableUnresolved(in: text, with: variables) | ||
| if !unresolved.isEmpty { byScript[name] = unresolved } | ||
| } | ||
| return byScript | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unreadable scripts silently bypass the strict-env gate.
try? on contentsOfDirectory and the guard let data ... String(...utf8) skip mean a scripts dir that can't be listed, or a script that isn't valid UTF-8, yields no unresolved entries — so --strict-env (see PackageBuilder.applyBuildVariables) passes on files it never inspected. Consider surfacing a warning for skipped entries so the silence is at least visible.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 175-175: A file is read from a path built from runtime/request input via FileManager.contents(atPath:), Data(contentsOf:), or String(contentsOfFile:). An attacker can supply '../' sequences or absolute paths to read files outside the intended directory (path traversal). Validate and canonicalize the path, reject '..' components, and confine reads to an allow-listed base directory (e.g. resolve with URL(fileURLWithPath:relativeTo:) and verify the resolved path is still inside the base) before reading.
Context: fileManager.contents(atPath: path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(path-traversal-file-read-request-input-swift)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@swiftpkg/EnvLoader.swift` around lines 170 - 181, Update
unresolvedPlaceholders(in:scriptsDir:given:fileManager:) to surface warnings
whenever the scripts directory cannot be listed or a script cannot be read or
decoded as UTF-8, instead of silently skipping those entries. Preserve the
existing unresolved-placeholder collection for successfully inspected scripts,
and use the project’s established warning/logging mechanism so
PackageBuilder.applyBuildVariables can expose skipped files during strict-env
processing.
2 participants
Loads a project
.envand substitutes${VAR}references in scripts at build time, leaving runtime shell variables (those not defined in.env) intact. Lets projects template values (URLs, IDs) into scripts without committing them.Tests:
EnvLoaderTests;verify-loop.shcovers it; full suite green.Part of a 9-PR series splitting a batch of features into small, themed, independently reviewable PRs. Each applies cleanly to
mainon its own; the ordering below only minimizes rebases as they land:Happy to squash, split, or reorder any of these to suit your review preferences.
Summary by CodeRabbit
.envvariable substitution for installer scripts during package builds, with optional strict validation for unresolved${VAR}placeholders.--env-filefor substitution input,--strict-envto fail on unresolved placeholders, and--no-inherit-envto stop inheriting build-related environment variables..envparsing/merge precedence, placeholder substitution/unresolved tracking, script processing and permission handling, plus an expansion/verification check to confirm resolved values are present in generated installers.