Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 115 additions & 35 deletions tests/test_ref_fetch.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,121 @@ def fake_fetcher(url, agent):
with pytest.raises(RefFetchError, match="no sha256"):
asyncio.run(fetch_by_ref(ref, fake_fetcher, "test-agent"))

def test_redirect_no_authorization_leak(self, monkeypatch):
import http.server
import threading
import time
import urllib.request
import urllib.error
import asyncio
import hashlib

from taosmd import service as svc
from taosmd import config as cfg

# shared state between test and HTTP servers
recorded = {"headers": {}}

# Origin B: records every request header it receives
class OriginBHandler(http.server.BaseHTTPRequestHandler):
def do_GET(self):
recorded["headers"] = dict(self.headers)
self.send_response(200)
self.send_header("Content-Type", "application/octet-stream")
self.end_headers()
self.wfile.write(b"ok")
def log_message(self, format, *args):
pass

# Origin A: returns 302 redirect to origin B
class OriginAHandler(http.server.BaseHTTPRequestHandler):
def do_GET(self):
self.send_response(302)
self.send_header("Location", f"http://localhost:{b_port}/")
self.end_headers()
def log_message(self, format, *args):
pass

# Start origin B on an ephemeral port
b_server = http.server.HTTPServer(("127.0.0.1", 0), OriginBHandler)
b_port = b_server.server_address[1]
b_thread = threading.Thread(target=b_server.serve_forever, daemon=True)
b_thread.start()
time.sleep(0.05)

# Start origin A on another ephemeral port
a_server = http.server.HTTPServer(("127.0.0.1", 0), OriginAHandler)
a_port = a_server.server_address[1]
a_thread = threading.Thread(target=a_server.serve_forever, daemon=True)
a_thread.start()
time.sleep(0.05)

try:
# Configure registry token and files URL pointing to origin A
monkeypatch.setattr(cfg, "get_files_url", lambda d: f"http://localhost:{a_port}")
monkeypatch.setattr(cfg, "get_registry_token", lambda d: "test-token")

ref = {
"uri": "taos://proj/files/hello.txt",
"sha256": hashlib.sha256(b"hello world").hexdigest(),
}

# ---- Part 1: with _NoRedirect (the fixed behaviour) ----
# The real fetch path uses _NoRedirect which returns None from
# redirect_request, preventing the 302 from being followed.
# Origin B must NOT receive the Authorization header.
auth_leaked = False
try:
asyncio.run(
svc.fetch_by_ref(ref, agent="test", data_dir="/tmp")
)
except urllib.error.HTTPError as exc:
if exc.code == 302:
# Expected: _NoRedirect prevents following the 302,
# so the HTTPError propagates and origin B is never contacted.
pass
else:
raise

# Assertion: origin B received NO Authorization header
auth_keys = [k for k in recorded["headers"] if k.lower() == "authorization"]
assert len(auth_keys) == 0, (
f"Authorization header leaked to origin B with _NoRedirect! "
f"Keys found: {auth_keys}"
)

# ---- Part 2: control without _NoRedirect ----
# Build an opener without _NoRedirect (default HTTPRedirectHandler
# follows redirects). Origin B MUST receive the Authorization header.
headers = {"Accept": "application/octet-stream", "Authorization": "Bearer test-token"}
req = urllib.request.Request(
f"http://localhost:{a_port}/api/projects/proj/files/hello.txt",
headers=headers,
method="GET",
)
# Default opener follows redirects -> Authorization leaks to origin B
opener = urllib.request.build_opener()
try:
with opener.open(req, timeout=30) as resp:
_ = resp.read()
except urllib.error.HTTPError:
# Even if an HTTPError occurs, the redirect was followed and
# the Authorization header was already sent to origin B via
# the separate TCP connection recorded below.
pass

# Assertion: origin B received the Authorization header
auth_keys = [k for k in recorded["headers"] if k.lower() == "authorization"]
assert len(auth_keys) > 0, (
"Control: Authorization header should have been leaked to origin B "
"without _NoRedirect!"
)

finally:
b_server.shutdown()
b_server.server_close()
a_server.shutdown()
a_server.server_close()

# ---------------------------------------------------------------------------
# Single-controller fallback tests
Expand Down Expand Up @@ -261,41 +376,6 @@ async def fake_fetch(ref, fetcher, agent, data_dir=None):
monkeypatch.setattr("taosmd.ref_fetch.fetch_by_ref", fake_fetch)

ref = {"uri": "taos://proj/files/hello.txt", "sha256": "abc"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CRITICAL: test_local_path_passes_data_dir_to_ref_fetch is incomplete — assertions were accidentally removed

The method body ends at ref = {...} with no call to svc.fetch_by_ref and no assertions. It will pass trivially without verifying that data_dir is forwarded or that the result is base64-encoded correctly. The removed lines were:

result = asyncio.run(svc.fetch_by_ref(ref, agent="test", data_dir="/tmp"))
assert captured["data_dir"] == "/tmp"
assert result["bytes"] == "aGVsbG8="

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

result = asyncio.run(svc.fetch_by_ref(ref, agent="test", data_dir="/tmp"))
assert captured["data_dir"] == "/tmp"
assert result["bytes"] == "aGVsbG8="

def test_fetcher_uses_no_redirect_opener(self, monkeypatch):
from taosmd import config as cfg
from taosmd import service as svc
import urllib.request

build_opener_calls = []

def fake_build_opener(*handlers):
build_opener_calls.append(handlers)
class FakeOpener:
def open(self, req, timeout=None):
class FakeResp:
def __enter__(self):
return self
def __exit__(self, *args):
pass
def read(self):
return b"hello world"
return FakeResp()
return FakeOpener()

monkeypatch.setattr(urllib.request, "build_opener", fake_build_opener)
monkeypatch.setattr(cfg, "get_files_url", lambda data_dir=None: "http://ctrl:8000")
monkeypatch.setattr(cfg, "get_registry_token", lambda data_dir=None: "secret-token")

ref = {"uri": "taos://proj/files/hello.txt", "sha256": hashlib.sha256(b"hello world").hexdigest()}
result = asyncio.run(svc.fetch_by_ref(ref, agent="test", data_dir="/tmp"))
assert len(build_opener_calls) == 1
assert len(build_opener_calls[0]) == 1
assert issubclass(build_opener_calls[0][0], urllib.request.HTTPRedirectHandler)

def test_service_fetch_by_ref_does_not_raise_when_files_url_unset_but_registry_set(self, tmp_path, monkeypatch):
from taosmd import config as cfg
from taosmd import service as svc
Expand Down