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

fix empty response body when allowedOrigins is set #185

Conversation

joshuaeilers
Copy link
Contributor

@joshuaeilers joshuaeilers commented Apr 29, 2022

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Provide links to any issues in this repository or elsewhere relating to this pull request.

Describe the solution you've provided

When setting the allowedOrigins to something other than *, as in a real domain, we do not see any response body for GET requests. This change stops the short-circuiting that was happening if an origin was matched.

Describe alternatives you've considered

I thought about always passing * in production as an absolute last resort.

Additional context

Add any other context about the pull request here.

@joshuaeilers joshuaeilers force-pushed the fix-allowed-origins-empty-response-body branch from 5933dec to f329872 Compare April 29, 2022 22:33
@joshuaeilers joshuaeilers marked this pull request as ready for review April 29, 2022 22:37
@joshuaeilers joshuaeilers force-pushed the fix-allowed-origins-empty-response-body branch from f329872 to 734f145 Compare April 29, 2022 22:46
@joshuaeilers
Copy link
Contributor Author

I'm not sure why the workflow is failing with 401's. Could someone from the LD team help me out with that?

@eli-darkly
Copy link
Contributor

I'm not sure why the workflow is failing with 401's. Could someone from the LD team help me out with that?

The integration test job requires a credential to connect to LD, and that credential is configured as a secret in CircleCI for our own projects - so I think it cannot work in a fork. In other words, this failure isn't related to your change.

Copy link
Contributor

@eli-darkly eli-darkly left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing this. The change looks correct, and we should be able to release a patch soon.

@eli-darkly eli-darkly merged commit 47f15e8 into launchdarkly:v6 Apr 29, 2022
@eli-darkly
Copy link
Contributor

We've released v6.7.6 with this fix - thanks again.

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

Successfully merging this pull request may close these issues.

2 participants