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

Access-Control-Allow-Origin #267

Open
wants to merge 4 commits into
base: luds
Choose a base branch
from
Open

Access-Control-Allow-Origin #267

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d3f40d
Access-Control-Allow-Origin
shocknet-justin Jun 16, 2024
90014dd
Apply suggestions from code review
shocknet-justin Jun 17, 2024
4273c0e
Update 01.md
shocknet-justin Jun 17, 2024
59ba7da
Update 01.md
shocknet-justin Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions 01.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ Once `LNURL` is decoded:

Neither status codes or any HTTP Header has any meaning. Servers may use whatever they want. Clients should ignore them (and be careful when using libraries that treat responses differently based on headers and status codes) and just parse the response body as JSON, then interpret it accordingly.

## CORS Browser Compatibility

Application endpoints (or their defined Reverse Proxy directives) accessed when using the LNURL must respond to `GET` having set the header `Access-Control-Allow-Origin: *` for LNURL's to work with browser-based wallets, services and other webapps. If they set the `Access-Control-Allow-Methods` header, they must set in such a way that `GET` requests are permitted.
Copy link

@AaronDewes AaronDewes Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
Application endpoints (or their defined Reverse Proxy directives) accessed when using the LNURL must respond to `GET` having set the header `Access-Control-Allow-Origin: *` for LNURL's to work with browser-based wallets, services and other webapps. If they set the `Access-Control-Allow-Methods` header, they must set in such a way that `GET` requests are permitted.
Application endpoints that need to be accessed when interacting with an LNURL must set [CORS headers](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS) in such a way that they are reachable from any web origin.

Based on feedback from @hsjoberg and @fiatjaf, maybe just this sentence would be best.

No someone else apart from me and @shocknet-justin should probably also decide if we should include reverse proxies, because we don't seem to be able to agree here.

Copy link

Choose a reason for hiding this comment

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

I think we should at least keep the Access-Control-Allow-Origin: * or/and a link to the MDN describing CORS, to make it easier & more straightforward for devs to actually implement.

The issues is that incorrectly configured CORS is not immediately noticeable by a dev who uses non webapp based wallet (majority of wallets currently), I am worried that if this is not straightforward to implement (e.g. devs having to research what this CORS even means) they might just skip through it thinking their LN service is working all good.

Maybe it'd make sense to have a simple online tool (webapp) for checking whether LNURLs adhere to the spec (including CORS)

Copy link

Choose a reason for hiding this comment

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

@adambor I added your suggestion to my comment above.

@kaloudis @Kukks @adambor What's your opinion about mentioning proxies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should remove the mention of proxies to keep the text shorter.

Isn't it already obvious for proxies that CORS header would be required? The proxy is really an "application endpoint" here if I understand it correctly.

Copy link

Choose a reason for hiding this comment

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

Feel like we're bikeshedding on the proxy text. On one hand it's not taking up much space, on the other it also feels obvious to me.

I could go either way, but I lean towards keeping it as is.

Copy link

Choose a reason for hiding this comment

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

Great! Two more people disagreeing 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's obvious and an unnecessary implementation detail to mention.


## Decoding examples

In Scala:
Expand Down