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

Remove real live api key #164

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Remove real live api key #164

merged 4 commits into from
Jan 24, 2024

Conversation

EmilyZhang777
Copy link
Contributor

This change removes the real live api key to resolve a vulnerability.

J=VULN-37164
TEST=auto

@EmilyZhang777 EmilyZhang777 requested a review from a team as a code owner January 23, 2024 17:31
Copy link
Contributor

github-actions bot commented Jan 23, 2024

Pull Request Test Coverage Report for Build 7644357355

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.972%

Totals Coverage Status
Change from base Build 7145357168: 0.0%
Covered Lines: 338
Relevant Lines: 357

💛 - Coveralls

Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

technically, removing the api key like this won't really help since the repo (and it's past versions) are open source. if security is concerned about these keys getting out, it might be worth looking into whether github has a way to fully redact/delete part of a file, including its past versions

@EmilyZhang777
Copy link
Contributor Author

I'm planning on deactivating the api keys, so this change should be sufficient.

Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

I'm planning on deactivating the api keys, so this change should be sufficient.

would that deactivate the slapshot test account? I find that pretty useful for testing and I believe a lot of our SDKs' test sites are built around the configuration of that experience. it would also be worth checking if the public-facing search-ui-react Storybook site is based off of this experience as well

@EmilyZhang777
Copy link
Contributor Author

We could generate a new api key for the account/experience and use that in places needed without deactivate the account/experience itself.

Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

if this is a new patch, you'll want to merge into a hotfix branch rather than develop

@EmilyZhang777
Copy link
Contributor Author

I'll update search-core first and revisit this PR to include version bump for search-core 👍

@EmilyZhang777 EmilyZhang777 deleted the dev/vulnLiveApiKey branch January 23, 2024 22:58
@EmilyZhang777 EmilyZhang777 restored the dev/vulnLiveApiKey branch January 24, 2024 15:41
@EmilyZhang777 EmilyZhang777 reopened this Jan 24, 2024
Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

I thought we were marking these vulnerabilities as a false positive. do we still need to remove the key?

@EmilyZhang777
Copy link
Contributor Author

Yeah, talked to mcgin and confirmed we still want to remove them

@EmilyZhang777 EmilyZhang777 merged commit 6d39acb into develop Jan 24, 2024
10 checks passed
@EmilyZhang777 EmilyZhang777 deleted the dev/vulnLiveApiKey branch January 24, 2024 18:58
EmilyZhang777 added a commit that referenced this pull request Jan 26, 2024
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.

3 participants