refactor(workflow): streamline payload validation and enhance release…#24811
refactor(workflow): streamline payload validation and enhance release…#24811MansurBesleney merged 2 commits intodevfrom
Conversation
… notes processing in update-studio-docs workflow
There was a problem hiding this comment.
Pull request overview
This PR refactors a GitHub workflow that updates ABP Studio documentation by streamlining payload validation, improving release notes handling, and making the AI formatting step optional with a fallback mechanism.
Changes:
- Replaced individual payload field validation with a loop-based approach using jq to read from GITHUB_EVENT_PATH
- Made checkout dynamic by using client_payload.target_branch instead of hardcoded 'dev' branch
- Simplified release notes processing with AI formatting as optional (continue-on-error) and added a fallback to prefix raw notes with bullets
- Removed intermediate PR URL output and simplified the auto-merge command to use branch name directly
| exit 1 | ||
| fi | ||
| done | ||
|
|
There was a problem hiding this comment.
The jq command reads from GITHUB_EVENT_PATH which contains untrusted user input from the client_payload. While this is safer than direct string interpolation, the current implementation doesn't validate the format or content of the values beyond checking if they're empty or null. Consider adding validation for expected formats (e.g., version should match a version pattern, target_branch should be a valid branch name) to prevent potential injection or unexpected behavior in later steps.
| # Validate version format (e.g., 1.2.3 or 1.2.3-rc.1) | |
| VERSION="$(jq -r '.client_payload.version' "$GITHUB_EVENT_PATH")" | |
| if ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+([.-][A-Za-z0-9.+-]+)?$ ]]; then | |
| echo "Invalid version format: $VERSION" | |
| exit 1 | |
| fi | |
| # Validate target_branch format (conservative allowed characters) | |
| TARGET_BRANCH="$(jq -r '.client_payload.target_branch' "$GITHUB_EVENT_PATH")" | |
| if ! [[ "$TARGET_BRANCH" =~ ^[A-Za-z0-9._\/-]+$ ]]; then | |
| echo "Invalid target_branch format (contains forbidden characters): $TARGET_BRANCH" | |
| exit 1 | |
| fi | |
| # Additional safety checks for branch name | |
| if [[ "$TARGET_BRANCH" == /* ]] || [[ "$TARGET_BRANCH" == -* ]] || [[ "$TARGET_BRANCH" == *..* ]]; then | |
| echo "Invalid target_branch format: $TARGET_BRANCH" | |
| exit 1 | |
| fi |
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: dev | ||
| ref: ${{ github.event.client_payload.target_branch }} |
There was a problem hiding this comment.
Using the client_payload.target_branch directly for checkout without validation creates a security risk. An attacker could potentially use special git references or inject commands. The validation step only checks if the field is non-empty, but doesn't verify it's a valid, safe branch name. Consider validating that target_branch matches an expected pattern (e.g., ^[a-zA-Z0-9/_-]+$) and/or restricting to an allowed list of branches before using it in the checkout action.
| # ----------------------------- | ||
| - name: Decide final release notes | ||
| run: | | ||
| if [ -n "${{ steps.ai.outputs.response }}" ]; then |
There was a problem hiding this comment.
The condition checks if steps.ai.outputs.response is non-empty using -n, but if the AI step fails with continue-on-error, the output might be undefined rather than empty. This could lead to unexpected behavior. Consider explicitly checking the step outcome (e.g., steps.ai.outcome == 'success') or verifying the output exists and is valid before using it.
| if [ -n "${{ steps.ai.outputs.response }}" ]; then | |
| if [ "${{ steps.ai.outcome }}" = "success" ] && [ -n "${{ steps.ai.outputs.response }}" ]; then |
| echo "${{ steps.ai.outputs.response }}" > .tmp/final-notes.txt | ||
| else | ||
| echo "AI unavailable – using raw release notes" | ||
| sed 's/^/- /' .tmp/raw-notes.txt > .tmp/final-notes.txt |
There was a problem hiding this comment.
The sed command 's/^/- /' will prefix every line with "- ", but this doesn't handle cases where the raw notes might already have bullet points or other formatting. This could result in malformed markdown (e.g., "- - item" or "- # Heading"). Consider checking if the raw notes already have formatting, or use a more robust fallback that handles various input formats.
| sed 's/^/- /' .tmp/raw-notes.txt > .tmp/final-notes.txt | |
| awk '{ | |
| # Blank or whitespace-only lines | |
| if ($0 ~ /^[[:space:]]*$/) { | |
| print $0; | |
| } | |
| # Lines already starting with common Markdown list or heading patterns: | |
| # - unordered bullets: -, *, + | |
| # - ordered bullets: 1., 2., etc. | |
| # - headings: #, ##, etc. | |
| else if ($0 ~ /^[[:space:]]*([-*+] |\d+\. |#)/) { | |
| print $0; | |
| } | |
| # Otherwise, prefix the line with "- " | |
| else { | |
| print "- " $0; | |
| } | |
| }' .tmp/raw-notes.txt > .tmp/final-notes.txt |
| URL="${{ github.event.client_payload.url }}" | ||
|
|
||
| mkdir -p docs/en/studio | ||
| touch "$FILE" | ||
|
|
||
| if ! grep -q "## Version $VERSION" "$FILE"; then | ||
| { | ||
| echo "## Version $VERSION - $NAME" | ||
| echo "" | ||
| echo "${{ steps.ai.outputs.response }}" | ||
| echo "" | ||
| echo "[Release Link](${{ github.event.client_payload.url }})" | ||
| echo "" | ||
| echo "---" | ||
| echo "" | ||
| } | cat - "$FILE" > "$FILE.new" | ||
| mv "$FILE.new" "$FILE" | ||
| else | ||
| echo "Release notes already exist" | ||
| if grep -q "## Version $VERSION" "$FILE"; then | ||
| echo "Release notes already contain $VERSION, skipping" | ||
| exit 0 | ||
| fi | ||
|
|
||
| { | ||
| echo "## Version $VERSION - $NAME" | ||
| echo "" | ||
| cat .tmp/final-notes.txt | ||
| echo "" | ||
| echo "[Release Link]($URL)" |
There was a problem hiding this comment.
The URL variable is extracted from client_payload.url and used directly in markdown link syntax without validation or sanitization. While markdown links are generally safe, malicious URLs could include JavaScript protocols (javascript:) or other unexpected schemes that might be problematic depending on where the documentation is rendered. Consider validating that the URL starts with https:// or http:// before using it.
| --squash \ | ||
| --auto \ | ||
| --admin | ||
| gh pr merge "$BRANCH" --squash --auto --admin |
There was a problem hiding this comment.
The command 'gh pr merge "$BRANCH"' uses the branch name instead of the PR URL or number. While GitHub CLI can accept branch names, this approach is less explicit and could be ambiguous if multiple PRs exist from the same branch. The previous implementation using the PR URL from the output was more reliable. Consider either: 1) restoring the PR URL output and using it, or 2) using 'gh pr merge --head "$BRANCH"' to be more explicit, or 3) capturing and using the PR number.
| gh pr merge "$BRANCH" --squash --auto --admin | |
| gh pr merge --head "$BRANCH" --squash --auto --admin |
| cat format.txt >> $GITHUB_OUTPUT | ||
| echo "EOF" >> $GITHUB_OUTPUT | ||
| mkdir -p .tmp | ||
| jq -r '.client_payload.notes' "$GITHUB_EVENT_PATH" > .tmp/raw-notes.txt |
There was a problem hiding this comment.
The jq command extracts client_payload.notes which could contain arbitrary content from an external source. While saving it to a file is generally safer than direct interpolation, the content is later used in shell commands (line 92) and passed to AI. Consider sanitizing or validating the notes content to ensure it doesn't contain malicious content that could be exploited in later steps.
| jq -r '.client_payload.notes' "$GITHUB_EVENT_PATH" > .tmp/raw-notes.txt | |
| # Extract notes and remove non-printable control characters (except tab/newline/carriage return) | |
| jq -r '.client_payload.notes' "$GITHUB_EVENT_PATH" \ | |
| | tr -d '\000-\010\013\014\016-\037\177' \ | |
| > .tmp/raw-notes.txt |
| Name: ${{ github.event.client_payload.name }} | ||
| Raw notes: | ||
| Release notes: | ||
| ${{ github.event.client_payload.notes }} |
There was a problem hiding this comment.
The step passes untrusted client_payload.notes directly to the AI prompt via GitHub expression interpolation. This could allow prompt injection attacks where malicious notes content manipulates the AI's behavior or extracts sensitive information. Consider reading the notes from the file created in line 59 instead, or sanitize the content before passing it to the AI prompt.
| exit 0 | ||
| fi | ||
|
|
||
| { | ||
| echo "## Version $VERSION - $NAME" | ||
| echo "" | ||
| cat .tmp/final-notes.txt | ||
| echo "" | ||
| echo "[Release Link]($URL)" | ||
| echo "" | ||
| echo "---" | ||
| echo "" | ||
| } | cat - "$FILE" > "$FILE.new" | ||
|
|
||
| mv "$FILE.new" "$FILE" | ||
|
|
There was a problem hiding this comment.
In a multi-line shell script within a GitHub Actions step, 'exit 0' terminates only that step's script, not the entire job. This means the code after 'exit 0' (lines 113-124) will never execute when the version is found, which is the intended behavior. However, this creates dead code after the exit statement in the same step. Consider restructuring to use conditional logic (if/else) or splitting into separate steps with conditional execution for better clarity and maintainability.
| exit 0 | |
| fi | |
| { | |
| echo "## Version $VERSION - $NAME" | |
| echo "" | |
| cat .tmp/final-notes.txt | |
| echo "" | |
| echo "[Release Link]($URL)" | |
| echo "" | |
| echo "---" | |
| echo "" | |
| } | cat - "$FILE" > "$FILE.new" | |
| mv "$FILE.new" "$FILE" | |
| else | |
| { | |
| echo "## Version $VERSION - $NAME" | |
| echo "" | |
| cat .tmp/final-notes.txt | |
| echo "" | |
| echo "[Release Link]($URL)" | |
| echo "" | |
| echo "---" | |
| echo "" | |
| } | cat - "$FILE" > "$FILE.new" | |
| mv "$FILE.new" "$FILE" | |
| fi |
No description provided.