Avoid collecting unsanitized process arguments#1993
Conversation
Raw command arguments can contain credentials or other sensitive data. Keep process.command, expose only process.args_count, and verify command_args and command_line remain unset. Closes open-telemetry#1604 Risk-Level: low AI-Agent: codex
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
![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: bbb83fa4d1
ℹ️ 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 (is_array($argv)) { | ||
| $command = $argv[0] ?? null; | ||
| if (is_string($command)) { | ||
| $attributes[ProcessIncubatingAttributes::PROCESS_COMMAND] = $command; |
There was a problem hiding this comment.
When this detector runs under CGI/FastCGI with register_argc_argv enabled, PHP populates $_SERVER['argv'] from the request query string rather than the OS process command. Keeping process.command sourced from $argv[0] means the default resource can still export user-supplied query data such as ?token=secret as the supposedly safe fallback after process.command_args was removed; gate this on CLI (or use a real process command source) before setting command-derived attributes.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1993 +/- ##
============================================
+ Coverage 68.02% 68.04% +0.02%
- Complexity 3075 3076 +1
============================================
Files 459 459
Lines 9017 9020 +3
============================================
+ Hits 6134 6138 +4
+ Misses 2883 2882 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@ChrisLightfootWild , can help me to review this PR? |
There was a problem hiding this comment.
Pull request overview
This PR updates the process resource detector to avoid collecting potentially sensitive raw process arguments by default, aligning with updated semantic convention guidance. Instead, it retains a non-sensitive fallback (process.command and process.args_count) and adds/updates tests to ensure process.command_args and process.command_line remain unset.
Changes:
- Stop emitting
process.command_argsfrom the Process resource detector by default. - Add
process.args_countas a non-sensitive replacement signal alongsideprocess.command. - Update unit/integration/config tests to assert the new behavior and attribute set.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/SDK/Resource/Detectors/Process.php | Update detector to emit process.command + process.args_count and omit process.command_args. |
| tests/Unit/SDK/Resource/Detectors/ProcessTest.php | Add a targeted argv scenario and assert args are not collected while count is reported. |
| tests/Integration/SDK/Resource/ResourceInfoFactoryTest.php | Update integration assertions to require process.args_count and ensure command args/line are unset. |
| tests/Integration/Config/ConfigurationTest.php | Update expected resource keys to include process.args_count and verify process.command_args is not present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
process.command_argsby default because arguments may contain sensitive dataprocess.commandand reportprocess.args_countas the non-sensitive fallbackprocess.command_argsandprocess.command_lineremain unsetCloses #1604.
Validation
make all-checksdoes not currently complete on a cleanmaincheckout because the unpinned local Rector installation reports 22 pre-existing dry-run suggestions. The checks above were run individually; Rector is not part of the upstream CI workflow.