-
-
Notifications
You must be signed in to change notification settings - Fork 574
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
Bail out early for non-HTTP but HTTP looking protocols #972
Bail out early for non-HTTP but HTTP looking protocols #972
Conversation
Url
to parse all types of schemes
Codecov Report
@@ Coverage Diff @@
## develop #972 +/- ##
===========================================
- Coverage 87.26% 87.18% -0.09%
===========================================
Files 146 146
Lines 6284 6313 +29
Branches 625 632 +7
===========================================
+ Hits 5484 5504 +20
- Misses 695 703 +8
- Partials 105 106 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@webknjaz Wondering if there is a workaround. What makes this worse is that I am wondering if we'll have to live with a red CI now :) (in case of a missing workaround). Related run here https://github.com/abhinavsingh/proxy.py/runs/4836507299?check_suite_focus=true |
643de14
to
7d576aa
Compare
I see two problems here:
|
@@ -68,29 +68,41 @@ def from_bytes(cls, raw: bytes) -> 'Url': | |||
For a HTTPS connect tunnel, url is like ``httpbin.org:443`` | |||
For a HTTP proxy request, url is like ``http://httpbin.org/get`` | |||
|
|||
proxy.py internally never expects a https scheme in the request line. | |||
But `Url` class provides support for parsing any scheme present in the URLs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not inline code but a reference.
e.g. ftp, icap etc. | ||
|
||
If a url with no scheme is parsed, e.g. ``//host/abc.js``, then scheme | ||
defaults to `http`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, reference syntax used, but you probably wanted it to be rst.
Thank you. Probably pinning is the right way to approach instead of fighting it immediately. I hope should be resolved in future releases. Good point about also updating bad references. |
FYI the deps are only unpinned for the spelling env and pinned for others. This is why only one invocation is failing. |
* Fix README instructions for embedded mode * Expose sleep_loop * [SshTunnel] WIP (#992) [SshTunnel] WIP * [Middleware] Capability in the core to allow custom client connection classes (#993) * Move all TCP server related flags within `tcp_server.py` and also move the encryption functionality within TCP base server * Templatize `BaseTcpServerHandler` which now expects a client connection object bound to `TcpClientConnection`. This will allow for custom `HttpClientConnection` object in future to be used by `HttpProtocolHandler` * Pass necessary flags to allow self-signed certificates * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix https integration tests * Affected by #994 * Fix docs Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [Middleware] `HttpClientConnection` preparation (#995) * Turn usual suspects to warnings, not error * Add `HttpClientConnection` skeleton * Fix doc build * Update references in http tests * Make `work` core agnostic to work object construction by adding an abstract static method to `Work` interface called `create` * Make mypy happy * Fix tests broken due to change in how work objects are now constructed * Doc ko bhi happy karo * Bail out early for non-HTTP but HTTP looking protocols (#972) * Add support in `Url` to parse all types of schemes * . * Guard handler against http looking protocol but not web or proxy requests * Fix condition for web server protocol detection * doc happy * Update flags and type check imports only * npm: bump eslint-plugin-import from 2.25.3 to 2.25.4 in /dashboard (#1005) Bumps [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) from 2.25.3 to 2.25.4. - [Release notes](https://github.com/import-js/eslint-plugin-import/releases) - [Changelog](https://github.com/import-js/eslint-plugin-import/blob/main/CHANGELOG.md) - [Commits](import-js/eslint-plugin-import@v2.25.3...v2.25.4) --- updated-dependencies: - dependency-name: eslint-plugin-import dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * npm: bump ws from 8.4.0 to 8.4.2 in /dashboard (#1007) Bumps [ws](https://github.com/websockets/ws) from 8.4.0 to 8.4.2. - [Release notes](https://github.com/websockets/ws/releases) - [Commits](websockets/ws@8.4.0...8.4.2) --- updated-dependencies: - dependency-name: ws dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix broken `--local-executor` logic for windows ever since it was made default (#1008) Co-authored-by: sowmyasudhasingh <sowmyasudhasingh@gmail.com> * [Windows] `--threaded` mode integration tests works locally but fails on GHA (#1009) * Enable remote threadless and threaded integration test for windows * Run only threaded on windows * Use powershell for execution of integration script on Windows * Update test_integration.py * Update test_integration.py Co-authored-by: sowmyasudhasingh <sowmyasudhasingh@gmail.com> Co-authored-by: Abhinav Singh <126065+abhinavsingh@users.noreply.github.com> * Restrict request handling to `DEFAULT_ALLOWED_URL_SCHEMES` (#1002) * Raise `HttpProtocolException` if request line scheme do not match `DEFAULT_ALLOWED_URL_SCHEMES` * ignore WPS329 * Fix tests * Pin to 4.3.2 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Test coverage for exception handling * type ignore Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [Doc] Threadless Remote vs Local Execution Mode (#1011) * [Coverage] For newly added components (#1014) * Add newly added code cov * Fix spelling * [Devtools] Build as part of GHA workflow (#1015) * Fix devtools build * Build devtools as part of GHA workflows * [isort] Lib modules (#1016) * isort `proxy.py` main class * isort init and main * isort common * pre-commit fix * isort dashboard and testing * isort plugins * isort core * Only sort top level http py files * isort http exception and websocket * Remove proxy auth plugin from proxy package exports and force discover `PLUGIN_PROXY_AUTH` flags * isort parser and web server * no setattr * isort all * Enable pre-commit isort hook * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sowmya Sudha Singh <83529764+sowmya-jaxl@users.noreply.github.com> Co-authored-by: sowmyasudhasingh <sowmyasudhasingh@gmail.com>
Url
to parse all types of schemes.HttpProtocolHandler
will bail out if parsed scheme from request line is nothttp
. We don't even expecthttps
, as for HTTPS request proxies,CONNECT
is used anyways, and not ahttps
URL in request line.TL;DR --
HttpProtocolHandler
will only accept web and proxy server requests