fix(serve): validate ECR registry host before docker login

[mohamedzeidan2021]

Tightens how LocalContainerMode identifies and handles ECR image URIs.

Previously the ECR classifier and the registry-host extraction used different views of the image string,
which could disagree for unusual/malformed URIs. This parses the registry host once, validates it against
a strict ECR endpoint pattern, and reuses that same value consistently.

Also adds a lightweight validation of hub-sourced image URIs at consumption time so malformed addresses
are caught early with a clear error, rather than surfacing as a confusing failure later in the flow.
Legitimate non-ECR images and other-partition ECR hosts (e.g. ISO/FIPS endpoints) pass through unchanged.

ECR_HOST_RE is consolidated into check_image_uri as a single source of truth. Adds unit tests covering
standard/China ECR URIs, malformed URIs, ISO/FIPS pass-through, and public images.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Claude finished @mohamedzeidan2021's task in 3m 49s —— View job

---

Code Review — PR #6160 (ECR host validation / parser-confusion hardening)

• Read the PR diff
• Review check_image_uri.py
• Review local_container_mode.py
• Review model_builder.py / model_builder_servers.py call sites
• Assess tests

Note: the inline-comment tool isn't available in this session, so I'm posting findings here referencing file/line.

Overall this is a well-scoped, well-documented security fix. The core idea — deriving both the "is this ECR?" classifier and the docker login target from a single strict-validated registry host rather than a substring check — closes the parser-confusion / credential-leak path cleanly, and the test coverage (valid, China partition, spoofed host, substrings-in-path, public images, ISO/FIPS pass-through, empty) is thorough. A few points worth considering, none blocking:

1. Regex anchor $ allows a trailing newline (low severity, defense-in-depth)

check_image_uri.py:15

ECR_HOST_RE = re.compile(r"^[0-9]{12}\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com(\.cn)?$")

In Python, $ matches at end-of-string or immediately before a trailing \n. So "123456789012.dkr.ecr.us-east-1.amazonaws.com\n" would still match. Since the entire premise here is validating untrusted hub input before it reaches docker login, prefer \Z (matches only the true end of string) for a strict host allowlist:

ECR_HOST_RE = re.compile(r"^[0-9]{12}\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com(\.cn)?\Z")

This is a hardening nit — image.split("/")[0] containing a newline is unlikely in practice — but cheap to make airtight.

2. Duplicated _ecr_registry_host logic (maintainability)

local_container_mode.py:292-301 reimplements the same host = image.split("/")[0]; return host if RE.match(host) else None that already exists as the module-level _ecr_registry_host in check_image_uri.py:327. Since local_container_mode already imports from that module (ECR_HOST_RE), consider importing/reusing the function directly so the two can't drift. The _ECR_HOST_RE = ECR_HOST_RE alias at line 39 is also redundant — the imported name could be used as-is. Minor.

3. Behavior tightening of _is_ecr_image — confirmed safe

_is_ecr_image moves from a loose substring test to requiring the first URI segment to be a strict ECR host. I checked the edge cases that could regress legitimate images:

• FIPS (...dkr.ecr-fips...) and ISO (...c2s.ic.gov) hosts were already classified non-ECR by the old substring check (they lack .dkr.ecr. and .amazonaws.com respectively), so no new regression, and validate_hub_ecr_address correctly lets them pass through.
• Standard/China ECR hosts (12-digit account, always-lowercase region) match both old and new.

So the only cases newly excluded are the malicious/malformed ones — which is the intent. Good. Worth calling out in the PR description that FIPS/ISO images are not treated as ECR for the local-login path (they never were), in case any consumer relied on that.

4. Test coverage of the call sites

The unit tests exercise validate_hub_ecr_address and the LocalContainerMode classifier/login-target well. The three new call sites (model_builder.py:1258, model_builder.py:1281, model_builder_servers.py:1004) are not directly covered — a small test asserting the validator is invoked (and raises) on a spoofed hub EcrAddress there would guard against a future refactor silently dropping the guard. Optional given the validator itself is well-tested.

Nice work — the shared-source-of-truth approach and the \Z-style rigor make this hard to regress.