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: unquote quoted URL's to avoid libcurl errors #2596

Merged

Conversation

therealdwright
Copy link
Contributor

@therealdwright therealdwright commented May 30, 2023

This commit will unquote URL's allowing them to be supported by
libcurl and eliminate any errors when a valid (quoted) URL is supplied
by a user.

Closes #2579

Signed-off-by: Daniel Wright danielwright@bitgo.com

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:
As mentioned in the associated issue, I was setting up some
values which contained special character as the URL had a
secret token in it, and wanted to quote to avoid any ambiguity
or issues when adding to a manifest. I encountered a libcurl
error that my valid (quoted) string was invalid.

Which issue(s) this PR fixes:
Fixes #2579

Special notes for your reviewer:
N/A

Does this PR introduce a user-facing change?:
N/A

fix: unquote quoted URL's to avoid libcurl errors

@poiana poiana requested review from Kaizhe and mstemm May 30, 2023 23:29
@poiana poiana added the size/XS label May 30, 2023
@therealdwright therealdwright force-pushed the unescape-quoted-urls-http-output branch from 7cd2c74 to eb0c5bb Compare May 31, 2023 04:51
@poiana poiana added size/S and removed size/XS labels May 31, 2023
@therealdwright therealdwright changed the title fix: unescape quoted URL's to avoid libcurl errors fix: unquote quoted URL's to avoid libcurl errors May 31, 2023
@therealdwright therealdwright force-pushed the unescape-quoted-urls-http-output branch from eb0c5bb to f830d65 Compare May 31, 2023 04:52
FedeDP
FedeDP previously approved these changes May 31, 2023
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link

poiana commented May 31, 2023

LGTM label has been added.

Git tree hash: f4393031fc317e391c48d351ca5add9d2ed975ef

@FedeDP
Copy link
Contributor

FedeDP commented May 31, 2023

Thank you! This is a nice and small fix :)
/milestone 0.35.0

@poiana poiana added this to the 0.35.0 milestone May 31, 2023
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

I believe this workaround should be ok in most cases. However, the root issue is the command line flag parsers instead of here.

That being said, I'm ok to merge this, but we should add a TODO somewhere to fix the root cause properly.

cc @jasondellaluce

@therealdwright
Copy link
Contributor Author

the root issue is the command line flag parsers

I thought this was the best place to fix as the issue is really with libcurl and I have this issue if I use the yaml OR the command line parser hence this approach. LMK if you still want a TODO added.

Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

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

I see what you're doing here. So, in my opinion this is something we should support properly with the -o option as well, and perhaps being consistent with what happens in falco.yaml. I understand that doing a more general solution that involves proper string escaping in the config and command line is not something that we can do easily few days before releasing Falco, so in this case I would be ok on doing having a workaround like this.

With my suggestion, the workaround should not break future developments because it will just become "protective", in the sense that it will attempt unescaping the string in case it was not unescaped before (which hopefully we'll do from -o and falco.yaml for the next release).

I will leave the rest of the reviewers and the release managers the decision on whether include this or not for 0.35! cc @leogr @FedeDP

userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
This commit will unquote URL's allowing them to be supported by
libcurl and eliminate any errors when a valid (quoted) URL is supplied
by a user.

Closes falcosecurity#2579

Signed-off-by: Daniel Wright danielwright@bitgo.com
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Jun 5, 2023
@poiana
Copy link

poiana commented Jun 5, 2023

LGTM label has been added.

Git tree hash: ccf3c9295007ec3faaf82fc312ebca242aefeb09

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM, now! 👍

@poiana
Copy link

poiana commented Jun 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr, therealdwright

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 9097d2c into falcosecurity:master Jun 5, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

http_output results in libcurl error: URL using bad/illegal format or missing URL
5 participants