-
Notifications
You must be signed in to change notification settings - Fork 173
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
Rename env variables to be used in React #142
base: main
Are you sure you want to change the base?
Conversation
REACT_APP_SEARCH_APP_NAME=some-search-application | ||
REACT_APP_SEARCH_APP_API_KEY=xxxxxxxxxxxxxxxxxxx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but this would make the API key available in the browser, which is a really bad idea security-wise, and not something we should promote or recommend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're correct, but actually the current app already does expose it. But this is the method documented in the Elasticsearch documentation.
This PR just fixes a bug that prevent developers to pass those parameters using an .env
file. If not, those parameters can be set through the web app itself.
Maybe we should add a disclaimer in the README about the security risks and point to the Elasticsearch documentation for more details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that we recommend this method, as it is very risky and depends on people having an understanding of what they're doing when generating their keys, which isn't always the case.
Even with a scoped API key, there is risk of a bad actor using the key to generate unexpected expenses for the application owner, or render the search functionality unusable by making enough calls to hit the rate limits, so this is still a bad idea.
At the very least there should be a very big warning in the readme explaning the risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a disclaimer in the Readme file, please let me know what you think.
No description provided.