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

add alert function to pacrunners #196

Merged
merged 2 commits into from
May 3, 2023
Merged

add alert function to pacrunners #196

merged 2 commits into from
May 3, 2023

Conversation

multiSnow
Copy link
Contributor

@multiSnow multiSnow commented Dec 19, 2022

This PR add the (widely used?) alert function to each pacrunner, which directly print to stderr, prefixed with PAC-alert:.

Update:
Unless the _PX_DEBUG_PACALERT environment is set, this alert function will do nothing (since there is no 'browser console'), which just create the function name in pacrunner context to avoid ReferenceError while interpreting pac script.

@Vogtinator
Copy link
Contributor

I wonder whether it's a good idea to unconditionally allow PAC scripts to pollute stderr of arbitrary programs. That can also cause fun with escape sequences.

@mcatanzaro
Copy link
Contributor

I wonder whether it's a good idea to unconditionally allow PAC scripts to pollute stderr of arbitrary programs. That can also cause fun with escape sequences.

I don't see why not. The PAC script is inherently trusted, and it seems useful. So this looks good to me.

Note this codebase will be obsoleted by https://github.com/janbrummer/libproxy2, so since this is new functionality, you'll want to submit this there too.

@Vogtinator
Copy link
Contributor

I don't see why not. The PAC script is inherently trusted, and it seems useful. So this looks good to me.

It's trusted to perform redirections (at most), but not to possibly cause remote code execution...

@mcatanzaro
Copy link
Contributor

Programs print untrusted stuff to stderr all the time. Is this really unsafe?

@Vogtinator
Copy link
Contributor

Programs print untrusted stuff to stderr all the time. Is this really unsafe?

Programs might, but not libraries.

@mcatanzaro
Copy link
Contributor

I think it's pretty normal for libraries to print error messages that may contain untrusted data to stderr.

@janbrummer
Copy link
Contributor

Please rebase.

@janbrummer
Copy link
Contributor

Thanks for the update, but i'm currently confused why there are some minor code style issues. I expected the commit hook to run our style checker before commiting. Have you build the current code?

@janbrummer
Copy link
Contributor

LGTM, but we should ensure that this parameter is also documented. Could you add a small section to the doc?

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 18.18% and project coverage change: -0.74 ⚠️

Comparison is base (de3bc39) 72.49% compared to head (87af768) 71.76%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
- Coverage   72.49%   71.76%   -0.74%     
==========================================
  Files          17       17              
  Lines         807      818      +11     
  Branches      215      217       +2     
==========================================
+ Hits          585      587       +2     
- Misses        130      139       +9     
  Partials       92       92              
Impacted Files Coverage Δ
...kend/plugins/pacrunner-duktape/pacrunner-duktape.c 58.82% <18.18%> (-6.05%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@janbrummer janbrummer merged commit f490cdf into libproxy:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants