fix(security): update module github.com/open-feature/flagd/core to v0.13.2 [security] #818
Conversation
github-actions
bot
requested review from
Kavindu-Dodan,
bacherfl,
liran2000 and
toddbaert
Summary of ChangesHello @connyay, 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 enhancing the security and stability of the Highlights
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
|
….13.2 [security] Signed-off-by: Connor Hindley <[email protected]>
Instead of returning early when GetAll fails (which could leave the provider stuck in an uninitialized state), treat it as if there are no old flags. This ensures the sync completes and events are emitted. Signed-off-by: Connor Hindley <[email protected]>
connyay
force-pushed
the
cjh-flagd-core-0132
branch
from
48343a4 to
b872a31
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates github.com/open-feature/flagd/core to v0.13.2 to address a security vulnerability. The changes primarily involve adapting the in-process service to a modified SetState signature from the flagd/core module, which no longer returns change information. The new implementation correctly computes flag changes by comparing the store's state before and after an update. The dependency updates in go.mod and go.sum are appropriate, and the test files have been updated accordingly, including improvements to test robustness. I have one suggestion to improve the efficiency and correctness of the new change detection logic.
| func computeChangedFlags(oldFlagMap map[string]model.Flag, newFlags []model.Flag) []string { | ||
| changedKeys := make([]string, 0) | ||
| newKeys := make(map[string]struct{}, len(newFlags)) | ||
|
|
||
| // Check for added or modified flags | ||
| for _, flag := range newFlags { | ||
| newKeys[flag.Key] = struct{}{} | ||
| oldFlag, exists := oldFlagMap[flag.Key] | ||
|
|
||
| if !exists || !reflect.DeepEqual(oldFlag, flag) { | ||
| changedKeys = append(changedKeys, flag.Key) | ||
| } | ||
| } | ||
|
|
||
| // Check for deleted flags | ||
| for key := range oldFlagMap { | ||
| if _, exists := newKeys[key]; !exists { | ||
| changedKeys = append(changedKeys, key) | ||
| } | ||
| } | ||
|
|
||
| return changedKeys | ||
| } |
There was a problem hiding this comment.
This function can be made more efficient. Instead of creating a newKeys map and then iterating over oldFlagMap to find deleted keys, you can delete keys from oldFlagMap as you iterate through newFlags. The remaining keys in oldFlagMap will then be the deleted ones. This avoids creating an extra map and a loop, simplifying the logic.
Additionally, a note on correctness: using reflect.DeepEqual on the model.Flag struct might cause false positives for changes. The Targeting field is a JSON string, and DeepEqual will detect changes for formatting differences (e.g., whitespace, key order) that are semantically irrelevant. A more robust solution would be to unmarshal the Targeting JSON and compare the resulting objects. While the current approach is acceptable, it's worth considering this for future improvements to avoid unnecessary change events.
func computeChangedFlags(oldFlagMap map[string]model.Flag, newFlags []model.Flag) []string {
changedKeys := make([]string, 0)
// Check for added or modified flags, and remove processed keys from oldFlagMap
for _, flag := range newFlags {
oldFlag, exists := oldFlagMap[flag.Key]
if !exists || !reflect.DeepEqual(oldFlag, flag) {
changedKeys = append(changedKeys, flag.Key)
}
delete(oldFlagMap, flag.Key)
}
// Any remaining keys in oldFlagMap are deleted flags
for key := range oldFlagMap {
changedKeys = append(changedKeys, key)
}
return changedKeys
}There was a problem hiding this comment.
mutating the oldFlagMap seems like an unexpected side effect for minimal performance gains. 👎
6 participants
Summary
Fixes #814
This is a rework of #815 with a small tweak/fix on error handling.