fix: use project root as boundary for hook path containment#7689
Merged
Conversation
The path containment check introduced in multi-language hooks (PR #7451) incorrectly used the service directory as the boundary, rejecting legitimate relative paths like ../../hooks/script.ps1 from service-level hooks. This changes the boundary to the project root directory where azure.yaml lives. Fixes #7666 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wbreza
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989 and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a regression where service-level hooks using relative script paths (e.g. ../../hooks/...) were incorrectly rejected by making the project root (where azure.yaml lives) the path-containment security boundary, while keeping cwd as the resolution base for relative paths.
Changes:
- Introduces
projectDiras a separate boundary directory for hook path containment checks. - Propagates
projectDirthrough hooks manager/runner initialization and CLI entry points. - Adds/updates tests covering service-relative paths, escape rejection, and fallback behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/ext/models.go | Splits cwd (resolution base) vs projectDir (containment boundary) during hook validation. |
| cli/azd/pkg/ext/hooks_manager.go | Adds projectDir to manager and injects it into each HookConfig. |
| cli/azd/pkg/ext/hooks_runner.go | Uses projectDir (fallback to cwd) as execution boundary. |
| cli/azd/cmd/middleware/hooks.go | Passes project root for boundary while keeping service path as cwd for service hooks. |
| cli/azd/cmd/hooks.go | Passes project root to hook manager for azd hooks execution/validation. |
| cli/azd/internal/cmd/provision.go | Passes project root as boundary during provisioning hooks. |
| cli/azd/pkg/ext/models_test.go | Adds regression + boundary/escape/fallback tests for the new projectDir behavior. |
| cli/azd/pkg/ext/hooks_runner_test.go | Updates hook manager constructor calls to include projectDir. |
| cli/azd/pkg/ext/hooks_manager_test.go | Updates hook manager constructor calls to include projectDir. |
| cli/azd/pkg/ext/python_hooks_e2e_test.go | Updates hook manager constructor call to include projectDir. |
jongio
approved these changes
Apr 13, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Clean separation of cwd (path resolution) from projectDir (security boundary). All callers updated correctly, tests cover the regression case plus escape rejection and backward compat. Looks good.
When both dir and run fields are configured on a hook, the kind inference used the raw run value instead of the actual file found on disk. This caused hooks without explicit kind to default to the OS shell (PowerShell on Windows) instead of inferring the correct kind from the file extension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inline hooks are saved to OS temp files for execution, so their script path and working directory are expected to be outside the project root. Only file-based hooks enforce the project root containment boundary. Also fixes doc comment on NewHooksManager and handles os.Getwd error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split the monolithic validate() method into four focused phases: parseRunTarget, resolveKind, resolvePaths, and enforceContainment. Rename private fields for clarity: path -> relativeScriptPath, cwd -> inputCwd, script -> inlineScript. No behavior change -- purely internal readability improvement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
approved these changes
Apr 13, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-reviewed after the two new commits (kind inference fix + inline hook exemption). Everything looks correct.
The core fix properly separates path resolution (cwd) from the security boundary (projectDir). File-based hooks are still constrained to the project root. Inline hooks are correctly exempt since they execute from OS temp files.
I verified:
- All 35 callers of
NewHooksManagerare updated with the new signature layerPathin provision.go is always resolved relative to the project root viaAbsolutePath()- no risk of external-path containment failures- Kind inference now uses
hc.path(resolved file path with extension) instead ofhc.Run(raw YAML value) - correct for Dir+Run scenarios - Backward compat maintained via empty-projectDir fallback to cwd
- Tests are thorough: regression scenario, escape rejection, inline exemption, all 6 hook kinds, kind inference
One minor note: two adjacent string params in NewHooksManager(cwd, projectDir, commandRunner) can be easy to swap, but the doc comment is clear and this matches the codebase style. Not a blocker.
Replace two adjacent string parameters (cwd, projectDir) with a HooksManagerOptions struct to prevent accidental argument swapping and make call sites self-documenting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ValidateHooks() ignored the Dir field when checking if a hook script exists on disk, causing dir+run combinations to be incorrectly classified as inline scripts and defaulting to the OS shell (PowerShell on Windows). The file detection now checks dir+run when Dir is set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Table-driven tests verify that every valid combination of kind, run, and dir fields resolves to the correct executor Kind after validation. Covers all 6 hook kinds for run-only and dir+run patterns, inline script defaults, and explicit kind overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
approved these changes
Apr 13, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Verified the HooksManagerOptions refactor commit - call sites are self-documenting and match the codebase's options-struct convention. Re-approving.
ValidateHooks() had its own file detection logic that ignored the Dir field, causing dir+run combinations to be misclassified as inline scripts. This pre-set Kind to the OS default shell before validate() ran, preventing correct kind inference from file extensions. validate() is now the sole authority for Kind assignment. ValidateHooks calls validate() first and reads post-validation state for warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The shared hook validation pipeline (resolveScript, resolveKind, resolvePaths, enforceContainment) treats all 6 hook kinds uniformly. Kind-specific validation (runtime checks, dependency discovery) lives in each executor's Prepare() method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
approved these changes
Apr 13, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-reviewed after the latest two commits (c874b82, d539a70). The ValidateHooks simplification is clean - removing the duplicated file detection in favor of letting validate() be the single authority for Kind assignment is the right call. OS override handling and the kind-agnostic doc comments are good additions.
LGTM - no new issues.
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Changed
Service-level hooks that reference shared scripts via relative paths (e.g.,
../../hooks/prepackage.ps1) no longer fail with "hook script path escapes project root." This was a regression in 1.23.15 introduced by PR #7451 (multi-language hooks Phase 1).Additionally, hook kind inference now works correctly when both
dirandrunfields are set — previously the system would default to PowerShell on Windows instead of inferring the correct executor from the file extension.Before (broken):
src/logicApp/with a hook pointing to../../hooks/script.ps1would fail because the security boundary was the service directory, not the project root.dir: hooks/preandrun: main.py(no explicitkind) would default to PowerShell instead of Python.After (fixed):
azure.yamllives). Service hooks can reach shared scripts via relative paths. Paths that escape the project root are still rejected.runvalue.How It Works
PR #7451 conflated two concepts into a single
cwdvariable: the working directory for resolving relative paths and the security boundary for path containment checks. This PR introduces a separateprojectDirfield onHookConfigthat always points to the project root. Path resolution still usescwd(which may be a service subdirectory), but containment validation checks againstprojectDir.For inline hooks (defined directly in YAML rather than as file references), both containment checks are skipped entirely — these hooks are saved to OS temp files for execution, so their script path and working directory are expected to be outside the project root.
The kind inference fix changes
InferKindFromPath(hc.Run)toInferKindFromPath(hc.path), ensuring the resolved file path (with extension) is used instead of the raw user input.Issue References
Resolves #7666
Resolves #7691
Notes
Comprehensive test coverage added across three commits:
projectDiris unset (falls back tocwd)