Skip to content

Commit

Permalink
Fixes #201: Connection: don't send default auth headers to other domains
Browse files Browse the repository at this point in the history
  • Loading branch information
soxofaan committed Apr 16, 2021
1 parent 2dd79d5 commit 84c60ca
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `Connection`: don't send default auth headers to non-backend domains ([#201](https://github.com/Open-EO/openeo-python-client/issues/201))


## [0.6.1] - 2021-03-29

Expand Down
6 changes: 4 additions & 2 deletions openeo/rest/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import warnings
from pathlib import Path
from typing import Dict, List, Tuple, Union, Callable, Optional, Any, Iterator
from urllib.parse import urljoin
from urllib.parse import urljoin, urlparse

import requests
from deprecated.sphinx import deprecated
Expand Down Expand Up @@ -76,7 +76,9 @@ def request(self, method: str, path: str, headers: dict = None, auth: AuthBase =
check_error=True, expected_status=None, **kwargs):
"""Generic request send"""
url = self.build_url(path)
auth = auth or self.auth
# Don't send default auth headers to external domains.
external = urlparse(url)[:2] != urlparse(self.root_url)[:2]

This comment has been minimized.

Copy link
@m-mohr

m-mohr Apr 16, 2021

Member

@soxofaan Just found this via issue #201 and for the JS implementation, I'm wondering whether this should be different.

Wouldn't it be more correct to check something like external = !url.startsWith(backendUrl) (where backendUrl is the URL taken from well-known discovery like http://openeo.vito.be/v1.0/. There could be rare cases where there are different services that are separated by folders, e.g. aws.com/sinergise vs. aws.com/vito (just as an example).

This comment has been minimized.

Copy link
@soxofaan

soxofaan Apr 16, 2021

Author Member

Would something like that happen for real production systems?
Cookies for example are also handled based on domain, and not on path level as far as I know

This comment has been minimized.

Copy link
@m-mohr

m-mohr Apr 16, 2021

Member

Cookies can also be restricted to specific paths, but I'm not sure whether that happens often. We get folders assigned by the university per institute, but I guess that's somehow trustworthy. I'm not sure whether there are cloud services that do such things. 🤷‍♀️

This comment has been minimized.

Copy link
@soxofaan

soxofaan Apr 19, 2021

Author Member

addressed in efd62a3

auth = auth or (self.auth if not external else None)
if _log.isEnabledFor(logging.DEBUG):
_log.debug("Request `{m} {u}` with headers {h}, auth {a}, kwargs {k}".format(
m=method.upper(), u=url, h=headers and headers.keys(), a=type(auth).__name__, k=list(kwargs.keys()))
Expand Down
41 changes: 41 additions & 0 deletions tests/rest/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,47 @@ def test_502_proxy_error(requests_mock):
conn.get("/bar")


@pytest.mark.parametrize(["api_root", "url"], [
("https://oeo.test", "https://evilcorp.test/download/hello.txt"),
("https://oeo.test", "https://oeo.test.evilcorp.test/download/hello.txt"),
("https://oeo.test", "http://oeo.test/download/hello.txt"),
])
def test_rest_api_other_domain_auth_headers(requests_mock, api_root, url):
"""https://github.com/Open-EO/openeo-python-client/issues/201"""
secret = "!secret token!"

def debug(request: requests.Request, context):
return repr(("hello world", request.headers))

requests_mock.get(url, text=debug)

con = RestApiConnection(api_root, auth=BearerAuth(secret))
res = con.get(url)
assert "hello world" in res.text
assert "User-Agent': 'openeo-python-client/" in res.text
assert secret not in res.text
assert "auth" not in res.text.lower()


def test_connection_other_domain_auth_headers(requests_mock, api_version):
"""https://github.com/Open-EO/openeo-python-client/issues/201"""
secret = "!secret token!"

def debug(request: requests.Request, context):
return repr(("hello world", request.headers))

requests_mock.get(API_URL, json={"api_version": api_version})
requests_mock.get(API_URL + 'credentials/basic', json={"access_token": secret})
requests_mock.get("https://evilcorp.test/download/hello.txt", text=debug)

con = Connection(API_URL).authenticate_basic("john", "j0hn")
res = con.get("https://evilcorp.test/download/hello.txt")
assert "hello world" in res.text
assert "User-Agent': 'openeo-python-client/" in res.text
assert secret not in res.text
assert "auth" not in res.text.lower()


def test_connection_default_https(requests_mock):
requests_mock.get("https://oeo.test/", json={"api_version": "1.0.0"})
con = Connection("oeo.test")
Expand Down
29 changes: 29 additions & 0 deletions tests/rest/test_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,3 +401,32 @@ def test_result_asset_load_bytes(con100, requests_mock):
res = asset.load_bytes()

assert res == TIFF_CONTENT


def test_get_results_download_file_other_domain(con100, requests_mock, tmp_path):
"""https://github.com/Open-EO/openeo-python-client/issues/201"""
secret = "!secret token!"
requests_mock.get(API_URL + '/credentials/basic', json={"access_token": secret})

def get_results(request, context):
assert "auth" in repr(request.headers).lower()
assert secret in repr(request.headers)
return {"assets": {
"1.tiff": {"href": "https://evilcorp.test/dl/jjr1.tiff", "type": "image/tiff; application=geotiff"},
}}

def download_tiff(request, context):
assert "auth" not in repr(request.headers).lower()
assert secret not in repr(request.headers)
return TIFF_CONTENT

requests_mock.get(API_URL + "/jobs/jj1/results", json=get_results)
requests_mock.get("https://evilcorp.test/dl/jjr1.tiff", content=download_tiff)

con100.authenticate_basic("john", "j0hn")
job = RESTJob("jj1", connection=con100)
target = as_path(tmp_path / "result.tiff")
res = job.get_results().download_file(target)
assert res == target
with target.open("rb") as f:
assert f.read() == TIFF_CONTENT

0 comments on commit 84c60ca

Please sign in to comment.