-
Notifications
You must be signed in to change notification settings - Fork 77
fix(flagd): update flagd/core and remove vulnerable dependencies [security] #815
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
Conversation
Summary of ChangesHello @kuznero, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on a critical security update for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request updates flagd/core to address a security vulnerability and adapts the code to the new API. The changes are mostly correct, but there are a couple of issues in the new change detection logic: errors are being ignored, and a brittle method is used for comparing flag states. I've provided comments with suggestions to improve robustness and correctness. The test improvements are good.
|
/gemini review |
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.
Code Review
This pull request primarily updates flagd/core to v0.13.1 to address a security vulnerability. The changes correctly adapt the in-process service to the modified SetState signature by implementing a new mechanism to compute flag changes. The logic for change detection is sound, and the test improvements using t.Cleanup and graceful server shutdown are excellent additions for test reliability. I've included a few suggestions to further improve the new change detection logic for better context handling and efficiency.
|
/gemini review |
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.
Code Review
This pull request updates flagd/core to address a security vulnerability and bumps several other dependencies. The changes correctly adapt the in-process service to the new SetState signature from flagd/core. The test suite is also improved with better cleanup logic. My review identifies a logic bug in the new change detection mechanism and suggests improvements for test robustness.
|
/gemini review |
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.
Code Review
This pull request primarily updates Go dependencies across the project, including Go version from 1.24.0 to 1.25.5, golangci-lint version, and various direct and indirect Go module dependencies such as flagd/core, golang.org/x/net, google.golang.org/grpc, and OpenTelemetry packages. The core logic for InProcess service in providers/flagd was refactored to explicitly track flagStore and introduce a new computeChangedFlags function. This function now determines flag changes by comparing old and new flag states using JSON marshalling for deep comparison, replacing the previous evaluator.SetState return values. Test files for the in_process provider were updated to reflect the SetState signature change and to properly manage gRPC server lifecycle with t.Cleanup.
|
/gemini review |
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.
Code Review
This pull request updates flagd/core to v0.13.1 to address a security vulnerability. The code changes correctly adapt the in-process service to the modified SetState API in the new version. Instead of receiving flag changes from SetState, the changes are now computed by comparing the flag store's state before and after the update. The logic for detecting added, modified, and deleted flags is sound. Additionally, the pull request includes valuable improvements to test cleanup and robustness by using t.Cleanup and graceful server shutdowns. The changes are of high quality. I have one suggestion to improve the robustness of the GOBIN path resolution in the Makefile.
|
/gemini review |
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.
Code Review
This pull request primarily updates github.com/open-feature/flagd/core to v0.13.1 to address a security vulnerability. The code changes correctly adapt the in-process service to the new SetState signature, which no longer returns change information. The new logic to compute flag changes by comparing store state before and after updates is sound. Additionally, this PR includes several beneficial improvements, such as bumping golangci-lint, fixing GOBIN path resolution in the Makefile, and enhancing test cleanup with t.Cleanup and graceful server shutdown. Overall, the changes are well-executed. I have one suggestion to improve the maintainability of the new change detection logic.
|
/gemini review |
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.
Code Review
This pull request effectively addresses a security vulnerability by updating github.com/open-feature/flagd/core to v0.13.1. The code changes correctly adapt the in-process service to the modified SetState API by implementing a new mechanism to compute flag changes, comparing the store's state before and after the update. The logic for detecting added, modified, and deleted flags is sound. Additionally, the pull request includes several valuable improvements, such as updating the Go linter, making the Makefile more robust by fixing the GOBIN path resolution, and enhancing test reliability with better cleanup procedures using t.Cleanup and graceful server shutdowns. The code quality is high, and the changes are well-implemented.
|
The PR is ready for review by maintainers. Please approve the CI workflow run. |
|
@kuznero Thank you for your work. Unfortunately, CI uses go 1.24 and it will fail for flagd go 1.25.5 requirement. And we have to support go 1.24 until 1.26 is released. It's better so split all your work into few PRs because |
This is a significant blocker. The module github.com/open-feature/flagd/core
go 1.25.5But the |
|
@toddbaert , all the fixes for CVEs listed in open-feature/flagd#1840 ... (GHSA-j5pm-7495-qmr3 | GHSA-qh38-484v-w52x | GHSA-7wwx-xj66-r44x | GHSA-hjx7-fpxx-mj48 | GHSA-frhw-mqj2-wxw2 | GHSA-7c64-f9jr-v9h2) are merged into Go 1.24. |
Different teams have their own security policies and must handle vulnerabilities according to defined timelines. They may not be able to wait. This also would require people to upgrade to the latest go (1.25.5), which may not be ideal when applying security fixes. @toddbaert |
Fair enough, I'll fix this with 1.24 tomorrow and re-release. Sorry all! |
This comment was marked as outdated.
This comment was marked as outdated.
|
I closed mine. Sorry. |
….13.1 [security] Signed-off-by: Roman Kuznetsov <[email protected]>
Update InProcess service to handle the new SetState signature that no longer returns change information. Flag changes are now computed by comparing store state before and after updates. Also includes: - Bump golangci-lint to v2.7.2 - Fix GOBIN path resolution - Improve test cleanup with t.Cleanup and graceful server shutdown - Fix import ordering in tests Signed-off-by: Roman Kuznetsov <[email protected]>
- Handle errors from flagStore.GetAll() calls instead of ignoring them - Use JSON marshaling for stable flag comparison instead of fmt.Sprintf which is not guaranteed to be stable across Go versions Signed-off-by: Roman Kuznetsov <[email protected]>
- Use service context (i.ctx) instead of context.Background() for flagStore.GetAll() calls to support cancellation during shutdown - Optimize computeChangedFlags by removing redundant newFlagMap and using delete-as-you-go pattern for detecting deleted flags Signed-off-by: Roman Kuznetsov <[email protected]>
- Fix bug in computeChangedFlags: move delete() before json.Marshal to prevent flags from being incorrectly reported as deleted when marshaling fails - Handle server.Serve errors in gRPC tests to catch unexpected server failures (ignoring expected ErrServerStopped) Signed-off-by: Roman Kuznetsov <[email protected]>
Replace JSON marshalling with reflect.DeepEqual for more reliable deep comparison of flag structs. This approach is more robust and doesn't depend on JSON serialization behavior. Signed-off-by: Roman Kuznetsov <[email protected]>
Use GNU Make's 'or' function to first check if GOBIN is set directly, and if not, fall back to using the first path from GOPATH (handling colon-separated multiple paths with cut -d: -f1). Signed-off-by: Roman Kuznetsov <[email protected]>
Refactor computeChangedFlags to not mutate its input parameter. Use a separate newKeys set to track new flag keys instead of deleting from oldFlagMap. This improves maintainability and makes the function safer to reuse. Signed-off-by: Roman Kuznetsov <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
a6ae141 to
36b227e
Compare
toddbaert
left a comment
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 updated to the latest module which works with go 1.24.x, rebased and fixed the (minor) conflicts. I reverted back to Go 1.24 in the consuming modules. Looks good to me. Thanks @kuznero
Signed-off-by: Roman Kuznetsov <[email protected]>
…urity] (open-feature#815) Signed-off-by: Roman Kuznetsov <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <[email protected]> Signed-off-by: leoff00 <[email protected]>
…urity] (open-feature#815) Signed-off-by: Roman Kuznetsov <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <[email protected]> Signed-off-by: leoff00 <[email protected]>
…urity] (open-feature#815) Signed-off-by: Roman Kuznetsov <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <[email protected]> Signed-off-by: leoff00 <[email protected]>
Summary
github.com/open-feature/flagd/coreto v0.13.1/v0.13.2 to address security vulnerabilitiesgithub.com/xeipuuv/gojsonpointergithub.com/xeipuuv/gojsonreferencegithub.com/xeipuuv/gojsonschemagithub.com/zeebo/xxh3github.com/klauspost/cpuid/v2SetStatesignature that no longer returns change informationFixes #814
Additional Changes
golang.org/x/expfrom direct to indirect dependency in flagd providert.Cleanupand graceful server shutdownTest plan