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

Requests that require the invocationUrl to sign the request no longer work #136

Open
supex0fan opened this issue Apr 2, 2021 · 6 comments
Assignees

Comments

@supex0fan
Copy link

I believe this is related to the recent QueryParam digest changes.

Here is an example from the XChange library.
From: https://docs.pro.coinbase.com/#signing-a-message

The CB-ACCESS-SIGN header is generated by creating a sha256 HMAC using the base64-decoded secret key on the prehash string timestamp + method + requestPath + body (where + represents string concatenation) and base64-encode the output.

From the XChange library (just using this method as an example, the error occurs on all methods that need to be signed)
https://github.com/knowm/XChange/blob/b8dd1a650191e3bfb706c9799864b423a57f573a/xchange-coinbasepro/src/main/java/org/knowm/xchange/coinbasepro/CoinbasePro.java#L102-L110

https://github.com/knowm/XChange/blob/b8dd1a650191e3bfb706c9799864b423a57f573a/xchange-coinbasepro/src/main/java/org/knowm/xchange/coinbasepro/service/CoinbaseProDigest.java#L25-L46

When digestParams() is called it throws a NullPointerException on line 28/29 as it can't call replace() on a null pointer. From my brief look, it looks like the digestAll() method is now called in the RestInvocation constructor before invocationUrl and queryString are set.

RestInvocation(Map<Class<? extends Annotation>, Params> paramsMap,
List<Object> unannanotatedParams,
RestMethodMetadata methodMetadata,
String methodPath,
String path,
RequestWriterResolver requestWriterResolver) {
this.paramsMap = paramsMap;
this.unannanotatedParams = unannanotatedParams;
this.methodMetadata = methodMetadata;
this.methodPath = methodPath;
this.path = path;
this.requestWriter = requestWriterResolver == null ? null : requestWriterResolver.resolveWriter(this.getMethodMetadata());
digestAll();
this.queryString = paramsMap.get(QueryParam.class).asQueryString();
this.invocationUrl = getInvocationUrl(methodMetadata.getBaseUrl(), path, this.queryString);
}

@mmazi mmazi self-assigned this Apr 3, 2021
@mmazi
Copy link
Owner

mmazi commented Apr 3, 2021

Could you provide a stack trace?

A test that demonstrates this would be even better. If you can't provide one, I'll try to produce one myself.

@mmazi
Copy link
Owner

mmazi commented Apr 3, 2021

The problem seems to be that there are two real-life situations that are somewhat in conflict:

  1. Digested parts of the request (which may include parts of the URL) must be sent in the body or headers (this is the situation in CoinbaseProDigest and others, mentioned above); and
  2. Digested parts of the request (which may include parts of the body or headers) must be sent in the query parameters, i.e. in the URL (this is the situation from Support disgest query param #135) .

The conflict is in the order in which parts of the request are constructed: 1. implies the URL must be constructed before headers or body, whereas 2. implies headers and body must be constructed before the URL.

I think the correct order may be determined in rescu (depending on where ParamsDigest parameters occur). I'll try to get this done in a week or so.

mmazi added a commit that referenced this issue Apr 3, 2021
@mmazi
Copy link
Owner

mmazi commented Apr 3, 2021

I wrote tests that demonstrate the above:
2371d14

Now for the fix..

@mmazi
Copy link
Owner

mmazi commented Apr 3, 2021

Actually, the "conflict" from above is nonexistent: query params can always be digested first, then URL constructed, then other params digested. Will push a fix shortly.

mmazi added a commit that referenced this issue Apr 3, 2021
@mmazi
Copy link
Owner

mmazi commented Apr 3, 2021

@supex0fan can you confirm that the issue is fixed?

I'll release rescu 2.1.0 when this is confirmed.

@supex0fan
Copy link
Author

I just checked and it is working. Thanks for fixing this so quickly.

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

No branches or pull requests

2 participants