Skip to content
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.

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

curl escape #534

Merged
merged 3 commits into from
Jun 11, 2024
Merged

curl escape #534

merged 3 commits into from
Jun 11, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Jun 11, 2024

Fix JSON escaping of curl commands

  • 🔄 The curl command in the script has been modified to explicitly use the POST method by adding -X POST.
  • 🛠️ The JSON data passed to the -d option of curl is now being sanitized to escape single quotes properly, ensuring that the JSON string is correctly formatted for the shell command.

generated by pr-describe

Copy link

The changes in the GIT_DIFF indicate that the curl command in a TypeScript file has been modified to explicitly use the POST method and to handle single quotes in the JSON payload more robustly.

Concerns:

  • The replacement of single quotes in the JSON string using .replace(/'/g, "'\\''") ensures that single quotes within the JSON payload are properly escaped for the shell command. This is a good practice for preventing issues with shell command execution. However, it's important to ensure that this replacement does not inadvertently alter the intended data within the JSON payload.

Suggestion:

  • Ensure that the escaping of single quotes is tested with various JSON payloads to confirm that it does not introduce any unintended changes to the data.

Given that the changes are related to command-line execution and not directly to TypeScript code functionality, and assuming the escaping has been properly tested, the changes look good.

Response: LGTM 🚀

generated by pr-review

@@ -130,12 +130,12 @@ export const OpenAIChatCompletion: ChatCompletionHandler = async (

trace.detailsFenced(
`✉️ fetch`,
`curl ${url} \\
`curl -X POST ${url} \\

Choose a reason for hiding this comment

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

The curl command should specify the -X POST method to ensure the request is sent as a POST request. Great catch on updating this! 👍

generated by pr-review-commit

@@ -14,7 +14,9 @@ The reviewer can also suggest changes to the code, documentation, or other files

The output of the LLM is inserted as a comment in the pull request conversation
(and updated as needed to avoid duplicates).
Here is an [example](https://github.com/microsoft/genaiscript/pull/504#issuecomment-2145273728) in the GenAIScript repository.

Here is an [pull request](https://github.com/microsoft/genaiscript/pull/534) in the GenAIScript repository

Choose a reason for hiding this comment

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

The link text "pull request" should be more descriptive to improve accessibility and SEO. Consider changing it to "example pull request in the GenAIScript repository".

generated by pr-docs-review-commit

@pelikhan pelikhan merged commit 2f19b60 into main Jun 11, 2024
9 checks passed
@pelikhan pelikhan deleted the azure-vshost branch June 11, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant