Skip to content
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

Improve FormData Clone Method to Maintain Boundary Consistency and Resolve Signature Verification Issues #2303

Closed
helloliuyiming opened this issue Sep 27, 2024 · 4 comments · Fixed by #2305
Labels
s: feature This issue indicates a feature request

Comments

@helloliuyiming
Copy link
Contributor

Request Statement

While developing with the dio library, I encountered an issue related to FormData cloning. This problem arises in scenarios where request body signing is required.

The core of the issue lies in the clone() method of FormData. The current implementation generates a new boundary during the cloning process, resulting in inconsistent boundaries between the cloned object and the original object. This inconsistency causes problems in certain scenarios, particularly when request body signature verification is needed.

Specifically, my workflow is as follows:

Create a FormData object
Clone the object
Use the readAsBytes() method of the cloned object to generate a request signature
However, because the cloned object's boundary differs from the original, the generated signature does not match the actual sent data, ultimately leading to signature verification failure.

This issue highlights the importance of maintaining FormData clone consistency in certain use cases. Especially in scenarios involving data integrity and security, ensuring that the cloned object is identical to the original is crucial.

Solution Brainstorm

To address this issue, I've implemented a custom solution using a CustomizableFormData class. This class extends FormData and allows for a custom boundary to be set. Here's the implementation:

class CustomizableFormData extends FormData{

  late String _boundary;

  CustomizableFormData(this._boundary){
    this._boundary = _boundary;
  }

  @override
  String get boundary => _boundary;
}

I've also created a custom method to clone FormData objects while preserving the boundary:

FormData cloneFormData(FormData origin) {
  final clone = CustomizableFormData(origin.boundary);
  // copy from FormData
  clone.fields.addAll(origin.fields);
  for (final file in origin.files) {
    clone.files.add(MapEntry(file.key, file.value.clone()));
  }
  return clone;
}

While the above solution works for my specific use case, I believe a more general solution could be beneficial for the dio library. I propose modifying the clone() method in the FormData class to include an optional parameter for preserving the boundary:

FormData clone({bool cloneBoundary = false})

This would allow users to choose whether they want to maintain the original boundary when cloning a FormData object, providing more flexibility while addressing the issue of signature verification.

I acknowledge that my programming skills may not be at an expert level. However, if the maintainers of the dio library find this proposal valuable, I would be willing to attempt submitting a pull request with the necessary changes.

Lastly, I want to express my sincere gratitude to the maintainers and contributors of the dio library. Your outstanding work has greatly benefited the developer community, and I appreciate the opportunity to contribute to its improvement.

@helloliuyiming helloliuyiming added the s: feature This issue indicates a feature request label Sep 27, 2024
@AlexV525
Copy link
Member

AlexV525 commented Oct 1, 2024

This sounds like a valid approach for me. WDYT? @kuhnroyal

@kuhnroyal
Copy link
Member

Sounds good, should this actually be the default behavior?

@AlexV525
Copy link
Member

AlexV525 commented Oct 1, 2024

Sounds good, should this actually be the default behavior?

Yeah it should be considered as a bug fix, then we don't even need that argument. FYI @helloliuyiming

@helloliuyiming
Copy link
Contributor Author

Sounds good, should this actually be the default behavior?

Yeah it should be considered as a bug fix, then we don't even need that argument. FYI @helloliuyiming

I have addressed this as a bug fix and submitted a pull request (#2305). Please review it when you have a chance. Let me know if you need any additional information or changes.

AlexV525 pushed a commit that referenced this issue Nov 11, 2024
Resolves #2303

## Fix boundary inconsistency in FormData.clone method

Description:

This PR addresses an issue where cloning FormData objects resulted in different boundaries each time. The problem was caused by allocating a new boundary for each clone without any boundary reuse logic.

Changes made:
- Added an `initBoundary` parameter to the FormData constructor.
- Implemented boundary reuse logic when cloning FormData objects.
- Updated documentation to explain how `initBoundary` affects `boundaryName`.

Technical details:
- The `FormData` constructor now accepts an optional `initBoundary` parameter.
- When `initBoundary` is provided, it overrides the `boundaryName` configuration.
- The cloning process now reuses the existing boundary when appropriate.

Impact:
This fix ensures consistent behavior when cloning FormData objects and improves efficiency by reusing boundaries where possible. It should resolve issues related to unexpected boundary changes during FormData manipulation.

Testing:
- Added unit tests to verify boundary consistency across cloned FormData objects.
- Manually tested with various use cases to ensure backward compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants