Skip to content

Conversation

@antongolub
Copy link
Collaborator

@antongolub antongolub commented Mar 29, 2025

  • Tests pass
  • Appropriate changes to README are included in PR

@antongolub antongolub requested a review from Copilot March 29, 2025 15:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a quoting issue by adding a test case to simulate broken quoting and updating documentation to warn users about potential unsafe injection due to double quoting.

  • Added a test case ("broken quoting") in test/core.test.js to capture unexpected output with mixed quoting.
  • Updated docs/quotes.md with a warning block that highlights the risk of unsafe injection when using additional quotes.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/core.test.js Introduces a test case demonstrating issues with broken quoting.
docs/quotes.md Enhances documentation with a warning about unsafe injection.
Comments suppressed due to low confidence (2)

test/core.test.js:105

  • [nitpick] Consider renaming the test to more explicitly describe the expected failure scenario or behavior for clarity.
test('broken quoting', async () => {

test/core.test.js:107

  • Using mixed shell quoting syntax here may lead to unexpected behavior in environments with different shell quoting rules. Consider confirming that the intended behavior is reliable across systems or adjusting the quoting method for clarity.
const p = $`echo --foo=$'${args}'`

@antongolub antongolub requested a review from antonmedv March 29, 2025 15:17
@antongolub antongolub merged commit 939cdfa into google:main Mar 29, 2025
26 checks passed
@antongolub antongolub deleted the docs-quote-warn branch March 29, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant