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

Use Proxy config when available #124

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ErikJPM
Copy link

@ErikJPM ErikJPM commented Dec 1, 2023

This enables you to use sign.js behind a WebProxy, which is sadly necessary in many "Enterprise" environments. This will automatically use the ENV variable HTTPS_PROXY that is standard for e.g. Curl and other unix utils. This should be compatible with MacOS, Linux and Windows environments with a posix like shell.

I have not tested this with a Authorization header against the Sitevision sign server, nor against the proxy. There is not support for proxy Authentication AFAIK at this time.

sign.js is impossible to use behind a WebProxy unless this is merged.

There are many things that could be improved upon here, e.g. doing a liveness check against the signing server, checking for errors that the proxy might introduce, or just making the client responsible for the signing. These are not in scope of this PR.

Copy link
Contributor

@albinohrn albinohrn left a comment

Choose a reason for hiding this comment

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

Hello Erik!

Thanks for your contribution!

Would you mind providing a brief description of your PR it would give us some valuable background to why this change is important to you.

body: formData,
headers: formData.getHeaders({
var options = {
method: 'POST',
Copy link
Contributor

Choose a reason for hiding this comment

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

Your code formatting is a bit off from the rest of the file. We use Prettier it's a simple tool that formats the files automatically and let you focus on more important stuff. :)

Copy link
Author

Choose a reason for hiding this comment

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

Jag måste tyvärr använda mig av online redigeringen så är lite begränsad till vad jag kan göra. Får titta mer på om jag kan ha någon vettigt dev miljö för detta.

}
if (process.env.HTTPS_PROXY) {
console.log(`using ${process.env.HTTPS_PROXY} as HTTPS_PROXY`)
const HttpsProxyAgent = require('https-proxy-agent');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest we add this as an explicit dependency now that it is used directly. Makes it more obvious that we actually use it.

Copy link
Author

Choose a reason for hiding this comment

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

Vi la den där pga lazy loading, men du har rätt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Det är helt ok med lazy-loading. Det jag tänkte på var att den bör specificeras i package.json, nu är det med som ett transient beroende.

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.

2 participants