A2A ref resolution + fetch auth: make a #212 ref fetchable cross-machine (reduced file-sharing, 1481) - #238
A2A ref resolution + fetch auth: make a #212 ref fetchable cross-machine (reduced file-sharing, 1481)#238jaylfc wants to merge 1 commit into
Conversation
…cation
Adds a client-side fetch-by-ref helper that resolves taos://<slug>/files/<path>
URIs to the controller Files API endpoint, fetches bytes using the registry
credential path, and verifies the returned content against the ref sha256.
- taosmd/config.py: new files_url config key (env TAOSMD_FILES_URL, config.json
files_url), falls back to registry_url for single-controller installs
- taosmd/ref_fetch.py: resolve_ref_uri maps taos:// URIs to /api/projects/{slug}/files/{path},
fetch_by_ref verifies sha256 and returns bytes or typed errors (mismatch,
not-found, unauthorized)
- taosmd/service.py: thin wrapper that builds a registry-token-authenticated
fetcher and returns base64-encoded verified bytes
- taosmd/remote.py: RemoteClient.fetch_by_ref proxy endpoint
- taosmd/mcp_server.py: a2a_fetch_ref MCP tool
- taosmd/cli.py: a2a-fetch-ref subcommand with --output flag
- tests: unit tests for resolver, fetch-match, fetch-mismatch-rejected,
non-taos-refused, and files_url config round-trip
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe PR adds taos:// reference fetching with Files URL configuration, URI validation, SHA-256 verification, service and remote APIs, plus CLI and MCP entry points. ChangesReference fetching
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟠 High · up to This PR adds cross-machine reference fetching, but the current implementation can fail normal CLI downloads, mishandle malformed references, send credentials to unsafe controller URLs or resolve paths outside the intended files route, and fail with custom or registry-only configuration. The new entry points also omit required session recording, so the PR is not merge-ready until these correctness, security, configuration, and state-handling issues are fixed. Sequence Diagram(s)sequenceDiagram
participant Client
participant taosmd.service
participant taosmd.ref_fetch
participant taOS_Files_API
Client->>taosmd.service: Submit reference and agent
taosmd.service->>taosmd.ref_fetch: Resolve and fetch reference
taosmd.ref_fetch->>taOS_Files_API: Request encoded Files URL
taOS_Files_API-->>taosmd.ref_fetch: Return bytes
taosmd.ref_fetch-->>taosmd.service: Return verified bytes
taosmd.service-->>Client: Return base64 content, hash, and size
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| from . import config as _config | ||
| from .ref_fetch import HashMismatchError, NotFoundError, RefFetchError, UnauthorizedError, fetch_by_ref as _fetch_by_ref | ||
|
|
||
| files_url = _config.get_files_url(data_dir) |
There was a problem hiding this comment.
WARNING: Missing _get_remote dispatch — this function never checks for a configured remote server, unlike every other service function in this file.
All sibling functions (a2a_send, search, stats, etc.) start with remote = _get_remote(data_dir) and forward to RemoteClient when a server URL is configured. fetch_by_ref skips this entirely, so when a remote server is configured the call always goes to the local controller directly. RemoteClient.fetch_by_ref (added in remote.py:403) is unreachable dead code in that configuration.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from . import config as _config | ||
| from .ref_fetch import HashMismatchError, NotFoundError, RefFetchError, UnauthorizedError, fetch_by_ref as _fetch_by_ref | ||
|
|
||
| files_url = _config.get_files_url(data_dir) |
There was a problem hiding this comment.
WARNING: get_files_url check is stricter than the fallback logic in ref_fetch._get_files_url.
service.fetch_by_ref calls _config.get_files_url(data_dir) and raises if it returns falsy, but _get_files_url() in ref_fetch.py falls back to registry_url when files_url is unset. This means service.fetch_by_ref raises "files_url is not configured" when only registry_url is set, while the underlying ref_fetch.fetch_by_ref would succeed via that fallback. The pre-check and the actual resolution have inconsistent behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Raises: | ||
| ValueError: If the uri scheme is not ``taos://`` or the shape is invalid. | ||
| """ | ||
| uri = ref.get("uri", "") |
There was a problem hiding this comment.
WARNING: No type validation on ref parameter — crashes if ref is not a dict.
ref.get("uri", "") raises AttributeError when ref is None, a list, or any non-dict object. A public-facing resolver should validate its input and raise ValueError with a clear message rather than leaking an internal attribute error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| f"invalid taos ref uri: {uri!r} (path is empty)" | ||
| ) | ||
| encoded_path = urllib.parse.quote(path, safe="/") | ||
| base = files_url.rstrip("/") |
There was a problem hiding this comment.
WARNING: No validation on files_url before calling .rstrip() — crashes on None.
files_url.rstrip("/") raises AttributeError if files_url is None. The function signature types it as str but provides no runtime guard. A None value (or any non-string) produces an unhelpful crash instead of a clear ValueError.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| url = get_files_url() | ||
| if url: | ||
| return url | ||
| except Exception: |
There was a problem hiding this comment.
WARNING: Bare except Exception: pass silently swallows all errors (lines 126–127 and 133–134).
If config.py has an import error, a runtime bug in get_files_url/get_registry_url, or any other exception, the user receives the generic "files_url is not configured" message — which is misleading and makes root-cause diagnosis nearly impossible. At minimum, log the exception before falling back.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| print(f"error: {exc}", file=sys.stderr) | ||
| return 2 | ||
|
|
||
| raw = base64.b64decode(result["bytes"]) |
There was a problem hiding this comment.
SUGGESTION: base64.b64decode can raise binascii.Error for malformed input.
raw = base64.b64decode(result["bytes"]) is not wrapped in a try/except. If the service layer ever returns malformed base64 (e.g., due to a bug or manual construction), the CLI crashes with a traceback instead of printing a clean error to stderr and returning a non-zero exit code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 95.2K · Output: 18.8K · Cached: 329K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@taosmd/cli.py`:
- Around line 508-510: Import pathlib.Path in the module before the
output-writing logic in the command handling the fetched result, so the existing
Path(args.output).write_bytes(raw) call works whenever --output is provided.
- Around line 496-506: Validate the parsed ref in the CLI flow before calling
service.fetch_by_ref, rejecting non-dict JSON values with the existing
invalid-input error and exit code 2. Extend the exception handling around
fetch_by_ref to catch ValueError for unsupported URIs alongside RefFetchError,
reporting the error and returning 2.
In `@taosmd/mcp_server.py`:
- Around line 254-264: Update taosmd/mcp_server.py lines 254-264 in
a2a_fetch_ref to capture the MCP tool input before _dispatch and archive its
response or error afterward, configuring manual transcript capture when FastMCP
does not provide it; run process_conversation_turn(...), crystallize the
session, and keep the session catalogue current. Update taosmd/cli.py lines
487-514 to archive the command input, output, and errors, run
process_conversation_turn(...) after user input, crystallize the session, and
update the archive catalogue before exit.
In `@taosmd/ref_fetch.py`:
- Around line 64-71: Update the ref URI resolver around path validation and
files_url handling: reject any path component equal to "." or ".." before
quoting, and validate files_url locally so only HTTP(S) controller URLs are
accepted, including environment-provided values that bypass set_files_url.
Preserve the existing empty-path rejection and URL construction behavior for
valid inputs.
In `@taosmd/service.py`:
- Around line 365-392: Update the fetch flow around _fetcher and _fetch_by_ref
so configuration is resolved once for the supplied data_dir, selecting files_url
or the documented registry_url fallback. Pass the resolved controller URL into
the lower helper instead of allowing taosmd.ref_fetch.fetch_by_ref to re-resolve
configuration without data_dir, while preserving the existing token headers and
HTTP error handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 58e08721-ce69-4617-9c9b-a963166d0c11
📒 Files selected for processing (8)
taosmd/cli.pytaosmd/config.pytaosmd/mcp_server.pytaosmd/ref_fetch.pytaosmd/remote.pytaosmd/service.pytests/test_config_files_url.pytests/test_ref_fetch.py
| try: | ||
| ref = json.loads(args.ref_json) | ||
| except (json.JSONDecodeError, TypeError) as exc: | ||
| print(f"error: invalid ref JSON: {exc}", file=sys.stderr) | ||
| return 2 | ||
|
|
||
| try: | ||
| result = asyncio.run(service.fetch_by_ref(ref, agent="cli", data_dir=data_dir)) | ||
| except RefFetchError as exc: | ||
| print(f"error: {exc}", file=sys.stderr) | ||
| return 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the reference object and handle invalid URIs.
Line 497 accepts JSON arrays and scalars. Those values later cause an unhandled AttributeError when the resolver calls ref.get. A dict with an unsupported URI raises ValueError, but line 504 does not catch it. Return exit code 2 for both input cases.
Proposed fix
try:
ref = json.loads(args.ref_json)
except (json.JSONDecodeError, TypeError) as exc:
print(f"error: invalid ref JSON: {exc}", file=sys.stderr)
return 2
+ if not isinstance(ref, dict):
+ print("error: ref JSON must be an object", file=sys.stderr)
+ return 2
try:
result = asyncio.run(service.fetch_by_ref(ref, agent="cli", data_dir=data_dir))
- except RefFetchError as exc:
+ except (RefFetchError, ValueError) as exc:
print(f"error: {exc}", file=sys.stderr)
return 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| ref = json.loads(args.ref_json) | |
| except (json.JSONDecodeError, TypeError) as exc: | |
| print(f"error: invalid ref JSON: {exc}", file=sys.stderr) | |
| return 2 | |
| try: | |
| result = asyncio.run(service.fetch_by_ref(ref, agent="cli", data_dir=data_dir)) | |
| except RefFetchError as exc: | |
| print(f"error: {exc}", file=sys.stderr) | |
| return 2 | |
| try: | |
| ref = json.loads(args.ref_json) | |
| except (json.JSONDecodeError, TypeError) as exc: | |
| print(f"error: invalid ref JSON: {exc}", file=sys.stderr) | |
| return 2 | |
| if not isinstance(ref, dict): | |
| print("error: ref JSON must be an object", file=sys.stderr) | |
| return 2 | |
| try: | |
| result = asyncio.run(service.fetch_by_ref(ref, agent="cli", data_dir=data_dir)) | |
| except (RefFetchError, ValueError) as exc: | |
| print(f"error: {exc}", file=sys.stderr) | |
| return 2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/cli.py` around lines 496 - 506, Validate the parsed ref in the CLI
flow before calling service.fetch_by_ref, rejecting non-dict JSON values with
the existing invalid-input error and exit code 2. Extend the exception handling
around fetch_by_ref to catch ValueError for unsupported URIs alongside
RefFetchError, reporting the error and returning 2.
| raw = base64.b64decode(result["bytes"]) | ||
| if args.output: | ||
| Path(args.output).write_bytes(raw) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Import Path before using --output.
Line 510 references Path without a binding. Every --output invocation raises NameError instead of writing the fetched bytes.
Proposed fix
def _a2a_fetch_ref_cmd(args: argparse.Namespace) -> int:
"""Handle ``taosmd a2a-fetch-ref``: fetch bytes for a taOS Files-backed ref."""
import asyncio # noqa: PLC0415
import base64 # noqa: PLC0415
+ from pathlib import Path # noqa: PLC0415📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raw = base64.b64decode(result["bytes"]) | |
| if args.output: | |
| Path(args.output).write_bytes(raw) | |
| def _a2a_fetch_ref_cmd(args: argparse.Namespace) -> int: | |
| """Handle ``taosmd a2a-fetch-ref``: fetch bytes for a taOS Files-backed ref.""" | |
| import asyncio # noqa: PLC0415 | |
| import base64 # noqa: PLC0415 | |
| from pathlib import Path # noqa: PLC0415 | |
| raw = base64.b64decode(result["bytes"]) | |
| if args.output: | |
| Path(args.output).write_bytes(raw) |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 510-510: Undefined name Path
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/cli.py` around lines 508 - 510, Import pathlib.Path in the module
before the output-writing logic in the command handling the fetched result, so
the existing Path(args.output).write_bytes(raw) call works whenever --output is
provided.
Source: Linters/SAST tools
| @mcp.tool() | ||
| async def a2a_fetch_ref(ref: dict, agent: str) -> dict: | ||
| """Fetch bytes for a taOS Files-backed ref and verify its sha256. | ||
|
|
||
| ``ref`` must be a dict with ``uri`` (a ``taos://`` uri) and ``sha256``. | ||
| Returns ``{"bytes": <base64-str>, "sha256": <hash>, "size": <int>}`` | ||
| on success, or raises a typed error (mismatch / not-found / unauthorized). | ||
| """ | ||
| return await _dispatch( | ||
| service.fetch_by_ref(ref, agent=agent, data_dir=data_dir) | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Add the required taOSmd conversation lifecycle to both reference-fetch entry points.
Neither entry point archives the tool call, result, or error. Neither entry point runs fact extraction, updates the session catalogue, or crystallizes the completed session.
taosmd/mcp_server.py#L254-L264: Record the MCP tool input before dispatch and the response or error afterward. Configure manual transcript capture if FastMCP does not provide it.taosmd/cli.py#L487-L514: Record the command input, output, and error. Run fact extraction after the user input. Crystallize the session and update the archive catalogue before exit.
As per coding guidelines, archive every user message, assistant response, tool call, and error; run process_conversation_turn(...); crystallize each session; and keep the session catalogue current.
📍 Affects 2 files
taosmd/mcp_server.py#L254-L264(this comment)taosmd/cli.py#L487-L514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/mcp_server.py` around lines 254 - 264, Update taosmd/mcp_server.py
lines 254-264 in a2a_fetch_ref to capture the MCP tool input before _dispatch
and archive its response or error afterward, configuring manual transcript
capture when FastMCP does not provide it; run process_conversation_turn(...),
crystallize the session, and keep the session catalogue current. Update
taosmd/cli.py lines 487-514 to archive the command input, output, and errors,
run process_conversation_turn(...) after user input, crystallize the session,
and update the archive catalogue before exit.
Source: Coding guidelines
| path = parts[1][len(_FILES_SEGMENT):] | ||
| if not path: | ||
| raise ValueError( | ||
| f"invalid taos ref uri: {uri!r} (path is empty)" | ||
| ) | ||
| encoded_path = urllib.parse.quote(path, safe="/") | ||
| base = files_url.rstrip("/") | ||
| return f"{base}/api/projects/{slug}/files/{encoded_path}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject traversal segments and non-HTTP(S) controller URLs.
Line 69 encodes unsafe characters but preserves . and .. segments. A ref can therefore create a URL outside the Files API route after path normalization. The service sends the registry bearer token with this request.
Reject . and .. path components. Validate files_url in this resolver because environment values bypass set_files_url.
Proposed fix
path = parts[1][len(_FILES_SEGMENT):]
if not path:
raise ValueError(
f"invalid taos ref uri: {uri!r} (path is empty)"
)
+ if any(segment in {".", ".."} for segment in path.split("/")):
+ raise ValueError(
+ f"invalid taos ref uri: {uri!r} (path traversal is not allowed)"
+ )
+ parsed_files_url = urllib.parse.urlsplit(files_url)
+ if parsed_files_url.scheme not in {"http", "https"} or not parsed_files_url.netloc:
+ raise ValueError("files_url must be an absolute HTTP(S) URL")
encoded_path = urllib.parse.quote(path, safe="/")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path = parts[1][len(_FILES_SEGMENT):] | |
| if not path: | |
| raise ValueError( | |
| f"invalid taos ref uri: {uri!r} (path is empty)" | |
| ) | |
| encoded_path = urllib.parse.quote(path, safe="/") | |
| base = files_url.rstrip("/") | |
| return f"{base}/api/projects/{slug}/files/{encoded_path}" | |
| path = parts[1][len(_FILES_SEGMENT):] | |
| if not path: | |
| raise ValueError( | |
| f"invalid taos ref uri: {uri!r} (path is empty)" | |
| ) | |
| if any(segment in {".", ".."} for segment in path.split("/")): | |
| raise ValueError( | |
| f"invalid taos ref uri: {uri!r} (path traversal is not allowed)" | |
| ) | |
| parsed_files_url = urllib.parse.urlsplit(files_url) | |
| if parsed_files_url.scheme not in {"http", "https"} or not parsed_files_url.netloc: | |
| raise ValueError("files_url must be an absolute HTTP(S) URL") | |
| encoded_path = urllib.parse.quote(path, safe="/") | |
| base = files_url.rstrip("/") | |
| return f"{base}/api/projects/{slug}/files/{encoded_path}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/ref_fetch.py` around lines 64 - 71, Update the ref URI resolver around
path validation and files_url handling: reject any path component equal to "."
or ".." before quoting, and validate files_url locally so only HTTP(S)
controller URLs are accepted, including environment-provided values that bypass
set_files_url. Preserve the existing empty-path rejection and URL construction
behavior for valid inputs.
Source: Linters/SAST tools
| files_url = _config.get_files_url(data_dir) | ||
| if not files_url: | ||
| raise RefFetchError( | ||
| "files_url is not configured: set TAOSMD_FILES_URL or files_url in config.json" | ||
| ) | ||
| registry_token = _config.get_registry_token(data_dir) | ||
|
|
||
| def _fetcher(url: str, agent: str) -> bytes: | ||
| import urllib.error | ||
| import urllib.request | ||
|
|
||
| headers = {"Accept": "application/octet-stream"} | ||
| if registry_token: | ||
| headers["Authorization"] = f"Bearer {registry_token}" | ||
| req = urllib.request.Request(url, headers=headers, method="GET") | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=30) as resp: | ||
| return resp.read() | ||
| except urllib.error.HTTPError as exc: | ||
| if exc.code in (401, 403): | ||
| raise UnauthorizedError(f"HTTP {exc.code} from {url}") from exc | ||
| if exc.code == 404: | ||
| raise NotFoundError(f"HTTP 404 from {url}") from exc | ||
| raise | ||
| except urllib.error.URLError as exc: | ||
| raise RefFetchError(f"fetch failed for {url}: {exc}") from exc | ||
|
|
||
| raw = await _fetch_by_ref(ref, _fetcher, agent) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the resolved controller URL to the verifier.
Line 365 resolves configuration for data_dir, but line 392 re-resolves configuration through taosmd.ref_fetch.fetch_by_ref without that directory. A custom CLI or MCP data directory then fails even when it contains files_url. Registry-only configuration also fails at lines 365-369 despite the documented fallback.
Resolve files_url or registry_url once. Pass that value into the lower helper.
Proposed fix
-async def fetch_by_ref(ref: dict, fetcher, agent: str) -> bytes:
+async def fetch_by_ref(
+ ref: dict, fetcher, agent: str, *, files_url: str | None = None
+) -> bytes:
...
- files_url = _get_files_url()
+ files_url = files_url or _get_files_url()- files_url = _config.get_files_url(data_dir)
+ files_url = _config.get_files_url(data_dir) or _config.get_registry_url(data_dir)
if not files_url:
raise RefFetchError(
"files_url is not configured: set TAOSMD_FILES_URL or files_url in config.json"
)
...
- raw = await _fetch_by_ref(ref, _fetcher, agent)
+ raw = await _fetch_by_ref(ref, _fetcher, agent, files_url=files_url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| files_url = _config.get_files_url(data_dir) | |
| if not files_url: | |
| raise RefFetchError( | |
| "files_url is not configured: set TAOSMD_FILES_URL or files_url in config.json" | |
| ) | |
| registry_token = _config.get_registry_token(data_dir) | |
| def _fetcher(url: str, agent: str) -> bytes: | |
| import urllib.error | |
| import urllib.request | |
| headers = {"Accept": "application/octet-stream"} | |
| if registry_token: | |
| headers["Authorization"] = f"Bearer {registry_token}" | |
| req = urllib.request.Request(url, headers=headers, method="GET") | |
| try: | |
| with urllib.request.urlopen(req, timeout=30) as resp: | |
| return resp.read() | |
| except urllib.error.HTTPError as exc: | |
| if exc.code in (401, 403): | |
| raise UnauthorizedError(f"HTTP {exc.code} from {url}") from exc | |
| if exc.code == 404: | |
| raise NotFoundError(f"HTTP 404 from {url}") from exc | |
| raise | |
| except urllib.error.URLError as exc: | |
| raise RefFetchError(f"fetch failed for {url}: {exc}") from exc | |
| raw = await _fetch_by_ref(ref, _fetcher, agent) | |
| files_url = _config.get_files_url(data_dir) or _config.get_registry_url(data_dir) | |
| if not files_url: | |
| raise RefFetchError( | |
| "files_url is not configured: set TAOSMD_FILES_URL or files_url in config.json" | |
| ) | |
| registry_token = _config.get_registry_token(data_dir) | |
| def _fetcher(url: str, agent: str) -> bytes: | |
| import urllib.error | |
| import urllib.request | |
| headers = {"Accept": "application/octet-stream"} | |
| if registry_token: | |
| headers["Authorization"] = f"Bearer {registry_token}" | |
| req = urllib.request.Request(url, headers=headers, method="GET") | |
| try: | |
| with urllib.request.urlopen(req, timeout=30) as resp: | |
| return resp.read() | |
| except urllib.error.HTTPError as exc: | |
| if exc.code in (401, 403): | |
| raise UnauthorizedError(f"HTTP {exc.code} from {url}") from exc | |
| if exc.code == 404: | |
| raise NotFoundError(f"HTTP 404 from {url}") from exc | |
| raise | |
| except urllib.error.URLError as exc: | |
| raise RefFetchError(f"fetch failed for {url}: {exc}") from exc | |
| raw = await _fetch_by_ref(ref, _fetcher, agent, files_url=files_url) |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 380-380: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(req, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🪛 Ruff (0.16.1)
[error] 379-379: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 381-381: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/service.py` around lines 365 - 392, Update the fetch flow around
_fetcher and _fetch_by_ref so configuration is resolved once for the supplied
data_dir, selecting files_url or the documented registry_url fallback. Pass the
resolved controller URL into the lower helper instead of allowing
taosmd.ref_fetch.fetch_by_ref to re-resolve configuration without data_dir,
while preserving the existing token headers and HTTP error handling.
Review: CHANGES REQUESTEDThree blocking findings. One is a security defect that no bot flagged; two are correctness bugs that Kilo raised and I verified independently against the source rather than taking the bot's word. 1. BLOCKING (security):
|
|
Closing in favour of card tsk-7xcsh5 (p4, fleet:claimable, security). Not a judgement on the effort, and the shape of the feature is right. But three findings block it and one of them is a security defect in the URL construction itself, so the branch needs rebuilding rather than patching: the traversal fix changes how the path is built, the The card carries the full review, the reproduction, the constraint the fix must satisfy (the resolved URL must not be able to leave Review for reference: #238 (comment) |
1 participant
CARD TITLE (intent, not commit subject): A2A ref resolution + fetch auth: make a #212 ref fetchable cross-machine (reduced file-sharing, 1481)
Autonomous build of board card tsk-doocq4.
Adds a client-side fetch-by-ref helper that resolves taos:///files/
URIs to the controller Files API endpoint, fetches bytes using the registry
credential path, and verifies the returned content against the ref sha256.
files_url), falls back to registry_url for single-controller installs
fetch_by_ref verifies sha256 and returns bytes or typed errors (mismatch,
not-found, unauthorized)
fetcher and returns base64-encoded verified bytes
non-taos-refused, and files_url config round-trip
Files:
taosmd/config.py | 52 +++++++++++++++
taosmd/mcp_server.py | 12 ++++
taosmd/ref_fetch.py | 147 +++++++++++++++++++++++++++++++++++++++++
taosmd/remote.py | 7 ++
taosmd/service.py | 57 ++++++++++++++++
tests/test_config_files_url.py | 42 ++++++++++++
tests/test_ref_fetch.py | 146 ++++++++++++++++++++++++++++++++++++++++
8 files changed, 510 insertions(+)
Summary by CodeRabbit
New Features
Tests