Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow the user to replace maps integration #5678

Merged
merged 6 commits into from
Nov 29, 2024

Conversation

r10s
Copy link
Member

@r10s r10s commented Jun 6, 2024

with this PR, when an .xdc with request_integration = map in the manifest is added to the "Saved Messages" chat, it is used locally as an replacement for the shipped maps.xdc (other devices will see the .xdc but not use it)

this allows easy development and adapting the map to use services that work better in some area.

there are lots of known discussions and ideas about adding more barriers of safety. however, after internal discussions, we decided to move forward and also to allow internet, if requested by an integration (as discussed at #3516).
the gist is to ease development and to make users who want to adapt, actionable now, without making things too hard and adding too high barriers or stressing our own resources/power too much.
note, that things are still experimental and will be the next time - without the corresponding switch being enabled, nothing will work at all, so we can be quite relaxed here :)

for android/ios, things will work directly. for desktop, allow_internet needs to be accepted unconditionally from core. for the future, we might add a question before using an integration and/or add signing. or sth. completely different - but for now, the thing is to get started.

nb: "integration" field in the webxdc-info is experimental as well and should not be used in UIs at all currently, it may vanish again and is there mainly for simplicity of the code; therefore, no need to document that.

successor of #5461

this is how it looks like currently - again, please note that all that is an experiment!

   

... when going out of experimental, there are loots of ideas, eg. changing "Start" to "integrate"

@r10s r10s changed the title allow the user to replace maps integration feat: allow the user to replace maps integration Jun 6, 2024
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from 4c4841e to 91dfe46 Compare June 6, 2024 15:46
@r10s r10s marked this pull request as draft June 6, 2024 16:19
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from b25e11f to 1ae8e9a Compare June 6, 2024 19:15
@r10s r10s marked this pull request as ready for review June 6, 2024 19:40
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from eaaeb05 to 802b821 Compare June 6, 2024 21:31
@r10s r10s requested review from link2xt, nicodh and adbenitez June 6, 2024 22:23
src/webxdc.rs Outdated Show resolved Hide resolved
src/webxdc/integration.rs Outdated Show resolved Hide resolved
src/webxdc/integration.rs Outdated Show resolved Hide resolved
src/webxdc/integration.rs Outdated Show resolved Hide resolved
@r10s
Copy link
Member Author

r10s commented Jun 7, 2024

i renamed the manifest entry to request_integration, i think, this matches better. currently integration is granted if locally added to "Saved Messages" - when following the path and going out of experimental, we may want to add an user option or so.

nb: i just realised that in a subsequent PR, we can restrict allow_internet to integrations only

@Simon-Laux
Copy link
Member

Simon-Laux commented Jun 7, 2024

for desktop, allow_internet needs to be accepted unconditionally from core

NO, just make it use the core http call that does not bypass socks5 proxy / tor.
Also the DNS Block is app wide on desktop and an important piece of making the app more secure, we can not modify it or disable it after the app is started AFAIK. we could just remove it entirely, but that would introduce a weakness to the entire app.

Adding user agent / header support to that method (https request over core) is not as hard as you seem to pretend.
There are also leaflet add ons for caching.

Also the better and MUCH easier solution to the "china blocks OSM" problem would be to add a menu to the map xdc that offers you 3 different endpoints to choose from and just add those domains to the DNS whitelist in desktop.

CC @missytake

@r10s
Copy link
Member Author

r10s commented Jun 7, 2024

NO, just make it use the core http call that does not bypass socks5 proxy / tor.

can one route the normal js http-get calls through core? that'd be great!

in general, we have allow_internet in "Saved Messages" working since 2years on android/ios, 1year on desktop (see deltachat/deltachat-desktop#3010, #3516) - seemed to work without weakening the app - what has changed since then?

as said above - we even aim to restrict allow_internet more, now that we have a maps integration and enable it only for that. (that would mitigate the theoretical attack of tricking a user to add an modified editor to "saved messages" where they should enter secrets)

note, this all is not about china, it is about china currently.

surely we will add an option to the map, there are already quite some suggestions for that.

but the gist is to enable js-developers and power-users to adapt, eg. to try out which things are working, making parts extensible. while dogfooding .xdc outselfes - also for us it's much easier to be able to test a map variation eg. on iOS without build things

@link2xt
Copy link
Collaborator

link2xt commented Jun 7, 2024

can one route the normal js http-get calls through core?

It is possible to use get_http_response JSON-RPC method, but this does not forward the headers such as User-Agent so we need to at least set meaningful User-Agent instead of the default library one. Default one is blocked by openstreetmap tile server.

@Simon-Laux
Copy link
Member

Simon-Laux commented Jun 7, 2024

seemed to work without weakening the app - what has changed since then?

DNS exfiltration(prefetch?) issue was found and fixed by restricting DNS, this broke the experimental allow_internet property. maybe there were also other reasons.

@r10s
Copy link
Member Author

r10s commented Jun 7, 2024

i see :/ how does html-message-view work with DNS restricted that way?

but anyways, this discussion can be continued when 1.46 is out. i do not wanted to shift things apart from that (but i also did not expect these issues)

@Simon-Laux
Copy link
Member

i see :/ how does html-message-view work with DNS restricted that way?

it uses the core http api

@r10s
Copy link
Member Author

r10s commented Jun 7, 2024

k, thanks a lot for insights. so, options so far:

  • if possible (still not clear to me) route <img src=...>, XMLHttpRequest etc over core's get_http_response().
    in case of leaflet, <img src=...> is sufficient - so maybe not so different from html-message-view
  • DNS-whitelist more tile servers that are useful and known (per-app does not make sense if it is a global option)

both options would be good enough for now (and would be needed also if we do not allow replacement of .xdc but adapt the shipped xdc and add a selector). again: when 1.46 is out :)

Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

Reviewed by reading
Successful tested with Desktop

@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from 68800d2 to 4ee66b4 Compare June 20, 2024 14:11
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch 2 times, most recently from 8a73194 to 1d4adf6 Compare July 10, 2024 09:43
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from 1d4adf6 to f39e739 Compare November 11, 2024 17:36
@Hocuri Hocuri force-pushed the r10s/replace-maps-integration2 branch from f39e739 to fbc9afa Compare November 25, 2024 16:08
@r10s r10s force-pushed the r10s/replace-maps-integration2 branch from fbc9afa to 04a47ef Compare November 29, 2024 14:01
@r10s
Copy link
Member Author

r10s commented Nov 29, 2024

to move forward, and as discussed with some ppl one-to-one, let's merge that in.

even if not fully working on desktop, it is - and was already by sending around apk with that feature in - useful for development.

once merged, we can also retire the generic "request_internet" flag, it is all experimental :)

@r10s r10s enabled auto-merge (squash) November 29, 2024 14:10
@r10s r10s merged commit d63a2b3 into main Nov 29, 2024
37 checks passed
@r10s r10s deleted the r10s/replace-maps-integration2 branch November 29, 2024 14:18
r10s added a commit that referenced this pull request Nov 29, 2024
…'s `manifest.toml`

this partly reverts experimental #3516
that allowed any .xdc sent to "Saved Messages" to request internet.
this helped on pushing map integration forward.

meanwhile, however, we have that map integration (#5461 and #5678),
that implies `info.internet_access` being set.
experimental `manifest.request_internet_access` is no longer needed therefore.

future will tell, if we revive the option at some point or
go for more intrations ('sending' is discussed often :) -
but currently it is not needed.
r10s added a commit that referenced this pull request Nov 29, 2024
…'s `manifest.toml`

this partly reverts experimental #3516
that allowed any .xdc sent to "Saved Messages" to request internet.
this helped on pushing map integration forward.

meanwhile, however, we have that map integration (#5461 and #5678),
that implies `info.internet_access` being set.
experimental `manifest.request_internet_access` is no longer needed therefore.

future will tell, if we revive the option at some point or
go for more intrations ('sending' is discussed often :) -
but currently it is not needed.
r10s added a commit that referenced this pull request Nov 29, 2024
…'s `manifest.toml`

this partly reverts experimental #3516
that allowed any .xdc sent to "Saved Messages" to request internet.
this helped on pushing map integration forward.

meanwhile, however, we have that map integration (#5461 and #5678),
that implies `info.internet_access` being set.
experimental `manifest.request_internet_access` is no longer needed therefore.

future will tell, if we revive the option at some point or
go for more intrations ('sending' is discussed often :) -
but currently it is not needed.
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.

5 participants