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

fix: escaped debugging output #5176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: escaped debugging output #5176

wants to merge 2 commits into from

Conversation

CommanderStorm
Copy link
Collaborator

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #5152 (comment)

Did not think about this because only the user can add these injections => why would anyone inject themselves, but better save than sorry.

I did not use any of the two packages, but rather rolled my own version.

  • execa seems a bit too heavy for our usecase
  • shell-escape is doing something similar as I am doing, but what they are not handling that something like $HOME which adds the home environment variable or the backticks which executes the shell can also escape the shell.

This is the result

curl --verbose --head --request GET \
 --user-agent 'Uptime-Kuma/2.0.0-dev' \
 --location --max-redirs 10 \
 --max-time 48 \
 --url 'https://google.com --not-injected \$Home \`exec\\\` \' <-> \" '

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings

Screenshots (if any)

image

@CommanderStorm CommanderStorm changed the title fix: Escaped debugging output fix: escaped debugging output Oct 9, 2024
@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Oct 9, 2024
@louislam
Copy link
Owner

louislam commented Oct 9, 2024

why would anyone inject themselves,

Not only about security, if the body contains a single quote, it could break the command.

Actually I found that the pr #5152 is buggy. I believe that it is quite a lot of things have to test, and maybe unit test is needed. I prefer to revert it and put this in maybe the 2.1 milestone.

Here I just randomly play some features and found some bugs:

1. The command do not work if the request data is not empty

Warning: You can only select one HTTP request method! You asked for both POST
Warning: (-d, --data) and HEAD (-I, --head).

METHOD: POST
Body:

{}

2. The following configs seems break the command.

METHOD: POST
Body:

{
    "msg": "Don't know."
}

3. Body Encoding: x-www-form-urlencoded does not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants