Skip to content

Disable CORS on /login/oauth/authorize #6657

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

Open
apata wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −1
Open

Disable CORS on /login/oauth/authorize #6657

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion lib/plausible_web/endpoint.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ defmodule PlausibleWeb.Endpoint do

plug(:runtime_session)

plug(CORSPlug)
plug(:cors)
plug(PlausibleWeb.Router)

def secure_cookie?, do: config!(:secure_cookie)
Expand All @@ -94,6 +94,13 @@ defmodule PlausibleWeb.Endpoint do
config!(:websocket_url)
end

@authorization_endpoint_path "/login/oauth/authorize"

@aerosol aerosol Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is it possible to use a path helper here, instead of hardcoding the literal?

Verified route maybe even? https://phoenix.hexdocs.pm/routing.html#verified-routes

@ukutaht ukutaht Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Last time I checked, verified routes are useless for us because our router has a catch-all entry for dashboards. This means any route like ~p"/completely-wrong" will pass verification because it would technically match the dashboard route handler. So as far as I could tell it provides no benefit over using literals.

I've thought that it would be nice to be able to tell the verified routes in Phoenix to ignore the catch-all. We don't really have a need to supply dynamic dashboard URLs as literals in code. Verified routes would be useful if they only verified all the remaining static routes. When I checked I didn't find a way to achieve that.

@zoldar zoldar Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could use verified routes now?

https://phoenix.hexdocs.pm/Phoenix.Router.html#match/5-options

:warn_on_verify - the boolean for whether matches to this route in verified routes should emit a warning, rather than being accepted as verified. It is useful to ignore an otherwise catch-all route definition from being matched when verifying routes. Defaults false

@apata apata Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks all for commenting! I looked those up in the context of the other PR as well, but didn't want to introduce the new way of doing things just yet.

@zoldar zoldar Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

JFTR I took a brief stab at a sweeping conversion to verified paths in #6660. The starting point was https://gist.github.com/andreaseriksson/e454b9244a734310d4ab74d8595f98cd, which converted a major chunk, fixing stuff manually afterwards. Stuff is still broken there, but it looks like wrapping it up would be doable.

@cors_opts CORSPlug.init([])

# As per OAuth 2.1, section 3.1, CORS must not be supported at the authorization endpoint
def cors(%Plug.Conn{request_path: @authorization_endpoint_path} = conn, _opts), do: conn
def cors(conn, _opts), do: CORSPlug.call(conn, @cors_opts)

def runtime_session(conn, _opts) do
Plug.run(conn, [{Plug.Session, runtime_session_opts()}])
end
Expand Down
Loading