Bail out early for non-HTTP but HTTP looking protocols #972

abhinavsingh · 2022-01-12T19:11:24Z

Add support in Url to parse all types of schemes.
HttpProtocolHandler will bail out if parsed scheme from request line is not http. We don't even expect https, as for HTTPS request proxies, CONNECT is used anyways, and not a https URL in request line.

TL;DR -- HttpProtocolHandler will only accept web and proxy server requests

codecov · 2022-01-12T19:57:54Z

Codecov Report

Merging #972 (643de14) into develop (552fb99) will decrease coverage by 0.08%.
The diff coverage is 88.63%.

❗ Current head 643de14 differs from pull request most recent head 7d576aa. Consider uploading reports for the commit 7d576aa to get more accurate results

@@             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

Flag	Coverage Δ
pytest	`87.07% <88.63%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
proxy/common/flag.py	`92.70% <ø> (ø)`
proxy/http/handler.py	`74.88% <33.33%> (-1.94%)`	⬇️
proxy/http/parser/parser.py	`95.85% <57.14%> (-1.32%)`	⬇️
proxy/core/acceptor/pool.py	`94.23% <100.00%> (-0.11%)`	⬇️
proxy/http/protocols.py	`100.00% <100.00%> (ø)`
proxy/http/url.py	`98.57% <100.00%> (+0.13%)`	⬆️
tests/http/parser/test_http_parser.py	`100.00% <100.00%> (ø)`
tests/http/test_url.py	`100.00% <100.00%> (ø)`
proxy/core/base/tcp_server.py	`89.02% <0.00%> (-1.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 552fb99...7d576aa. Read the comment docs.

…ing-proto

…ests

abhinavsingh · 2022-01-17T05:12:43Z

@webknjaz Wondering if there is a workaround. What makes this worse is that make lib-lint doesn't fails locally. But it does on GHA. Sphinx doc running into warnings while it shouldn't , related Sphinx bug here sphinx-doc/sphinx#4961. Opened since 2018.

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

webknjaz · 2022-01-17T19:31:37Z

I see two problems here:

The new (released earlier today or yesterday) version of Sphinx started to emit warnings for hardcoded links that have matching extlink/role equivalents. This seems to not work under macOS according to the bug on the upstream tracker I saw today. Pinning the dependency versions properly should help avoid sudden dependency upgrades with surprises.
There are also warnings for ambiguous references, you should fix them by using the proper/full notation.

webknjaz · 2022-01-17T19:33:09Z

proxy/http/url.py

@@ -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.


This is not inline code but a reference.

webknjaz · 2022-01-17T19:33:46Z

proxy/http/url.py

+        e.g. ftp, icap etc.
+
+        If a url with no scheme is parsed, e.g. ``//host/abc.js``, then scheme
+        defaults to `http`.


Same here, reference syntax used, but you probably wanted it to be rst.

abhinavsingh · 2022-01-17T19:38:58Z

Pinning the dependency versions properly should help avoid sudden dependency upgrades with surprises.

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.

webknjaz · 2022-01-17T19:48:05Z

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>

Add support in Url to parse all types of schemes

227144c

abhinavsingh changed the title ~~Add support in Url to parse all types of schemes~~ Bail out early for non-HTTP but HTTP looking protocols Jan 12, 2022

.

473bd9f

abhinavsingh added the bot:chronographer:skip PR using this label is exempted from CHANGELOG management label Jan 12, 2022

abhinavsingh added 5 commits January 16, 2022 21:20

Merge branch 'develop' into bail-out-early-for-non-http-but-http-look…

f0810bc

…ing-proto

Guard handler against http looking protocol but not web or proxy requ…

4781ed0

…ests

Fix condition for web server protocol detection

bd80e49

doc happy

d234339

Update flags and type check imports only

7d576aa

abhinavsingh force-pushed the bail-out-early-for-non-http-but-http-looking-proto branch from 643de14 to 7d576aa Compare January 17, 2022 18:29

abhinavsingh merged commit 21499ad into develop Jan 17, 2022

abhinavsingh deleted the bail-out-early-for-non-http-but-http-looking-proto branch January 17, 2022 18:41

webknjaz reviewed Jan 17, 2022

View reviewed changes

abhinavsingh mentioned this pull request Jan 17, 2022

[Broken CI] Pin Sphinx to last working version #1004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bail out early for non-HTTP but HTTP looking protocols #972

Bail out early for non-HTTP but HTTP looking protocols #972

abhinavsingh commented Jan 12, 2022 •

edited

codecov bot commented Jan 12, 2022 •

edited

abhinavsingh commented Jan 17, 2022 •

edited

webknjaz commented Jan 17, 2022

webknjaz Jan 17, 2022

webknjaz Jan 17, 2022

abhinavsingh commented Jan 17, 2022

webknjaz commented Jan 17, 2022

Bail out early for non-HTTP but HTTP looking protocols #972

Bail out early for non-HTTP but HTTP looking protocols #972

Conversation

abhinavsingh commented Jan 12, 2022 • edited

codecov bot commented Jan 12, 2022 • edited

Codecov Report

abhinavsingh commented Jan 17, 2022 • edited

webknjaz commented Jan 17, 2022

webknjaz Jan 17, 2022

Choose a reason for hiding this comment

webknjaz Jan 17, 2022

Choose a reason for hiding this comment

abhinavsingh commented Jan 17, 2022

webknjaz commented Jan 17, 2022

abhinavsingh commented Jan 12, 2022 •

edited

codecov bot commented Jan 12, 2022 •

edited

abhinavsingh commented Jan 17, 2022 •

edited