Skip to content
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

CORS #9

Closed
Blacksmoke16 opened this issue Mar 11, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@Blacksmoke16
Copy link
Owner

commented Mar 11, 2019

One of the next bigger features I'm planning on implementing is better support for CORS. However, I wanted to take some time before starting development to hopefully gather some use cases/feedback from various developers. The hope is to use this feedback to to develop the feature in the best way.

The current idea that I had was to use a config file with Regex based paths to control how CORs get applied. Where default settings can be defined. An array of paths can be defined using regex to control what regex header values get applied based on request url. If the header value is not defined on a path, it would use the default value, otherwise would use the path specific value.

cors:
  enable_cors: true
  defaults:
      allow_credentials: false
      allow_origin: api.myapp.com
      allow_headers: ['X-Custom-Auth']
      allow_methods: ['POST', 'PUT', 'GET', 'DELETE']
      max_age: 3600
      expose_headers: []
      origin_header_name: Origin
  paths:
      '^public/api/':
          allow_origin: *
      '^/api':

Reasoning for liking this is it provides the most flexibility. The regex could just match all routes and apply the default settings, or certain routes could be excluded, or certain routes could have diff settings if that was needed.

I also plan on looking into doing something similar to how rails did yaml config files, where you are able to pull ENV vars/etc into it, which would make for easy env based setups like Docker.

Any feedback/concerns/other ideas would be welcome. Would like to have a solid idea that provides a clean, easy to develop, flexible experience.

  • Refactor handlers
  • Implement CORS handler
  • Refactor params
  • Update specs
@Blacksmoke16

This comment has been minimized.

Copy link
Owner Author

commented Mar 12, 2019

Option 2:

Do something similar to Serialization Groups but for CORS.

The yaml file would still be used, but instead of regex based matching, use the config file to configure CORS defaults and/or a set of groups. The config file would also have a setting on how to apply the settings. Either all routes get the default settings unless overridden by a group or disabled explicitly on each route or only enabled for routes with a group. In practice this would be a field in the route annotation:

athena.yaml

cors:
  enable_cors: true
  defaults:
    allow_credentials: false
    allow_origin: api.myapp.com
    allow_headers: ['X-Custom-Auth']
    allow_methods: ['POST', 'PUT', 'GET', 'DELETE']
    max_age: 3600
    expose_headers: []
    origin_header_name: Origin
  strategy: include 
  groups:
    - public:
        allow_origin: *

Where this defines the defaults to use for all routes, where all routes will have the defaults, unless explicitly disabled or overridden. The public group includes settings from defaults if not set. A controller could also be assigned a group, which would affect all routes within that controller. Groups set on individual routes would take precedence.

  # CORS explicitly disabled for this route
  @[Athena::Routing::Get(path: "/add/:val1/:val2", cors: false)]
  def self.add(val1 : Int32, val2 : Int32) : Int32
    val1 + val2
  end

  # CORS settings overridden by a group
  @[Athena::Routing::Get(path: "/me", cors: "public")]
  def self.get_me : String
    "Jim"
  end

  # Uses the default settings defined in the yaml file
  @[Athena::Routing::Post(path: "/test/:expected")]
  def self.post_body(expected : String, body : String) : Bool
    expected == body
  end
@bararchy

This comment has been minimized.

Copy link

commented Mar 16, 2019

Just make sure that default is not Allow-Origin: * to avoid CSRF issues

@Blacksmoke16

This comment has been minimized.

Copy link
Owner Author

commented Mar 16, 2019

Yea for sure. Is a good question of what to make the defaults. I'm thinking im just not going to set any defaults as I don't really have a way to know their domain or what their settings should be. Would be kinda strange to enable CORS and see nothing change but is a better alternative than a less secure default.

@Blacksmoke16

This comment has been minimized.

Copy link
Owner Author

commented Mar 28, 2019

To give an update on this. I did go with option 2. It just provides the most flexibility, with a more easy to maintain setup. High level plan of its functionality, subject to change:

cors:
  enabled: true
  strategy: whitelist 
  defaults: &defaults
    allow_credentials: false
    allow_origin: <%= ENV["ORIGIN"] %>
    allow_headers: ['X-Custom-Auth']
    allow_methods:
      - GET
      - POST
      - PUT
    max_age: 3600
    expose_headers: []
    origin_header_name: Origin
  groups:
    - legacy:
        <<: *defaults
        allow_origin: <%= ENV["LEGACY_ORIGIN"] %>
        allow_methods:
          - GET
    - public:
        <<: *defaults
        allow_origin: *

The config is rendered via ECR before read; mostly to give support inserting ENV vars into the config. In the future I could see this adding in settings dynamically based on ENV or some custom construct.

The config file uses YAML anchor tags to include options from the defaults group into other groups. This allows for a clean syntax to reuse properties and only override the necessary ones.

The strategy property, either whitelist or blacklist, determines how the CORS settings get applied.

  • whitelist - Defaults are not applied to anything by default. Will only apply the settings to controllers/actions explicitly set.
  • blacklist - Defaults are applied to all paths, unless explicitly disabled for a given controller/action.

In both cases, fields have been added to both the ControllerOptions and GET/.. method annotations.

@[Athena::Routing::ClassOptions(cors: false)]
struct MyController < Athena::Routing::Controller
  ...
  @[Athena::Routing::Get(path: "/public/test", cors: "public")]
  def self.public_test
    ...
  end
end

In this example, CORS would be disabled all actions within the MyController controller. However it would be enabled with the settings from the public group on the public_test action.

The priority is action > controller > defaults. Where defining a group on an action would override the settings of the controller. Defining a group on a controller would override the default settings.

CORS priority, with inheritance, groups, and strategy should provide pretty flexible feature. But it would also be easy to setup/use if your application does not have any complex requirements. I.e, set the defaults, don't define any groups, and set the strategy to blacklist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.