fix(install): warn when --allow-protocol-fallback reuses a custom port across schemes (#786)#789
Conversation
…t across schemes (microsoft#786) When cross-protocol fallback is enabled and a dependency carries a custom port, `_build_repo_url` passes the same `dep_ref.port` to both the SSH and HTTPS URL builders. On servers that serve each protocol on a different port (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990), the off-protocol URL is wrong. Strict-by-default (microsoft#778) already prevents this for the default path. The bug only surfaces when the user opts into fallback via `--allow-protocol-fallback` or `APM_ALLOW_PROTOCOL_FALLBACK=1`. Per the maintainer's Option D direction (warning + docs, no schema change), emit a default-visibility `[!]` warning before the first clone attempt when a custom port is set and the plan includes both SSH and HTTPS. The message tells the user to pin the protocol with an explicit `ssh://` or `https://` URL. The warning is routed through the existing `_rich_warning` idiom and deduped per (host, repo, port) on the downloader instance, so a single install run with multiple internal `_clone_with_fallback` calls (ref resolution + actual clone) still emits exactly one warning per dep. Docs callouts under the transport-selection and authentication guides cover the limitation and the pin-the-protocol workaround; the `--allow-protocol-fallback` help text picks up a one-line caveat. No schema changes (`DependencyReference` / `LockedDependency` untouched, no `ssh_port` / `https_port` / `fallback:` fields). The existing `test_https_attempt_preserves_same_port_across_protocols` invariant is unchanged — the warning is what makes that behaviour loud.
edenfunf
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
|
@danielmeppiel -- ready for review when you have a moment. This is the small PR you sketched on #786: the 30-50 line Option D change (warning + docs, no schema impact). It sits downstream of #778's strict-by-default plan without touching the One deviation worth flagging from your verdict text: the warning emission is deduped per Manual verification matrix is in the PR body. Happy to iterate on the wording or move the emission site to |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #786 by making cross-protocol fallback with custom ports safer and more discoverable: when --allow-protocol-fallback is enabled and a dependency has a custom port, APM now warns that the same port will be reused across SSH/HTTPS attempts (which can be wrong on servers with different per-protocol ports).
Changes:
- Emit a deduped
[!]warning in_clone_with_fallbackwhen fallback is permissive and both SSH+HTTPS attempts are planned for a dependency with a custom port. - Update
apm install --allow-protocol-fallbackhelp text with a custom-port caveat. - Add docs callouts + changelog entry and introduce a focused unit test suite for the warning behavior.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/deps/github_downloader.py |
Adds per-dependency dedup + emits the new custom-port cross-protocol fallback warning. |
src/apm_cli/commands/install.py |
Extends --allow-protocol-fallback help text with the port-reuse caveat. |
tests/unit/test_protocol_fallback_warning.py |
New unit tests validating warning emission, strict-mode silence, and dedup behavior. |
docs/src/content/docs/guides/dependencies.md |
Adds a caution block documenting the limitation and workaround. |
docs/src/content/docs/getting-started/authentication.md |
Adds a caution block documenting the limitation and workaround. |
CHANGELOG.md |
Adds an [Unreleased] / Changed entry describing the new warning + doc updates. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/apm_cli/deps/github_downloader.py:771
- This warning advises users to "pin the protocol with an explicit ssh:// or https:// URL", but when
--allow-protocol-fallbackis enabled the TransportSelector intentionally ignores explicit schemes and still plans cross-protocol attempts. Reword the warning to state that avoiding cross-protocol port reuse requires disabling protocol fallback (strict mode), and then using an explicit scheme/URL that matches the intended port.
_rich_warning(
f"Custom port {dep_port} is set on this dependency. "
f"Cross-protocol fallback will reuse the same port for the "
f"other scheme. If your server uses different ports per "
f"protocol (e.g., Bitbucket Datacenter: SSH 7999, HTTPS 7990), "
f"pin the protocol with an explicit ssh:// or https:// URL.",
symbol="warning",
)
- Files reviewed: 6/6 changed files
- Comments generated: 5
| # run calls _clone_with_fallback multiple times per dep (ref-resolution | ||
| # clone, then the actual dep clone). We want the warning exactly once | ||
| # per (host, repo, port) identity across all those calls. | ||
| self._fallback_port_warned: set = set() |
There was a problem hiding this comment.
self._fallback_port_warned is used as a set of (host, repo, port) tuples, but it is annotated as a plain set, which loses useful type information. Consider annotating it as a typed set (e.g., set[tuple[Optional[str], str, int]]) to improve readability and static checking.
This issue also appears on line 764 of the same file.
| self._fallback_port_warned: set = set() | |
| self._fallback_port_warned: set[tuple[Optional[str], str, int]] = set() |
| is_flag=True, | ||
| default=False, | ||
| help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1).", | ||
| help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1). Caveat: fallback reuses the same port across schemes, so pin the protocol with an explicit ssh:// or https:// URL on servers that use different SSH and HTTPS ports.", |
There was a problem hiding this comment.
This help text suggests that using an explicit ssh:// or https:// URL will "pin" the protocol even when --allow-protocol-fallback is enabled. In the current transport selection logic, enabling protocol fallback intentionally allows cross-protocol attempts even for explicit-scheme URLs. Please adjust the help text to clarify that pinning requires not using --allow-protocol-fallback (strict mode), and then using an explicit scheme.
| help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1). Caveat: fallback reuses the same port across schemes, so pin the protocol with an explicit ssh:// or https:// URL on servers that use different SSH and HTTPS ports.", | |
| help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1). Caveat: fallback reuses the same port across schemes, so on servers that use different SSH and HTTPS ports, pin the protocol by staying in strict mode (do not use --allow-protocol-fallback) and using an explicit ssh:// or https:// URL.", |
| Fallback reuses `dep_ref.port` for both schemes. On servers that use | ||
| different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, | ||
| HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` | ||
| warning before the first clone attempt when a custom port is set and | ||
| fallback is enabled. The fix is to pin the protocol with an explicit | ||
| `ssh://...` or `https://...` URL in `apm.yml`, which bypasses the | ||
| fallback chain entirely. |
There was a problem hiding this comment.
This caution claims that using an explicit ssh://... or https://... URL "bypasses the fallback chain entirely", but when --allow-protocol-fallback is enabled the transport selector still permits cross-protocol attempts even for explicit-scheme URLs. Please revise this section to reflect that explicit URLs bypass cross-protocol fallback only in strict mode (fallback disabled), and consider avoiding the internal name dep_ref.port in user docs (just describe it as the dependency's custom port).
| Fallback reuses `dep_ref.port` for both schemes. On servers that use | |
| different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, | |
| HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` | |
| warning before the first clone attempt when a custom port is set and | |
| fallback is enabled. The fix is to pin the protocol with an explicit | |
| `ssh://...` or `https://...` URL in `apm.yml`, which bypasses the | |
| fallback chain entirely. | |
| Fallback reuses the dependency's custom port for both schemes. On | |
| servers that use different ports per protocol (e.g. Bitbucket | |
| Datacenter: SSH 7999, HTTPS 7990), the off-protocol URL will be | |
| wrong. APM emits a `[!]` warning before the first clone attempt when | |
| a custom port is set and fallback is enabled. To avoid cross-protocol | |
| retries, pin the protocol with an explicit `ssh://...` or | |
| `https://...` URL in `apm.yml` and leave fallback disabled. If | |
| fallback is enabled, APM can still try the other protocol. |
| dependency's port on both SSH and HTTPS attempts. On servers that use | ||
| different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, | ||
| HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` | ||
| warning before the first clone attempt to flag this. Pin the protocol | ||
| with an explicit `ssh://...` or `https://...` URL in `apm.yml` to | ||
| bypass fallback entirely. |
There was a problem hiding this comment.
This caution suggests that pinning with an explicit ssh://... or https://... URL will bypass fallback, but with --allow-protocol-fallback enabled explicit schemes are not strict and cross-protocol attempts may still occur. Please update the wording to clarify that pinning requires strict mode (fallback disabled), and avoid referencing internal implementation names in docs (e.g., dep_ref.port).
| dependency's port on both SSH and HTTPS attempts. On servers that use | |
| different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, | |
| HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` | |
| warning before the first clone attempt to flag this. Pin the protocol | |
| with an explicit `ssh://...` or `https://...` URL in `apm.yml` to | |
| bypass fallback entirely. | |
| dependency URL's port on both SSH and HTTPS attempts. On servers that | |
| use different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, | |
| HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` | |
| warning before the first clone attempt to flag this. To strictly pin a | |
| dependency to `ssh://...` or `https://...`, leave fallback disabled. If | |
| `--allow-protocol-fallback` is enabled, APM may still try the other | |
| protocol even when the dependency URL uses an explicit scheme. |
|
|
||
| ### Changed | ||
|
|
||
| - `apm install --allow-protocol-fallback` now emits a `[!]` warning before the first clone attempt when a dependency has a custom port and both SSH and HTTPS attempts are planned, explaining that fallback reuses the same port across schemes and suggesting a pinned `ssh://` or `https://` URL for servers that use different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990). Docs note the limitation in the transport-selection and authentication guides. Closes #786 |
There was a problem hiding this comment.
The new changelog bullet does not follow the established entry format: changelog entries consistently end with the PR number in parentheses (e.g. "(#778)" above). Update this line to end with "(#786)" and avoid relying on "Closes #786" (issue reference) as the sole trailer.
| - `apm install --allow-protocol-fallback` now emits a `[!]` warning before the first clone attempt when a dependency has a custom port and both SSH and HTTPS attempts are planned, explaining that fallback reuses the same port across schemes and suggesting a pinned `ssh://` or `https://` URL for servers that use different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990). Docs note the limitation in the transport-selection and authentication guides. Closes #786 | |
| - `apm install --allow-protocol-fallback` now emits a `[!]` warning before the first clone attempt when a dependency has a custom port and both SSH and HTTPS attempts are planned, explaining that fallback reuses the same port across schemes and suggesting a pinned `ssh://` or `https://` URL for servers that use different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990). Docs note the limitation in the transport-selection and authentication guides. (#786) |
APM Review Panel — PR #789[>] Verdict: APPROVE-WITH-FIXES. Land after a single polish pass; no re-queue specialist review needed. Four specialists (python-architect, cli-logging-expert, auth-expert, devx-ux-expert) reviewed. Two approved cleanly; two flagged the same warning-text gap. Convergence is on a quick, mechanical polish pass. What's excellent
Findings
Merge callAuthor addresses (1) + (2) inline — both are mechanical, ~15 minutes. No re-review needed; maintainer can self-confirm against the warning shape above and the Growth angle
Reviewed via the |
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
[x] BLOCKER — warning text is unactionable. "this dependency" with N deps in flight = noise. Name the offender; repo_url_base and dep_host are already in scope. Target shape:
[!] Custom port 7999 on bitbucket.example.com/project/repo: SSH unreachable, falling back to HTTPS.
Pin the URL scheme, or drop --allow-protocol-fallback to fail fast.
See: <docs URL to the :::caution block>
This single edit absorbs the cli-logging blocker, the devx escape-hatch nit, and the devx doc-link nit in one line. Both :::caution blocks already exist as the link target.
[x] BLOCKER — cli-commands.md:101 stale. Per the cli.instructions contract, a CLI behavior change without the reference doc updated in the same PR is incomplete. The --allow-protocol-fallback description must mention the new pre-attempt port warning, not just the per-retry one. Non-negotiable; same-PR fix.
Polish pass from maintainer review of the cross-protocol-fallback port
warning. No functional surface change; message content, docs accuracy,
and CLI reference sync only.
Warning text (blocker 1 -- unactionable wording):
- Name the offender: "Custom port {port} on {host}/{repo}" instead of
"this dependency" (ambiguous under N-dep installs).
- Name the planned initial + fallback schemes so the user sees which
scheme will be tried first and which one the port will be reused on.
- Offer both remediations in one line: "Pin the URL scheme, or drop
--allow-protocol-fallback to fail fast."
- Link to the public docs anchor under the dependencies guide where
the corresponding :::caution block lives.
- Add a NOTE that the (host, repo, port) dedup key is case-sensitive;
follow-up tracks case-insensitive normalization.
Reference doc sync (blocker 2 -- cli-commands.md:101 stale):
- Extend the `--allow-protocol-fallback` entry in the CLI reference to
describe the new pre-attempt port warning, not only the existing
per-retry warning. Required by the cli.instructions contract for CLI
behaviour changes.
Docs accuracy (Copilot review findings):
- dependencies.md / authentication.md cautions previously claimed an
explicit `ssh://` or `https://` URL "bypasses the fallback chain
entirely". False under `--allow-protocol-fallback`: explicit URLs
still go through the permissive chain (transport_selection.py:255).
Both cautions now state the invariant correctly -- pinning only
hard-stops cross-protocol retries in strict mode.
- Drop the internal `dep_ref.port` identifier from user docs; refer to
"the dependency's custom port" / "the dependency URL's port".
- `apm install --allow-protocol-fallback` help text picks up the same
correction (pin AND omit the flag).
CHANGELOG formatting:
- End the entry with "Closes microsoft#786 (microsoft#789)" to match the convention used
by the surrounding BREAKING and Fixed entries ("Closes microsoft#328 (microsoft#778)",
"(microsoft#780)", "(microsoft#784)") instead of the bare "Closes microsoft#786" trailer.
Tests:
- Refresh assertions to cover the new shape: host named, repo named,
both schemes named, both remediations present, docs URL present.
- Drop assertions on the removed Bitbucket-Datacenter example string
and literal "ssh://"/"https://" substrings (no longer in the terse
three-line message).
- Existing dedup / strict-mode-silent / no-port-silent / distinct-dep
coverage untouched.
Full unit suite: 4170 passed. Manual `apm install` verified the four
scenarios (port + fallback, strict + port, fallback + no-port, --help).
|
@danielmeppiel Polish pass pushed (9a0b022). All comments addressed; CI 5/5 green on the new head. Maintainer blockers
Copilot findings
Follow-ups (to become issues linked from the merge commit, per your verdict)
Verification
Ready for your re-read. |
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
APM Review Panel -- PR #789 (re-review)
[+] APPROVE. All five blockers and copilot findings from my prior CHANGES_REQUESTED are addressed in 9a0b022. CI green on the new head.
Verified
- Warning text (B1): names dep + port + both planned schemes + both remediations + docs anchor. The tense shift ("if HTTPS fails, APM will retry over SSH") correctly reflects pre-attempt emission. Plan-derived scheme order means the user always sees what the selector will actually do.
cli-commands.md(B2): documents both warnings with consistent "omit + pin" framing.- Help text + caution blocks (#2, #3, #4): correctly state the invariant that scheme-pinning alone does not prevent retries under fallback. Internal
dep_ref.portremoved from user-facing copy. - CHANGELOG trailer (#5):
Closes #786 (#789)matches house convention. - Test refresh: assertions cover host+repo names, both schemes, both remediations, docs URL substring, dedup-across-calls, distinct-dep regression.
Accepted as-is
- set type annotation (#1): style argument is reasonable -- the surrounding instance vars are unannotated and the warn-key construction three lines below makes the tuple shape obvious. Not worth diverging the file's style for one line. Module-wide PEP 585 cleanup is a separate PR.
Follow-up issues to file post-merge (per your verdict)
- Case-insensitive host normalization in dedup key.
- Thread-safety of
_fallback_port_warnedif/when installs parallelize.
Nice work, @edenfunf -- this is exactly the surgical Option D shape #786 asked for. Strict-by-default keeps holding the line; the warning makes the one remaining sharp edge loud.
Re-reviewed via apm-review-panel skill: prior CHANGES_REQUESTED items verified individually against 9a0b022 diff.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… format
CodeQL flagged the bare "bitbucket.example.com" in msg substring check
as "Incomplete URL substring sanitization" -- a false positive here
(this is a test, not URL validation), but the fix strengthens the
assertion at the same time.
Replace `assert "{host}" in msg` + `assert "{repo}" in msg` with a
single anchored check against the exact emitted prefix:
assert "Custom port {port} on {host}/{repo}:" in msg
Same treatment for the docs URL: `"microsoft.github.io/apm/" in msg`
becomes `"See: https://microsoft.github.io/apm/" in msg`, anchoring on
the `See: ` emit prefix.
Two wins: the CodeQL finding goes away (no bare hostname substring
check), and the tests now document the exact wire format so a
regression that re-splits host and repo, or changes the "Custom port
... on ..." phrasing, will fail loudly instead of silently passing via
substring containment.
Head branch was pushed to by a user without write access
|
Follow-up push ( CodeQL flagged # before
assert "bitbucket.example.com" in msg
assert "project/repo" in msg
assert "microsoft.github.io/apm/" in msg
# after
assert "Custom port 7999 on bitbucket.example.com/project/repo:" in msg
assert "See: https://microsoft.github.io/apm/" in msgTwo benefits in one:
Same treatment for the HTTPS fixture ( Target run: |
|
@edenfunf |
Description
When
--allow-protocol-fallback(orAPM_ALLOW_PROTOCOL_FALLBACK=1) is enabled and a dependency carries a custom port,_build_repo_urlpasses the samedep_ref.portto bothbuild_ssh_urlandbuild_https_clone_url. On servers that serve each protocol on a different port (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990), the off-protocol URL is wrong.Strict-by-default (#778) already prevents this for the default path; the bug only surfaces when the user opts into cross-protocol fallback.
Per the maintainer's Option D direction on #786 (warning + docs, no schema change):
_clone_with_fallback: when the plan is permissive (fallback active) ANDdep_ref.port is not NoneAND the planned attempts include both SSH and HTTPS, emit a default-visibility[!]warning before the first clone attempt, directing the user to pin the protocol with an explicitssh://orhttps://URL. The warning routes through the existing_rich_warningidiom (same path as the current per-attempt fallback warning) and is deduped per(host, repo, port)on theGitHubPackageDownloaderinstance, so a single install run with multiple internal_clone_with_fallbackcalls (ref resolution + actual clone) still emits exactly one warning per dep.--allow-protocol-fallbackhelp text: one-line caveat that fallback reuses the same port across schemes.docs/.../guides/dependencies.md+getting-started/authentication.md: short:::cautioncallouts under the transport-selection / fallback sections documenting the limitation and the pin-the-protocol workaround.CHANGELOG.md: one entry under[Unreleased]/Changed.Explicit non-goals (per the review panel's rejected options): no
fallback:schema field, nossh_port/https_portfields, no schema migration, no multi-port cascade. The existingtest_https_attempt_preserves_same_port_across_protocolsinvariant attests/unit/test_auth_scoping.py:423is unchanged -- the warning is what makes that behaviour loud.Fixes #786
Type of change
Testing
Manual verification
End-to-end against a custom-port dep on an unreachable host:
apm install --allow-protocol-fallbackwithssh://git@host:7999/owner/repo.git->[!] Custom port 7999 ...fires exactly once, before any clone attempt (confirmed single emission even though ref-resolution and actual-clone phases each call_clone_with_fallback).--allow-protocol-fallback(strict mode) -> no warning.--allow-protocol-fallback-> no warning.apm install --helprenders the updated--allow-protocol-fallbackcaveat.Automated tests
New file
tests/unit/test_protocol_fallback_warning.py(7 tests):ssh://URL with port +allow_fallback, citing the port and the Bitbucket Datacenter example verbatim.https://URL with port +allow_fallback.#778invariant).dep_ref.port is None._clone_with_fallbackcall (not per planned attempt)._clone_with_fallbackcalls for the same dep identity (regression guard for the ref-resolution + actual-clone multi-call path).(host, repo, port)identity (dedup is per-dep, not global).Full unit suite: 4170 passed, 0 new failures.