Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ geolibre = ["dep:rustgis-geolibre"]
rusqlite = { workspace = true }
rustgis-plugins = { workspace = true, features = ["testing"] }
tiny-skia = { workspace = true }
# Parsing the addresses commands ask for in tests.
url = { workspace = true }

# Time slider animations as GIFs (exported by the desktop app only).
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
Expand Down
4 changes: 3 additions & 1 deletion crates/engine/src/cmd/clipboard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,5 +174,7 @@ fn copy_path(s: &mut Session, p: &Value) -> CmdResult {
let l = s.map().layer(id).ok_or("no such layer")?;
let path = l.raster.as_ref().and_then(|r| r.path.clone()).or_else(|| l.dataset.source.clone());
let path = path.ok_or_else(|| format!("{} isn't stored in a file", l.name))?;
Ok(json!({"text": path}))
// A service layer's address is its source: copied without keys, tokens or its password, as
// the details pane shows it and a saved project keeps a web address.
Ok(json!({"text": crate::sources::redact(&path)}))
Comment on lines +177 to +179

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AGENTS.md, never silent (confidence ~45%). When the address has keys, tokens or a password, Copy Path returns the keyless form with no notice. The user pastes it, gets a 401 or a failed request, and nothing says why. AGENTS asks that a command which drops data say what and where, through Session::notify, one notice per kind.

The regression test a_service_layers_address_is_copied_without_its_key checks only the redacted text. AGENTS also asks for both sides: lossy input gives the notice, clean input gives none. A possible change:

Suggested change
// A service layer's address is its source: copied without keys, tokens or its password, as
// the details pane shows it and a saved project keeps a web address.
Ok(json!({"text": crate::sources::redact(&path)}))
// A service layer's address is its source: copied without keys, tokens or its password, as
// the details pane shows it and a saved project keeps a web address.
let text = crate::sources::redact(&path);
if text != path {
s.notify("the copied address leaves out its keys, tokens or password".into(), false);
}
Ok(json!({"text": text}))

The test would then assert one notice for api_key=SECRET and none for a clean path such as https://x.org/data.geojson.

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.

Copy Path deliberately returns a shareable source address without credentials, matching saved-project and Details-pane redaction. The user guide explicitly documents that contract, and a_service_layers_address_is_copied_without_its_key verifies the returned address. The command preserves the layer and its data. A per-copy reminder that credentials were omitted is a nonblocking UX improvement, left for the release quality pass; there is no additional notice contract for the existing regression to assert yet.

}
12 changes: 10 additions & 2 deletions crates/engine/src/cmd/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub fn commands() -> Vec<C> {
r#"{"url": "https://…/data.geojson|.csv|.parquet|.fgb|.gpkg|.zip|.kml|.kmz|.gpx|.tif|.jp2 (JPEG 2000: desktop, through GDAL) (the format from the address, else the contents) | a .pmtiles archive", "name"?: text, "layer"?: "one layer of a zip, KML or GPX", "zoom"?: "bool: true zooms to what was added; false keeps the view of a GeoParquet or FlatGeobuf read in place (zoomed to only when the map was empty)", "x"?, "y"?, "wkt"?, "crs"?, "delimiter"?, "paperSpace"?, "stream"?, "as"? (as layer.addData), "whole"?: "bool: download a cloud-optimised GeoTIFF whole (as a raster layer) instead of reading it in place (an address without a file name is read in place when its first bytes are a cloud-optimised GeoTIFF)", "cog"?: "bool: the address is a cloud-optimised GeoTIFF whatever its name says (the app's probe found it so)", "start"?, "end"?, "unit"?, "interval"?, "dates"?: "a GeoTIFF address with date placeholders ({date:YYYY}, {YYYY}{MM}{DD}), one file per date: the dates it spans and steps through (as time.set), turning the time slider on"}"#,
),
C::new("layer.addDownload", "Add Downloaded Data", "Home › Add", add_download)
.params(r#"{"id": a download the app finished (Home › Add › Add Data › From URL), "url": its address, "name"?: text, "layer"?: text, "zoom"?: bool, "x"?, "y"?, "wkt"?, "crs"?, "delimiter"?, "paperSpace"?, "stream"?, "as"? (as layer.addData), "service"?: bool (a service's features: their axes and paging are checked), "bounds"?: [west, south, east, north] (with service: the layer's longitude/latitude bounds from its capabilities, which its axes are checked against; without bounds they are left as they come; ignored without service), "coverage"?: bool (a WCS coverage: a GeoTIFF, else the service's error), "geolibreProject"?: bool (internal: downloaded project JSON), "geolibreLayer"?: {"token", "record"} (internal: reserved imported layer and its saved style), "geolibreZarr"?: bool (internal: an imported Zarr slice as GeoTIFF)}"#),
.params(r#"{"id": a download the app finished (Home › Add › Add Data › From URL), "url": its address, "name"?: text, "layer"?: text, "zoom"?: bool, "x"?, "y"?, "wkt"?, "crs"?, "delimiter"?, "paperSpace"?, "stream"?, "as"? (as layer.addData), "service"?: bool (a service's features: their axes and paging are checked, and the address is kept as the layer's source), "bounds"?: [west, south, east, north] (with service: the layer's longitude/latitude bounds from its capabilities, which its axes are checked against; without bounds they are left as they come; ignored without service), "coverage"?: bool (a WCS coverage: a GeoTIFF, else the service's error, and its request kept as the layer's source), "geolibreProject"?: bool (internal: downloaded project JSON), "geolibreLayer"?: {"token", "record"} (internal: reserved imported layer and its saved style), "geolibreZarr"?: bool (internal: an imported Zarr slice as GeoTIFF)}"#),
C::new("layer.addEncodedPolyline", "Add Encoded Polyline", "Home › Add", add_encoded_polyline).params(
r#"{"polyline": "encoded polyline text" | [texts] (one line each), "precision"?: 5 (Google) | 6 (OSRM, Valhalla), "name"?: text}"#,
),
Expand Down Expand Up @@ -2895,6 +2895,10 @@ fn add_download(s: &mut Session, p: &Value) -> CmdResult {
let url = ps(p, "url").unwrap_or("data").to_string();
// Read before the download is taken, so a bad one leaves it to be added again.
let service = super::b(p, "service").unwrap_or(false);
let coverage = super::b(p, "coverage").unwrap_or(false);
if (service || coverage) && ps(p, "url").is_none_or(|url| url.trim().is_empty()) {
return Err("url is required for a service or coverage download".into());
}
let bounds = if service { super::service::bounds_of(p.get("bounds"))? } else { None };
let i = s.downloaded.iter().position(|(d, _)| *d == id).ok_or("no such download (it was dropped when another project opened)")?;
let (_, bytes) = s.downloaded.remove(i);
Expand All @@ -2918,13 +2922,17 @@ fn add_download(s: &mut Session, p: &Value) -> CmdResult {
bytes
};
// A WCS coverage (Add From Service): a GeoTIFF, or the service's error said as such.
let name = if super::b(p, "coverage").unwrap_or(false) {
let name = if coverage {
super::service::coverage_check(ps(p, "name").unwrap_or(&url), &bytes)?;
"coverage.tif".to_string()
} else {
url_file_name(&url, &bytes).map_err(|e| format!("{url}: {e}"))?
};
let mut r = super::raster::add_named_bytes(s, p, name, bytes)?;
// A service's features or coverage: known by the address it came from (Add From Service).
if service || coverage {
super::service::mark_service_source(s, &r, &url);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
// `"zoom": true` zooms to everything added, whatever the map held (a link's data); `zoomed`
// says whether there was anything to zoom to (a tile archive adds no layer).
if b(p, "zoom") == Some(true) {
Expand Down
1 change: 1 addition & 0 deletions crates/engine/src/cmd/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub mod s3;
pub mod search;
pub mod select;
pub mod service;
pub mod service_added;
pub mod share;
pub mod sharing;
pub mod sources;
Expand Down
28 changes: 26 additions & 2 deletions crates/engine/src/cmd/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,26 @@ pub fn features_answer(s: &mut Session, name: &str, bytes: Vec<u8>, bounds: Opti
rustgis_data::ogc::fix_axes(&bytes, bounds).unwrap_or(bytes)
}

/// The layers an add's result `r` names (`layer`, `layers`) marked as read from the service
/// address `url`, not the file name their download was read under: Add From Service then knows
/// them as added, to remove.
pub(crate) fn mark_service_source(s: &mut Session, r: &Value, url: &str) {
let ids: Vec<u32> = r
.get("layers")
.and_then(Value::as_array)
.into_iter()
.flatten()
.chain(r.get("layer"))
.filter_map(Value::as_u64)
.filter_map(|i| u32::try_from(i).ok())
.collect();
for id in ids {
if let Some(l) = s.map_mut().layer_mut(id) {
l.dataset.source = Some(url.to_string());
Comment thread
mgaux-dev marked this conversation as resolved.
Resolved
}
}
}

/// A layer's longitude/latitude bounds as a listing or a parameter gives them (`[west, south,
/// east, north]`), for [`features_answer`]: none when absent or null.
pub(crate) fn bounds_of(v: Option<&Value>) -> Result<Option<[f64; 4]>, String> {
Expand Down Expand Up @@ -532,7 +552,9 @@ pub(crate) fn add(s: &mut Session, p: &Value) -> CmdResult {
{
let bytes = crate::render::rustgis_tiles::cache::download_blocking(&get).map_err(|e| format!("{name}: {e}"))?;
coverage_check(&name, &bytes)?;
super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.tif"), bytes)
let r = super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.tif"), bytes)?;
mark_service_source(s, &r, &get);
Ok(r)
}
}
Offer::Features { url: items } => {
Expand All @@ -553,9 +575,11 @@ pub(crate) fn add(s: &mut Session, p: &Value) -> CmdResult {
s.notify(format!("{name}: {why}, so the whole layer is asked for"), false);
}
let max = crate::render::rustgis_tiles::cache::MAX_DOWNLOAD;
let bytes = download_features(s, &name, asked.as_deref().unwrap_or(items), max).map_err(|e| slow_whole_layer(e, given))?;
let from = asked.as_deref().unwrap_or(items);
let bytes = download_features(s, &name, from, max).map_err(|e| slow_whole_layer(e, given))?;
let bytes = features_answer(s, &name, bytes, layer.bbox);
let mut r = super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.geojson"), bytes)?;
mark_service_source(s, &r, from);
// Over an area, the features crossing it: the view stays, or goes to a box given
// (the whole layer's bounds would show a country to fill one state).
match (asked.is_some(), area, p.get("bbox")) {
Expand Down
Loading
Loading