Skip to content

RFC: Simplifying CORS #1029

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.

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
jeromevdl opened this issue Jan 12, 2023 · 1 comment
Open

RFC: Simplifying CORS #1029

jeromevdl opened this issue Jan 12, 2023 · 1 comment
Assignees
Labels
priority:3 Neutral - not a core feature or affects less than 40% of users RFC

Comments

@jeromevdl
Copy link
Contributor

Key information

Summary

When working with API Gateway + Lambda, developers need to handle CORS Headers properly, to make sure their API can be called by a web frontend and to avoid cross origin requests to be blocked. This module would simplify adding CORS headers in the response of their Lambda function when used with an API Gateway Proxy.

Motivation

Adding CORS in the response headers is either forgotten (and nothing works as expected), either boring (a lot of boilerplate code, with very specific syntax). With the wish to simplify developer life, Lambda Powertools could bring something.

Proposal

  • Adding a new annotation for the function handler method: @CrossOrigin
  • When using this annotation with a handler implementing RequestHandler<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent>, the annotation will automatically add the headers Access-Control-Expose-Headers, Access-Control-Allow-Origin, Access-Control-Allow-Methods, Access-Control-Allow-Credentials, Access-Control-Max-Age to the response object.
public class FunctionProxy implements RequestHandler<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent> {
    @CrossOrigin
    public APIGatewayProxyResponseEvent handleRequest(final APIGatewayProxyRequestEvent input, final Context context) {
        // CORS headers will be automatically added in the following object:
        return new APIGatewayProxyResponseEvent()
            .withStatusCode(200)
            .withBody(body);
    }
}
  • The annotation can obviously be configured to specify the different values (there are some default values to simplify the job of developpers):
parameter Default Description
allowedHeaders Authorization, * (*) Access-Control-Allow-Headers header
exposedHeaders * Access-Control-Expose-Headers header
origins * Access-Control-Allow-Origin header
methods * Access-Control-Allow-Methods header
allowCredentials true Access-Control-Allow-Credentials header
maxAge 29 Access-Control-Max-Age header

Example:

@CrossOrigin(
        origins = "http://origin.com, https://other.origin.com",
        allowedHeaders = "Authorization, Content-Type, X-API-Key",
        methods = "POST, OPTIONS"
)
public APIGatewayProxyResponseEvent handleRequest(final APIGatewayProxyRequestEvent input, final Context context) {
    // ...
}
  • The annotation can also be configured with environment variables, to simplify even further and allow configuration in infra as code (SAM for example)
Environment variable Default Description
ACCESS_CONTROL_ALLOW_HEADERS Authorization, * (*) Access-Control-Allow-Headers header
ACCESS_CONTROL_EXPOSE_HEADERS * Access-Control-Expose-Headers header
ACCESS_CONTROL_ALLOW_ORIGIN * Access-Control-Allow-Origin header
ACCESS_CONTROL_ALLOW_METHODS * Access-Control-Allow-Methods header
ACCESS_CONTROL_ALLOW_CREDENTIALS true Access-Control-Allow-Credentials header
ACCESS_CONTROL_MAX_AGE 29 Access-Control-Max-Age header
  • When using multiple origins in the configuration (comma-separated), the annotation will find the one matching the request and use this specific one in the header, as browsers don't support multiple origins in the header.

Drawbacks

This new module can bring a few additional kb to the lambda package size. It does not use any dependency which is not already in the core module.

Rationale and alternatives

  • What other designs have been considered? Why not them? N/A
  • What is the impact of not doing this? N/A

Unresolved questions

N/A

@jeromevdl jeromevdl added the feature-request New feature or request label Jan 12, 2023
@ijemmy
Copy link

ijemmy commented Jan 13, 2023

@jeromevdl

This is one of the most common issues people face. It will greatly improve DX.

Some comments:

  1. From security perspective, we should force most fields to be required rather than provide '*' as a default. Many developers do not know why we have CORS and may use the default values (out of convenient) in production without knowing the consequence.
  2. origin header doesn't support multiple value. I use usually information from request header to determine which value should I return (e.g. dev.site.com, staging.site.com, etc.). But I think we should leave this to clients to keep this feature generic.
  3. There are two additional steps required to enable CORS. People usually miss them. We should be document or link as a prerequisite in the doc. (to avoid "why this doesn't work" issues). They are are:
  4. Should document if it overrides values if the handler has already specified .withHeader() with the same key. I suggest not to override so that the handler can override the origin header value from event/context data.

@msailes msailes added RFC and removed feature-request New feature or request labels Feb 18, 2023
@jeromevdl jeromevdl added priority:3 Neutral - not a core feature or affects less than 40% of users and removed triage labels Jul 17, 2023
@mriccia mriccia self-assigned this Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:3 Neutral - not a core feature or affects less than 40% of users RFC
Projects
Status: On hold
Development

No branches or pull requests

6 participants