From 3abbd888cfe6b603dc6d0735285d6247b974ac79 Mon Sep 17 00:00:00 2001 From: "lixuefei.nice" Date: Fri, 6 Nov 2026 18:59:24 +0800 Subject: [PATCH 1/2] fix(skills): support signed TOS URL downloads --- ...026-11-06-signed-tos-url-skill-download.md | 79 +++++++++++++++++++ ...-11-06-signed-tos-url-skill-download.zh.md | 79 +++++++++++++++++++ tests/skills/test_adk_skill_materializer.py | 47 +++++++++++ tests/skills/test_skill_space_policy.py | 25 ++++++ veadk/skills/materializer.py | 50 ++++++++++-- veadk/skills/utils.py | 16 +++- 6 files changed, 289 insertions(+), 7 deletions(-) create mode 100644 prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md create mode 100644 prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md diff --git a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md new file mode 100644 index 000000000..e010d9b02 --- /dev/null +++ b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md @@ -0,0 +1,79 @@ +# Signed TOS URL Skill Download + +## Background and Evidence + +`VeSkillRegistry.get_skill()` refreshes Skill Space metadata with `load_skills_from_cloud()`, selects a remote skill, and materializes it with `materialize_remote_skill()`. For legacy AgentKit Skill Spaces, `_build_skill_from_space_item()` maps `TosPath` to `Skill.path` and `BucketName` to `Skill.bucket_name`. + +Today `_download_legacy_skill_space_skill()` assumes `Skill.path` is a TOS object key such as `skills///.zip`. If the cloud API returns a complete HTTPS pre-signed TOS URL in `TosPath`, the non-Vestack branch passes that URL to `VeTOS.download()` as an object key, while the Vestack branch tries to parse it as a legacy object path. + +## Goals + +- Load a normal Skill Space skill when `TosPath` is a complete pre-signed HTTPS TOS ZIP URL. +- Preserve existing behavior for object-key `TosPath` values. +- Reuse the existing archive safety checks: streaming size limit, zip validation, safe extraction, and ADK `SKILL.md` load validation. +- Keep signed URL content out of error messages and source fixtures except redacted, synthetic test URLs. + +## Non-Goals + +- Do not change Harness `tos_public_url` validation. User-provided Harness public URLs must continue to reject TOS credential query parameters. +- Do not change private TOS directory loading through `tos://bucket/prefix/`. +- Do not add new cloud APIs or change `ListSkillsBySpaceId`. + +## Scenarios + +1. `ListSkillsBySpaceId` returns `BucketName` plus object-key `TosPath`; the SDK TOS download path remains unchanged. +2. `ListSkillsBySpaceId` returns `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`; the materializer downloads that URL directly and loads the Skill. +3. A signed URL redirects to a non-TOS host or returns a non-ZIP payload; materialization fails closed. + +## Requirements + +- Detect signed HTTPS TOS ZIP URLs only for cloud-returned Skill Space metadata. +- Accept TOS signing query keys for this cloud-returned path. +- Require HTTPS, a TOS endpoint host, a `.zip` path, and no username/password in the URL. +- Stream downloads with the same 64 MiB archive limit used by public TOS ZIPs. +- Preserve cache key behavior by using `version_id` when present; otherwise the metadata digest includes the URL. + +## Design + +Add a materializer branch for cloud-returned signed TOS ZIP URLs. `_build_skill_from_space_item()` will classify a `TosPath` that is an HTTPS TOS ZIP URL as a signed URL source. `materialize_remote_skill()` will then route that source through a signed-URL downloader instead of the legacy TOS object-key downloader. + +The signed-URL downloader should share the public TOS ZIP streaming implementation, but use a validator that allows TOS signature query keys. Harness `tos_public_url` continues to use the stricter public validator that rejects credential query keys. + +## Affected Files + +- `veadk/skills/utils.py` +- `veadk/skills/materializer.py` +- `tests/skills/test_adk_skill_materializer.py` + +## Component Contract Assessment + +No component spec update is required. The external Skill Space metadata contract already allows `TosPath` to carry the location returned by AgentKit; this change only fixes local materialization of one returned shape. Public Python APIs, Harness request schema, and user-facing configuration remain unchanged. + +## Tasks + +- Add a failing regression test for a cloud-returned pre-signed TOS URL in `TosPath`. +- Classify signed TOS URL metadata during Skill construction. +- Add a materializer download path that allows signed query keys only for cloud-returned TOS URLs. +- Run targeted Skill tests and whitespace checks. + +## Tests + +- `uv run --extra dev pytest tests/skills/test_adk_skill_materializer.py` +- `uv run --extra dev pytest tests/skills/test_adk_skill_registry.py tests/skills/test_skill_space_policy.py` +- `git diff --check` + +## Risks + +- Signed URLs contain credentials in query parameters; logs and error messages must not echo raw URLs. +- URL cache keys may change when a refreshed signed URL changes. This is acceptable when no stable version is provided; `version_id` remains authoritative when available. + +## Acceptance Criteria + +- A Skill Space skill whose `TosPath` is a pre-signed HTTPS TOS ZIP URL downloads and loads successfully. +- Existing object-key Skill Space downloads still pass. +- Harness `tos_public_url` tests that reject signed query credentials still pass. +- No real credentials or production signed URLs are committed. + +## Design Review + +Direct review completed because no repository-local `review-spec` skill is available in this checkout. The design is bounded to cloud-returned metadata, preserves Harness user-input validation, fails closed on unsafe URL shapes, and has targeted regression coverage. diff --git a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md new file mode 100644 index 000000000..326232fd8 --- /dev/null +++ b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md @@ -0,0 +1,79 @@ +# 预签名 TOS URL Skill 下载修复 + +## 背景和证据 + +`VeSkillRegistry.get_skill()` 会通过 `load_skills_from_cloud()` 刷新 Skill Space 元数据，选中远端 Skill 后调用 `materialize_remote_skill()` 本地物化。对于 legacy AgentKit Skill Space，`_build_skill_from_space_item()` 会把 `TosPath` 映射到 `Skill.path`，把 `BucketName` 映射到 `Skill.bucket_name`。 + +当前 `_download_legacy_skill_space_skill()` 假设 `Skill.path` 是 `skills///.zip` 这类 TOS 对象 key。如果云端 API 在 `TosPath` 中返回完整 HTTPS 预签名 TOS URL，非 Vestack 分支会把这个 URL 当作 object key 传给 `VeTOS.download()`，Vestack 分支则会把它误拆成 legacy 对象路径。 + +## 目标 + +- 当 `TosPath` 是完整预签名 HTTPS TOS ZIP URL 时，普通 Skill Space Skill 可以正常加载。 +- 保留现有 object-key 形式 `TosPath` 的行为。 +- 复用现有归档安全检查：流式大小限制、ZIP 校验、安全解压和 ADK `SKILL.md` 加载校验。 +- 除脱敏的合成测试 URL 外，不在错误信息和源码 fixture 中保留 signed URL 内容。 + +## 非目标 + +- 不修改 Harness `tos_public_url` 校验。用户传入的 Harness public URL 仍然必须拒绝 TOS 凭证 query 参数。 +- 不修改通过 `tos://bucket/prefix/` 加载私有 TOS 目录的逻辑。 +- 不新增云端 API，也不修改 `ListSkillsBySpaceId`。 + +## 场景 + +1. `ListSkillsBySpaceId` 返回 `BucketName` 加 object-key `TosPath`；SDK TOS 下载路径保持不变。 +2. `ListSkillsBySpaceId` 返回 `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`；materializer 直接下载该 URL 并加载 Skill。 +3. signed URL 重定向到非 TOS host，或返回非 ZIP 内容；物化失败关闭。 + +## 需求 + +- 只对云端返回的 Skill Space 元数据识别 signed HTTPS TOS ZIP URL。 +- 对这条云端返回路径允许 TOS 签名 query key。 +- 要求 URL 使用 HTTPS、TOS endpoint host、`.zip` 路径，并且不包含 username/password。 +- 下载必须流式执行，并沿用 public TOS ZIP 的 64 MiB 归档大小限制。 +- 缓存 key 优先使用 `version_id`；没有版本时，metadata digest 可以包含 URL。 + +## 设计 + +为云端返回的 signed TOS ZIP URL 增加 materializer 分支。`_build_skill_from_space_item()` 会把 HTTPS TOS ZIP URL 形式的 `TosPath` 识别为 signed URL source。`materialize_remote_skill()` 随后走 signed URL 下载器，而不是 legacy TOS object-key 下载器。 + +signed URL 下载器应复用 public TOS ZIP 的流式下载实现，但使用允许 TOS 签名 query key 的校验器。Harness `tos_public_url` 继续使用更严格的 public 校验器，仍然拒绝凭证 query key。 + +## 影响文件 + +- `veadk/skills/utils.py` +- `veadk/skills/materializer.py` +- `tests/skills/test_adk_skill_materializer.py` + +## 组件契约评估 + +不需要更新组件 spec。外部 Skill Space 元数据契约已经允许 `TosPath` 承载 AgentKit 返回的位置；本次只修复本地物化对其中一种返回形态的处理。公开 Python API、Harness 请求 schema 和用户配置均不变化。 + +## 任务 + +- 为 `TosPath` 中云端返回的预签名 TOS URL 增加失败回归测试。 +- 在 Skill 构造时识别 signed TOS URL 元数据。 +- 增加 materializer 下载路径，仅对云端返回的 TOS URL 允许签名 query key。 +- 运行目标 Skill 测试和空白检查。 + +## 测试 + +- `uv run --extra dev pytest tests/skills/test_adk_skill_materializer.py` +- `uv run --extra dev pytest tests/skills/test_adk_skill_registry.py tests/skills/test_skill_space_policy.py` +- `git diff --check` + +## 风险 + +- signed URL 的 query 参数包含凭证；日志和错误信息不能回显原始 URL。 +- 如果刷新后的 signed URL 改变且没有稳定版本号，URL 可能影响缓存 key。存在 `version_id` 时仍以版本号为准，这一点保持权威。 + +## 验收标准 + +- `TosPath` 为预签名 HTTPS TOS ZIP URL 的 Skill Space Skill 可以成功下载并加载。 +- 现有 object-key Skill Space 下载测试仍然通过。 +- Harness `tos_public_url` 拒绝签名 query 凭证的测试仍然通过。 +- 不提交真实凭证或生产 signed URL。 + +## 设计评审 + +当前 checkout 没有仓库本地 `review-spec` skill，因此已执行直接评审。设计边界限定在云端返回元数据，保留 Harness 用户输入校验，对不安全 URL 形态失败关闭，并具备目标回归覆盖。 diff --git a/tests/skills/test_adk_skill_materializer.py b/tests/skills/test_adk_skill_materializer.py index 7c1d22d15..e2c54f120 100644 --- a/tests/skills/test_adk_skill_materializer.py +++ b/tests/skills/test_adk_skill_materializer.py @@ -566,6 +566,53 @@ def download_legacy_skill(skill: VeADKSkill, zip_path: Path) -> bool: ) +def test_legacy_skillspace_signed_tos_url_downloads_archive( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + signed_url = ( + "https://bucket.tos-cn-beijing.volces.com/skills/s-123/v1/" + "signed-skill.zip?X-Tos-Algorithm=TOS4-HMAC-SHA256&" + "X-Tos-Signature=synthetic" + ) + remote_skill = VeADKSkill( + name="signed-skill", + description="Signed skill.", + path=signed_url, + skill_space_id="space-1", + bucket_name="bucket", + id="s-123", + source_type="tos-signed-url", + version_id="v1", + ) + fake_client = _FakeHttpClient( + [ + _FakeStreamResponse( + signed_url, + _zip_bytes( + { + "signed-skill/SKILL.md": ( + "---\nname: signed-skill\ndescription: Signed skill.\n---\n" + "Signed body.\n" + ) + } + ), + ) + ] + ) + monkeypatch.setattr(materializer.httpx, "Client", lambda **kwargs: fake_client) + + skill_dir = materialize_remote_skill(remote_skill, cache_dir=tmp_path) + + assert fake_client.requested == [signed_url] + skill = load_skill_from_dir(skill_dir) + assert skill.name == "signed-skill" + assert skill.instructions == "Signed body." + assert skill_dir == ( + tmp_path / "tos" / "space-1" / "signed-skill" / "v1" / "signed-skill" + ) + + def test_legacy_skillspace_download_uses_explicit_region( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, diff --git a/tests/skills/test_skill_space_policy.py b/tests/skills/test_skill_space_policy.py index c3261a821..cbed9da4c 100644 --- a/tests/skills/test_skill_space_policy.py +++ b/tests/skills/test_skill_space_policy.py @@ -81,6 +81,31 @@ def test_space_policy_json_is_compact_and_deduplicated(): ) +def test_space_item_signed_tos_path_is_classified_as_signed_url_source(): + signed_url = ( + "https://bucket.tos-cn-beijing.volces.com/skills/s-123/v1/" + "reporting.zip?X-Tos-Signature=synthetic" + ) + + skill = utils._build_skill_from_space_item( + { + "Name": "reporting", + "Description": "Reporting skill.", + "BucketName": "bucket", + "TosPath": signed_url, + "SkillId": "s-123", + "Version": "v1", + }, + "ss-test", + ) + + assert skill is not None + assert skill.path == signed_url + assert skill.public_url == signed_url + assert skill.source_type == "tos-signed-url" + assert skill.bucket_name == "bucket" + + @pytest.mark.parametrize( ("mode", "ids", "expected"), [ diff --git a/veadk/skills/materializer.py b/veadk/skills/materializer.py index 9e296cad5..b1f91625b 100644 --- a/veadk/skills/materializer.py +++ b/veadk/skills/materializer.py @@ -248,6 +248,10 @@ def _download_remote_skill( _download_public_tos_zip(skill, zip_path) return + if skill.source_type == "tos-signed-url": + _download_signed_tos_zip(skill, zip_path) + return + legacy_options: dict[str, Any] = {} if credentials is not None: legacy_options["credentials"] = credentials @@ -286,13 +290,36 @@ def _download_findskill_skill(skill: Skill, zip_path: Path) -> None: def _download_public_tos_zip(skill: Skill, zip_path: Path) -> None: - public_url = _validate_public_tos_zip_url(skill.public_url or skill.path) + _download_tos_zip_url( + _validate_public_tos_zip_url(skill.public_url or skill.path), + zip_path, + allow_signed_query=False, + ) + + +def _download_signed_tos_zip(skill: Skill, zip_path: Path) -> None: + _download_tos_zip_url( + _validate_signed_tos_zip_url(skill.public_url or skill.path), + zip_path, + allow_signed_query=True, + ) + + +def _download_tos_zip_url( + url: str, + zip_path: Path, + *, + allow_signed_query: bool, +) -> None: try: final_response = None with httpx.Client(timeout=60, follow_redirects=False) as client: - next_url = public_url + next_url = url for _ in range(_MAX_TOS_PUBLIC_REDIRECTS + 1): - _validate_public_tos_zip_url(next_url) + _validate_tos_zip_url( + next_url, + allow_signed_query=allow_signed_query, + ) response = client.build_request("GET", next_url) streamed_response = client.send(response, stream=True) if streamed_response.is_redirect: @@ -302,7 +329,10 @@ def _download_public_tos_zip(skill: Skill, zip_path: Path) -> None: raise SkillMaterializeError( "Public TOS skill redirect has no Location header." ) - next_url = _validate_public_tos_zip_url(urljoin(next_url, location)) + next_url = _validate_tos_zip_url( + urljoin(next_url, location), + allow_signed_query=allow_signed_query, + ) continue final_response = streamed_response break @@ -389,7 +419,7 @@ def _validate_relative_object_path(value: str) -> None: def _cache_source_type(skill: Skill) -> str: if skill.source_type in {"skillhub", "findskill"}: return "skillhub" - if skill.source_type in {"tos", "tos-public"}: + if skill.source_type in {"tos", "tos-public", "tos-signed-url"}: return "tos" return "skillspace" @@ -399,6 +429,14 @@ def _skill_name_from_prefix(prefix: str) -> str: def _validate_public_tos_zip_url(url: str) -> str: + return _validate_tos_zip_url(url, allow_signed_query=False) + + +def _validate_signed_tos_zip_url(url: str) -> str: + return _validate_tos_zip_url(url, allow_signed_query=True) + + +def _validate_tos_zip_url(url: str, *, allow_signed_query: bool) -> str: parsed = urlparse(url.strip()) host = (parsed.hostname or "").lower() if parsed.scheme != "https" or not host: @@ -410,7 +448,7 @@ def _validate_public_tos_zip_url(url: str) -> str: if not _is_tos_public_host(host): raise SkillMaterializeError("Public TOS skill URL host is not a TOS endpoint.") query_keys = {key.lower() for key, _value in parse_qsl(parsed.query)} - if query_keys & _TOS_PUBLIC_CREDENTIAL_QUERY_KEYS: + if not allow_signed_query and query_keys & _TOS_PUBLIC_CREDENTIAL_QUERY_KEYS: raise SkillMaterializeError( "Public TOS skill URL must not include credentials." ) diff --git a/veadk/skills/utils.py b/veadk/skills/utils.py index f599937e4..520e15aaa 100644 --- a/veadk/skills/utils.py +++ b/veadk/skills/utils.py @@ -17,6 +17,7 @@ import os import frontmatter from typing import Literal +from urllib.parse import urlparse from google.adk.tools import BaseTool, ToolContext from typing import Any, Dict, Optional, Callable @@ -284,15 +285,28 @@ def _build_skill_from_space_item( skill_name = item.get("Name") if not skill_name: return None + tos_path = item.get("TosPath") or "" + source_type = "tos-signed-url" if _is_https_zip_url(tos_path) else None return Skill( name=skill_name, description=item.get("Description") or "", - path=item.get("TosPath") or "", + path=tos_path, skill_space_id=skill_space_id, bucket_name=item.get("BucketName"), id=item.get("SkillId"), version_id=item.get("Version") or item.get("SkillVersion"), + source_type=source_type, + public_url=tos_path if source_type == "tos-signed-url" else None, + ) + + +def _is_https_zip_url(value: str) -> bool: + parsed = urlparse(value.strip()) + return ( + parsed.scheme == "https" + and bool(parsed.netloc) + and parsed.path.endswith(".zip") ) From 683d8e57cf717e13e13669be5d793dfa23147576 Mon Sep 17 00:00:00 2001 From: "lixuefei.nice" Date: Fri, 6 Nov 2026 19:43:07 +0800 Subject: [PATCH 2/2] fix(skills): fallback to sharing signed URL downloads --- ...026-11-06-signed-tos-url-skill-download.md | 22 ++-- ...-11-06-signed-tos-url-skill-download.zh.md | 22 ++-- tests/skills/test_adk_skill_materializer.py | 76 ++++++++++++ veadk/skills/materializer.py | 110 +++++++++++++++++- 4 files changed, 211 insertions(+), 19 deletions(-) diff --git a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md index e010d9b02..fd9da4309 100644 --- a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md +++ b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.md @@ -4,12 +4,12 @@ `VeSkillRegistry.get_skill()` refreshes Skill Space metadata with `load_skills_from_cloud()`, selects a remote skill, and materializes it with `materialize_remote_skill()`. For legacy AgentKit Skill Spaces, `_build_skill_from_space_item()` maps `TosPath` to `Skill.path` and `BucketName` to `Skill.bucket_name`. -Today `_download_legacy_skill_space_skill()` assumes `Skill.path` is a TOS object key such as `skills///.zip`. If the cloud API returns a complete HTTPS pre-signed TOS URL in `TosPath`, the non-Vestack branch passes that URL to `VeTOS.download()` as an object key, while the Vestack branch tries to parse it as a legacy object path. +Today `_download_legacy_skill_space_skill()` assumes `Skill.path` is a TOS object key such as `skills///.zip`. If the cloud API returns a complete HTTPS pre-signed TOS URL in `TosPath`, the non-Vestack branch passes that URL to `VeTOS.download()` as an object key, while the Vestack branch tries to parse it as a legacy object path. Real validation against `ss-yey3lbgruoq7vcs7xlp4` also showed the object-key path can return `403 AccessDenied` for sharing-bucket skills, while `GenTempTosObjectDownloadUrlSharing` returns a usable signed URL for the same skill. ## Goals - Load a normal Skill Space skill when `TosPath` is a complete pre-signed HTTPS TOS ZIP URL. -- Preserve existing behavior for object-key `TosPath` values. +- Preserve direct object-key downloads when they work, and fall back to a sharing signed URL when they are denied. - Reuse the existing archive safety checks: streaming size limit, zip validation, safe extraction, and ADK `SKILL.md` load validation. - Keep signed URL content out of error messages and source fixtures except redacted, synthetic test URLs. @@ -17,13 +17,14 @@ Today `_download_legacy_skill_space_skill()` assumes `Skill.path` is a TOS objec - Do not change Harness `tos_public_url` validation. User-provided Harness public URLs must continue to reject TOS credential query parameters. - Do not change private TOS directory loading through `tos://bucket/prefix/`. -- Do not add new cloud APIs or change `ListSkillsBySpaceId`. +- Do not change `ListSkillsBySpaceId`. ## Scenarios -1. `ListSkillsBySpaceId` returns `BucketName` plus object-key `TosPath`; the SDK TOS download path remains unchanged. -2. `ListSkillsBySpaceId` returns `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`; the materializer downloads that URL directly and loads the Skill. -3. A signed URL redirects to a non-TOS host or returns a non-ZIP payload; materialization fails closed. +1. `ListSkillsBySpaceId` returns `BucketName` plus object-key `TosPath`; direct SDK TOS download still succeeds for readable objects. +2. `ListSkillsBySpaceId` returns object-key `TosPath`, but direct SDK TOS download is denied; the materializer requests `GenTempTosObjectDownloadUrlSharing`, downloads `Result.SignedUrl`, and loads the Skill. +3. `ListSkillsBySpaceId` returns `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`; the materializer downloads that URL directly and loads the Skill. +4. A signed URL redirects to a non-TOS host or returns a non-ZIP payload; materialization fails closed. ## Requirements @@ -32,6 +33,7 @@ Today `_download_legacy_skill_space_skill()` assumes `Skill.path` is a TOS objec - Require HTTPS, a TOS endpoint host, a `.zip` path, and no username/password in the URL. - Stream downloads with the same 64 MiB archive limit used by public TOS ZIPs. - Preserve cache key behavior by using `version_id` when present; otherwise the metadata digest includes the URL. +- Do not log raw signed URLs or credential-bearing query strings during fallback. ## Design @@ -39,6 +41,8 @@ Add a materializer branch for cloud-returned signed TOS ZIP URLs. `_build_skill_ The signed-URL downloader should share the public TOS ZIP streaming implementation, but use a validator that allows TOS signature query keys. Harness `tos_public_url` continues to use the stricter public validator that rejects credential query keys. +For legacy object-key metadata, keep the current direct `VeTOS.download()` attempt first. If that attempt fails or raises, request `GenTempTosObjectDownloadUrlSharing` with the skill id and version, read `Result.SignedUrl`, and pass it through the same signed-URL downloader. + ## Affected Files - `veadk/skills/utils.py` @@ -52,8 +56,10 @@ No component spec update is required. The external Skill Space metadata contract ## Tasks - Add a failing regression test for a cloud-returned pre-signed TOS URL in `TosPath`. +- Add a failing regression test for object-key `TosPath` falling back to a sharing signed URL after direct TOS download denial. - Classify signed TOS URL metadata during Skill construction. - Add a materializer download path that allows signed query keys only for cloud-returned TOS URLs. +- Reuse that signed URL download path for sharing signed URL fallback. - Run targeted Skill tests and whitespace checks. ## Tests @@ -66,11 +72,13 @@ No component spec update is required. The external Skill Space metadata contract - Signed URLs contain credentials in query parameters; logs and error messages must not echo raw URLs. - URL cache keys may change when a refreshed signed URL changes. This is acceptable when no stable version is provided; `version_id` remains authoritative when available. +- Sharing signed URL generation may fail for skills with incomplete backend metadata; such skills still fail closed and should be diagnosed separately. ## Acceptance Criteria - A Skill Space skill whose `TosPath` is a pre-signed HTTPS TOS ZIP URL downloads and loads successfully. -- Existing object-key Skill Space downloads still pass. +- Existing readable object-key Skill Space downloads still pass. +- Object-key Skill Space downloads that are denied by direct TOS access can load through the sharing signed URL fallback. - Harness `tos_public_url` tests that reject signed query credentials still pass. - No real credentials or production signed URLs are committed. diff --git a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md index 326232fd8..a0608146e 100644 --- a/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md +++ b/prd-spec/bugfixes/2026-11-06-signed-tos-url-skill-download.zh.md @@ -4,12 +4,12 @@ `VeSkillRegistry.get_skill()` 会通过 `load_skills_from_cloud()` 刷新 Skill Space 元数据，选中远端 Skill 后调用 `materialize_remote_skill()` 本地物化。对于 legacy AgentKit Skill Space，`_build_skill_from_space_item()` 会把 `TosPath` 映射到 `Skill.path`，把 `BucketName` 映射到 `Skill.bucket_name`。 -当前 `_download_legacy_skill_space_skill()` 假设 `Skill.path` 是 `skills///.zip` 这类 TOS 对象 key。如果云端 API 在 `TosPath` 中返回完整 HTTPS 预签名 TOS URL，非 Vestack 分支会把这个 URL 当作 object key 传给 `VeTOS.download()`，Vestack 分支则会把它误拆成 legacy 对象路径。 +当前 `_download_legacy_skill_space_skill()` 假设 `Skill.path` 是 `skills///.zip` 这类 TOS 对象 key。如果云端 API 在 `TosPath` 中返回完整 HTTPS 预签名 TOS URL，非 Vestack 分支会把这个 URL 当作 object key 传给 `VeTOS.download()`，Vestack 分支则会把它误拆成 legacy 对象路径。针对 `ss-yey3lbgruoq7vcs7xlp4` 的真实验证还显示，sharing bucket 中的 object-key Skill 可能返回 `403 AccessDenied`，但同一 Skill 通过 `GenTempTosObjectDownloadUrlSharing` 可以拿到可用 signed URL。 ## 目标 - 当 `TosPath` 是完整预签名 HTTPS TOS ZIP URL 时，普通 Skill Space Skill 可以正常加载。 -- 保留现有 object-key 形式 `TosPath` 的行为。 +- 保留可直接读取的 object-key 下载路径，并在直连被拒绝时 fallback 到 sharing signed URL。 - 复用现有归档安全检查：流式大小限制、ZIP 校验、安全解压和 ADK `SKILL.md` 加载校验。 - 除脱敏的合成测试 URL 外，不在错误信息和源码 fixture 中保留 signed URL 内容。 @@ -17,13 +17,14 @@ - 不修改 Harness `tos_public_url` 校验。用户传入的 Harness public URL 仍然必须拒绝 TOS 凭证 query 参数。 - 不修改通过 `tos://bucket/prefix/` 加载私有 TOS 目录的逻辑。 -- 不新增云端 API，也不修改 `ListSkillsBySpaceId`。 +- 不修改 `ListSkillsBySpaceId`。 ## 场景 -1. `ListSkillsBySpaceId` 返回 `BucketName` 加 object-key `TosPath`；SDK TOS 下载路径保持不变。 -2. `ListSkillsBySpaceId` 返回 `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`；materializer 直接下载该 URL 并加载 Skill。 -3. signed URL 重定向到非 TOS host，或返回非 ZIP 内容；物化失败关闭。 +1. `ListSkillsBySpaceId` 返回 `BucketName` 加 object-key `TosPath`；可读对象仍然通过 SDK TOS 直连下载。 +2. `ListSkillsBySpaceId` 返回 object-key `TosPath`，但 SDK TOS 直连下载被拒绝；materializer 调用 `GenTempTosObjectDownloadUrlSharing`，下载 `Result.SignedUrl` 并加载 Skill。 +3. `ListSkillsBySpaceId` 返回 `TosPath=https://bucket.tos-cn-beijing.volces.com/...zip?X-Tos-Signature=...`；materializer 直接下载该 URL 并加载 Skill。 +4. signed URL 重定向到非 TOS host，或返回非 ZIP 内容；物化失败关闭。 ## 需求 @@ -32,6 +33,7 @@ - 要求 URL 使用 HTTPS、TOS endpoint host、`.zip` 路径，并且不包含 username/password。 - 下载必须流式执行，并沿用 public TOS ZIP 的 64 MiB 归档大小限制。 - 缓存 key 优先使用 `version_id`；没有版本时，metadata digest 可以包含 URL。 +- fallback 过程中不得记录原始 signed URL 或携带凭证的 query string。 ## 设计 @@ -39,6 +41,8 @@ signed URL 下载器应复用 public TOS ZIP 的流式下载实现，但使用允许 TOS 签名 query key 的校验器。Harness `tos_public_url` 继续使用更严格的 public 校验器，仍然拒绝凭证 query key。 +对于 legacy object-key 元数据，先保留当前 `VeTOS.download()` 直连尝试。如果直连失败或抛错，则使用 skill id 和 version 调用 `GenTempTosObjectDownloadUrlSharing`，读取 `Result.SignedUrl`，并交给同一个 signed URL 下载器处理。 + ## 影响文件 - `veadk/skills/utils.py` @@ -52,8 +56,10 @@ signed URL 下载器应复用 public TOS ZIP 的流式下载实现，但使用 ## 任务 - 为 `TosPath` 中云端返回的预签名 TOS URL 增加失败回归测试。 +- 为 object-key `TosPath` 在 TOS 直连被拒绝后 fallback 到 sharing signed URL 增加失败回归测试。 - 在 Skill 构造时识别 signed TOS URL 元数据。 - 增加 materializer 下载路径，仅对云端返回的 TOS URL 允许签名 query key。 +- 复用 signed URL 下载路径处理 sharing signed URL fallback。 - 运行目标 Skill 测试和空白检查。 ## 测试 @@ -66,11 +72,13 @@ signed URL 下载器应复用 public TOS ZIP 的流式下载实现，但使用 - signed URL 的 query 参数包含凭证；日志和错误信息不能回显原始 URL。 - 如果刷新后的 signed URL 改变且没有稳定版本号，URL 可能影响缓存 key。存在 `version_id` 时仍以版本号为准，这一点保持权威。 +- sharing signed URL 生成可能因 Skill 后端元数据不完整而失败；这类 Skill 仍然失败关闭，需要单独诊断。 ## 验收标准 - `TosPath` 为预签名 HTTPS TOS ZIP URL 的 Skill Space Skill 可以成功下载并加载。 -- 现有 object-key Skill Space 下载测试仍然通过。 +- 现有可读 object-key Skill Space 下载测试仍然通过。 +- TOS 直连被拒绝的 object-key Skill Space 下载可以通过 sharing signed URL fallback 加载。 - Harness `tos_public_url` 拒绝签名 query 凭证的测试仍然通过。 - 不提交真实凭证或生产 signed URL。 diff --git a/tests/skills/test_adk_skill_materializer.py b/tests/skills/test_adk_skill_materializer.py index e2c54f120..de4c97233 100644 --- a/tests/skills/test_adk_skill_materializer.py +++ b/tests/skills/test_adk_skill_materializer.py @@ -613,6 +613,82 @@ def test_legacy_skillspace_signed_tos_url_downloads_archive( ) +def test_legacy_skillspace_object_key_falls_back_to_sharing_signed_url( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + remote_skill = VeADKSkill( + name="pdf", + description="PDF skill.", + path="skills/s-123/v1/pdf.zip", + skill_space_id="space-1", + bucket_name="sharing-bucket", + id="s-123", + version_id="v1", + ) + signed_url = ( + "https://sharing-bucket.tos-cn-beijing.volces.com/skills/s-123/v1/" + "pdf.zip?X-Tos-Algorithm=TOS4-HMAC-SHA256&X-Tos-Signature=synthetic" + ) + tos_calls: list[dict[str, object]] = [] + top_calls: list[dict[str, object]] = [] + + class FakeVeTOS: + def __init__(self, **kwargs: object) -> None: + tos_calls.append(kwargs) + + def download(self, **kwargs: object) -> bool: + tos_calls.append(kwargs) + return False + + def fake_ve_request(**kwargs: object) -> dict[str, object]: + top_calls.append(kwargs) + return {"Result": {"SignedUrl": signed_url}} + + fake_client = _FakeHttpClient( + [ + _FakeStreamResponse( + signed_url, + _zip_bytes( + { + "pdf/SKILL.md": ( + "---\nname: pdf\ndescription: PDF skill.\n---\nPDF body.\n" + ) + } + ), + ) + ] + ) + + monkeypatch.setattr( + materializer, + "_get_cloud_credentials", + lambda: ("ak", "sk", "sts"), + ) + monkeypatch.setattr( + materializer, + "_get_agentkit_endpoint", + lambda region=None: ( + "agentkit", + "cn-beijing", + "agentkit.cn-beijing.volcengineapi.com", + ), + ) + monkeypatch.setattr("veadk.integrations.ve_tos.ve_tos.VeTOS", FakeVeTOS) + monkeypatch.setattr(materializer, "ve_request", fake_ve_request, raising=False) + monkeypatch.setattr(materializer.httpx, "Client", lambda **kwargs: fake_client) + + skill_dir = materialize_remote_skill(remote_skill, cache_dir=tmp_path) + skill = load_skill_from_dir(skill_dir) + + assert skill.name == "pdf" + assert skill.instructions == "PDF body." + assert fake_client.requested == [signed_url] + assert top_calls[0]["action"] == "GenTempTosObjectDownloadUrlSharing" + assert top_calls[0]["request_body"] == {"SkillId": "s-123", "SkillVersion": "v1"} + assert tos_calls[1]["raise_on_error"] is False + + def test_legacy_skillspace_download_uses_explicit_region( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, diff --git a/veadk/skills/materializer.py b/veadk/skills/materializer.py index b1f91625b..531daa4a7 100644 --- a/veadk/skills/materializer.py +++ b/veadk/skills/materializer.py @@ -37,6 +37,7 @@ _get_agentkit_endpoint, _get_cloud_credentials, download_skillhub_skill, + ve_request, ) from veadk.utils.logger import get_logger @@ -506,11 +507,110 @@ def _download_legacy_skill_space_skill( bucket_name=skill.bucket_name, region=resolved_region, ) - return tos_client.download( - bucket_name=skill.bucket_name, - object_key=skill.path, - save_path=str(zip_path), - raise_on_error=raise_on_error, + try: + downloaded = tos_client.download( + bucket_name=skill.bucket_name, + object_key=skill.path, + save_path=str(zip_path), + raise_on_error=raise_on_error, + ) + except Exception as exc: + logger.warning( + f"Direct TOS download failed for skill-space skill '{skill.name}': " + f"{type(exc).__name__}. Trying sharing signed URL." + ) + downloaded = False + + if downloaded: + return True + + return _download_legacy_skill_via_sharing_signed_url( + skill=skill, + access_key=access_key, + secret_key=secret_key, + session_token=session_token, + service=service, + region=resolved_region, + host=host, + scheme=scheme, + zip_path=zip_path, + ) + + +def _download_legacy_skill_via_sharing_signed_url( + *, + skill: Skill, + access_key: str, + secret_key: str, + session_token: str, + service: str, + region: str, + host: str, + scheme: str, + zip_path: Path, +) -> bool: + skill_id, skill_version = _legacy_skill_id_and_version(skill) + if not skill_id or not skill_version: + logger.error( + f"Cannot request sharing signed URL for skill '{skill.name}': " + "missing skill id or version." + ) + return False + + try: + response = ve_request( + request_body={ + "SkillId": skill_id, + "SkillVersion": skill_version, + }, + action="GenTempTosObjectDownloadUrlSharing", + ak=access_key, + sk=secret_key, + service=service, + version="2025-10-30", + region=region, + host=host, + header={"X-Security-Token": session_token}, + scheme=scheme, # type: ignore[arg-type] + ) + if isinstance(response, str): + response = json.loads(response) + if _top_response_has_error(response): + logger.error(f"Failed to get sharing signed URL for skill '{skill.name}'.") + return False + signed_url = ( + response.get("Result", {}).get("SignedUrl") + if isinstance(response, dict) + else None + ) + if not signed_url: + logger.error( + f"Sharing signed URL response missing SignedUrl for '{skill.name}'." + ) + return False + + _download_tos_zip_url(signed_url, zip_path, allow_signed_query=True) + return True + except Exception as exc: + logger.error( + f"Failed to download skill '{skill.name}' via sharing signed URL: " + f"{type(exc).__name__}" + ) + return False + + +def _legacy_skill_id_and_version(skill: Skill) -> tuple[str | None, str | None]: + path_parts = (skill.path or "").split("/") + skill_id = skill.id or (path_parts[1] if len(path_parts) > 1 else None) + skill_version = skill.version_id or (path_parts[2] if len(path_parts) > 2 else None) + return skill_id, skill_version + + +def _top_response_has_error(response: Any) -> bool: + return bool( + isinstance(response, dict) + and isinstance(response.get("ResponseMetadata"), dict) + and "Error" in response["ResponseMetadata"] )