fix(egress): allow single-layer %2f in raw request path

_path_is_ambiguous() rejected every request whose raw path contained a
percent-encoded slash. npm scoped package registry paths are required to
be encoded as /@scope%2fname on the wire (npm registry protocol), so
credential injection for those requests started returning 403 as soon as
the Credential Vault was active — breaking 'npm install @scope/pkg' in
sandboxes.

Split the two concerns:

  * At the raw-request stage in request(), tolerate a single-layer %2f
    (allow_single_encoded_slash=True). Nested encodings (%252f etc.),
    raw backslash, encoded backslash and dot-segments still trip the
    guard.
  * A new _path_encoded_slash_changes_binding() re-runs binding path
    matching against unquote(raw_path). If the encoded slash would move
    the request across a credential binding boundary, the request is
    still rejected with 403.
  * The post-substitution check in _apply_path_query_substitutions()
    keeps the strict semantics (allow_single_encoded_slash=False), so
    substitutions that inject a %2f into the path are still refused.

Also wire mitmscripts unit tests into egress-test.yml so this regression
is caught in CI, and add a Python E2E case that drives the same wire
format through the real sidecar + credential vault + curl inside a
sandbox against the echo target.

Fixes: https://project.aone.alibaba-inc.com/v2/project/2135082/workitem/84550489

Author: Pangjiping

.github/workflows/egress-test.yml

@@ -31,6 +31,11 @@ jobs:
         with:
           go-version: '1.25.9'
 
+      - name: Set up Python
+        uses: actions/setup-python@v6
+        with:
+          python-version: '3.10'
+
       - name: Check gofmt
         working-directory: components/egress
         run: |
@@ -51,6 +56,11 @@ jobs:
         run: |
           go test ./...
 
+      - name: Run mitmscripts unit tests
+        working-directory: components/egress
+        run: |
+          python -m unittest discover -v -s tests -p "test_*.py"
+
   smoke:
     needs: changes
     if: needs.changes.outputs.relevant == 'true'

components/egress/mitmscripts/system.py

@@ -191,13 +191,29 @@ def _request_path(flow: http.HTTPFlow) -> str:
 _DOT_SEGMENT_RE = re.compile(r"/\.\.(/|$)")
 
 
-def _path_is_ambiguous(raw_path: str) -> bool:
+def _path_is_ambiguous(raw_path: str, *, allow_single_encoded_slash: bool = False) -> bool:
     """Return True if the raw request path contains ambiguous segments.
 
     Legitimate HTTP clients resolve dot segments before sending. Raw ``..``
     or percent-encoded separators on the wire indicate an attempt to confuse
     path-based authorization checks because the canonical path seen by the
     upstream server may differ from the raw prefix matched here.
+
+    ``allow_single_encoded_slash`` toggles how a single-layer ``%2f`` on the
+    wire is treated:
+
+    * ``False`` (default) — historical strict behavior. Suitable for paths
+      produced by our own substitution pipeline, where the credential proxy
+      injects a ``%2f`` on the fly. Such a rewrite always shifts the
+      canonical path relative to what the client sent, so it is treated as
+      ambiguous.
+    * ``True`` — used when checking the raw path the client sent. Legit
+      ecosystems require a single ``%2f`` (npm scoped package registry paths
+      like ``/@scope%2fname``). Nested encodings (``%252f`` etc.) and raw
+      backslash / dot-segments are still rejected. The complementary
+      :func:`_path_encoded_slash_changes_binding` check rejects a
+      single-layer ``%2f`` if decoding it would cross an authorization
+      boundary.
     """
     path = raw_path.split("?", 1)[0]
 
@@ -209,7 +225,15 @@ def _path_is_ambiguous(raw_path: str) -> bool:
     decoded = path
     for _ in range(10):
         lower = decoded.lower()
-        if "%2f" in lower or "%5c" in lower:
+        if "%2f" in lower:
+            # A single-layer ``%2f`` on the wire is legitimate for some
+            # ecosystems (e.g. npm scoped package registry paths use
+            # ``/@scope%2fname``). Tolerate it on the first pass when the
+            # caller opts in; a nested ``%252f`` still trips this check on
+            # the next iteration because it decodes back to ``%2f``.
+            if not (allow_single_encoded_slash and decoded is path):
+                return True
+        if "%5c" in lower:
             return True
         if "\\" in decoded:
             return True
@@ -223,6 +247,67 @@ def _path_is_ambiguous(raw_path: str) -> bool:
     return False
 
 
+def _path_encoded_slash_changes_binding(
+    flow: http.HTTPFlow, vault: ActiveVault
+) -> bool:
+    """Return True if decoding ``%2f`` in the raw path would change which
+    credential binding matches.
+
+    Called only when the raw request path contains ``%2f``. Legitimate uses
+    (e.g. npm scoped package registry paths ``/@scope%2fname``) decode to a
+    path that still matches the same binding, so this returns False and the
+    request proceeds. A crafted path such as
+    ``/api/v8/projects/123%2f..%2f456/variables`` decodes to a different
+    scope and this returns True so the caller can respond 403 before
+    injecting credentials.
+    """
+    raw_path = _request_path(flow)
+    if "%2f" not in raw_path.lower():
+        return False
+
+    decoded_path = unquote(raw_path)
+    if decoded_path == raw_path:
+        return False
+
+    # If the decoded form contains dot-segments, treat it as ambiguous.
+    if _DOT_SEGMENT_RE.search(decoded_path):
+        return True
+
+    scheme = (flow.request.scheme or "").lower()
+    host = _request_host(flow)
+    port = _request_port(flow)
+    method = (flow.request.method or "").upper()
+
+    def _non_path_matches(binding: dict[str, Any]) -> bool:
+        match = binding.get("match") or {}
+        schemes = match.get("schemes") or ["https"]
+        if scheme not in schemes:
+            return False
+        canonical_port = 443 if scheme == "https" else 80
+        if port != canonical_port:
+            return False
+        methods = [m.upper() for m in (match.get("methods") or ["GET", "POST", "PUT", "PATCH", "DELETE"])]
+        if method not in methods:
+            return False
+        for pattern in match.get("hosts") or []:
+            ok, _ = _host_matches(host, pattern)
+            if ok:
+                return True
+        return False
+
+    def _matches_with_path(path: str) -> set[int]:
+        matched: set[int] = set()
+        for idx, binding in enumerate(vault.bindings):
+            if not _non_path_matches(binding):
+                continue
+            paths = (binding.get("match") or {}).get("paths") or ["/*"]
+            if any(_path_matches(path, p) for p in paths):
+                matched.add(idx)
+        return matched
+
+    return _matches_with_path(raw_path) != _matches_with_path(decoded_path)
+
+
 def _host_matches(host: str, pattern: str) -> tuple[bool, int]:
     pattern = pattern.rstrip(".").lower()
     if pattern.startswith("*."):
@@ -499,8 +584,12 @@ def request(flow: http.HTTPFlow) -> None:
     # Raw dot-segments or encoded variants on the wire are not produced by
     # legitimate HTTP clients and can trick prefix-based path matching into
     # injecting credentials for a scope the canonical path does not belong to.
+    # A single-layer ``%2f`` is tolerated at this stage because some legit
+    # ecosystems require it (npm scoped packages send ``/@scope%2fname``);
+    # the follow-up ``_path_encoded_slash_changes_binding`` check rejects any
+    # ``%2f`` whose decoded form would cross a credential binding boundary.
     raw_path = flow.request.path or "/"
-    if _path_is_ambiguous(raw_path):
+    if _path_is_ambiguous(raw_path, allow_single_encoded_slash=True):
         flow.response = http.Response.make(
             403,
             b"request path contains ambiguous segments\n",
@@ -512,6 +601,25 @@ def request(flow: http.HTTPFlow) -> None:
         )
         return
 
+    # A single-layer ``%2f`` is tolerated by ``_path_is_ambiguous`` (legit
+    # ecosystems like npm scoped packages require it). Reject it here only
+    # when decoding the ``%2f`` would change which credential binding
+    # matches, i.e. the encoded slash actually crosses an authorization
+    # boundary. This keeps ``/@scope%2fname`` requests working while still
+    # stopping crafted paths such as ``/api/v8/projects/123%2f..%2f456/...``.
+    if _path_encoded_slash_changes_binding(flow, vault):
+        flow.response = http.Response.make(
+            403,
+            b"request path contains ambiguous segments\n",
+            {"content-type": "text/plain"},
+        )
+        ctx.log.warn(
+            "credential proxy: rejected request whose encoded slash crosses "
+            "the credential binding boundary: "
+            f"{flow.request.method} {_request_host(flow)}{_request_path(flow)}"
+        )
+        return
+
     binding = _select_binding(flow, vault)
     if not binding:
         return

components/egress/tests/test_mitmscripts_system.py

@@ -696,6 +696,161 @@ def test_no_vault_active_allows_dot_dot_through(self) -> None:
         # (flow.response is the pre-initialized _Response, not a 403)
         self.assertNotIn("Private-Token", flow.request.headers._values)
 
+    def test_encoded_slash_within_binding_scope_allowed(self) -> None:
+        """A ``%2f`` whose decoded form still matches the same binding must
+        be allowed. Regression for the npm scoped package case where the
+        registry path ``/@scope%2fname`` was rejected as a false positive."""
+        system = self._make_system_with_vault()
+        flow = _Flow()
+        # ``/api/v8/*`` covers both raw and decoded forms, so this is the
+        # tolerated case: the encoded slash does not cross a binding boundary.
+        system._load_active_vault = lambda: system.ActiveVault(
+            1,
+            [
+                {
+                    "name": "gitlab-api",
+                    "match": {
+                        "hosts": ["code.example.com"],
+                        "methods": ["GET"],
+                        "paths": ["/api/v8/*"],
+                    },
+                    "headers": [{"name": "Private-Token", "value": "secret-token"}],
+                }
+            ],
+            ["secret-token"],
+        )
+        flow.request.path = "/api/v8/projects/123%2fnested/variables"
+
+        system.request(flow)
+
+        self.assertEqual("secret-token", flow.request.headers.get("Private-Token"))
+
+    def test_encoded_slash_crossing_binding_rejected(self) -> None:
+        """A ``%2f`` that decodes across a binding boundary must be rejected.
+
+        Two bindings are configured: one for a broad scope with a
+        low-privilege token, and one for a narrow scope with a high-privilege
+        token. A crafted path can match the narrow binding literally while
+        its decoded form belongs only to the broad one — that mismatch is
+        what the new guard must catch before injecting the wrong credential.
+        """
+        system = _load_system_module()
+        system._load_active_vault = lambda: system.ActiveVault(
+            1,
+            [
+                {
+                    "name": "gitlab-api-broad",
+                    "match": {
+                        "hosts": ["code.example.com"],
+                        "methods": ["GET"],
+                        "paths": ["/api/v8/*"],
+                    },
+                    "headers": [{"name": "Private-Token", "value": "broad-token"}],
+                },
+                {
+                    "name": "gitlab-api-narrow",
+                    "match": {
+                        "hosts": ["code.example.com"],
+                        "methods": ["GET"],
+                        # Literal ``%2f`` in the pattern only matches the raw
+                        # form; the decoded path stops matching this pattern.
+                        "paths": ["/api/v8/projects/123%2f*"],
+                    },
+                    "headers": [{"name": "Private-Token", "value": "narrow-token"}],
+                },
+            ],
+            ["broad-token", "narrow-token"],
+        )
+        flow = _Flow()
+        flow.request.path = "/api/v8/projects/123%2fescape/variables"
+
+        system.request(flow)
+
+        # Raw match set = {broad, narrow}; decoded match set = {broad}.
+        # The two differ, so the request is rejected before injection.
+        self.assertIsNotNone(flow.response)
+        self.assertEqual(403, flow.response.status_code)
+        self.assertNotIn("Private-Token", flow.request.headers._values)
+
+
+class SystemAddonNpmScopedPackageTest(unittest.TestCase):
+    """npm scoped package registry paths must not be blocked as ambiguous."""
+
+    def _make_system_with_npm_vault(self):
+        system = _load_system_module()
+        system._load_active_vault = lambda: system.ActiveVault(
+            1,
+            [
+                {
+                    "name": "npm-registry",
+                    "match": {
+                        "hosts": ["registry.npmjs.org"],
+                        "methods": ["GET"],
+                        "paths": ["/*"],
+                    },
+                    "headers": [{"name": "Authorization", "value": "Bearer npm-token"}],
+                }
+            ],
+            ["npm-token"],
+        )
+        return system
+
+    def test_scoped_package_path_receives_credential(self) -> None:
+        """``/@scope%2fname`` is the npm-mandated wire format for scoped
+        packages. It must reach the upstream with credentials attached."""
+        system = self._make_system_with_npm_vault()
+        flow = _Flow()
+        flow.request.pretty_host = "registry.npmjs.org"
+        flow.request.host = "registry.npmjs.org"
+        flow.request.path = "/@ali%2forion-claude-plugin"
+
+        system.request(flow)
+
+        self.assertEqual("Bearer npm-token", flow.request.headers.get("Authorization"))
+        self.assertIsNone(getattr(flow.response, "status_code", None))
+
+    def test_scoped_package_uppercase_encoding_receives_credential(self) -> None:
+        """Uppercase ``%2F`` variant used by some clients is equally valid."""
+        system = self._make_system_with_npm_vault()
+        flow = _Flow()
+        flow.request.pretty_host = "registry.npmjs.org"
+        flow.request.host = "registry.npmjs.org"
+        flow.request.path = "/@ali%2Forion-claude-plugin"
+
+        system.request(flow)
+
+        self.assertEqual("Bearer npm-token", flow.request.headers.get("Authorization"))
+
+    def test_scoped_package_encoded_backslash_still_rejected(self) -> None:
+        """``%5c`` (encoded backslash) has no legitimate use even in scoped
+        registry paths and must still be rejected."""
+        system = self._make_system_with_npm_vault()
+        flow = _Flow()
+        flow.request.pretty_host = "registry.npmjs.org"
+        flow.request.host = "registry.npmjs.org"
+        flow.request.path = "/@ali%5corion-claude-plugin"
+
+        system.request(flow)
+
+        self.assertIsNotNone(flow.response)
+        self.assertEqual(403, flow.response.status_code)
+        self.assertNotIn("Authorization", flow.request.headers._values)
+
+    def test_scoped_package_double_encoded_slash_still_rejected(self) -> None:
+        """A double-encoded ``%252f`` has no legitimate use and is rejected
+        even under the relaxed single-layer ``%2f`` policy."""
+        system = self._make_system_with_npm_vault()
+        flow = _Flow()
+        flow.request.pretty_host = "registry.npmjs.org"
+        flow.request.host = "registry.npmjs.org"
+        flow.request.path = "/@ali%252forion-claude-plugin"
+
+        system.request(flow)
+
+        self.assertIsNotNone(flow.response)
+        self.assertEqual(403, flow.response.status_code)
+        self.assertNotIn("Authorization", flow.request.headers._values)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/python/tests/support/credential_vault_echo_server.py

@@ -45,6 +45,24 @@
     },
 }
 
+# npm-style scoped package registry paths are sent on the wire with the
+# ``/`` between scope and package name percent-encoded (``/@scope%2fname``).
+# The credential proxy used to reject these as ambiguous, which broke
+# ``npm install`` for scoped packages inside sandboxes. This route echoes
+# whichever ``Authorization`` header the upstream actually received so the
+# same route can back two E2E cases: one where a binding matches and the
+# credential is injected, and one where no binding matches so the request
+# passes through unchanged. Both slash encodings and the canonical form are
+# accepted because mitmproxy is free to canonicalize the path when
+# forwarding.
+NPM_SCOPED_PATHS: frozenset[str] = frozenset(
+    {
+        "/npm-scoped/@ali%2forion-claude-plugin",
+        "/npm-scoped/@ali%2Forion-claude-plugin",
+        "/npm-scoped/@ali/orion-claude-plugin",
+    }
+)
+
 
 class CredentialVaultEchoHandler(BaseHTTPRequestHandler):
     server_version = "OpenSandboxCredentialVaultE2E/1.0"
@@ -70,6 +88,19 @@ def do_GET(self) -> None:  # noqa: N802 - BaseHTTPRequestHandler API
             )
             return
 
+        if route in NPM_SCOPED_PATHS:
+            received = {name.lower(): value for name, value in self.headers.items()}
+            self._write_json(
+                HTTPStatus.OK,
+                {
+                    "ok": True,
+                    "case": "npm-scoped",
+                    "receivedPath": route,
+                    "authorization": received.get("authorization"),
+                },
+            )
+            return
+
         if route == "/tenant/vault-path-secret/resource":
             query = parse_qs(parsed.query, keep_blank_values=True)
             ok = query.get("tenant") == ["__path_secret__"]