fix(workflows): escape remaining untrusted fields in workflow info#3731
Open
Noor-ul-ain001 wants to merge 2 commits into
Open
fix(workflows): escape remaining untrusted fields in workflow info#3731Noor-ul-ain001 wants to merge 2 commits into
workflow info#3731Noor-ul-ain001 wants to merge 2 commits into
Conversation
Follow-up to github#3690, which escaped only the step-graph brackets. Every other metadata field `workflow info` prints is untrusted content (workflow.yml or catalog JSON), and console.print has Rich markup enabled, so an unescaped `[...]` in any of them is parsed as a style tag and silently swallowed: - definition path: name, version, author, description, integration, and each input's name/type - catalog path: name, version, description, tags, and the "not found" workflow id A description of `Does [stuff] nicely` rendered as `Does nicely`; an integration of `claude [code]` rendered as `claude `. Route every field through _escape_markup, matching the sibling `workflow list` / catalog `search` commands, so bracketed text renders literally. Add two regression tests covering the definition and catalog paths; both fail on the pre-fix source (fields with brackets come back truncated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Escapes untrusted workflow metadata before Rich rendering to preserve bracketed content.
Changes:
- Escapes definition, catalog, input, tag, and error fields.
- Adds definition- and catalog-path regression tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/_commands.py |
Escapes workflow-info output fields. |
tests/test_workflows.py |
Tests bracketed metadata rendering. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
tests/test_workflows.py:7993
- The catalog-path test likewise uses a plain version and has no version assertion, leaving the new escape at
_commands.py:2418uncovered. Make the fixture version contain brackets and assert that literal version in the rendered output.
"name": "Cat [WF]",
"version": "2.0.0",
"description": "From [catalog]",
"tags": ["a [b]", "c [d]"],
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
+7947
to
+7950
| name="My [WF]", | ||
| id="my-wf", | ||
| version="1.0.0", | ||
| author="Jane [Doe]", |
Comment on lines
+2426
to
+2428
| console.print( | ||
| f"[red]Error:[/red] Workflow '{_escape_markup(str(workflow_id))}' not found" | ||
| ) |
Collaborator
|
Please address Copilot feedback |
Addresses Copilot review feedback on the workflow-info markup-escape tests: - The definition-path and catalog-path regression tests left `version` bracket-free and never asserted it, so the version escapes could be removed without failing. Use bracketed version values and assert they survive verbatim. - The newly escaped not-found identifier is a separate output path that no test reached. Add a case where local load raises FileNotFoundError and catalog lookup returns None, invoke `workflow info` with a bracketed ID, and assert the literal ID is preserved in the error. Verified each new assertion fails when its source escape is removed (test-the-test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed both Copilot points in 0069726:
Verified each new assertion fails when its source escape is removed (test-the-test). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Follow-up to #3690, which escaped only the step-graph brackets in
workflow info. Every other metadata field the command prints is untrusted content (fromworkflow.ymlor catalog JSON), andconsole.printhas Rich markup enabled — so an unescaped[...]in any of them is parsed as a style tag and silently swallowed.Affected fields:
'<id>' not founderror idConcretely, a description of
Does [stuff] nicelyrendered asDoes nicely; an integration ofclaude [code]rendered asclaude; an input type ofstr [ing]rendered asstr.Fix
Route every field through
_escape_markup(already imported and used by the siblingworkflow listand catalogsearchcommands), so bracketed text renders literally. This is the same defect and the same remedy as #3690, extended to the rest of the command.Tests
Two regression tests added to
TestWorkflowInfoStepGraph, covering the definition path and the catalog path. Both fail on the pre-fix source (bracketed fields come back truncated) and pass with the fix. Verified viagit stashtest-the-test.🤖 Generated with Claude Code