Repository navigation
port(upstream#1891): opt-in OpenTopoMap and USGS map layers - #31
Merged
Merged
Conversation
This adds two optional map tile providers: - OpenTopoMap - USGS They are disabled by default, but can be quite useful for visualizing repeater sites with terrain features visible. (cherry picked from commit 34b41fd) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings the branch up to master 7813755 so CI runs on current master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pa-clawbot#1891 The port added three third-party tile layers with no test coverage. The default-off gating is the security-relevant part — these layers send the user's IP to opentopomap.org and basemap.nationalmap.gov — so it is now pinned in the CI-registered suite (test-issue-1420-tile-providers.js): - absent with no MC_MAP_CFG, and absent under the shipped config.example.json (proving what we ship stays opt-in) - each provider appears only under its own enabled=true, and enabling one does not pull in the other - OpenTopoMap keeps its CC-BY-SA credit and USGS its agency credit; both keep the zoom caps their services actually enforce - URL templates pin OpenTopoMap's {z}/{x}/{y} against ArcGIS's {z}/{y}/{x}, require TLS, and assert no credentials are appended All four mutations fail: removing the USGS gate (4 failures), dropping the CC-BY-SA credit (1), swapping the USGS coordinate order (1), and raising maxZoom past the service limit (1). Verified against the live services rather than by reading: OpenTopoMap returns 200 image/png worldwide; both USGS endpoints return 200 for US tiles (San Francisco, Denver) and 404 outside the US. That US-only coverage was undocumented, so config.example.json now says so — an operator enabling USGS from Europe would otherwise get a blank map with no explanation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-party exposure Independent review confirmed the port is privacy-clean — including the case I was most concerned about: after an operator disables a provider, a stale localStorage id does NOT leak requests, because _hasId tests the rebuilt REGISTRY. Verified end to end in a browser: 0 requests to the disabled host. Secrets clean (both URLs are constant literals, no config interpolation), and Leaflet disables the radios for capped layers rather than requesting tiles that do not exist. Two licence findings addressed here: - The OpenTopoMap attribution was the canonical leaflet-providers string with every hyperlink stripped. CC BY-SA 3.0 4(a) asks for the licence URI where the medium makes it reasonable, and Leaflet renders HTML in , so the credit and the licence are now links. The test pins both URIs, not just the words. - Neither config comment said that enabling a layer makes USERS' BROWSERS contact a third party, exposing their IP and approximate viewing area. The existing osm/stamen warnings only cover the operator's own token leaking. Both comments now say it, and the OpenTopoMap one states the CC-BY-SA duty and that it is a small volunteer-run service. Also fixed the cwd-relative config read in my previous test commit to use path.join(__dirname, …) like the rest of that file. NOT fixed here, filed as follow-up: map.js:342 and live.js:1454 mutate tileLayer.options.attribution after the layer was added, which Leaflet ignores, so a live provider swap or a theme toggle leaves the previous provider's credit on screen — OpenTopoMap tiles under a CartoDB credit with no CC-BY-SA notice. It is pre-existing (osm-standard already trips it) and in files this port does not touch, and the layers ship default-off so nothing is live, but it must be fixed before anyone enables OpenTopoMap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #25 (commit
34173fb3there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
34b41fd5b6df5b685e69482d9b74ff00528641dbgit cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.public/map-tile-providers.js,config.example.json.Problem
There was no terrain basemap for looking at repeater sites.
Change
Adds three opt-in base layers: OpenTopoMap, USGS Topographic and USGS Imagery. They are registered only when
map.tiles.providers.opentopomap.enabled/map.tiles.providers.usgs.enabledis true; both arefalseinconfig.example.json. With default config nothing changes.Adaptation to this fork
None in content. The changed lines are identical to upstream; only the surrounding diff context differs because this fork's files have diverged around them.
Notes for review
Enabling them makes browsers fetch tiles from tile.opentopomap.org / basemap.nationalmap.gov. Touches the same files as open #13 (CARTO key), but a merge simulation of the two is conflict-free.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5xss-gate-diffchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-linttest-issue-1375-scope-stats-fetch.jsExactly oneapi('/scope-stats'call exists (the fixed loader) — found 2test-issue-1648-m4-emoji-scan.jsmap.js has 1 emoji/misc-icon hit(s):test-a11y-axe-routes-coverage.jsaxe ROUTES missing analytics tabs (issue #1706): areas, foreign-traffic, wardrivingtest-frontend-helpers.jsfavStar returns empty star for non-favorite: The expression evaluated to a falsy value:,favStar returns filled star for favorite: The expression evaluated to a falsy value:Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Local Go server (built from master) on the committed E2E fixture DB (freshened, migrated and seeded exactly like CI), serving this branch's
public/, compared side by side with the same server serving master'spublic/. Both servers started with a config enablingmap.tiles.providers.opentopomapand.usgs. master with that config:window.MC_TILE_PROVIDERSholds only carto-* / positron-dark / esri-darkgray-labels. this branch: registry additionally holdsopentopomap(label OpenTopoMap, maxZoom 17),usgs-topoandusgs-imagery; switching the light basemap toopentopomaploaded tiles from a/b/c.tile.opentopomap.org and the map rendered terrain (screenshot). With the default config (servers without that block) the registry is unchanged, as on master.Not run:
eslint(not installed locally; CI installs it on the fly).-race/tests for modules this PR does not touch (unchanged code, identical to master).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34749785664 on
e023c11f. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code