fix(x2a): Fix/policy lock token leak#2636
fix(x2a): Fix/policy lock token leak#2636elai-shalev wants to merge 2 commits intoredhat-developer:mainfrom
Conversation
Review Summary by QodoFix SCM token leak in Policyfile.lock.json copying
WalkthroughsDescription• Prevent Policyfile.lock.json token leak by removing wildcard JSON copy • Replace generic JSON copy with specific metadata file only • Improve URL credential sanitization regex pattern • Remove JSON copying from analyze and convert phases Diagramflowchart LR
A["Wildcard JSON copy<br/>*.json"] -->|Remove| B["Specific file copy<br/>generated-project-metadata.json"]
C["Generic regex<br/>token@host"] -->|Improve| D["Enhanced regex<br/>credentials sanitization"]
E["Init/Analyze/Convert<br/>phases"] -->|Update| F["Secure file handling"]
File Changes1. workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh
|
Code Review by Qodo
1. Metadata read can fail
|
Changed Packages
|
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
| # Copy generated metadata (only specific files — avoid copying Chef artifacts like Policyfile.lock.json) | ||
| cp -v "${SOURCE_BASE}/generated-project-metadata.json" "${PROJECT_PATH}/" 2>/dev/null || true |
There was a problem hiding this comment.
1. Metadata read can fail 🐞 Bug ⛯ Reliability
In the init phase, the script suppresses errors when copying generated-project-metadata.json but then unconditionally reads it with cat under set -e, which will terminate the job if the file wasn’t produced and prevent artifact reporting. This contradicts the backend behavior, which explicitly supports init success without any project_metadata artifact.
Agent Prompt
### Issue description
`x2a-job-script.sh` treats `generated-project-metadata.json` as optional (copy errors suppressed) but then unconditionally reads it with `cat` while the script is running with `set -e`. If the file is absent, the init phase exits and the job can’t report artifacts.
### Issue Context
The backend explicitly supports init success without any `project_metadata` artifact (phase action is skipped when the artifact is missing), so the job script should either (a) only add the artifact when the file exists, or (b) make the metadata file required and fail with a clear error message (and remove `|| true`).
### Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh[234-256]
### Suggested change (behavior: optional)
- After copying, check `-f "${PROJECT_PATH}/generated-project-metadata.json"`.
- Only then `cat` it and append `ARTIFACTS+=("project_metadata:${METADATA}")`.
- Otherwise, log that metadata wasn’t produced and continue (still report migration_plan).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Note: x2a tool produces migration-plan-{module_name}.md (spaces replaced with underscores) | ||
| echo "Copying output to ${OUTPUT_DIR}/" | ||
| cp -v "${SOURCE_BASE}/migration-plan-${MODULE_NAME_SANITIZED}.md" "${OUTPUT_DIR}/" | ||
| cp -v "${SOURCE_BASE}"/*.json "${OUTPUT_DIR}/" 2>/dev/null || true |
There was a problem hiding this comment.
wait, these files are important, and we don't have here migration-dependencies when we should!
When migration cache, we are referencing all inside the migration-dependencies, inside the migration-plan-module, and we should be able to analyze that files to write the ansible code correctly.
This file is the main lock file for when we donwload all the vendor dependencies when we used, and it's our "DNS" for the dependencies.
Without this folder, and the json files we shouldn't be able to do the right on cache, because cache to_ansible will try to read files that are not in there
There was a problem hiding this comment.
Were still copying the ""${SOURCE_BASE}/generated-project-metadata.json" file, which has the needed metadata
I've just ommitted the Policyfile.lock.json and the solo.json
There was a problem hiding this comment.
both files are needed, also the migration_dependencies which are referenced on migration-plan-cache.md
There was a problem hiding this comment.
ok, I'll revert the changes and add more guards around sanitization all of the committed files.
2 participants
This PR will remove the generic copy command that includes the policy.lock.json in the commited code to the target repo, and will improve the sanitization.