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

Use $.ajax instead of iframe to make remote control requests #3760

Closed
wants to merge 1 commit into from

Conversation

AntonKhorev
Copy link
Collaborator

iframe with src set to remote control request urls first appeared in c3453cf and was updated with timeout alert in d413539.

jQuery's $.ajax can make the same requests and has builtin timeout support.

@tomhughes
Copy link
Member

I can't remember the details but there were very good reasons it was done with an iframe... Maybe things have moved on and it's no longer necessary but it would be good to know what the original reasoning was if anybody can remember.

@AntonKhorev
Copy link
Collaborator Author

The most likely reason is this one described here:
"For older browsers not implementing ​Cross-Origin Resource Sharing (CORS)...". Basically for compatibility with really old browsers that weren't as old in 2010 when this code was written.

Changing src on elements is a way of making a GET request without caring whether you have the permissions to read its response when you don't care about the response. JOSM just sends back "OK" on success.

@tomhughes
Copy link
Member

Right, but even with CORS the editor will need to send back an Access-Control-Allow-Origin header for AJAX to work right? Do all the editors do that?

@tomhughes
Copy link
Member

Maybe that's not required for localhost in modern browsers though? But how modern?

@AntonKhorev
Copy link
Collaborator Author

You're doing cross-domain requests with $.ajax() since at least 2014 for overpass searches.

@tomhughes
Copy link
Member

Yes but that is making a request to an overpass server that we know sends an appropriate Access-Control-Allow-Origin header - the remote editing API is sending requests to whatever random editor the user happens to be running.

@AntonKhorev
Copy link
Collaborator Author

  • JOSM sends Access-Control-Allow-Origin
  • Merkaartor also sends it.
  • Potlatch doesn't

@AntonKhorev
Copy link
Collaborator Author

Without Access-Control-Allow-Origin the rc command will still work but the browser will think that it failed.

@AntonKhorev
Copy link
Collaborator Author

In modern browsers it's possible to do no-cors fetch instead of adding hidden elements and altering their src:

fetch(url, { mode: "no-cors" })

@tomhughes
Copy link
Member

I haven't had much luck figuring out how new exactly a browser has to be to support that but it's been in the specification (under that name) since 2014 so I'm guessing it should be reasonably well supported by now.

@AntonKhorev
Copy link
Collaborator Author

I'm not completely sure about Safari:
https://caniuse.com/?search=Sec-Fetch-Mode

@tomhughes
Copy link
Member

You don't actually need that for XHR to support it though, as the XHR support is client side - whether it chooses to abort or return the limited access response when there is no CORS response header.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 4, 2022

I believe using $.ajax should be fine here. I don't see much reason to spend time on the Potlatch 3 topic, though. These users are probably used to the error popup. When switching to $.ajax, nothing will change for them.

Maybe you could open an issue on https://github.com/systemed/potlatch3 and discuss adding an "Access-Control-Allow-Origin" header somewhere around here: https://github.com/systemed/potlatch3/blob/master/com/airhttp/ActionController.as#L149-L174

@AntonKhorev
Copy link
Collaborator Author

These users are probably used to the error popup.

What is the error popup they are used to? Without this PR there's no CORS error.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

I tested Potlatch 3 remote control on osm.org, which shows the error popup. But again, let’s not spend much time on Potlatch.

@AntonKhorev
Copy link
Collaborator Author

I tested Potlatch 3 remote control on osm.org, which shows the error popup.

When launching it from a note page or from any page?

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

I didn't use the note page. Just plain remote edit.

@AntonKhorev
Copy link
Collaborator Author

I don't get any error popups.

You may get one if the request takes more than 5 seconds. Actually I increased it from one second when I added the second request to note pages. With just one second I was sometimes getting the popup even when using josm.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

I think potlatch 3 immediately loaded the respective area, I don't see where a delay should come from. It's not like JOSM where you might have to accept the remote call depending on configuration.

@AntonKhorev
Copy link
Collaborator Author

Are you talking about this popup in the browser window: "Editing failed - make sure JOSM or Merkaartor is loaded and the remote control option is enabled"?

Do you have the "OK" response from "load_and_zoom" request when the popup appears?

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

Yes, exactly. I don't know what Potlatch' response looked like, but the remote control did work.

@AntonKhorev
Copy link
Collaborator Author

Can you check the response in the Developer Tools (F12, Network tab)? If the response appears there, how long does it takes? (this should be visible in the timeline column)

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

I tried it again today, still on Ubuntu 22.04, but I cannot reproduce the error popup at this time. 🤷

@tomhughes
Copy link
Member

To be clear are you talking about testing Potlatch 3 with the current site? or with this PR merged?

Before I merge this is there any good reason not to add the mode: "no-cors" change? What would happen with that if somebody was somehow using a browser that didn't understand it? Would it fail or would it just behave as it would without that and do a CORS fetch?

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

So with this pull request in place, I'm getting:

grafik

I also tested the current site yesterday, and thought I saw the same popup. Upon retesting this today, I don't see the timeout popup anymore. Most likely, I mixed up the master branch and this pull request in my local install at one point.

If needed I can also test mode: "no-cors" with a code snippet that includes error handling and is ready for merging.

@tomhughes
Copy link
Member

Yes please do try it after changing this:

$.ajax(url, { timeout: 5000 })

to this:

$.ajax(url, { timeout: 5000, mode: "no-cors" })

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 6, 2022

Unfortunately, that didn't work, I'm getting the same "Editing failed" popup as above.

By the way, you can also simulate what Potlatch is sending by running:

echo -e "HTTP/1.1 200 OK\nContent-Type: text/plain\n\nOK" | netcat -q 1 -l -p 8111

(includes a delay of 1s)

Based on:

curl -v -g "http://127.0.0.1:8111/load_and_zoom?left=10.226884024047852&top=52.73576802171561&right=10.39694295654297&bottom=52.69522073112219"
*   Trying 127.0.0.1:8111...
* Connected to 127.0.0.1 (127.0.0.1) port 8111 (#0)
> GET /load_and_zoom?left=10.226884024047852&top=52.73576802171561&right=10.39694295654297&bottom=52.69522073112219 HTTP/1.1
> Host: 127.0.0.1:8111
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain
* no chunk, no close, no size. Assume close to signal end
< 
* Closing connection 0
OK

@AntonKhorev
Copy link
Collaborator Author

$.ajax(url, { timeout: 5000, mode: "no-cors" })

Is mode a valid option of $.ajax?

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

The reason this is erroring is not because of CORS it's because the CSP is not allowing the XHR fetches - the rules at https://github.com/openstreetmap/openstreetmap-website/blob/master/app/controllers/application_controller.rb#L289 need to be changed to move the 127.0.0.1 rules from child_src and frame_src to connect_src.

@systemed
Copy link
Contributor

Maybe you could open an issue on https://github.com/systemed/potlatch3 and discuss adding an "Access-Control-Allow-Origin" header somewhere around here: https://github.com/systemed/potlatch3/blob/master/com/airhttp/ActionController.as#L149-L174

Yes, I think that'd work. I'll probably do a new release of P3 in the New Year so it'd be good to have a steer before then if this is going to happen.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 18, 2022

The reason this is erroring is not because of CORS it's because the CSP is not allowing the XHR fetches

I tried this today without much luck. It seems we would still need the Access-Control-Allow-Origin header to get rid of the error message.

@simonpoole
Copy link
Contributor

Does anybody happen to have an instance with these changes running somewhere? Maybe the change would make #1478 superfluous as the main reason for the complications was the iframe use, so it would be nice to test Vespucci against it.

@HolgerJeromin
Copy link
Contributor

My personal feeling would to avoid jQuery for this.
jQuery is a wrapper which does funny stuff sometimes.

Do the same as jQuery does which gives more control (and is more stable with jQuery updates).

@tomhughes
Copy link
Member

Well my personal preference would be to use it because it's what all the rest of our code does so unless you have an actual concrete reason not to use it rather than vague accusations then I think I win.

@HolgerJeromin
Copy link
Contributor

Funny stuff example:
jquery/jquery#4145
But you are right. My single-page application has different problems with it than this website (for which jQuery was designed for).

@AntonKhorev
Copy link
Collaborator Author

Funny stuff example: jquery/jquery#4145

What does it have to do with $.ajax?

@AntonKhorev
Copy link
Collaborator Author

Moved "http://127.0.0.1:8111" to connect_src which should fix CSP errors.

Did nothing about CORS because looks like jQuery doesn't support no-cors mode. But that shouldn't stop the request from working.

@AntonKhorev AntonKhorev requested a review from tomhughes December 4, 2024 09:09
@AntonKhorev AntonKhorev added the javascript Pull requests that update Javascript code label Dec 4, 2024
@tomhughes
Copy link
Member

Superseded by #5375.

@tomhughes tomhughes closed this Dec 8, 2024
@AntonKhorev AntonKhorev deleted the rc-ajax branch December 9, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants