Skip to content

fix popup auth broken due to new COOP header#138

Merged
bhousel merged 1 commit intoosmlab:mainfrom
k-yle:kh/auth-broken
Jul 11, 2025
Merged

fix popup auth broken due to new COOP header#138
bhousel merged 1 commit intoosmlab:mainfrom
k-yle:kh/auth-broken

Conversation

@k-yle
Copy link
Copy Markdown
Member

@k-yle k-yle commented Jul 8, 2025

Caused by openstreetmap/openstreetmap-website@2ff4d6 , Closes #137

window.opener can't be used anymore, so we have to use a BroadcastChannel instead. BroadcastChannels only support comms within the same origin, so this shouldn't introduce any new security risks in comparison to the existing solution, to the best of my knowledge.

This is a breaking change because users need to manually update their land.html file.

Unfortunately, this also reverts #135, which isn't possible anymore, since the popup window sets a restrictive COOP header

k-yle referenced this pull request in openstreetmap/openstreetmap-website Jul 8, 2025
Reported-by: Sam Jose <samjosep512@gmail.com>
@bhousel
Copy link
Copy Markdown
Member

bhousel commented Jul 8, 2025

Well this is the first I'm hearing about it.
I'll have to give a big change like this a proper review.

@bhousel bhousel marked this pull request as draft July 8, 2025 04:02
@bhousel bhousel self-assigned this Jul 8, 2025
@mattrobmattrob
Copy link
Copy Markdown

mattrobmattrob commented Jul 8, 2025

FWIW, confirmed this change works in an internal iD fork. Let me know if I can be of any help to you all otherwise.

Comment thread dist/osm-auth.cjs Outdated
Copy link
Copy Markdown
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

I’ve only used BroadcastChannel on occasion, but this looks about right to me.

Comment thread CHANGELOG.md Outdated
Comment thread src/osm-auth.mjs Outdated
@k-yle k-yle force-pushed the kh/auth-broken branch from 3424133 to 971de2b Compare July 9, 2025 00:33
@k-yle k-yle marked this pull request as ready for review July 9, 2025 00:34
k-yle added a commit to k-yle/iD that referenced this pull request Jul 9, 2025
k-yle added a commit to osm-nz/RapiD that referenced this pull request Jul 9, 2025
@HarelM
Copy link
Copy Markdown
Contributor

HarelM commented Jul 10, 2025

How do you know if the popup window was closed by the user after this change?
Here's a similar comment I added to the change made in OSM:
openstreetmap/openstreetmap-website@2ff4d6a#commitcomment-161835429

Any insight would be helpful. Thanks!

@k-yle
Copy link
Copy Markdown
Member Author

k-yle commented Jul 10, 2025

How do you know if the popup window was closed by the user after this change?

Correct me if I'm wrong, but the COOP header intentionally makes this impossible, hence why this PR also reverts #135 🫤

This also prevents a dodgy referrer from polling Window#closed to track how longer the user spends in a new tab

@HarelM
Copy link
Copy Markdown
Contributor

HarelM commented Jul 10, 2025

That's my understanding as well, I hope you might have found a way to work around that...
GenAI told me that I can poll for popup.name and if I get an exception, I might know that the popup is still open, but on a different URL. I doubt the exception won't happen after the popup was closed too, but I haven't tested it yet...

@jjiglesiasg
Copy link
Copy Markdown

HOW THIS COULD BE SOLVED?, THERE IS NO POSSIBLE TO LOG IN

@bhousel
Copy link
Copy Markdown
Member

bhousel commented Jul 11, 2025

I tested this out with a local build of Rapid and it seems ok.

I had some concerns about how the messages get passed around,
per https://developer.mozilla.org/en-US/docs/Web/API/BroadcastChannel

Messages are broadcasted via a message event fired at all BroadcastChannel objects listening to the channel, except the object that sent the message.

So it sounds like a malicious app on the same origin could just listen for these messages. However I agree that this doesn't open up any new security risks, because anyway an app on the same origin can also just inspect the localStorage keys.

I'm not sure whether this was ever documented clearly, but this is why multiple osm-auth apps really need to be deployed like:

https://rapid.example.org
https://osmcha.example.org

and not

https://example.org/rapid
https://example.org/osmcha

because the latter shares localStorage and the keys can conflict.

@bhousel bhousel merged commit 07fb418 into osmlab:main Jul 11, 2025
3 checks passed
@k-yle k-yle deleted the kh/auth-broken branch July 12, 2025 04:26
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.

osm-auth example fails unless "single page" is selected

7 participants