Repository navigation
Conversation
Signed-off-by: tman <tman@redhat.com>
PR Summary by QodoScan changed operator files for secret leaks
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
1.
|
![qodo-redhat-openshift-ecosystem[bot]](https://avatars.githubusercontent.com/u/59707467?s=60&v=4){kind=small}
| absolute_path = repo_root / rel_path | ||
| if absolute_path.is_file(): | ||
| paths_to_scan.append(absolute_path) |
There was a problem hiding this comment.
4. A linked operator file scans outside checkout 🐞 Bug ⛨ Security
check_leaks_in_changed_files accepts a joined path when is_file() succeeds, which also accepts symbolic links to files outside the repository. If a changed operator path links to an external file readable by the task, that external file is submitted to LeakTK and its reported path can appear in the check result.
Agent Prompt
## Issue description
The new scan follows changed-file symlinks outside the operator checkout.
## Fix Focus Areas
- operatorcert/entrypoints/detect_changed_operators.py[500-506]
- operatorcert/static_tests/common/operator.py[34-39]
## Recommended Fix
Reject symlinks or resolve each candidate and require its target to remain inside the checkout and intended operator directory before adding it to the scan. Test a link to an external readable file.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Newline-separated paths under operators/ (avoids Tekton result size limits). | ||
| jq -r '.affected_operator_files[]?' < changes.json > affected_operator_files.txt |
There was a problem hiding this comment.
3. Newline filenames evade the leak check 🐞 Bug ≡ Correctness
parse-repo-changes writes raw paths separated by newlines, while load_affected_operator_files treats each line as a separate path. If a changed filename beneath a valid bundle contains a newline, its two fragments do not identify the existing file and the check silently omits it.
Agent Prompt
## Issue description
A newline within a changed filename splits the new path list into entries that cannot be scanned.
## Fix Focus Areas
- ansible/roles/operator-pipeline/templates/openshift/tasks/parse-repo-changes.yml[114-116]
- operatorcert/entrypoints/static_tests.py[67-87]
- operatorcert/static_tests/common/operator.py[34-42]
## Recommended Fix
Store and read the affected paths using a format that preserves arbitrary filename characters, such as a JSON array, instead of newline delimiting. Cover a filename containing a newline.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| results_jsonl = subprocess.check_output( | ||
| ["leaktk", "listen"], | ||
| input=requests, | ||
| text=True, | ||
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| except subprocess.CalledProcessError as exc: | ||
| # LeakTK output may contain secret match text; do not propagate it. | ||
| raise RuntimeError( | ||
| f"LeakTK scan failed with exit code {exc.returncode}" | ||
| ) from None |
There was a problem hiding this comment.
5. Leaktk failures leave no diagnostic trail 🐞 Bug ◔ Observability
scan now sends LeakTK's stderr to subprocess.DEVNULL and raises a RuntimeError that carries only the exit code. check_leaks_in_changed_files then logs only the exception type, so the leak check, upload_artifacts and create_github_gist all fail with no hint of the cause. That includes config or pattern-fetch errors, a missing binary config, or a timeout, and operators can't tell which one it was.
Agent Prompt
## Issue description
`scan` discards LeakTK stderr and only reports the exit code. Every leaktk failure in the static check and in the artifact/gist upload flows has no root cause in the logs.
## Fix Focus Areas
- operatorcert/redact.py[63-74]
- operatorcert/static_tests/common/operator.py[49-58]
## Recommended Fix
Use `stderr=subprocess.PIPE` instead of DEVNULL. When `CalledProcessError` is raised, run the captured stderr through `leaktk redact --kind Stdio`, or truncate it, then log it at error level. Keep the raised exception message generic. Don't log stdout, because it can contain match text.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for rel_path in affected_files: | ||
| if not rel_path.startswith(operator_prefix): | ||
| continue |
There was a problem hiding this comment.
2. Second operator's secrets go unscanned 🔗 Cross-repo conflict ⛨ Security
check_leaks_in_changed_files filters changed files to the operator passed to it, while ParserResults.enrich_result() selects only the first affected operator for static tests. When an allowed community-operators-prod pull request changes non-bundle files for multiple operators, such as their ci.yaml files, the other operators’ paths remain in change detection but are excluded from the scan.
Agent Prompt
## Issue description
An allowed multi-operator pull request can change non-bundle files for several operators, but the hosted pipeline selects one operator as its static-test target and the operator-scoped leak check excludes the others’ files.
## Fix Focus Areas
- operatorcert/static_tests/common/operator.py[27-50]
- operatorcert/parsed_file.py[205-225]
- operatorcert/entrypoints/static_tests.py[109-121]
- ansible/roles/operator-pipeline/templates/openshift/pipelines/operator-hosted-pipeline.yml[474-506]
## Recommended Fix
Ensure the leak check scans changed files for every affected operator, independently of the single operator and bundle selected for other static checks. Either scan all affected operator paths directly or run the operator check for every affected operator; keep the changed-file list in the workspace so the community pipeline can check each one. Add a test covering non-bundle changes to multiple operators.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This branch has not been deployed
1 participant
Added static check for leak detection in affected operator files (catalog files intentionally excluded after discussion).
leaktkerrorsTested on separate deployment - example PR here with the check output in gist here.
Closes: ISV-7642
Merge Request Checklists