Skip to content

fix(lsp): restrict registry completion endpoint schemes - #36477

Merged
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/lsp-registry-endpoint-schemes
Aug 25, 2026
Merged

fix(lsp): restrict registry completion endpoint schemes#36477
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/lsp-registry-endpoint-schemes

Conversation

@nathanwhit

@nathanwhit nathanwhit commented Aug 7, 2026

Copy link
Copy Markdown
Member

Registry completion configuration describes network endpoints, but endpoint URL resolution previously accepted any URL scheme. That allowed completion and documentation requests to be routed through handlers that are not intended for registry services.

This change:

  • validates resolved registry endpoints as HTTP or HTTPS while retaining relative and cross-origin endpoints
  • resolves first-key relative endpoints against the current resolved module URL, matching the other registry completion paths
  • rechecks documentation URLs during completion resolution
  • adds coverage for supported URL forms and unsupported schemes

HTTP and HTTPS cross-origin endpoints remain supported by design for registry origins explicitly enabled by the user. This change prevents routing through unsupported URL-scheme handlers; it does not add a same-origin restriction.

Validation:

  • cargo test -p deno --lib 'lsp::registries::tests::' -- --nocapture
  • cargo clippy -p deno --lib --no-deps -- -D warnings
  • deno run -A --no-config npm:dprint@0.47.2 check --config=.dprint.json cli/lsp/registries.rs

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Real bug, correct fix. I traced all five paths that reach fetch_bypass_permissions with a config-derived URL and they're all covered now: get_variable_items and the hover path via get_endpoint_with_match β†’ parse_url_with_base, get_data/get_data_with_match via get_endpoint, get_items converted here, and get_documentation with the explicit recheck.

That last one deserves more credit than the description gives it. get_documentation is called from language_server.rs:3032 with data.url deserialized out of params.data on completionItem/resolve β€” the client hands that value back, so it isn't merely re-checking something already validated upstream. It's the one place the URL is genuinely untrusted at the point of use, and it would have been easy to skip on the assumption that the creation-time check covered it.

Three comments inline: one I'd fix before this lands, two follow-ups.

Comment thread cli/lsp/registries.rs Outdated
Token::Key(k) => {
if let Some(prefix) = &k.prefix {
let maybe_url = registry.get_url_for_key(k);
let base = Url::parse(&origin).ok()?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This resolves against origin, but every other endpoint resolution in this same function uses resolved: get_variable_items(..., &resolved, ...) on line 753, get_data_with_match(..., &resolved, ...) on 829, and get_data(registry, &resolved, k, &path) on 972 β€” the last one inside this very block. origin is base_url(&resolved), so scheme+host only.

Absolute and root-relative endpoints resolve identically either way, which is why the new tests don't catch it. They diverge for a path-relative endpoint like api/modules:

  • base https://deno.land/ β†’ https://deno.land/api/modules
  • base https://deno.land/std/fs/ β†’ https://deno.land/std/fs/api/modules

Since get_items previously used bare ModuleSpecifier::parse β€” relative endpoints just failed outright β€” this PR is what introduces relative support on this path. Worth introducing it with the same base as everywhere else rather than baking in a divergence that'll be confusing to track down later.

resolved is already in scope here, and switching to it lets this Url::parse(&origin).ok()? line go away entirely.

Comment thread cli/lsp/registries.rs
Ok(url)
}

fn validate_endpoint_scheme(url: &Url) -> Result<(), AnyError> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up suggestion: worth calling this from validate_config (line 311) as well.

Right now validate_config checks version, schema/variable consistency, and replacement-variable ordering β€” but not endpoint schemes. So a registry declaring file:// endpoints passes check_origin/enable cleanly and the user simply gets no completions, with at best an error! buried in the LSP log. Checking there would surface it the way the other misconfigurations already do ("Invalid registry configuration…").

This complements the runtime checks rather than replacing them β€” get_documentation still needs its own, since that URL arrives from the client rather than from the validated config.

Comment thread cli/lsp/registries.rs

fn validate_endpoint_scheme(url: &Url) -> Result<(), AnyError> {
match url.scheme() {
"http" | "https" => Ok(()),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not asking for a change here β€” retaining cross-origin endpoints is a legitimate feature and narrowing to http/https is the right call for this PR. But it's worth stating the residual explicitly somewhere (doc comment or the PR description) rather than leaving it implied by "retains cross-origin endpoints":

A malicious registry config can still point the LSP at http://localhost:8080/admin or http://169.254.169.254/latest/meta-data/, with the JSON response rendered into the editor as completion items or hover docs. The mitigating factor is real β€” the user has to have explicitly enabled that origin in suggest.imports.hosts β€” but the scheme restriction narrows this class rather than closing it, and someone reading this function later should be able to tell that was deliberate.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sensible restriction β€” registry import-intellisense endpoints are network endpoints by definition, and letting a config point one at file: or data: meant the LSP would happily fetch through a handler that was never meant to serve them. Keeping relative and cross-origin endpoints working while gating on scheme is the right line.

Routing get_items through parse_url_with_base rather than ModuleSpecifier::parse does two useful things at once: relative first-key endpoints now resolve against the origin like the other endpoints already did, and they pick up the same scheme check. The //cdn.example/modules case in the test is a nice one to have pinned.

Two notes:

  1. CI's fmt check is red on all three platforms (exit 20). The log points at the new error!("Internal error mapping endpoint \"{}\". {}", url, err); line. Needs ./tools/format.js.
  2. Minor behavior shift: hoisting let base = Url::parse(&origin).ok()?; above the if let Some(url) = maybe_url means an unparseable origin now short-circuits even when there's no URL to resolve. Origins reaching here are always parseable in practice, so this is fine β€” just flagging that it wasn't purely a move.

Approving on the fmt fix.

@nathanwhit
nathanwhit merged commit 3a37565 into denoland:main Aug 25, 2026
137 checks passed
@nathanwhit
nathanwhit deleted the fix/lsp-registry-endpoint-schemes branch August 25, 2026 00:56
bartlomieju pushed a commit that referenced this pull request Aug 27, 2026
Registry completion configuration describes network endpoints, but
endpoint URL resolution previously accepted any URL scheme. That allowed
completion and documentation requests to be routed through handlers that
are not intended for registry services.

This change:

- validates resolved registry endpoints as HTTP or HTTPS while retaining
relative and cross-origin endpoints
- resolves first-key relative endpoints against the current resolved
module URL, matching the other registry completion paths
- rechecks documentation URLs during completion resolution
- adds coverage for supported URL forms and unsupported schemes

HTTP and HTTPS cross-origin endpoints remain supported by design for
registry origins explicitly enabled by the user. This change prevents
routing through unsupported URL-scheme handlers; it does not add a
same-origin restriction.

Validation:

- `cargo test -p deno --lib 'lsp::registries::tests::' -- --nocapture`
- `cargo clippy -p deno --lib --no-deps -- -D warnings`
- `deno run -A --no-config npm:dprint@0.47.2 check --config=.dprint.json
cli/lsp/registries.rs`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants