Access mode detection (transcode vs grpcweb vs grpc) by aseigo · Pull Request #412 · elixir-grpc/grpc

aseigo · 2025-04-21T08:09:27Z

This introduces:

preflight request detection, allowing sensible responses to CORS preflight checks using the OPTIONS http verb
client type detection, which may be one of grpc, grpcweb, or web (the latter implying a need for http transcoding)
an interceptor that applies CORS headers for non-grpc requests

This is a draft for discussion in #237

These typically come from grpcweb clients doing OPTIONS checks for CORS

This replaces the `http_transcode` member of the `Server.Stream` struct as transcoding is correlated with being a web client (*not* a grpcweb client, however!) Knowing the type of client also allows for detection of when to send different types of headers, for e.g. CORS

It an option `:allow` option to define which origins are allowed. This may be a static string or a function which will be passed the current `req` and `stream` structs to choose what to allow.

sleipnir

I have only one objection to the approach and I explained it in the comment

lib/grpc/server.ex

NOTE: will merge this with commit c39c834 post-review/discussion. I am leaving it as a separate commit for now for ease of continued discussion on the current PR.

sleipnir

Minor suggestion

lib/grpc/server/adapters/cowboy/handler.ex

this more accurately and directly maps to the grpc terminology, and is probably clearer.

aseigo · 2025-04-23T15:33:35Z

I just pushed a couple more commits to this branch which:

adds the http req headers to the Stream struct
uses those in the included CORS Interceptor to only add headers when it's a cors request.

I debated adding the http request headers to the Stream struct until I looked at some of the use cases I already have in projects that do similar things, as well as the documentation for CORS extenstions for other languages and frameworks ... and the headers they need to look at to make an informed decision varies greatly: authorization, host, etc.

To make the included Interceptor generically usable (or make it plausible for others to write their own), it appears "necessary" to include all the req headers. Without changing the Interceptor API pattern, this means adding it to the Stream.

Thoughts on that are welcome.

FWIW, I have done functional testing with a SPA written with React, next.js, and the grpc-web npm package.

Other summary points that I will reiterate here for clarity and comment:

http_transcoding members stay where they are, and with defaults retained as they currently are as well
for grpcweb, the default is no CORS headers, which restricts access by default
CORS headers are sent via an Interceptor, making inclusion and configuration of CORS (and therefore grpcweb access) explicit and opt-in

Anything else? Other thoughts?

I may need to write some more unit tests somewhere around Server .. I have cleaned up formatting (still have to rebuild my lsp for the version of Elixir/Erlang this repo uses.. )

sleipnir · 2025-04-23T15:38:06Z

I just pushed a couple more commits to this branch which:

adds the http req headers to the Stream struct

uses those in the included CORS Interceptor to only add headers when it's a cors request.

I debated adding the http request headers to the Stream struct until I looked at some of the use cases I already have in projects that do similar things, as well as the documentation for CORS extenstions for other languages and frameworks ... and the headers they need to look at to make an informed decision varies greatly: authorization, host, etc.

To make the included Interceptor generically usable (or make it plausible for others to write their own), it appears "necessary" to include all the req headers. Without changing the Interceptor API pattern, this means adding it to the Stream.

Thoughts on that are welcome.

FWIW, I have done functional testing with a SPA written with React, next.js, and the grpc-web npm package.

Other summary points that I will reiterate here for clarity and comment:

http_transcoding members stay where they are, and with defaults retained as they currently are as well

for grpcweb, the default is no CORS headers, which restricts access by default

CORS headers are sent via an Interceptor, making inclusion and configuration of CORS (and therefore grpcweb access) explicit and opt-in

Anything else? Other thoughts?

I may need to write some more unit tests somewhere around Server .. I have cleaned up formatting (still have to rebuild my lsp for the version of Elixir/Erlang this repo uses.. )

Great job!
It would be great to add a usage example with CORS settings and a client example to the documentation.

sleipnir

I have filed a request for changes but just so you can check a question I raised regarding the specification. Can you check it for us?

lib/grpc/server/interceptors/cors.ex

Only send access-control-allow-headers when the client requests it with access-control-request-headers and allow the same configurability as access-control-allow-origin provides

aseigo · 2025-04-23T18:33:59Z

It would be great to add a usage example with CORS settings and a client example to the documentation.

I have added one to the README.md. The module documentation already contained an example.

sleipnir · 2025-04-23T19:03:16Z

I approved this PR to approve the pipeline, but in this specific case I would like a view from @polvalente before moving on to the merge, maybe I missed something along the way, a second opinion is always good

sleipnir · 2025-04-23T19:43:53Z

After some adjustments to the pipeline the tests ran ok.

polvalente

Some security concerns, but overall looks good to me

README.md

polvalente · 2025-04-25T20:54:42Z

README.md

+end
+```
+
+By default, the CORS `Interceptor` responds to CORS requests with a permissive response that allows *all* points of origin to access the service. This may be undesirable, particularly in production environments. For this reason, both the allowed origin and headers are configurable:


I think we should have a default that requires the values and a separate allow_all option for dev mode that the user must opt-in to activate

I think we should have a default that requires the values and a separate allow_all option

'*' is allow all, and is widely documented in relation to CORS, so special values really are not necessary for that.

The default would then be to not send any CORS headers, requiring some value to be set explicitly. Would that be OK for you?

Yeah, make the option required then, without any defaults. We just can't default to allowing everything

README.md

polvalente · 2025-04-25T20:55:32Z

README.md

+The set of configuration directives supported include:
+
+  * `allow_origin` - A string containing the allowed origin(s), or a remote function (e.g. `&MyApp.MyModule.function/2)`) which takes a `req` and a `stream` and returns a string. Defaults to `"*"`, which will allow all origins to access this endpoint.
+  * `allow_headers` - A string containing the allowed headers, or a remote function (e.g. `&MyApp.MyModule.function/2)`) which takes a `req` and a `stream` and returns a string. Defaults to the value of the `"access-control-request-headers"` request header from the client.


What's the format for allowed headers?

In both cases I think we should receive {mod, name} instead of function because users might end up passing fn x, y -> end which will fail at runtime

What's the format for allowed headers?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Access-Control-Allow-Headers

{mod, name}

If that's what you want. I personally find this uglier and less idiomatic than a function capture (even the official Elixir lang docs recommend avoiding apply when possible). But the bigger issue is that this would also mean it wouldn't get checked and caught at compile time, but fail at runtime, which is not great.

users might end up passing fn x, y -> end which will fail at runtime

It actually fails at compile-time, as an anonymous function is not a remote function.

Re headers:

We should add that link to the docs

Re function:

Given that we can enforce that it's a capture, we can keep it as is.

polvalente · 2025-04-25T21:03:52Z

lib/grpc/server/interceptors/cors.ex

+    allow_origin =
+      case Keyword.get(opts, :allow_origin) do
+        {:&, [], [{:/, [], [_signature, 2]}]} = fun -> fun
+        static when is_binary(static) -> static
+        _ -> "*"
+      end


As this is running in a macro context (I can't recall why, but let's roll with this), the current code does not allow for a variable to be referenced here, I think. It will end up defaulting to the catch-all silently and this is a security risk.

Perhaps this validation should be done at run time?

It will end up defaulting to the catch-all silently and this is a security risk.

There are unit tests for this that say otherwise. See CORS allow origin header value is configuraable with a two-arity function in test/grpc/server/interceptors/cors_test.exs.

Pass an atom/integer/tuple/list/variable referencing a binary instead of a capture or literal binary and this will fall through to "*".

We should raise with ArgumentError

Pass an atom/integer/tuple/list/variable referencing a binary instead of a capture or literal binary and this will fall through to "*".

This is actually covered in the documentation: a string or a remote function, otherwise is defaults to '*'. As was the default values used.

It's a moot point now, in any case, as I've removed the default for allow_origin altogether in 18ad11f which should address this and another of your concerns.

lib/grpc/server/interceptors/cors.ex

Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>

polvalente · 2025-04-26T22:15:46Z

test/grpc/server/interceptors/cors_test.exs

+        request,
+        %{stream | access_mode: :grpcweb},
+        fn _request, _stream -> {:ok, :ok} end,
+        CORSInterceptor.init()


This style of calling init explicitly won't allow for passing the capture without also allowing a function.

If we keep the capture as the chosen syntax, we need to guard against allowing the function AST.

IMO moving to the tuple will be simpler to support in both situations

This style of calling init explicitly won't allow for passing the capture without also allowing a function.

This is a convenience for the unit test only. (... and is what the unit tests for the logging Interceptor does.) The unit tests for utilizing a remote function use an Endpoint, as that way init is called at compile-time in a macro context. I would do this for all the tests, but it's far uglier and less clear so I followed the precedent set in logger_test.exs where possible.

Of course, an Interceptor's init is always, and only, called within a macro as this library treats an Interceptor's init as a source of compile-time configuration data.

If we keep the capture as the chosen syntax, we need to guard against allowing the function AST.

If I understand what you are suggesting here, that already isn't possible as it is being run within a macro and so fails at compile time.

Let's use the other method, though, as your code heavily relies on macro comparison and that can't be tested if you're calling things as you are here

polvalente · 2025-04-27T00:12:45Z

lib/grpc/server/interceptors/cors.ex

+    allow_origin =
+      case Keyword.get(opts, :allow_origin) do
+        {:&, [], [{:/, [], [_signature, 2]}]} = fun -> fun
+        static when is_binary(static) -> static


You can add a explicit catch all here raising and ArgumentError for better UX.

This matching mode is also already enough to guard against runtime functions as I'd mentioned below.

You can add a explicit catch all here raising and ArgumentError for better UX.

see b51feda

This matching mode is also already enough to guard against runtime functions as I'd mentioned below.

When called outside of a macro, yes. When called in a macro, it fails at compilation before it even gets there.

lib/grpc/server/interceptors/cors.ex

aseigo · 2025-04-27T01:55:41Z

My available time for this PR has run out. Please feel free to do with it as you wish from here.

polvalente

Thank you for the contribution! This was a tricky one to get through.

aseigo added 2 commits April 21, 2025 11:43

Detect preflight requests

36986fe

These typically come from grpcweb clients doing OPTIONS checks for CORS

aseigo force-pushed the feature/client-type-detection branch from e42b573 to c2ae07d Compare April 21, 2025 09:43

Add a CORS header injection Interceptor

e536825

It an option `:allow` option to define which origins are allowed. This may be a static string or a function which will be passed the current `req` and `stream` structs to choose what to allow.

aseigo force-pushed the feature/client-type-detection branch from c2ae07d to e536825 Compare April 21, 2025 11:12

aseigo marked this pull request as ready for review April 21, 2025 15:58

sleipnir requested changes Apr 21, 2025

View reviewed changes

lib/grpc/server.ex Outdated Show resolved Hide resolved

aseigo added 2 commits April 21, 2025 23:37

Reintroduce the boolean http_transcode member of the Stream struct

39353bc

NOTE: will merge this with commit c39c834 post-review/discussion. I am leaving it as a separate commit for now for ease of continued discussion on the current PR.

web can be detected in extract_subtype

ed43898

aseigo changed the title ~~Client type detection~~ Access mode detection (transcode vs grpcweb vs grpc) Apr 22, 2025

sleipnir requested changes Apr 22, 2025

View reviewed changes

lib/grpc/server/adapters/cowboy/handler.ex Outdated Show resolved Hide resolved

aseigo added 6 commits April 23, 2025 17:28

change access_type to access_mode, :web to :http_transcoding

2da7bb2

this more accurately and directly maps to the grpc terminology, and is probably clearer.

default opts to an empty list, whitespace fix

c25d76a

add tests for the CORS header interceptor

0361416

Assign to a temporary variable and assign that to Stream.http_transcode

7ea68ab

provide access to request headers in the Stream object

e547b8d

only set cors when the sec-fetch-mode header indicates it

61f4a78

aseigo force-pushed the feature/client-type-detection branch from 8c4ab5a to 61f4a78 Compare April 23, 2025 15:31

another test: if there is NO sec-fetch-mode header

a22f2fa

sleipnir requested changes Apr 23, 2025

View reviewed changes

lib/grpc/server/interceptors/cors.ex Outdated Show resolved Hide resolved

sleipnir requested changes Apr 23, 2025

View reviewed changes

lib/grpc/server/interceptors/cors.ex Outdated Show resolved Hide resolved

aseigo added 4 commits April 23, 2025 20:12

Make access-control-allow-headers configurable, and optional

bcd4ddb

Only send access-control-allow-headers when the client requests it with access-control-request-headers and allow the same configurability as access-control-allow-origin provides

align variable names with http header names

81e1def

Add a CORS example to the README

f6493d6

format

a32654f

sleipnir approved these changes Apr 23, 2025

View reviewed changes

Merge branch 'master' into feature/client-type-detection

cd5f108

polvalente reviewed Apr 25, 2025

View reviewed changes

Verbage.

0ea8c8e

Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>

polvalente reviewed Apr 26, 2025

View reviewed changes

remove default for allow_origin in CORS interceptor

18ad11f

polvalente reviewed Apr 27, 2025

View reviewed changes

aseigo added 2 commits April 27, 2025 02:30

raise on failed allow_origin type, and test for that

b51feda

allow_origin, not allow_header

0bb4c56

polvalente added 5 commits April 27, 2025 01:13

test: add missing compile-time test

5c832ca

chore: format readme

4ba9460

chore: fix Note bold

6700d4e

docs: cleanup readme linking

7c34baa

docs: proper module linking in exdoc

99e6593

polvalente approved these changes Apr 27, 2025

View reviewed changes

fix: do not use Keyword.validate due to Elixir 1.12 support

3d3933c

polvalente merged commit 37fd806 into elixir-grpc:master Apr 27, 2025
8 checks passed

sleipnir mentioned this pull request May 29, 2025

Current State of gRPC-Web and Handling CORS/Pre-flight Requests #394

Closed

sleipnir mentioned this pull request Jul 15, 2025

grpc-web allow custom setting custom cors headers #237

Closed

Conversation

aseigo commented Apr 21, 2025

Uh oh!

sleipnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sleipnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aseigo commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sleipnir commented Apr 23, 2025

Uh oh!

sleipnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aseigo commented Apr 23, 2025

Uh oh!

sleipnir commented Apr 23, 2025

Uh oh!

sleipnir commented Apr 23, 2025

Uh oh!

polvalente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aseigo commented Apr 27, 2025

Uh oh!

polvalente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aseigo commented Apr 23, 2025 •

edited

Loading