Feat/feature ext #55
Feat/feature ext #55
Conversation
Summary of ChangesHello @kooksee, 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 introduces several new features and improvements to the funk library. It adds modules for HTTP-based feature flag management and file-based configuration monitoring, enhances configuration capabilities with CEL expressions and environment variable validation, and refactors logging for better consistency and maintainability. These changes aim to provide more flexible, secure, and manageable runtime configuration options. 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
|
![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 introduces a new self-upgrade command and significantly refactors the configuration management and error handling mechanisms. The config package now leverages a CEL expression engine for dynamic configuration values and enforces explicit declaration of environment variables, enhancing security and maintainability. The result package has been updated to provide more consistent error wrapping and logging. Additionally, new featurehttp and featurewatcher modules are introduced for managing feature flags. The changes are well-structured and improve the overall robustness and security of the codebase.
| safePath, err := securePath(cfg.workDir, name) | ||
| if err != nil { | ||
| log.Error().Err(err). | ||
| Str("name", name). | ||
| Str("workDir", cfg.workDir). | ||
| Msg("embed: path security check failed") | ||
| return types.String("") | ||
| } |
There was a problem hiding this comment.
The securePath function is correctly used within the embed CEL function to prevent path traversal attacks. This is a critical security measure, ensuring that embed can only access files within the designated workDir and cannot be exploited to read arbitrary files on the system.
| func securePath(baseDir, name string) (string, error) { | ||
| if name == "" { | ||
| return "", errors.New("file name is empty") | ||
| } | ||
|
|
||
| // Reject absolute paths immediately | ||
| if filepath.IsAbs(name) { | ||
| return "", errors.Errorf("path traversal detected: absolute path %q not allowed", name) | ||
| } | ||
|
|
||
| // Clean and resolve the path | ||
| absBase, err := filepath.Abs(baseDir) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "failed to get absolute base path") | ||
| } | ||
|
|
||
| // Join and clean the target path | ||
| targetPath := filepath.Join(absBase, name) | ||
| absTarget, err := filepath.Abs(targetPath) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "failed to get absolute target path") | ||
| } | ||
|
|
||
| // Ensure the target is within the base directory | ||
| // Add trailing separator to prevent prefix attacks (e.g., /base-evil matching /base) | ||
| if !strings.HasPrefix(absTarget, absBase+string(filepath.Separator)) && absTarget != absBase { | ||
| return "", errors.Errorf("path traversal detected: %q escapes base directory %q", name, baseDir) | ||
| } | ||
|
|
||
| return absTarget, nil |
There was a problem hiding this comment.
The securePath function is a critical security component. It correctly validates that any resolved path stays within the specified base directory, effectively preventing path traversal vulnerabilities. This is essential for functions like embed() that read files based on user-provided paths.
| if cfg.envSpecMap != nil { | ||
| if _, defined := cfg.envSpecMap[name]; !defined { | ||
| return -1, fmt.Errorf("env: variable %q is not defined in envs, all env vars must be declared", name) | ||
| } |
There was a problem hiding this comment.
This check ensures that any environment variable used in the configuration via ${ENV} syntax must be explicitly declared in patch_envs. This is a high-severity security and maintainability feature, preventing accidental use of undeclared environment variables and making the configuration more explicit and auditable.
| ProgressListener: defaultProgressBar, | ||
| } | ||
| assert.Must(c.Get()) | ||
| assert.Must(os.Rename(filepath.Join(downloadDir, asset.Filename), execFile)) |
There was a problem hiding this comment.
Renaming the downloaded file directly over the running executable (execFile) is a high-risk operation. On Windows, this operation will likely fail if the executable is in use. On POSIX systems, while os.Rename is atomic for files on the same filesystem, an interruption or failure could leave the executable in a corrupted state. A safer approach would involve:
- Downloading to a temporary file.
- Verifying the downloaded file (e.g., checksum).
- Moving the old executable to a backup location.
- Renaming the new executable to the original executable's path.
- Cleaning up the backup on successful restart or keeping it for rollback.
| if cfg.envSpecMap != nil { | ||
| key = strings.TrimSpace(strings.ToUpper(key)) | ||
| if _, defined := cfg.envSpecMap[key]; !defined { | ||
| return types.NewErr("env: variable %q is not defined in patch_envs, all env vars must be declared", key) | ||
| } |
There was a problem hiding this comment.
The env CEL function correctly validates that environment variables are explicitly defined in patch_envs. This is a high-severity security and maintainability feature, preventing accidental exposure of undeclared environment variables and ensuring that all configuration dependencies are explicit.
|
|
||
| // FlatMap calls fn with the value if the result is OK, then returns the result unchanged. | ||
| func (r Result[T]) FlatMap(fn func(val T) Result[T]) Result[T] { | ||
| // MapVal calls fn with the value if the result is OK, then returns the result unchanged. |
There was a problem hiding this comment.
Renaming FlatMap to MapVal improves semantic clarity, as it better describes the operation of mapping a value to another Result.
| // panicIfError logs the error and panics | ||
| // This maintains backward compatibility with existing code that expects panics | ||
| func panicIfError(err error, events ...func(e *zerolog.Event)) { | ||
| func panicIfError(err error, events ...func(e Event)) { |
There was a problem hiding this comment.
Using the new result.Event type for panicIfError ensures a consistent logging interface within the result package, abstracting the underlying logger implementation.
| // err - The error to log | ||
| // events - Optional functions to add additional log fields | ||
| func logErr(ctx context.Context, skip int, err error, events ...func(e *zerolog.Event)) { | ||
| func logErr(ctx context.Context, skip int, err error, events ...func(e Event)) { |
There was a problem hiding this comment.
Using the new result.Event type for logErr ensures a consistent logging interface within the result package, abstracting the underlying logger implementation.
| evt := Event{e} | ||
| for _, fn := range events { | ||
| fn(e) | ||
| fn(evt) |
There was a problem hiding this comment.
The logErr function now correctly wraps the zerolog.Event into a result.Event before passing it to the provided event functions. This maintains the new consistent logging interface and ensures that custom log fields are added through the result.Event abstraction.
| result.ErrOf(cmd.Run()).Log(func(e result.Event) { | ||
| e.Msgf("failed to execute: %q", args) | ||
| }) |
There was a problem hiding this comment.
Using the new result.Event type and e.Msgf for logging in Run ensures a consistent logging interface within the result package, abstracting the underlying logger implementation.
kooksee
force-pushed
the
feat/feature_ext
branch
from
2342b37 to
16b646b
Compare
No description provided.