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: support callv and proxies for lsp.js #10172

Merged
merged 10 commits into from Apr 29, 2024
Merged

Conversation

ajbt200128
Copy link
Contributor

@ajbt200128 ajbt200128 commented Apr 26, 2024

Previously we added support for proxies in #9994. But it broke LSP.js networking as we were forced to use callv to support proxies as a workaround, and callv isn't supported in cohttp-jsoo!. So we have to patch callv and patch our XHR js library to enable proxies for this stuff. I'm going to try and make a bunch of PRs to cohttp and ca-certs etc to fix this upstream, but for now we have this fix.

Test plan

For testing js w/out proxy

make build-semgrep-jsoo-debug
make -C js build
make -C js test

For testing JS + proxy

make build-semgrep-jsoo-debug
make -C js build
make -C js/language_server
# copy js/language_server/dist into extension
# Run vscode with proxy and Use JS to confirm it works

TODO add MIT license to VSCode

Previously we added support for proxies in #9994. But it broke LSP.js
networking as we were forced to use callv to support proxies as a
workaround, and callv isn't supported in cohttp-jsoo!. So we have to
patch callv and patch our XHR js library to enable proxies for this
stuff. I'm going to try and make a bunch of PRs to cohttp and ca-certs
etc to fix this upstream, but for now we have this fix
@ajbt200128 ajbt200128 requested review from kopecs, a team and aryx and removed request for a team April 26, 2024 18:39
Copy link
Contributor

github-actions bot commented Apr 26, 2024

PR checklist:

  • Purpose of the code is evident to future readers
  • Tests included or PR comment includes a reproducible test plan
  • Documentation is up-to-date
  • A changelog entry was added to changelog.d for any user-facing change
  • Change has no security implications (otherwise, ping security team)

If you're unsure about any of this, please see:

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 5910465

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 5910465 && git push
    

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 0a1d9a9

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 0a1d9a9 && git push
    

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 6bf3b9c

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 6bf3b9c && git push
    

normal shared cant have cohttp in it because then it expects xhr.
There's xhr in the browser so it'd be weird to include it regardless of node
Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 6716786

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 6716786 && git push
    

let callv ?ctx (uri : Uri.t) stream =
(* Differs from request uri if HTTP(S)_PROXY is set. But we handle this in
the node_shared XHR*)
ignore uri;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignore uri;

Dead now?

Copy link
Contributor

@kopecs kopecs left a comment

Choose a reason for hiding this comment

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

Seems mostly good. I think we would benefit from some more documentation.

js/node_shared/XMLHttpRequest.js Show resolved Hide resolved
src/osemgrep/language_server/Test_LS_e2e.ml Outdated Show resolved Hide resolved
js/language_server/dune Outdated Show resolved Hide resolved
js/node_shared/Semgrep_js_node_shared.ml Outdated Show resolved Hide resolved
Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 1bf0502

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 1bf0502 && git push
    

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 3effa81

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 3effa81 && git push
    

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 016a54d

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 016a54d && git push
    

@r2c-argo
Copy link
Contributor

r2c-argo bot commented Apr 26, 2024

semgrep-compare-github-uwue0 results

Ran benchmark on 38 repositories

The number of findings differs for 3 repos

Whole benchmark is 6.5% faster (a bit of noise is expected)

Relative speed improvement is 1.01 on average

  • 3% of scans are significantly faster

Relative memory improvement is 1.00 on average

  • 3% of scans use significantly less memory

Copy link
Contributor

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

  1. Review the changes at 64be254

  2. Accept the new snapshots with

    git fetch origin && git cherry-pick 64be254 && git push
    

@ajbt200128 ajbt200128 merged commit 6c3df7a into develop Apr 29, 2024
47 of 49 checks passed
@ajbt200128 ajbt200128 deleted the austin/fix-js-proxy branch April 29, 2024 18:40
@ajbt200128
Copy link
Contributor Author

pro checks failing seem unrelated

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

Successfully merging this pull request may close these issues.

None yet

2 participants