Revert "Revert "feat(par): add default restricted shell paths with co…#48482
Revert "Revert "feat(par): add default restricted shell paths with co…#48482AlexandreYang wants to merge 1 commit into7.78.xfrom
Conversation
…ntainer-aware prefixing"" (#48462) Reverts #48460 Revert + this test fix: All tests pass. Here's a summary of the fix: Root cause: Both filepath.Join calls used OS path separators. On Windows, filepath.Join("/host", "/var/log") produces \host\var\log, breaking tests that expected Linux-style paths. Similarly, filepath.Join("/host", "/proc") produced \host\proc which didn't match the mock key /host/proc, causing resolveProcPath to fall back to /proc. Fix: Replaced filepath.Join with path.Join (POSIX package) in three files: - pkg/config/setup/privateactionrunner.go — container path prefix joining - pkg/config/setup/privateactionrunner_test.go — test assertion using same join - pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go — resolveProcPath These are Linux container paths (/host/...) that must always use forward slashes regardless of the host OS. Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com>
AlexandreYang
added
changelog/no-changelog
AlexandreYang
requested review from
dd-gplassard
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d3380c13e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if env.IsContainerized() { | ||
| for i, v := range defaultPaths { | ||
| hostPath := path.Join(containerizedPathPrefix, v) | ||
| defaultPaths[i] = hostPath | ||
| } |
There was a problem hiding this comment.
In containerized mode this always rewrites the default rshell allowlist entry from /var/log to /host/var/log. In the no-host-mount case (the scenario covered by TestPrivateActionRunnerAllowedPathsContainerizedWithoutHostMounts), RunCommand still forwards that path to interp.AllowedPaths, and rshell sandbox creation fails for missing directories, so runCommand can fail with failed to create runner for every task. Please keep a usable fallback (e.g., /var/log) or drop non-existent paths before constructing the runner.
Useful? React with 👍 / 👎.
| log.Debugf("rshell runCommand: command=%q allowedCommands=%v allowedPaths=%v", | ||
| inputs.Command, inputs.AllowedCommands, h.allowedPaths) |
There was a problem hiding this comment.
This now logs inputs.Command verbatim, which can include secrets (for example API tokens passed in command arguments). When debug logging is enabled, sensitive task payload data will be written to agent logs and potentially forwarded downstream. Redact/scrub the command contents (or log only metadata like command name) to avoid leaking credentials.
Useful? React with 👍 / 👎.
1 participant
…ntainer-aware prefixing"" (#48462)
Reverts #48460
Revert + this test fix:
All tests pass. Here's a summary of the fix:
Root cause: Both filepath.Join calls used OS path separators. On Windows, filepath.Join("/host", "/var/log") produces \host\var\log, breaking tests that expected Linux-style paths. Similarly,
filepath.Join("/host", "/proc") produced \host\proc which didn't match the mock key /host/proc, causing resolveProcPath to fall back to /proc.
Fix: Replaced filepath.Join with path.Join (POSIX package) in three files:
These are Linux container paths (/host/...) that must always use forward slashes regardless of the host OS.
What does this PR do?
Motivation
Describe how you validated your changes
Additional Notes