Skip to content

[azure-ai-ml] Validate artifact tool download request URLs - #49186

Open
ayushhgarg-work wants to merge 5 commits into
Azure:mainfrom
ayushhgarg-work:ayushhgarg-microsoft-artifact-request-validation
Open

ayushhgarg-work wants to merge 5 commits into
Azure:mainfrom
ayushhgarg-work:ayushhgarg-microsoft-artifact-request-validation

Conversation

@ayushhgarg-work

@ayushhgarg-work ayushhgarg-work commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Description

Validate request destinations in the Azure DevOps artifact-tool download fallback used by component additional_includes.

  • Validate the complete supported organization URL before acquiring credentials and construct the authenticated metadata request against the canonical service endpoint.
  • Validate the HTTPS binary URL returned by that service, omit the bearer authorization header from the binary request, and disable Azure Core redirect handling on both requests.
  • Reject unsuccessful responses and malformed metadata explicitly, require the extracted platform-specific executable at the archive root, and clean up failures before changing the override or cached tool path.
  • Preserve archive traversal protection and narrow fallback exception handling so validation errors are not swallowed.
  • Add offline regressions, including the real Azure Core redirect policy exercised with a mocked transport.

Cache containment and concurrent cache handling are addressed separately in #49185. This PR preserves the existing cache implementation and authentication scope; it does not add a speculative CDN allowlist or release/version changes. The two branches were checked for conflict-free integration.

Validation

Review follow-up at cd6710979e638092a0ed05ab1f448398c3bf3846:

  • Official SDK MCP playback tests: 164 passed, including eight new malformed-archive cases and existing request/component/additional-includes coverage.
  • Official SDK MCP formatting check passed.
  • The full local build was not green: dependency bootstrap encountered a public-package TLS failure, and MyPy reported an unrelated unchanged component diagnostic. The affected component and checker configuration match the previous green CI source; no unrelated code or verification rules were changed.

The original request fix was also checked on Windows and native Linux, including both override modes. The final SDK review branches merge without conflicts in either order; their combined tree passed all 400 selected tests through the official MCP tool in both override modes.

Source imports were asserted against the selected checkout and playback used the owned loopback proxy. These local results are not a claim that the latest PR CI is green.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

ayushhgarg03 and others added 2 commits September 23, 2026 13:14
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A valid archive without the ArtifactTool executable is incorrectly marked as installed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Hardens Azure DevOps ArtifactTool downloads against unsafe URLs, redirects, credential forwarding, and failed extraction.

Changes:

  • Adds strict URL validation and redirect handling.
  • Cleans failed installations and narrows fallback exceptions.
  • Adds security-focused unit tests and fixtures.
File Description
azure/​ai/​ml/​_utils/​_artifact_utils.py Hardens ArtifactTool download and installation.
tests/​utils/​unittests/​test_artifact_tool_requests.py Tests validation and failure scenarios.
tests/​test_utilities/​artifact_fixtures.py Adds isolated ArtifactTool fixtures.
tests/​test_utilities/​utils.py Isolates artifact-cache test state.
CHANGELOG.md Documents the fix.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread sdk/ml/azure-ai-ml/azure/ai/ml/_utils/_artifact_utils.py
@ayushhgarg-work ayushhgarg-work changed the title Ayushhgarg microsoft artifact request validation [azure-ai-ml] Validate artifact tool download request URLs Sep 25, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 25, 2026 06:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Linux installations omit executable permissions, leaving the downloaded artifact tool unusable.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment on lines +273 to 276
tool_name = "artifacttool.exe" if os_name == "Windows" else "artifacttool"
if not (artifacts_tool_path / tool_name).is_file():
raise RuntimeError(f"Artifact tool archive does not contain {tool_name}.")
os.environ["AZURE_DEVOPS_EXT_ARTIFACTTOOL_OVERRIDE_PATH"] = str(artifacts_tool_path.resolve())
@ayushhgarg-work
ayushhgarg-work enabled auto-merge (squash) September 25, 2026 08:01
Copilot AI review requested due to automatic review settings September 25, 2026 08:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The credential-handling and executable-download path is security-sensitive and warrants final human review despite strong targeted coverage.

Review effort: Balanced
Findings: 1 High severity

Open (1)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants