fix(lsp): restrict registry completion endpoint schemes - #36477
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
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.
| Token::Key(k) => { | ||
| if let Some(prefix) = &k.prefix { | ||
| let maybe_url = registry.get_url_for_key(k); | ||
| let base = Url::parse(&origin).ok()?; |
There was a problem hiding this comment.
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.
| Ok(url) | ||
| } | ||
|
|
||
| fn validate_endpoint_scheme(url: &Url) -> Result<(), AnyError> { |
There was a problem hiding this comment.
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.
|
|
||
| fn validate_endpoint_scheme(url: &Url) -> Result<(), AnyError> { | ||
| match url.scheme() { | ||
| "http" | "https" => Ok(()), |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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:
- 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. - Minor behavior shift: hoisting
let base = Url::parse(&origin).ok()?;above theif let Some(url) = maybe_urlmeans 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.
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`
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:
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::' -- --nocapturecargo clippy -p deno --lib --no-deps -- -D warningsdeno run -A --no-config npm:dprint@0.47.2 check --config=.dprint.json cli/lsp/registries.rs