Add route option for CORS

[peterdee]

Hi.

I could not find any examples of the Pogo server / route with enabled CORS.

Here's a couple of examples for a CORS-enabled route:

1. When using the Toolkit:

server.route({
  handler: (request: Request, tk: Toolkit): Response => {
    const response = tk.response({ info: 'OK' });
    response.headers.append('access-control-allow-origin', '*');
    response.headers.append(
      'access-control-allow-headers',
      'Origin, X-Requested-With, Content-Type, Accept, Range',
    );
    return response.code(200);
  },
  method: 'GET',
  path: '/cors',
});

2. When using the request.response:

server.route({
  handler: (request: Request, tk: Toolkit): Response => {
    request.response.headers.append('access-control-allow-origin', '*');
    request.response.headers.append(
      'access-control-allow-headers',
      'Origin, X-Requested-With, Content-Type, Accept, Range',
    );
    request.response.body = {
      info: 'OK',
    };
    return request.response;
  },
  method: 'GET',
  path: '/cors',
});

I think this can be added to the documentation.

[sholladay]

I want the framework to handle CORS automatically, as it's fairly complex. We will add CORS support to the framework and you'll be able to enable it with a cors: true route option. See hapi for reference: https://github.com/hapijs/hapi/blob/master/API.md#-routeoptionscors

PR welcome, otherwise I'll get around to it when I have time.

[peterdee]

Ok, got it.

The code that I provided above works fine, I guess I will use it as a workaround for now.

You can close this.

[vivekchauhan12000]

is automatic CORS handling is applied?

[sholladay]

This issue remains open because no one has worked on adding the feature yet. Are you interested in working on it, @vivekchauhan12000?

[vivekchauhan12000]

Yes I will

[KaKi87]

Hello, any news on this ? Thanks

[sholladay]

@KaKi87 No one has picked this up yet. It isn't something I personally need, so I recommend submitting a PR for it if you need it any time soon.

[KaKi87]

Well, here's what I had to do for my use case.

I would love to have something as simple as fastify.register(require('fastify-cors'));, but I don't know to do such thing, plus I don't do TypeScript.

[sholladay]

It will be even simpler than that. It will work like this:

pogo.server({ cors : true });

To implement it in Pogo, you'll just need to call response.header() internally and set the appropriate headers as per the specification. Writing the code should be pretty easy, it's just that the spec has a number of special cases. We can start with a partial implementation instead of doing it all at once if you'd like.

As for TypeScript, you'll only need to add cors: Boolean to the server options interface. Everything else will be internal and you can use as much or little TypeScript as you are comfortable with. Feel free to write it in plain JavaScript. I can help with improving the type safety of the internal code.

[KaKi87]

For Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers response headers the value * counts as a wildcard

Those three, then ?

Also, it's not only about adding headers to each endpoint, but also creating an OPTIONS one for each.

[KaKi87]

Hello ?

[sholladay]

Hi, @KaKi87. I'm not sure we should include Access-Control-Expose-Headers by default. Also, I think it would be best to reflect the exact requested origin, method, and headers back to the client instead of using a * wildcard. But generally yes, that's what I'm thinking.

The cors: true setting should cause the server to respond to preflight requests with:

Access-Control-Allow-Origin: <origin>
Access-Control-Allow-Methods: <methods>
Access-Control-Allow-Headers: <headers>

For requests to the actual resource, the server only needs to send Access-Control-Allow-Origin, and ideally also Vary: Origin.

[KaKi87]

I'm not sure we should include Access-Control-Expose-Headers by default.

How about automatically including it when the handler send developer-defined headers ?

I think it would be best to reflect the exact requested origin, method, and headers

Agreed.

---

If I were going to try implementing this, where should I start ?

[sholladay]

How about automatically including it when the handler send developer-defined headers ?

So basically, if someone uses response.header(), then that header would be automatically added as an exposed header when CORS is enabled? That is an interesting idea and I can see how that could be convenient in some cases. How often do people expose headers like that and what is it typically used for? I don't think I've ever needed it, but my feeling is that doing this might be too magical and violate POLA. How would the user disable the behavior if they don't want it? I often send headers that don't need to be exposed to JavaScript. I'm not sure the framework is in the best position to make that security related decision.

If I were going to try implementing this, where should I start ?

it would happen during the response processing that happens around this part of the code:

https://github.com/sholladay/pogo/blob/334163e24dc15a588936048f1f12930be4f682a6/lib/server.ts#L61

The route handler returns some data or a Response object, which the framework then normalizes into a Response object via Response.wrap(source) inside of the serialize() function. Then it adds the appropriate Content-Type for the response and massages the response body a bit to make it compatible with Deno's std/http server.

https://github.com/sholladay/pogo/blob/334163e24dc15a588936048f1f12930be4f682a6/lib/serialize.ts#L11

CORS would just be another step in this processing pipeline.

In terms of pseudocode, we basically want to do something like this inside of server.inject():

return serialize(cors(await route.handler(request, new Toolkit(request))));

There are a variety of ways to structure this. I think ideally serialize() and cors() would both be called in a function that performs all response processing steps (there will be more in the future). And perhaps Response.wrap() should be moved outside of serialize() so that cors() can also make use of a normalized response. Frankly, the existing serialize() implementation is a bit sloppy and it's not very functional. But hopefully that won't get in the way here.

[KaKi87]

How often do people expose headers like that and what is it typically used for?

I never need headers that are automatically generated by the server, but I always need headers that I add myself.

For example, a Location header on a 201 response from a REST API.

this might be too magical and violate POLA

It proposes that a component of a system should behave in a way that most users will expect it to behave

Personally I experience CORS being itself against POLA.

When I return something from my backend, I expect my frontend to have access to it, because I intend to use it.

So, when I add an header to my backend response, I do it in order to use it from my frontend request.

[KaKi87]

The headers auto-exposing feature could also be opt-in, using a corsHeaders: true setting, for example.

[sholladay]

CORS is confusing, for sure, and I agree that it arguably violates POLA. Still, at least there is a spec that helps set expectations. Generally, I would say we should comply with the spirit of that spec to avoid any further complexity. There is a default safelist of headers that are exposed. Perhaps it would be good enough to expose a few additional, commonly needed headers by default, as opposed to exposing literally anything?

[KaKi87]

There is a default safelist of headers that are exposed. Perhaps it would be good enough to expose a few additional, commonly needed headers by default, as opposed to exposing literally anything?

Well, turns out even the cors package does not expose headers by default, so I agree with your solution.

Additionally to cors: true, we could add exposeHeaders: [], containing Location by default, how do we determine the rest ?

[sholladay]

Yeah, doesn't surprise me that the cors package behaves that way. I think they too would have a hard time coming up with a reasonable list of headers to expose. Generally, the user should decide.

I'm not necessarily against having an exposeHeaders option, but it also doesn't feel necessary. Pogo makes it very easy to add response headers, so this shouldn't be a big issue for most people, I would think. In a route handler, all you have to do is return response.header('Access-Control-Expose-Headers', 'foo') and you can chain .header() calls, too. I'd also accept a PR to support objects as input for .header() to pass multiple headers in one call, which could be useful for more advanced CORS scenarios.

[KaKi87]

I'd prefer adding exposeHeaders because it's more intuitive than figuring out the header name and value.

[sholladay]

Sounds good. Increasing compatibility with hapi is a good thing, too.

[sholladay]

Jake Archibald just published a new resource for CORS with a ton of useful details, including a Chrome bug I wasn't aware of. This is required reading for any implementation.