Skip to content

fix(x2a): Fix/policy lock token leak #2636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elai-shalev wants to merge 2 commits into
base: main
Choose a base branch
from
+10 −7
Open

fix(x2a): Fix/policy lock token leak #2636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ddd703
fix policy lock json secret commit
elai-shalev Mar 29, 2026
cc8c485
changeset
elai-shalev Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions workspaces/x2a/.changeset/fix-policy-lock-token-leak.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@red-hat-developer-hub/backstage-plugin-x2a-backend': patch
---

Fix SCM token leak in Policyfile.lock.json: stop copying wildcard JSON files from Chef source directory and improve URL credential sanitization regex
12 changes: 5 additions & 7 deletions workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ sanitize_secrets() {
# Match GitHub PATs (ghp_, gho_, github_pat_) and generic token@host patterns in URLs
local count=0
while IFS= read -r -d '' file; do
if grep -qE 'https?://[^@/:[:space:]]+@' "$file" 2>/dev/null; then
# Strip token from URLs: https://ghp_xxx@github.com/... → https://github.com/...
sed -i 's|https\?://[^@/:[:space:]]*@|https://|g' "$file"
if grep -qE 'https?://[^@/[:space:]]+@' "$file" 2>/dev/null; then
# Strip credentials from URLs: https://user:token@host/... → https://host/...
sed -i 's|https\?://[^@/[:space:]]*@|https://|g' "$file"
echo " Sanitized: ${file#/workspace/target/}"
count=$((count + 1))
fi
Expand Down Expand Up @@ -235,8 +235,8 @@ case "${PHASE}" in
# Note: x2a tool writes files to the source directory (--source-dir)
echo "Copying output to ${PROJECT_PATH}/"
cp -v "${SOURCE_BASE}/migration-plan.md" "${PROJECT_PATH}/"
# Copy any other generated files (like metadata)
cp -v "${SOURCE_BASE}"/*.json "${PROJECT_PATH}/" 2>/dev/null || true
# 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
Comment on lines +238 to +239
Copy link
Copy Markdown

@rhdh-qodo-merge rhdh-qodo-merge bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

cp -v "${SOURCE_BASE}"/*.yaml "${PROJECT_PATH}/" 2>/dev/null || true

# Show what was created
Expand Down Expand Up @@ -299,7 +299,6 @@ case "${PHASE}" in
# 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
Copy link
Copy Markdown
Contributor

@eloycoto eloycoto Mar 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@elai-shalev elai-shalev Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@eloycoto eloycoto Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both files are needed, also the migration_dependencies which are referenced on migration-plan-cache.md

Copy link
Copy Markdown
Contributor Author

@elai-shalev elai-shalev Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll revert the changes and add more guards around sanitization all of the committed files.

cp -v "${SOURCE_BASE}"/*.yaml "${OUTPUT_DIR}/" 2>/dev/null || true

echo ""
Expand Down Expand Up @@ -361,7 +360,6 @@ case "${PHASE}" in
# Note: x2a tool writes to ansible/roles/{module}/ in the source directory
echo "Copying output to ${OUTPUT_DIR}/"
cp -rv "${SOURCE_BASE}/ansible" "${OUTPUT_DIR}/" 2>/dev/null || true
cp -v "${SOURCE_BASE}"/*.json "${OUTPUT_DIR}/" 2>/dev/null || true
cp -v "${SOURCE_BASE}"/*.yaml "${OUTPUT_DIR}/" 2>/dev/null || true

echo ""
Expand Down
Loading