feat: (validation) check connector config payload more thoroughly before inserting into the DB #251
Conversation
WalkthroughThe pull request introduces changes to the connector configuration update and installation processes across multiple files. The primary modifications include adding a maximum body size limit using Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #251 +/- ##
==========================================
+ Coverage 67.59% 67.70% +0.11%
==========================================
Files 539 539
Lines 26937 26980 +43
==========================================
+ Hits 18207 18268 +61
+ Misses 7556 7542 -14
+ Partials 1174 1170 -4 ☔ View full report in Codecov by Sentry. |
laouji
force-pushed
the
fix/check-connector-config-payload
branch
from
0783973 to
b169fbc
Compare
|
@coderabbitai review |
✅ Actions performedReview 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: 2
🧹 Nitpick comments (8)
internal/connectors/engine/plugins/plugin.go (2)
15-19: Consider definingCalleras a custom type for stronger type safetyCurrently,
CallerWorkerandCallerEngineare defined as string constants. To enhance type safety and reduce the risk of typos, consider defining a customCallertype.Apply this diff to define a custom
Callertype:const ( - CallerWorker = "worker" - CallerEngine = "engine" +type Caller string +const ( + CallerWorker Caller = "worker" + CallerEngine Caller = "engine" +) )
42-43: Updatecallerfield and parameter to use the customCallertypeAfter defining a custom
Callertype, update thecallerfield in thepluginsstruct and theNewfunction parameter to use this type.Apply this diff to update the
callerfield and constructor:type plugins struct { logger logging.Logger plugins map[string]pluginInformation rwMutex sync.RWMutex - caller string + caller Caller debug bool } func New( - caller string, + caller Caller, logger logging.Logger, debug bool, ) *plugins { return &plugins{ caller: caller, logger: logger, plugins: make(map[string]pluginInformation), debug: debug, } }Also applies to: 52-52, 57-57
internal/api/v3/handler_connectors_install.go (1)
13-13: Consider making the size limit configurableWhile 500KB is a reasonable default, consider making
connectorConfigMaxBytesconfigurable via environment variables to allow for flexibility in different deployment scenarios.internal/api/v3/utils.go (2)
15-17: Consider adding JSON validation tagsThe
TestDatastruct could benefit from additional JSON validation tags to ensure test data better represents real-world scenarios.type TestData struct { - Val string `json:"val"` + Val string `json:"val" validate:"required,max=255"` }
19-27: Improve test data generationThe current implementation has several potential improvements:
- Consider calculating the exact size needed to exceed
connectorConfigMaxBytesinstead of using a fixed 1M iterations- Add a comment explaining the German word choice
- Consider making the function more flexible by accepting a target size parameter
-func oversizeRequestBody() []TestData { +// oversizeRequestBody generates a payload that exceeds connectorConfigMaxBytes +// using a long German compound word as test data +func oversizeRequestBody(targetSizeBytes ...int64) []TestData { + targetSize := connectorConfigMaxBytes + if len(targetSizeBytes) > 0 { + targetSize = targetSizeBytes[0] + } var data []TestData - for i := 0; i < 1000000; i++ { + // Calculate iterations needed to exceed target size + bytesPerEntry := len(`{"val":"Rindfleischetikettierungsüberwachungsaufgabenübertragungsgesetz 0"}`) + iterations := (targetSize / int64(bytesPerEntry)) + 1 + + for i := 0; i < int(iterations); i++ { data = append(data, TestData{ Val: fmt.Sprintf("Rindfleischetikettierungsüberwachungsaufgabenübertragungsgesetz %d", i), }) } return data }internal/api/v3/handler_connectors_install_test.go (1)
47-52: Consider adding edge case testsWhile the current test verifies the oversized request case, consider adding tests for:
- Request body exactly at the size limit
- Request body slightly under the limit
- Empty request body
Example test case:
It("should accept request body at size limit", func(ctx SpecContext) { data := oversizeRequestBody(connectorConfigMaxBytes - 1) // Generate payload just under limit m.EXPECT().ConnectorsInstall(gomock.Any(), conn, gomock.Any()).Return( models.ConnectorID{}, nil, ) handlerFn(w, prepareJSONRequestWithQuery(http.MethodPost, "connectorID", conn, &data)) assertExpectedResponse(w.Result(), http.StatusAccepted, "data") })test/e2e/api_connectors_test.go (1)
86-96: LGTM! Good test coverage for invalid configurations.The test table effectively validates error handling for invalid plugin configurations across both API versions.
Consider adding more test cases for other validation scenarios, such as:
- Invalid polling period format
- Invalid page size values
- Missing required fields
internal/connectors/engine/engine.go (1)
31-32: Enhance method documentation.While the method signature is correct, consider expanding the documentation comment to describe validation behavior, potential errors, and when this method should be used.
- // Update a connector with the given configuration. + // UpdateConnector updates the configuration of an existing connector. + // It validates the raw configuration against the connector's schema, + // registers the updated plugin configuration, and persists the changes. + // Returns ErrValidation if the configuration is invalid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
internal/api/backend/backend.go(1 hunks)internal/api/backend/backend_generated.go(1 hunks)internal/api/services/connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update_test.go(1 hunks)internal/api/v3/handler_connectors_install.go(1 hunks)internal/api/v3/handler_connectors_install_test.go(1 hunks)internal/api/v3/utils.go(2 hunks)internal/connectors/engine/engine.go(6 hunks)internal/connectors/engine/module.go(1 hunks)internal/connectors/engine/plugins/plugin.go(5 hunks)internal/worker/module.go(1 hunks)test/e2e/api_connectors_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Default
internal/api/v3/handler_connectors_config_update_test.go
[error] 73-73: Not enough arguments in call to m.EXPECT().ConnectorsConfigUpdate: have (gomock.Matcher, gomock.Matcher), want (any, any, any)
[error] 81-81: Not enough arguments in call to m.EXPECT().ConnectorsConfigUpdate: have (gomock.Matcher, models.Connector), want (any, any, any)
🔇 Additional comments (13)
internal/api/services/connectors_config_update.go (1)
10-13: Ensure error handling is consistent across the service layerThe function
ConnectorsConfigUpdatenow useshandleEngineErrors(err)for error handling. Verify that this approach is consistent with other service methods and thathandleEngineErrorsappropriately maps engine errors to service-level errors.internal/connectors/engine/plugins/plugin.go (1)
113-116: 🛠️ Refactor suggestionReview access restrictions based on
callercontextThe methods
GetandGetConfignow restrict access based on thecallercontext, returningErrInvalidOperationwhenp.caller != CallerWorker. Ensure that this restriction aligns with the overall design and that no necessary functionality is inadvertently blocked. Confirm that all components interacting with these methods are aware of this change and handleErrInvalidOperationappropriately.If this is intentional, no action is needed. Otherwise, consider whether the engine should have access to these methods or if the access control logic needs adjustment.
Also applies to: 129-132
internal/connectors/engine/module.go (1)
15-17:⚠️ Potential issuePotential access issues due to caller context in
pluginsIn the
Modulefunction, thepluginsinstance is initialized withCallerEngine. Given thatplugins.Getandplugins.GetConfigmethods returnErrInvalidOperationwhen the caller is notCallerWorker, the engine may be unable to access these methods. This could lead to functionality issues if the engine needs to interact with these methods.Please verify if the engine requires access to
GetorGetConfig. If so, consider modifying the access control inpluginsor reassessing the caller contexts to ensure the engine can perform necessary operations.Also applies to: 22-24
internal/api/v3/handler_connectors_install.go (1)
20-29: LGTM! Good error handling for oversized requestsThe implementation correctly uses
http.MaxBytesReaderand properly handles theMaxBytesErrorcase with a specific HTTP 413 status code.internal/api/v3/handler_connectors_config_update.go (1)
27-34: LGTM! Consistent error handling with install handlerThe implementation maintains consistency with the install handler by using the same size limit and error handling pattern.
internal/api/v3/handler_connectors_config_update_test.go (1)
65-70: LGTM! Good test coverage for request body size validation.The test case properly validates that oversized request bodies are rejected with the correct status code and error message.
internal/worker/module.go (1)
48-48: LGTM! Enhanced type safety with explicit caller type.The addition of
plugins.CallerWorkerparameter improves type safety by explicitly specifying the caller type.internal/api/backend/backend.go (1)
36-36: LGTM! Improved method signature with better separation of concerns.The updated signature separating
connectorIDandrawConfigallows for more granular validation and better error handling.test/e2e/api_connectors_test.go (1)
129-135: LGTM! Proper validation testing for config updates.The test case properly verifies that invalid configurations are rejected during updates.
internal/api/backend/backend_generated.go (1)
198-208: LGTM - Generated mock implementation looks correct.The updated method signatures in the mock implementation correctly reflect the interface changes, separating the connector ID and raw config parameters for better validation control.
internal/connectors/engine/engine.go (3)
78-82: Well-documented field addition.The new plugins field is clearly documented with its purpose and scope limitations, which helps future maintainers understand its role in validation.
92-92: LGTM - Constructor updated appropriately.The constructor properly initializes the new plugins field while maintaining a logical parameter order.
Also applies to: 99-99
311-348: 🛠️ Refactor suggestion
⚠️ Potential issueFix CreatedAt timestamp handling in connector updates.
The implementation sets CreatedAt to the current time during updates, which would overwrite the original creation timestamp.
- CreatedAt: time.Now().UTC(), + // CreatedAt should be preserved during updatesConsider adding context cancellation handling.
Unlike other methods in the engine, this implementation doesn't use a detached context which could lead to incomplete updates if the request is cancelled.
func (e *engine) UpdateConnector(ctx context.Context, connectorID models.ConnectorID, rawConfig json.RawMessage) error { ctx, span := otel.Tracer().Start(ctx, "engine.UpdateConnector") defer span.End() + // Detach context to avoid incomplete updates on request cancellation + detachedCtx := context.WithoutCancel(ctx) + e.wg.Add(1) + defer e.wg.Done() + config := models.DefaultConfig() - if err := json.Unmarshal(rawConfig, &config); err != nil { + if err := json.Unmarshal(rawConfig, &config); err != nil { otel.RecordError(span, err) return err }Consider adding validation for connector existence.
The implementation should verify that the connector exists before attempting to update its configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/api/v3/handler_connectors_config_update_test.go (2)
58-63: Consider using a more specific error code for oversized request.While the test logic is correct, the error code "MISSING_OR_INVALID_BODY" could be more specific for the oversized request case. Consider using something like "REQUEST_TOO_LARGE" to better indicate the actual error condition.
66-66: Make mock expectations more specific.Using
gomock.Any()for all parameters makes the test less precise. Consider making the test more specific by matching at least the connector ID:-m.EXPECT().ConnectorsConfigUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return( +m.EXPECT().ConnectorsConfigUpdate(gomock.Any(), connectorID, gomock.Any()).Return(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/api/v3/handler_connectors_config_update_test.go(1 hunks)
🔇 Additional comments (1)
internal/api/v3/handler_connectors_config_update_test.go (1)
74-74: LGTM! Good use of specific mock expectations.The mock expectation correctly uses
connector.IDinstead ofgomock.Any(), making the test more precise and meaningful.
| @@ -10,14 +10,21 @@ import ( | |||
| "go.opentelemetry.io/otel/attribute" | |||
| ) | |||
|
|
|||
| var connectorConfigMaxBytes int64 = 500000 | |||
There was a problem hiding this comment.
Should we have this one as configurable ?
There was a problem hiding this comment.
Maybe. But I figure it's kind of an edge case to need to accept anything bigger since right now we don't have any connectors that have configuration payloads big enough that they could conceivably exceed 500Kb.
If we find that we have pods crashing with even lower thresholds we can revise this in the future.
| @@ -32,7 +39,8 @@ type plugins struct { | |||
| plugins map[string]pluginInformation | |||
| rwMutex sync.RWMutex | |||
|
|
|||
| debug bool | |||
| caller string | |||
There was a problem hiding this comment.
Instead of an enum, should we have interface or build tags? (in future code)
There was a problem hiding this comment.
I thought about this but since the plugins constructors are only called from the fx.Modules it doesn't really make much of a difference whether it's embedded directly into the logic of the module itself.
laouji
force-pushed
the
fix/check-connector-config-payload
branch
from
901e4db to
81824f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/api/v3/handler_connectors_config_update.go (1)
31-34: Consider using a more specific error message for size limit errors.While the error handling is correct, consider using a more specific error message for the size limit case instead of the generic
ErrMissingOrInvalidBody. This would help clients better understand why their request was rejected.- api.WriteErrorResponse(w, http.StatusRequestEntityTooLarge, ErrMissingOrInvalidBody, err) + api.WriteErrorResponse(w, http.StatusRequestEntityTooLarge, "REQUEST_BODY_TOO_LARGE", err)internal/api/v3/handler_connectors_config_update_test.go (2)
66-67: Consider using more specific mock matchers.Instead of using
gomock.Any()for all parameters, consider using more specific matchers to ensure the correct values are being passed to the backend.- m.EXPECT().ConnectorsConfigUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return( + m.EXPECT().ConnectorsConfigUpdate( + gomock.Any(), + gomock.Eq(connectorID), + gomock.Any(), + ).Return(
74-76: LGTM! Consider matching the expected config.The test case correctly verifies the success scenario. Consider using
gomock.Eq(config)for the third parameter to ensure the exact config is being passed to the backend.internal/connectors/engine/engine.go (1)
344-348: Add transaction for atomic updatesConsider wrapping the storage update in a transaction to ensure atomicity with plugin registration.
Consider applying this pattern:
+ tx, err := e.storage.BeginTx(ctx) + if err != nil { + return err + } + defer tx.Rollback() + if err := e.storage.ConnectorsConfigUpdate(ctx, connector); err != nil { otel.RecordError(span, err) return err } + + return tx.Commit()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
internal/api/backend/backend.go(1 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connectors_config_update.go(1 hunks)internal/api/services/connectors_config_update_test.go(3 hunks)internal/api/v3/handler_connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update_test.go(1 hunks)internal/api/v3/handler_connectors_install.go(1 hunks)internal/api/v3/handler_connectors_install_test.go(1 hunks)internal/api/v3/utils.go(2 hunks)internal/connectors/engine/engine.go(6 hunks)internal/connectors/engine/engine_generated.go(2 hunks)internal/connectors/engine/module.go(1 hunks)internal/connectors/engine/plugins/plugin.go(5 hunks)internal/worker/module.go(1 hunks)test/e2e/api_connectors_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/api/v3/utils.go
- internal/api/v3/handler_connectors_install_test.go
- internal/connectors/engine/module.go
- internal/worker/module.go
- internal/api/v3/handler_connectors_install.go
- test/e2e/api_connectors_test.go
- internal/connectors/engine/plugins/plugin.go
- internal/api/backend/backend.go
- internal/api/backend/backend_generated.go
- internal/api/services/connectors_config_update.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (9)
internal/api/v3/handler_connectors_config_update.go (2)
40-40: LGTM! Backend call changes align with validation objectives.The updated signature passing raw config to the backend allows for more thorough validation before DB insertion, which aligns well with the PR objectives.
27-28: Verify the maximum request body size limit.The implementation of
MaxBytesReaderis a good security measure. However, let's verify the value ofconnectorConfigMaxBytesto ensure it's appropriate for your use case.✅ Verification successful
Maximum request body size limit is appropriate
The
connectorConfigMaxByteslimit is set to 500KB, which provides a good balance between accommodating complex configurations while preventing potential memory exhaustion attacks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the connectorConfigMaxBytes constant definition rg -A 1 "connectorConfigMaxBytes.*=.*"Length of output: 176
internal/api/v3/handler_connectors_config_update_test.go (1)
58-63: Verify the oversizeRequestBody helper function implementation.The test case is well structured. Let's verify the implementation of
oversizeRequestBody()to ensure it generates appropriate test data.✅ Verification successful
Helper function implementation is appropriate for size limit testing
The
oversizeRequestBody()function generates an array of 1 million entries, each containing a long string, which produces a payload well over common HTTP body size limits (approximately 67MB). This is suitable for verifying request size validation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the oversizeRequestBody function implementation ast-grep --pattern 'func oversizeRequestBody() $_ { $$$ }'Length of output: 556
Script:
#!/bin/bash # Find TestData struct definition and size limit constants ast-grep --pattern 'type TestData struct { $$$ }' rg -A 2 'MaxBodySize|RequestBodyLimit|BodyLimit' --type goLength of output: 248
internal/api/services/connectors_config_update_test.go (3)
40-41: LGTM: Error handling update aligns with engine-level validationThe test correctly updates error handling to use
engine.ErrNotFoundandErrNotFound, aligning with the shift from storage-level to engine-level validation.
52-55: LGTM: Mock expectations match new interfaceThe test correctly sets up mock expectations for the new
UpdateConnectorinterface, usingjson.RawMessagefor config and separateconnectorID.
59-59: LGTM: Improved error checkingUsing
errors.Isfor error comparison is more robust than direct equality check, as it properly handles wrapped errors.internal/connectors/engine/engine_generated.go (1)
291-303: LGTM: Generated mock matches interfaceThe generated mock implementation for
UpdateConnectorcorrectly matches the interface signature and includes proper recording functionality.internal/connectors/engine/engine.go (2)
79-82: LGTM: Clear documentation of plugins field purposeGood documentation explaining that plugins field is specifically for config validation before DB insertion.
335-342: LGTM: Proper plugin validation with error wrappingThe implementation correctly:
- Validates plugin config
- Wraps validation errors appropriately
- Handles invalid config cases
| func (e *engine) UpdateConnector(ctx context.Context, connectorID models.ConnectorID, rawConfig json.RawMessage) error { | ||
| ctx, span := otel.Tracer().Start(ctx, "engine.UpdateConnector") | ||
| defer span.End() | ||
|
|
||
| config := models.DefaultConfig() | ||
| if err := json.Unmarshal(rawConfig, &config); err != nil { | ||
| otel.RecordError(span, err) | ||
| return err | ||
| } | ||
|
|
||
| if err := config.Validate(); err != nil { | ||
| otel.RecordError(span, err) | ||
| return errors.Wrap(ErrValidation, err.Error()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in config validation
The validation logic should include size validation before attempting to unmarshal the config.
Apply this diff to add size validation:
func (e *engine) UpdateConnector(ctx context.Context, connectorID models.ConnectorID, rawConfig json.RawMessage) error {
ctx, span := otel.Tracer().Start(ctx, "engine.UpdateConnector")
defer span.End()
+ if len(rawConfig) > maxConfigSize {
+ return errors.Wrap(ErrValidation, "config size exceeds maximum allowed size")
+ }
+
config := models.DefaultConfig()
if err := json.Unmarshal(rawConfig, &config); err != nil {
otel.RecordError(span, err)
return err
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This is already validated in the controller plane.
There was a problem hiding this comment.
@laouji Understood, thanks for the clarification.
✏️ Learnings added
Learnt from: laouji
PR: formancehq/payments#251
File: internal/connectors/engine/engine.go:312-325
Timestamp: 2025-01-09T09:55:14.505Z
Learning: Config size validation for connector configurations is handled in the controller plane, not in the engine layer.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Fixes: ENG-1578