Skip to content

Maps with own session#5455

Merged
nicodh merged 10 commits intomainfrom
maps-with-own-session
Nov 2, 2025
Merged

Maps with own session#5455
nicodh merged 10 commits intomainfrom
maps-with-own-session

Conversation

@nicodh
Copy link
Member

@nicodh nicodh commented Sep 10, 2025

#skip-changelog

It should be noted that the core request to openstreetmap.org fails.

image

Maybe since the core sends a UserAgent header that is blocked from openstreepap.org.

Technical Usage Requirements
Valid HTTP User-Agent identifying application. Faking another app’s User-Agent WILL get you blocked. Using a library’s default User-Agent is NOT recommended as they may be blocked if another user of the library is misusing it. If a device automatically sends an X-Requested-With header with an application specific Application ID, this will be considered an acceptable substitute for the HTTP User-Agent, although we still recommend setting a valid HTTP User-Agent for the application.
When coming from a web page, a valid HTTP Referer. Apps generally do not have a HTTP referer.
DO NOT send no-cache headers. (“Cache-Control: no-cache”, “Pragma: no-cache” etc.)
Cache Tile downloads locally according to HTTP Expiry Header, alternatively a minimum of 7 days.
Recommended: HTTP/2 or HTTP/3 support to allow multiplexed downloads

https://operations.osmfoundation.org/policies/tiles/

edit: chatmail/core#7302 would fix the problem of openstreetmap.org

@nicodh nicodh mentioned this pull request Sep 10, 2025
@nicodh nicodh force-pushed the maps-with-own-session branch from f7ded59 to 2985cd8 Compare September 12, 2025 06:49
@nicodh nicodh force-pushed the maps-with-own-session branch from 2985cd8 to f593922 Compare September 12, 2025 06:56
@nicodh nicodh changed the title Maps with own session (WIP) Maps with own session Sep 15, 2025
@nicodh nicodh force-pushed the maps-with-own-session branch from f593922 to 39eea04 Compare October 14, 2025 06:46
@nicodh nicodh added the new-core involves or requires an core update label Oct 16, 2025
@nicodh nicodh marked this pull request as ready for review October 16, 2025 14:39
return makeResponse(null, blob, {}, mimeType)
} catch (error) {
log.error('map: load blob:', error)
return makeResponse(null, '', { status: 404 })
Copy link
Member

Choose a reason for hiding this comment

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

nitpick/correctness: we don't know exact error code, could be temporary error, not sure if leaflet uses it / if it makes a difference in this case.
I believe we should make a new core method that also returns the error code, so that we can return the correct error code here.

@nicodh
Copy link
Member Author

nicodh commented Oct 24, 2025

Update: I reduced the sessions to just 2 types of sessions which also simplifies the cleanup process and added a reduced version of maps.xdc to avoid additional dependant decisions deltachat/maps#7

Copy link
Member

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

OK, so the main purpose of this MR is to allow a WebXDC app that claims to be a maps app to connect to arbitrary destinations, and not just openstreetmap.org, for the purpose of loading the tiles from other services, right?

However, this implementation requires that the app is going to use the Delta Chat Desktop -specific maps:// protocol for that.

Do we want to support third-party maps WebXDC apps? If yes, then this MR will break them, and require that they explicitly support Delta Chat Desktop.
If not, then I'd suggest to have a white-list of map service domains.

Has https://www.electronjs.org/docs/latest/api/web-request API been considered? i.e. intercept https requests.

@nicodh
Copy link
Member Author

nicodh commented Oct 31, 2025

Do we want to support third-party maps WebXDC apps? If yes, then this MR will break them, and require that they explicitly support Delta Chat Desktop. If not, then I'd suggest to have a white-list of map service domains.

The main idea is to have an easy way to extend the map sources (i.e. to extend the mapServices list in the version with add-ons) and yes these can be added to Saved messages and replace the intergrated maps.xdc

Has https://www.electronjs.org/docs/latest/api/web-request API been considered? i.e. intercept https requests.

We could got for that if we have a clear concept of use cases for this "integrated apps have Internet access"
I still don't feel comfortable with the fact that we

  1. claim that webxdc apps have no internet access
  2. allow to replace the integrated app with a provided webxdc app
  3. integrated apps have internet access

A more generic approach would be to ask the user once if the replaced & integrated app accesses an external URL and remember the answer

@WofWca
Copy link
Member

WofWca commented Oct 31, 2025

yes these can be added to Saved messages and replace the intergrated maps.xdc

OK, so are we willing to break existing maps apps with this? Looking at my comment above, #5455 (comment), it appears that this maps:// approach will break the feature for all existing users.

@nicodh
Copy link
Member Author

nicodh commented Oct 31, 2025

yes these can be added to Saved messages and replace the intergrated maps.xdc

OK, so are we willing to break existing maps apps with this? Looking at my comment above, #5455 (comment), it appears that this maps:// approach will break the feature for all existing users.

That's what "experimental" means. Breaking changes can occur.

@WofWca
Copy link
Member

WofWca commented Oct 31, 2025

So do we then wait for the respective maps MR (deltachat/maps#7) to get merged? Or for DC Android / iOS client devs to confirm that they're OK with this? Or does it all just not matter?

@r10s
Copy link
Member

r10s commented Oct 31, 2025

to sum up, what i got from one to one discussions with @nico, i think, it is fine.

at a glance, imu, it offers to maps: protocol to be used on desktop where http: protocol is not working for $reasons

  • in contrast to webxdc, the maps integration is heavily bounded to the main app and there may be changes at every time we change the sendUpdate() payloads. that one needs to you maps: does not add much on that

  • i suspect that there are many integrations used in the wild. i know only from experiments as that from china user - which was actually the reason for that change, and imu, all that is build in the main app now

  • if there are other, more advanced adaptions, they will break, that is true - but that is also easy to adapt. replacing integration was always for developers (as us) to be able to adapt and test things, it was not for the end user

  • in the past, the difference between android/ios and desktop was even worse as there was no way at all to use a different server than the default

  • from the view of android/ios it is fine as well as the maps.xdc will continue running there using http: - at some point, it is worth checking if routing things over maps: is possible on android/ios as well - that way, it would also get eg. proxy support

  • this PR does not offer more rights to the maps integrations as initially intended, thing is that this was not working on desktop before

  • it is experimental :)

Copy link
Member

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

Alright, this looks to be about finished up.

Finally the rendered processes doesn't need direct internet access anymore!

The irrelevant changes also appear OK.

As long as we are OK with breaking the "maps" feature for everyone who uses it currently with no obvious way to fix it, let's merge.

@nicodh nicodh merged commit dd42471 into main Nov 2, 2025
19 checks passed
@nicodh nicodh deleted the maps-with-own-session branch November 2, 2025 09:16
@nicodh nicodh removed the new-core involves or requires an core update label Nov 2, 2025
@WofWca WofWca mentioned this pull request Nov 14, 2025
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.

4 participants