fix(integrations): escape Rich markup in --integration-options error messages#3458
fix(integrations): escape Rich markup in --integration-options error messages#3458Quratulain-bilal wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Handles malformed integration-option quoting without exposing ValueError tracebacks.
Changes:
- Converts
shlex.splitfailures intotyper.Exit(1). - Adds regression coverage for unbalanced quotes.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/integrations/_helpers.py |
Handles malformed quoting during option parsing. |
tests/integrations/test_integration_subcommand.py |
Tests clean failure on unbalanced quotes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
|
escaped the value before interpolating it in aa810ba — you're right that swept the two sibling branches in the same function too (the 'unexpected option value' and 'unknown option' prints both interpolate the raw token), since a bare regression covers both: an unbalanced-quote value carrying markup, and a plain markup token. confirmed both raise MarkupError on the pre-fix code and exit cleanly with the escape. |
|
Please resolve conflicts |
…oting
_parse_integration_options called shlex.split(raw_options) unguarded. an
unbalanced quote (e.g. --integration-options='--commands-dir "foo') makes
shlex raise ValueError('No closing quotation'), so a raw traceback escaped
instead of the typer.Exit(1) error every other bad-input path in this function
produces. reachable from specify init and every integration install/switch/
upgrade/migrate that accepts --integration-options.
wrap the split and convert ValueError into the same clean CLI error. added a
regression test; confirmed it fails on the pre-fix code (raw ValueError).
the malformed-quoting handler (and the unexpected/unknown option branches) interpolate raw_options/token into console.print. a value carrying an unbalanced rich tag like '--commands-dir "[/red]foo' first trips the intended shlex ValueError, but the error print then raises rich.errors.MarkupError and leaks a traceback anyway. escape all three before printing so the clean typer.Exit survives. added a regression covering both the shlex path and a bare markup token.
aa810ba to
8655efe
Compare
| token = tokens[i] | ||
| if not token.startswith("-"): | ||
| console.print(f"[red]Error:[/red] Unexpected integration option value '{token}'.") | ||
| console.print(f"[red]Error:[/red] Unexpected integration option value '{escape(token)}'.") |
| raw_options is user-controlled. A value like '--commands-dir "[/red]foo' | ||
| first trips the shlex ValueError path, but the error message then | ||
| interpolates the raw value into console.print — an unbalanced Rich tag | ||
| such as '[/red]' would raise rich.errors.MarkupError there and leak a | ||
| traceback anyway. The value must be escaped so the clean typer.Exit |
|
Please address Copilot feedback |
The removed test's docstring claimed the shlex failure branch
interpolates raw_options into console.print; it only prints {exc},
which never contains the caller's markup. Its two assertions also
duplicated test_bad_option_token_with_rich_markup_exits_cleanly (the
'[/red]foo' case) and the shlex-path case already covered by
test_unbalanced_quote_exits_cleanly. The real change here remains the
escape() of the two user-controlled token prints.
|
Addressed the Copilot feedback. The flagged test ( |
Quratulain-bilal
changed the title
|
Updated the PR title and description to match the actual scope. You're right that the |
Follow-up hardening to #3457 (the unbalanced-quote crash, since fixed on
main)._parse_integration_optionsinterpolated user-controlled option tokens straight intoconsole.printin two branches:-;-but is not declared.Because
console.printparses Rich markup, a token carrying an unbalanced tag (e.g.[/red]fooor--[/red]bad) raisesrich.errors.MarkupErrorand leaks a traceback instead of the cleantyper.Exit(1)every other bad-input path produces.fix: wrap both token values in
rich.markup.escape(...)before printing, so malformed markup is shown literally and the command still exits 1.scope note: the
shlex.splitValueErrorconversion for #3457 is already onmain; this PR does not re-introduce it. The only production change here is the markup escaping of the two token prints.Added
test_bad_option_token_with_rich_markup_exits_cleanly, which asserts a cleantyper.Exitfor both the unexpected-value ([/red]foo) and unknown-option (--[/red]bad) branches; it fails on the pre-escape code (rawMarkupError) and passes with the fix. The existingtest_unbalanced_quote_exits_cleanlycontinues to cover the shlex path.