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

fix signature error while the content includes @ #646

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

xyq-c-cpp
Copy link
Collaborator

  1. fix Fix /sapi/v1/sub-account/universalTransfer Signature for this request is not valid. #639 , This is the fundamental approach to fix this issue. Fix issue [Signature error when request parameter content includes @]
    1. Currently, only the binance module has been repaired, and other modules have not been involved yet. Do not modify them for now.

@adshao
Copy link
Owner

adshao commented Dec 5, 2024

Has the bug been confirmed? Is there a better way to fix it without hardcoding the special symbols?

@adshao
Copy link
Owner

adshao commented Dec 5, 2024

#639 (comment)

@xyq-c-cpp
Copy link
Collaborator Author

Has the bug been confirmed? Is there a better way to fix it without hardcoding the special symbols?

Yes, I have confirmed that the modified code has been tested in real time environment, for the interface POST /sapi/v1/sub-account/universalTransfer.
Other open-source libraries binance-futures-connector-python also perform similar processing after URL encoding. At present, this is a perfect approach, it seems. @adshao

@xyq-c-cpp
Copy link
Collaborator Author

image

The response appears to be @, which is a secure string and does not require URL encoding. @adshao

@adshao
Copy link
Owner

adshao commented Dec 6, 2024

Has the bug been confirmed? Is there a better way to fix it without hardcoding the special symbols?

Yes, I have confirmed that the modified code has been tested in real time environment, for the interface POST /sapi/v1/sub-account/universalTransfer. Other open-source libraries binance-futures-connector-python also perform similar processing after URL encoding. At present, this is a perfect approach, it seems. @adshao

OK, please add tests for this change and include comments explaining why the ‘@’ symbol must be escaped.

@xyq-c-cpp
Copy link
Collaborator Author

xyq-c-cpp commented Dec 13, 2024

Has the bug been confirmed? Is there a better way to fix it without hardcoding the special symbols?

Yes, I have confirmed that the modified code has been tested in real time environment, for the interface POST /sapi/v1/sub-account/universalTransfer. Other open-source libraries binance-futures-connector-python also perform similar processing after URL encoding. At present, this is a perfect approach, it seems. @adshao

OK, please add tests for this change and include comments explaining why the ‘@’ symbol must be escaped.

'@' is a safe character, no need to escape. Supplementing this test is not effective, as how to handle this special character depends on the feedback from the Binance real disk interface. The actual test has been conducted, and if possible, you can also test and verify it. @adshao

@xyq-c-cpp
Copy link
Collaborator Author

In actual testing, when passing in query_string, @ escape or no escape is acceptable. When passing in the body, @ escape is not allowed, @ not escape is allowed. So not escaping is the best solution. @adshao

@xyq-c-cpp
Copy link
Collaborator Author

In actual testing, when passing in query_string, @ escape or no escape is acceptable. When passing in the body, @ escape is not allowed, @ not escape is allowed. So not escaping is the best solution. @adshao

When parameters are passed through request-body, the main issue is that the signature is correct when the @ in the signature content is not escaped; Signature error if escaped. @adshao

@adshao adshao merged commit f635eb8 into adshao:master Dec 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants