Refactor 2 #15
Refactor 2 #15
Conversation
…ionality to integrations implementation
PR Summary
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 26.19% 28.21% +2.01%
==========================================
Files 18 18
Lines 2012 2028 +16
==========================================
+ Hits 527 572 +45
+ Misses 1444 1415 -29
Partials 41 41
☔ View full report in Codecov by Sentry. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
WalkthroughThis update introduces new integrations for secret management, specifically Infisical and dotenv-vault, along with their client libraries and test coverage. The workflow and task management code is refactored to support integrations, and documentation is updated accordingly. Codecov configuration and build workflow triggers are also adjusted. Changes
Sequence Diagram(s)Integration Initialization and Run FlowsequenceDiagram
participant Workflow
participant Integrations
participant Integration
participant EnvProvider
Workflow->>Integrations: List(getWorkflow)
Integrations->>Integration: New(getWorkflow)
loop For each Integration
Workflow->>Integration: IsEnabled()
alt If enabled
Workflow->>Integration: Run()
Integration->>EnvProvider: Load secrets (if applicable)
EnvProvider->>Workflow: Set environment variables
end
end
Infisical Integration Secret InjectionsequenceDiagram
participant Workflow
participant InfisicalIntegration
participant InfisicalClient
participant InfisicalAPI
Workflow->>InfisicalIntegration: Run()
loop For each env entry
InfisicalIntegration->>InfisicalIntegration: parseInfisicalURL(entry)
InfisicalIntegration->>InfisicalClient: GetSecrets(workspace, env, path)
InfisicalClient->>InfisicalAPI: Fetch secrets
InfisicalAPI-->>InfisicalClient: Encrypted secrets
InfisicalClient->>InfisicalIntegration: Decrypted secrets
InfisicalIntegration->>Workflow: Set env variables
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/codecov.yml(1 hunks).github/workflows/build-and-test.yml(0 hunks)README.md(2 hunks)Taskfile.dist.yaml(1 hunks)go.mod(1 hunks)lib/app/tasks.go(3 hunks)lib/app/tasks_test.go(1 hunks)lib/app/workflow.go(5 hunks)lib/app/workflow_test.go(1 hunks)lib/integrations/dotenv_vault/dotenv_vault.go(1 hunks)lib/integrations/dotenv_vault/dotenv_vault_test.go(1 hunks)lib/integrations/infisical/client.go(1 hunks)lib/integrations/infisical/infiscal_test.go(1 hunks)lib/integrations/infisical/infisical.go(1 hunks)lib/integrations/integration.go(1 hunks)lib/types/types.go(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-and-test.yml
🧰 Additional context used
🧬 Code Graph Analysis (6)
lib/app/tasks.go (1)
lib/support/helpers.go (1)
SkippedMessageWithSymbol(16-18)
lib/integrations/integration.go (3)
lib/types/types.go (1)
AppWorkflowContract(44-49)lib/integrations/dotenv_vault/dotenv_vault.go (1)
New(15-17)lib/integrations/infisical/infisical.go (1)
New(17-19)
lib/integrations/infisical/infiscal_test.go (1)
lib/integrations/infisical/client.go (4)
Secret(34-49)NewClient(20-25)CreateSecretRequest(51-63)UpdateSecretRequest(65-73)
lib/app/tasks_test.go (1)
lib/app/tasks.go (1)
Task(15-34)
lib/integrations/dotenv_vault/dotenv_vault_test.go (2)
lib/types/types.go (2)
JavaScriptEngineContract(25-35)AppWorkflowContract(44-49)lib/settings/settings.go (1)
Settings(5-16)
lib/integrations/dotenv_vault/dotenv_vault.go (2)
lib/types/types.go (1)
AppWorkflowContract(44-49)lib/utils/utils.go (3)
ArrayContains(245-267)IsFile(159-166)WorkingDir(85-95)
🔇 Additional comments (29)
.github/codecov.yml (1)
1-4: Validate coverage precision and range settings
Theprecision: 2andrange: "70...100"look reasonable, but please confirm they align with your current coverage goals and reporting needs.Taskfile.dist.yaml (1)
96-97: LGTM: Improved checksum generation process.The changes make the checksum generation more explicit and cleaner by directly targeting the yaml files and removing path prefixes from the output. This enhances the template integrity checking process.
go.mod (1)
40-40: LGTM: Standard indirect dependency addition.The addition of
github.com/stretchr/objx v0.5.0as an indirect dependency is appropriate for supporting the new integrations functionality.lib/types/types.go (1)
48-48: LGTM: Clean interface extension for integrations.The addition of
GetEnvSection() []stringmethod to theAppWorkflowContractinterface is well-designed and follows Go conventions. It provides appropriate access to environment configuration for the new integrations functionality.lib/app/tasks.go (3)
56-68: LGTM: Method exported for better testability.Converting
canRunOnCurrentPlatformtoCanRunOnCurrentPlatform(exported) is a good improvement that enables direct testing of this platform compatibility logic.
70-76: LGTM: Method exported for better testability.Converting
canRunConditionallytoCanRunConditionally(exported) enables direct testing of the conditional execution logic, improving test coverage.
166-174: LGTM: Consistent usage of exported methods.The calls to the newly exported methods are updated correctly and maintain the existing functionality while enabling better testability.
lib/integrations/integration.go (2)
9-13: Well-designed integration interface.The
Integrationinterface follows good design principles with a clean, focused contract. The three methods provide a clear separation of concerns: identification (Name), conditional execution (IsEnabled), and the main functionality (Run).
15-23: Effective factory pattern with dependency injection.The
Listfunction implements a clean factory pattern that:
- Uses dependency injection via the
getWorkflowfunction parameter, enabling flexible testing- Returns integrations keyed by their names for easy lookup
- Centralizes integration instantiation logic
This design makes the integration system extensible and testable.
README.md (2)
44-44: Documentation properly updated.The Infisical integration is correctly added to the table of contents in the appropriate section.
503-519: Comprehensive integration documentation.The Infisical integration documentation is well-structured and includes:
- Clear environment variable requirements (
INFISICAL_API_TOKEN)- Practical configuration examples with the
infisical://workspace-id:environmentsyntax- Important security note about service token access scope
- Consistent formatting that matches other integration documentation
This provides users with all the necessary information to configure and use the integration effectively.
lib/app/workflow.go (7)
5-5: Import addition is justified.The
fmtimport is correctly added to support the new error logging functionality in the integration execution loop.
16-16: Integration package import correctly added.The integration package import enables the workflow to use the new integration system.
45-45: Integrations field properly added to workflow struct.The
Integrationsfield uses the correct type (map[string]integrations.Integration) to store integration instances keyed by their names, facilitating easy lookup and management.
53-67: Well-implemented workflow initialization.The
CreateWorkflowfunction properly:
- Initializes the
Integrationsmap as an empty map before population- Uses the new
AsContractmethod for dependency injection- Calls
integrations.Listto populate the integrations mapThis follows good initialization patterns and maintains clean separation of concerns.
69-71: Clean interface adapter method.The
AsContractmethod provides a clean way to convert the workflow to its contract interface, supporting dependency injection and interface segregation principles.
97-99: Method correctly implements interface contract.The
GetEnvSectionmethod satisfies theAppWorkflowContractinterface requirement and provides access to the workflow's environment configuration.
129-137: Improved integration execution with proper error handling.The new integration execution loop:
- Iterates through all available integrations
- Checks if each integration is enabled before running
- Includes proper error handling with descriptive error messages
- Replaces the previous single dotenv-vault call with a more extensible approach
This design makes the system more modular and provides better visibility into integration failures.
lib/app/workflow_test.go (3)
12-25: Comprehensive workflow creation test.The
TestCreateWorkflowfunction properly verifies that all workflow fields are initialized, including the newIntegrationsfield. This ensures the refactoring doesn't break existing functionality and that new fields are properly set up.
27-32: Contract conversion test covers new functionality.The
TestAsContracttest verifies that the newAsContractmethod works correctly, ensuring the workflow can be properly converted to its interface contract for dependency injection.
34-50: Thorough task lookup test coverage.The
TestFindTaskByIdfunction provides comprehensive coverage with:
- Positive case: finding an existing task
- Negative case: attempting to find a non-existent task
- Type assertion verification for the returned interface
This ensures the task lookup functionality works correctly after the interface changes.
lib/app/tasks_test.go (2)
21-66: Excellent test coverage for GetDisplayName method.The test cases comprehensively cover the priority logic: Include (with URL processing) → Name → Id → Uuid → empty string. The test scenarios are well-structured and validate the expected fallback behavior.
68-86: Good platform compatibility test coverage.The tests properly cover all edge cases including nil/empty platforms list and case-sensitive platform matching using
runtime.GOOS.lib/integrations/dotenv_vault/dotenv_vault.go (3)
11-17: Well-designed integration structure.The use of dependency injection with the workflow function parameter enables clean testing and loose coupling. The constructor pattern is appropriate for this integration.
23-25: Efficient integration enablement check.The implementation correctly uses
utils.ArrayContainsto check for the "dotenv://vault" marker in the environment section, providing a clean way to enable/disable the integration.
27-46: Robust Run method implementation.The method has good defensive programming with early returns when disabled or when the vault file doesn't exist. Error handling from
godotenvvault.Read()is properly propagated, and environment variables are correctly set usingos.Setenv.lib/integrations/dotenv_vault/dotenv_vault_test.go (3)
80-84: Simple and effective Name method test.The test correctly verifies that the integration returns the expected name "dotenv-vault".
87-104: Good IsEnabled method test coverage.The tests properly verify both enabled and disabled states by checking the presence of "dotenv://vault" in the environment section.
30-43: Appropriate use of panic for unimplemented mock methods.Using
panic("unimplemented")for unused interface methods in test mocks is acceptable since these methods aren't exercised in the current tests. This follows the principle of implementing only what's needed for testing.
| patch: | ||
| default: | ||
| target: 30% | ||
| threshold: 30% |
There was a problem hiding this comment.
Quote patch-level threshold to avoid YAML parsing errors
Similarly, the unquoted 30% can break the YAML loader. Please wrap it in quotes.
--- a/.github/codecov.yml
+++ b/.github/codecov.yml
@@ -12,7 +12,7 @@ status:
default:
target: 30%
- threshold: 30%
+ threshold: "30%"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patch: | |
| default: | |
| target: 30% | |
| threshold: 30% | |
| patch: | |
| default: | |
| target: 30% | |
| threshold: "30%" |
🤖 Prompt for AI Agents
In .github/codecov.yml around lines 10 to 13, the patch-level threshold value
30% is unquoted, which can cause YAML parsing errors. To fix this, wrap the
threshold value 30% in double quotes to ensure it is parsed correctly as a
string.
| status: | ||
| project: | ||
| default: | ||
| target: auto | ||
| threshold: 15% |
There was a problem hiding this comment.
Quote project-level threshold to avoid YAML parsing errors
Unquoted 15% may not be parsed correctly by YAML. Wrap it in quotes to ensure it’s interpreted as a string.
Apply this diff:
--- a/.github/codecov.yml
+++ b/.github/codecov.yml
@@ -8,7 +8,7 @@ status:
default:
target: auto
- threshold: 15%
+ threshold: "15%"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status: | |
| project: | |
| default: | |
| target: auto | |
| threshold: 15% | |
| status: | |
| project: | |
| default: | |
| target: auto | |
| threshold: "15%" |
🤖 Prompt for AI Agents
In .github/codecov.yml around lines 5 to 9, the project-level threshold value
15% is unquoted, which can cause YAML parsing errors. Fix this by wrapping the
threshold value in quotes, changing it from 15% to "15%".
| // func TestCanRunConditionally(t *testing.T) { | ||
| // assert := assert.New(t) | ||
|
|
||
| // // Test case: When If field is empty | ||
| // task1 := &app.Task{If: ""} | ||
| // assert.True(task1.CanRunConditionally()) | ||
|
|
||
| // // Test case: When JsEngine.Evaluate returns true | ||
| // getJsEngine := func(mockEval func(script string) interface{}) interface{} { | ||
| // engine := &MockJsEngine{ | ||
| // MockEvaluate: mockEval, | ||
| // } | ||
|
|
||
| // return engine | ||
| // } | ||
|
|
||
| // jsengine2 := getJsEngine(func(script string) interface{} { | ||
| // return true | ||
| // }) | ||
|
|
||
| // task2 := &app.Task{ | ||
| // If: "{{ some condition }}", | ||
| // JsEngine: jsengine2.(*scripting.JavaScriptEngine), | ||
| // } | ||
| // assert.True(task2.CanRunConditionally()) | ||
|
|
||
| // // Test case: When JsEngine.Evaluate returns false | ||
| // jsengine3 := getJsEngine(func(script string) interface{} { | ||
| // return false | ||
| // }) | ||
| // task3 := &app.Task{ | ||
| // If: "some condition", | ||
| // JsEngine: jsengine3.(*scripting.JavaScriptEngine), | ||
| // } | ||
| // assert.False(task3.CanRunConditionally()) | ||
| // } |
There was a problem hiding this comment.
Fix type casting issue in commented test.
The commented test has a type casting issue on line 110 that would prevent compilation. The getJsEngine function returns interface{} but tries to cast to *scripting.JavaScriptEngine.
If you plan to uncomment this test, fix the type issue:
-// task2 := &app.Task{
-// If: "{{ some condition }}",
-// JsEngine: jsengine2.(*scripting.JavaScriptEngine),
-// }
+// task2 := &app.Task{
+// If: "{{ some condition }}",
+// JsEngine: jsengine2.(*MockJsEngine),
+// }However, note that this would still not work since JsEngine expects *scripting.JavaScriptEngine, not *MockJsEngine. Consider creating a proper interface or adjusting the test design.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/app/tasks_test.go between lines 88 and 123, the commented test has a type
casting issue where getJsEngine returns interface{} but the code casts it to
*scripting.JavaScriptEngine, which is incorrect since the returned object is
actually *MockJsEngine. To fix this, either change getJsEngine to return
*scripting.JavaScriptEngine or adjust the test to use an interface that both
*MockJsEngine and *scripting.JavaScriptEngine implement, then assign JsEngine to
that interface type. This will resolve the type mismatch and allow the test to
compile and run correctly.
| type MockJsEngine struct { | ||
| MockEvaluate func(script string) interface{} | ||
| mock.Mock | ||
| } | ||
|
|
||
| func (m *MockJsEngine) Evaluate(script string) interface{} { | ||
| return m.MockEvaluate(script) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix MockJsEngine implementation inconsistency.
The MockJsEngine struct embeds mock.Mock but uses a custom MockEvaluate function field instead of leveraging the mock framework properly. This creates confusion and doesn't follow testify best practices.
Choose one of these approaches:
Option 1: Use testify mock framework properly:
type MockJsEngine struct {
- MockEvaluate func(script string) interface{}
mock.Mock
}
func (m *MockJsEngine) Evaluate(script string) interface{} {
- return m.MockEvaluate(script)
+ args := m.Called(script)
+ return args.Get(0)
}Option 2: Use simple function field approach:
type MockJsEngine struct {
MockEvaluate func(script string) interface{}
- mock.Mock
}🤖 Prompt for AI Agents
In lib/app/tasks_test.go around lines 12 to 19, the MockJsEngine struct
inconsistently mixes testify's mock.Mock embedding with a custom MockEvaluate
function field, which is confusing and not aligned with testify best practices.
To fix this, either remove the MockEvaluate field and implement Evaluate by
calling m.Called(script).Get(0) to use testify's mocking properly, or remove the
mock.Mock embedding and keep only the function field for a simpler approach.
Choose one consistent mocking strategy and update the Evaluate method
accordingly.
| func TestGetSecrets(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| secrets := []Secret{ | ||
| {Name: "test-secret", Value: "test-value"}, | ||
| } | ||
| json.NewEncoder(w).Encode(map[string][]Secret{"secrets": secrets}) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := NewClient(server.URL) | ||
| secrets, err := client.GetSecrets("workspaceId", "environment", "path", false) | ||
| if err != nil { | ||
| t.Fatalf("Expected no error, got %v", err) | ||
| } | ||
|
|
||
| if len(secrets) != 1 { | ||
| t.Fatalf("Expected 1 secret, got %d", len(secrets)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good basic test structure for GetSecrets method.
The test properly mocks the HTTP server and validates basic JSON response parsing. The test structure is clean and follows good testing practices.
Consider adding error case testing:
func TestGetSecretsError(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
}))
defer server.Close()
client := NewClient(server.URL)
_, err := client.GetSecrets("workspaceId", "environment", "path", false)
if err == nil {
t.Fatal("Expected error for 500 status code")
}
}🤖 Prompt for AI Agents
In lib/integrations/infisical/infiscal_test.go around lines 11 to 29, the
existing TestGetSecrets function tests the successful case but lacks coverage
for error scenarios. Add a new test function named TestGetSecretsError that
mocks the HTTP server to return a 500 Internal Server Error status. In this
test, call client.GetSecrets and assert that it returns an error, failing the
test if no error is returned. This will ensure error handling paths are properly
tested.
| // body := make([]byte, 81920) | ||
| // resp.Body.Read(body) | ||
| // fmt.Printf("resp.Body: %v\n", string(body)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented debug code.
Debug code should not be included in production code.
- // body := make([]byte, 81920)
- // resp.Body.Read(body)
- // fmt.Printf("resp.Body: %v\n", string(body))
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // body := make([]byte, 81920) | |
| // resp.Body.Read(body) | |
| // fmt.Printf("resp.Body: %v\n", string(body)) |
🤖 Prompt for AI Agents
In lib/integrations/infisical/client.go around lines 166 to 169, remove the
commented-out debug code that reads from resp.Body and prints it. This debug
code is not needed in production and should be deleted to keep the code clean.
| func (c *Client) GetSecret(workspaceId, environment, secretName, path string) (Secret, error) { | ||
| endpoint := "/api/v3/secrets/" + secretName + "?workspaceId=" + workspaceId + "&environment=" + environment + "&path=" + path | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Apply URL encoding to prevent injection in other endpoints.
Similar to GetSecrets, other methods that build URLs should use proper encoding.
Run the following script to verify all URL construction patterns in the codebase:
Also applies to: 291-293
🏁 Script executed:
#!/bin/bash
# Description: Find all instances of URL construction to ensure proper encoding is used
# Search for potential URL construction patterns
rg -A 3 'endpoint.*"\/api\/.*\+' --type go
# Search for methods that might construct URLs
ast-grep --pattern 'endpoint := $_ + $_'Length of output: 858
Apply proper URL encoding for all path segments and query parameters
We’re currently concatenating raw IDs and names into endpoints, which can lead to injection or malformed‐URL issues. Please update every URL construction in lib/integrations/infisical/client.go to use Go’s net/url escaping functions:
• lib/integrations/infisical/client.go:
- Line 151 (GetSecrets)
- Line 232 (GetSecret)
- Lines 252 & 272 (DeleteSecret / RestoreSecret)
- Line 292 (UpdateSecret or similar)
Example diff for GetSecret (line 232):
- endpoint := "/api/v3/secrets/" + secretName + "?workspaceId=" + workspaceId + "&environment=" + environment + "&path=" + path
+ // URL‐escape path segments and query values
+ endpoint := fmt.Sprintf(
+ "/api/v3/secrets/%s?workspaceId=%s&environment=%s&path=%s",
+ url.PathEscape(secretName),
+ url.QueryEscape(workspaceId),
+ url.QueryEscape(environment),
+ url.QueryEscape(path),
+ )Repeat this pattern for each method. Alternatively, consider constructing URLs with url.URL and url.Values for cleaner code and automatic encoding.
🤖 Prompt for AI Agents
In lib/integrations/infisical/client.go around lines 231 to 233, the URL for the
GetSecret method is constructed by concatenating raw strings, which risks
injection and malformed URLs. Fix this by applying proper URL encoding to all
path segments and query parameters using Go's net/url package functions such as
url.PathEscape for path elements and url.QueryEscape or url.Values for query
parameters. Repeat this encoding approach for all similar URL constructions in
the file at lines 151, 252, 272, and 292 to ensure consistent and safe URL
building.
| result := secretsResponse.Secrets | ||
| token := os.Getenv("INFISICAL_API_TOKEN") | ||
|
|
||
| lastIndex := strings.LastIndex(token, ".") | ||
| tokenSecret := "" | ||
|
|
||
| if lastIndex != -1 && lastIndex+1 < len(token) { | ||
| tokenSecret = token[lastIndex+1:] | ||
| } | ||
|
|
||
| tokenData, _ := c.GetTokenData(workspaceId, environment) | ||
|
|
||
| secretKey, err := decrypt(tokenData.EncryptedKey, tokenData.Iv, tokenData.Tag, tokenSecret) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, secret := range result { | ||
| if secret.SecretKeyCiphertext != "" { | ||
| plainTextKey, err := decrypt(secret.SecretKeyCiphertext, secret.SecretKeyIV, secret.SecretKeyTag, secretKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| plainTextValue, err := decrypt(secret.SecretValueCiphertext, secret.SecretValueIV, secret.SecretValueTag, secretKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| secret.Name = plainTextKey | ||
| secret.Value = plainTextValue | ||
| } | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Fix mutation of secrets and improve error handling.
The code attempts to mutate secret objects within a range loop, but since Go passes by value, these mutations won't affect the original slice. Additionally, there's no validation of the token format.
Fix the mutation issue and add token validation:
result := secretsResponse.Secrets
token := os.Getenv("INFISICAL_API_TOKEN")
+ if token == "" {
+ return nil, errors.New("INFISICAL_API_TOKEN is not set")
+ }
lastIndex := strings.LastIndex(token, ".")
tokenSecret := ""
if lastIndex != -1 && lastIndex+1 < len(token) {
tokenSecret = token[lastIndex+1:]
+ } else {
+ return nil, errors.New("invalid token format")
}
tokenData, _ := c.GetTokenData(workspaceId, environment)
secretKey, err := decrypt(tokenData.EncryptedKey, tokenData.Iv, tokenData.Tag, tokenSecret)
if err != nil {
return nil, err
}
- for _, secret := range result {
+ for i := range result {
+ secret := &result[i]
if secret.SecretKeyCiphertext != "" {
plainTextKey, err := decrypt(secret.SecretKeyCiphertext, secret.SecretKeyIV, secret.SecretKeyTag, secretKey)
if err != nil {
return nil, err
}
plainTextValue, err := decrypt(secret.SecretValueCiphertext, secret.SecretValueIV, secret.SecretValueTag, secretKey)
if err != nil {
return nil, err
}
secret.Name = plainTextKey
secret.Value = plainTextValue
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result := secretsResponse.Secrets | |
| token := os.Getenv("INFISICAL_API_TOKEN") | |
| lastIndex := strings.LastIndex(token, ".") | |
| tokenSecret := "" | |
| if lastIndex != -1 && lastIndex+1 < len(token) { | |
| tokenSecret = token[lastIndex+1:] | |
| } | |
| tokenData, _ := c.GetTokenData(workspaceId, environment) | |
| secretKey, err := decrypt(tokenData.EncryptedKey, tokenData.Iv, tokenData.Tag, tokenSecret) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for _, secret := range result { | |
| if secret.SecretKeyCiphertext != "" { | |
| plainTextKey, err := decrypt(secret.SecretKeyCiphertext, secret.SecretKeyIV, secret.SecretKeyTag, secretKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| plainTextValue, err := decrypt(secret.SecretValueCiphertext, secret.SecretValueIV, secret.SecretValueTag, secretKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| secret.Name = plainTextKey | |
| secret.Value = plainTextValue | |
| } | |
| } | |
| return result, nil | |
| } | |
| result := secretsResponse.Secrets | |
| token := os.Getenv("INFISICAL_API_TOKEN") | |
| if token == "" { | |
| return nil, errors.New("INFISICAL_API_TOKEN is not set") | |
| } | |
| lastIndex := strings.LastIndex(token, ".") | |
| tokenSecret := "" | |
| if lastIndex != -1 && lastIndex+1 < len(token) { | |
| tokenSecret = token[lastIndex+1:] | |
| } else { | |
| return nil, errors.New("invalid token format") | |
| } | |
| tokenData, _ := c.GetTokenData(workspaceId, environment) | |
| secretKey, err := decrypt(tokenData.EncryptedKey, tokenData.Iv, tokenData.Tag, tokenSecret) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for i := range result { | |
| secret := &result[i] | |
| if secret.SecretKeyCiphertext != "" { | |
| plainTextKey, err := decrypt(secret.SecretKeyCiphertext, secret.SecretKeyIV, secret.SecretKeyTag, secretKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| plainTextValue, err := decrypt(secret.SecretValueCiphertext, secret.SecretValueIV, secret.SecretValueTag, secretKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| secret.Name = plainTextKey | |
| secret.Value = plainTextValue | |
| } | |
| } | |
| return result, nil | |
| } |
🤖 Prompt for AI Agents
In lib/integrations/infisical/client.go lines 175 to 210, the code mutates
secret objects inside a range loop, but since Go passes loop variables by value,
these changes do not affect the original slice. To fix this, iterate using index
and update the slice elements directly. Also, add validation to ensure the token
is not empty and has the expected format before extracting the tokenSecret.
Return an error if the token is invalid.
| func (c *Client) makeRequest(method, endpoint string, payload interface{}) (*http.Response, error) { | ||
| url := c.APIEndpoint + endpoint | ||
| var req *http.Request | ||
| var err error | ||
|
|
||
| if method == http.MethodGet || method == http.MethodDelete { | ||
| req, err = http.NewRequest(method, url, nil) | ||
| } else { | ||
| jsonPayload, _ := json.Marshal(payload) | ||
| req, err = http.NewRequest(method, url, bytes.NewBuffer(jsonPayload)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| } | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| token := os.Getenv("INFISICAL_API_TOKEN") | ||
| apiKey := os.Getenv("INFISICAL_API_KEY") | ||
|
|
||
| req.Header.Set("User-Agent", "StackUp-cli/v1") | ||
|
|
||
| if token != "" { | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| } | ||
|
|
||
| if apiKey != "" { | ||
| req.Header.Del("Authorization") | ||
| req.Header.Set("X-API-KEY", apiKey) | ||
| } | ||
|
|
||
| if apiKey == "" && token == "" { | ||
| return nil, errors.New("INFISICAL_API_KEY or INFISICAL_API_TOKEN environment variable must be set") | ||
| } | ||
|
|
||
| return c.HTTPClient.Do(req) | ||
| } |
There was a problem hiding this comment.
Improve error handling in makeRequest method.
Several issues need attention:
- The error from
json.Marshalis ignored - Authentication validation should happen at the beginning
- Setting API key header removes Authorization header unnecessarily
Apply these improvements:
func (c *Client) makeRequest(method, endpoint string, payload interface{}) (*http.Response, error) {
+ token := os.Getenv("INFISICAL_API_TOKEN")
+ apiKey := os.Getenv("INFISICAL_API_KEY")
+
+ if apiKey == "" && token == "" {
+ return nil, errors.New("INFISICAL_API_KEY or INFISICAL_API_TOKEN environment variable must be set")
+ }
+
url := c.APIEndpoint + endpoint
var req *http.Request
var err error
if method == http.MethodGet || method == http.MethodDelete {
req, err = http.NewRequest(method, url, nil)
} else {
- jsonPayload, _ := json.Marshal(payload)
+ jsonPayload, err := json.Marshal(payload)
+ if err != nil {
+ return nil, err
+ }
req, err = http.NewRequest(method, url, bytes.NewBuffer(jsonPayload))
req.Header.Set("Content-Type", "application/json")
}
if err != nil {
return nil, err
}
- token := os.Getenv("INFISICAL_API_TOKEN")
- apiKey := os.Getenv("INFISICAL_API_KEY")
-
req.Header.Set("User-Agent", "StackUp-cli/v1")
- if token != "" {
- req.Header.Set("Authorization", "Bearer "+token)
- }
-
if apiKey != "" {
- req.Header.Del("Authorization")
req.Header.Set("X-API-KEY", apiKey)
- }
-
- if apiKey == "" && token == "" {
- return nil, errors.New("INFISICAL_API_KEY or INFISICAL_API_TOKEN environment variable must be set")
+ } else if token != "" {
+ req.Header.Set("Authorization", "Bearer "+token)
}
return c.HTTPClient.Do(req)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *Client) makeRequest(method, endpoint string, payload interface{}) (*http.Response, error) { | |
| url := c.APIEndpoint + endpoint | |
| var req *http.Request | |
| var err error | |
| if method == http.MethodGet || method == http.MethodDelete { | |
| req, err = http.NewRequest(method, url, nil) | |
| } else { | |
| jsonPayload, _ := json.Marshal(payload) | |
| req, err = http.NewRequest(method, url, bytes.NewBuffer(jsonPayload)) | |
| req.Header.Set("Content-Type", "application/json") | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| token := os.Getenv("INFISICAL_API_TOKEN") | |
| apiKey := os.Getenv("INFISICAL_API_KEY") | |
| req.Header.Set("User-Agent", "StackUp-cli/v1") | |
| if token != "" { | |
| req.Header.Set("Authorization", "Bearer "+token) | |
| } | |
| if apiKey != "" { | |
| req.Header.Del("Authorization") | |
| req.Header.Set("X-API-KEY", apiKey) | |
| } | |
| if apiKey == "" && token == "" { | |
| return nil, errors.New("INFISICAL_API_KEY or INFISICAL_API_TOKEN environment variable must be set") | |
| } | |
| return c.HTTPClient.Do(req) | |
| } | |
| func (c *Client) makeRequest(method, endpoint string, payload interface{}) (*http.Response, error) { | |
| token := os.Getenv("INFISICAL_API_TOKEN") | |
| apiKey := os.Getenv("INFISICAL_API_KEY") | |
| if apiKey == "" && token == "" { | |
| return nil, errors.New("INFISICAL_API_KEY or INFISICAL_API_TOKEN environment variable must be set") | |
| } | |
| url := c.APIEndpoint + endpoint | |
| var req *http.Request | |
| var err error | |
| if method == http.MethodGet || method == http.MethodDelete { | |
| req, err = http.NewRequest(method, url, nil) | |
| } else { | |
| jsonPayload, err := json.Marshal(payload) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req, err = http.NewRequest(method, url, bytes.NewBuffer(jsonPayload)) | |
| req.Header.Set("Content-Type", "application/json") | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("User-Agent", "StackUp-cli/v1") | |
| if apiKey != "" { | |
| req.Header.Set("X-API-KEY", apiKey) | |
| } else if token != "" { | |
| req.Header.Set("Authorization", "Bearer "+token) | |
| } | |
| return c.HTTPClient.Do(req) | |
| } |
🤖 Prompt for AI Agents
In lib/integrations/infisical/client.go lines 75 to 111, improve error handling
in makeRequest by first validating that either INFISICAL_API_KEY or
INFISICAL_API_TOKEN is set before creating the request. Then, capture and handle
the error returned by json.Marshal instead of ignoring it. Finally, when setting
the API key header, do not remove the Authorization header unnecessarily;
instead, set headers based on which credential is present without deleting
others arbitrarily.
| func (c *Client) GetSecrets(workspaceId, environment, path string, includeImports bool) ([]Secret, error) { | ||
| endpoint := "/api/v3/secrets?workspaceId=" + workspaceId + "&environment=" + environment + "&path=" + path | ||
| if includeImports { | ||
| endpoint += "&include_imports=true" | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix potential URL injection vulnerability.
Building URLs by string concatenation without proper encoding can lead to URL injection attacks.
Use URL encoding to prevent injection:
+import (
+ "net/url"
+ // ... other imports
+)
+
func (c *Client) GetSecrets(workspaceId, environment, path string, includeImports bool) ([]Secret, error) {
- endpoint := "/api/v3/secrets?workspaceId=" + workspaceId + "&environment=" + environment + "&path=" + path
+ params := url.Values{}
+ params.Add("workspaceId", workspaceId)
+ params.Add("environment", environment)
+ params.Add("path", path)
if includeImports {
- endpoint += "&include_imports=true"
+ params.Add("include_imports", "true")
}
+ endpoint := "/api/v3/secrets?" + params.Encode()🤖 Prompt for AI Agents
In lib/integrations/infisical/client.go around lines 150 to 155, the URL is
constructed by concatenating query parameters directly, which risks URL
injection. Fix this by using proper URL encoding for the workspaceId,
environment, and path parameters before appending them to the endpoint string.
Use a URL encoding function to safely encode these values to prevent injection
vulnerabilities.
This PR refactors the integrations functionality:
Summary by CodeRabbit
.env.vaultfile.