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.

Sign up for GitHub

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

Jump to bottom

Use HTTPS if location protocol is HTTPS in iD.Connection #2681

Merged
merged 1 commit into from
Jul 17, 2015
Merged

Use HTTPS if location protocol is HTTPS in iD.Connection #2681

merged 1 commit into from
Jul 17, 2015

Conversation

frewsxcv
Copy link
Contributor

@frewsxcv frewsxcv commented Jun 7, 2015

Relevant to strava#5 (comment)

@pnorman
Copy link
Contributor

pnorman commented Jun 13, 2015

Looks like the tests need revising too.

@frewsxcv frewsxcv force-pushed the patch-1 branch 2 times, most recently from 5352ea0 to 8be0086 Compare June 13, 2015 16:56
@frewsxcv
Copy link
Contributor Author

I fixed all the tests except one. If anyone has ideas about what is going wrong there, let me know

@pnorman
Copy link
Contributor

pnorman commented Jun 14, 2015 

  iD.modes.AddPoint
    clicking the map
XMLHttpRequest cannot load file://www.openstreetmap.org/api/0.6/map?bbox=-77.02514648437499,38.89958342598271,-77.01965332031249,38.90385833966776. Cross origin requests are only supported for HTTP.
  1) "before each" hook

Ugh. This is a nasty problem.

You can't assume that the OSM API is accessible on the same protocol that iD is loaded by, because iD could be loaded locally over file://

@jfirebaugh
Copy link
Member

Chrome has some restrictions for resources loaded via file:// that generally make loading it that way unadvisable. That said, it's probably better to do something like location.protocol === 'http' ? 'http' : 'https'.

@frewsxcv
Copy link
Contributor Author

How does it look now? I added an optional parameter to the iD.Connection constructor mainly because I didn't want to figure out how to mock window.location in order to write a unit test

@frewsxcv frewsxcv changed the title Utilize protocol agnostic URLs in iD.Connection Use HTTPS if location protocol is HTTPS in iD.Connection Jun 27, 2015
@frewsxcv
Copy link
Contributor Author

frewsxcv commented Jul 1, 2015

Alternatively, we could just make all connections secure and disallow HTTP...

@frewsxcv
Copy link
Contributor Author

Anyone have thoughts on this?

@pnorman
Copy link
Contributor

pnorman commented Jul 13, 2015

Not all API instances allow HTTPS.

@frewsxcv
Copy link
Contributor Author

My current changes do not force HTTPS, unless the user is already on an HTTPS site

@pnorman
Copy link
Contributor

pnorman commented Jul 13, 2015

My current changes do not force HTTPS, unless the user is already on an HTTPS site

Yes, but you were suggesting disallowing HTTP

@frewsxcv
Copy link
Contributor Author

Yes, but you were suggesting disallowing HTTP

Yeah, I understand. Sorry I didn't make that clear in my previous message.

@bhousel
Copy link
Member

bhousel commented Jul 17, 2015

I notice that using the source switcher on an https site will break this, but it's broken anyway because api06.dev.openstreetmap.org doesn't support https, so I guess not that big a deal.

Thanks for adding this, @frewsxcv

bhousel added a commit that referenced this pull request Jul 17, 2015
@bhousel
Merge pull request #2681 from frewsxcv/patch-1
bc7b781 
Use HTTPS if location protocol is HTTPS in iD.Connection
@bhousel bhousel merged commit bc7b781 into openstreetmap:master Jul 17, 2015
1ec5
1ec5 reviewed Jul 18, 2015
View reviewed changes
js/id/core/connection.js
connection = {},
inflight = {},
loadedTiles = {},
tileZoom = 16,
oauth = osmAuth({
url: 'http://www.openstreetmap.org',
url: protocol + '//www.openstreetmap.org',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unnecessary: just start the URL with //, and the current protocol will be used (except in Netscape Navigator 4, I suppose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look through he discussion the associated pull request. Sometimes, we are retrieving this js file via the file:// protocol which results in an undesired outcome

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doh! Sorry about that. 😄

@frewsxcv frewsxcv mentioned this pull request Jul 18, 2015
Closed
@bhousel bhousel mentioned this pull request Feb 3, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frewsxcv @pnorman @jfirebaugh @bhousel @1ec5