Added Access-Control-Max-Age to CORS hool with maxAge config option

[dcaravana]

Just a quick one but can help a lot reducing the number of HTTP calls.

Not sure about defining maxAge as default since the header is optional.
Also not sure if changes are needed to sails-generate.

[mikermcneil]

Hey @dcaravana, thanks for sending this over. Since exposing a new configuration option is a change in behavior/usage rather than a bug fix, this would need to be submitted as a proposal. Would you please review our updated contribution guide, then submit a proposal explaining your use case and how this approach would solve it, as well as much of a spec as you have time for? Beyond the other reasons discussed in our new contribution guide, one of the most important things about understanding your use case is so that we can ensure this is appropriately documented and tested now, and maintained properly in the future.

I appreciate your help and understanding re: the recent changes to our process, and I look forward to learning more about your goals for this enhancement.

[dcaravana]

Aha! Thanks Mike, that's really interesting and shows how much Sails is well done.

Mine was only an attempt to complete the implementation of CORS spec in Sails core (here the relevant lines https://www.w3.org/TR/cors/#access-control-max-age-response-header ), an interesting one from an optimization/performance standpoint because it could help halving the number of calls directed to an API (BTW Chrome ignores this header, though).

I mean this change is not strictly security-related and basically Sails and anyone else can live without it.

Since the same result can be achieved by anyone simply duplicating/editing the default Sails CORS hook, I think you can safely close this pull request.

[mikermcneil]

@dcaravana thanks for the details!

Ok will do-- although this sounds like something that would be a great addition for the core cors hook. I just want to be consistent about running this sort of change through the proposal process since it changes usage. It's the only way to make sure new features/enhancements are documented and tested, and for me to keep my sanity ^_^

[dcaravana]

Just to make things clear: I totally love the principles on which you've built the proposal process.

I do think this could be a good addition to the code, too, just let me find some time and I'll have a try at it (there's only do no try, I know :) ).

Cool?

On 20/01/16 01:11, Mike McNeil wrote:

@dcaravana https://github.com/dcaravana thanks for the details!

Ok will do-- although this sounds like something that would be a great addition for the core |cors| hook. I just want to be consistent about running this sort
of change through the proposal process since it changes usage. It's the only way to make sure new features/enhancements are documented and tested, and for me
to keep my sanity ^_^

—
Reply to this email directly or view it on GitHub #3493 (comment).

Diego Caravana - diego@caravana.to - +393482683376 - http://diego.caravana.to/
http://twitter.com/dcaravana - http://www.linkedin.com/in/dcaravana

[mikermcneil]

@dcaravana Mmmm fantastic, that sounds. Appreciate the kind words about the proposal process, I do.

[mikermcneil]

@dcaravana btw, if you're going to write up a proposal for this soon, feel free to just use this PR (i.e. add a commit that adds the link to the backlog in ROADMAP.md ). Since you've already got some good info here, it makes sense to keep everything in one place (but no worries if you've already started a separate PR either). Also, in case this helps: The most important thing for the proposal other than your use case (which you've done a great job setting up already) is documenting the intended usage of the enhancement in a clear and thorough-enough way that the rest of the community and the core team can easily provide feedback (and importantly, apprehend any potential unintended consequences)

[fenichelar]

@mikermcneil this would really help clean up my logs, what can I do to help get this moving along??

[fenichelar]

@mikermcneil It looks like there isn't anything in the contributing guide about a proposal process... So I am just going to wing this:

Background

The CORS standard includes a header used to inform the browser how long to cache the CORS response for. Without this header, browsers cache the response for a short time (5 seconds is common). This means that there can be many, many, many preflight CORS requests made. Because these requests are made before the actual request can be made, this can significantly increase the latency of making requests to an API. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age for more information.

Use

Set sails.config.cors.maxAge to the value that should be returned in the Access-Control-Max-Age header. This value is the number of seconds the CORS preflight response should be cached.

This can also be set on a route by route level using cors.maxAge on the specific route.

ToDo

@mikermcneil Please provide some feedback on this PR. If it looks good, I will submit a PR with the CORS documentation updated to reflect this option. What branch of sails-doc do you want this documented in?

[fenichelar]

Just to emphasize the importance of caching CORS preflight responses. Here is an example of the requests made when a user navigates around an Ember.js application served on the root domain with the Sails.js application served on a subdomain:

Multiple requests are made very quickly and so the browser makes multiple CORS requests. Once the first one has responded it is cached for 5 seconds but the other requests were already made so the short cache is practically useless. So a page load triggers a bunch of preflight requests and unless the next page is navigated to in under 5 seconds, this keeps happening for every page navigated to.

On top of this, the CORS preflight responses are actually larger than the data response because the data response is a 304.

It's maddening.

[0xGREG]

Bumping this, would appreciate if it's merged or implemented

[0xGREG]

If you want to do this and don't want to wait for this PR to be merged, just add this to your http config:

module.exports.http = {
    middleware: {
        maxAge: async (req, res, next) => {
            res.set("Access-Control-Max-Age", "600")
            next()
        },

        order: [
            'cookieParser',
            'session',
            'bodyParser',
            'compress',
            'poweredBy',
            "maxAge",
            'router',
            'www',
            'favicon',
        ]
    },

// ...
}