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

Open
helloliuyiming opened this issue Sep 27, 2024 · 4 comments · May be fixed by #2305
Labels
s: feature This issue indicates a feature request

Comments

@helloliuyiming
Copy link

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 helloliuyiming linked a pull request Oct 3, 2024 that will close this issue
7 tasks
@helloliuyiming
Copy link
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.

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