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

feat(cors): Configurable whitelist of origins that are allowed to make… #891

Merged
merged 1 commit into from
Sep 9, 2019
Merged

feat(cors): Configurable whitelist of origins that are allowed to make… #891

merged 1 commit into from
Sep 9, 2019

Conversation

srekapalli
Copy link
Contributor

  • Currently 'Gate' allows all origins and this fix is to allow only whitelisted origins if specified in the config.
  • Existing behavior kept as see and the new scheme will be enabled only if 'cors.allow-mode' set to 'originlist'
  • Tests here exercise both the old mode (regex) and the new (originlist).
  • @cfieber : The current implementation is still out there , Pl. LMK if we need to make that also strict and not allow the requests if the origin doesn't match the RegEx. Currently we just log and won't deny the request I think🤔

@srekapalli srekapalli marked this pull request as ready for review September 6, 2019 19:42
Copy link
Contributor

@cfieber cfieber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great start - there is a chance that the failing test is due to something missing in the CI environment since it is trying to spool up the whole app

@srekapalli
Copy link
Contributor Author

srekapalli commented Sep 6, 2019

great start - there is a chance that the failing test is due to something missing in the CI environment since it is trying to spool up the whole app

It's because I was hitting /health and that was giving 503 because it is actually running a health check behind. Switched to /version and that works just fine. '/health' works fines locally but CI server might be making a hit to a downstream app or something.

Copy link
Contributor

@ajordens ajordens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor points for discussion but otherwise 👍- Good job on your first PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants