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.

Sign up for GitHub

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

Jump to bottom

fix: add accept: application/json header to remote signer requests and improve invalid response handling #6587

Merged
merged 23 commits into from
Apr 4, 2024
Merged

fix: add accept: application/json header to remote signer requests and improve invalid response handling #6587

Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e062c15
fix: handle remote signer non "application/json" response content type
cnupy Mar 24, 2024
5f54826
Merge branch 'ChainSafe:unstable' into unstable
cnupy Mar 25, 2024
b02f666
Merge branch 'ChainSafe:unstable' into unstable
cnupy Mar 26, 2024
657caab
Merge branch 'ChainSafe:unstable' into unstable
cnupy Mar 27, 2024
ecdcc6a
restrict logger to minimal set and remove cast
cnupy Mar 27, 2024
383b07b
Fix typo - `minamal`; refactor/inline response handling functions; re…
cnupy Mar 27, 2024
23ca7ba
fix: exception message text
cnupy Mar 27, 2024
8ce75ab
Removed logger in externalSignerClient; Improved handling of response…
cnupy Mar 28, 2024
22f7131
fix: use response statusText as error message if response body is emp…
cnupy Mar 28, 2024
6791ae3
fix: exception message text proper epmpty string check
cnupy Mar 28, 2024
f36052c
fix: exception message text proper epmpty string check
cnupy Mar 28, 2024
6742089
fix: lint errors
cnupy Mar 29, 2024
282cc86
reintroduce handleExternalSignerResponse generic function with better…
cnupy Apr 4, 2024
94580d5
Merge branch 'ChainSafe:unstable' into unstable
cnupy Apr 4, 2024
2b8e722
fix: remove forgotten check in externalSignerUpCheck function
cnupy Apr 4, 2024
2696ea8
Simplify checking response media type
nflaig Apr 4, 2024
12b7bd9
Include json parse error message in thrown Error
nflaig Apr 4, 2024
4364c8b
Remove unnecessary string cast of err body
nflaig Apr 4, 2024
bec5c5b
Fix json parsing
nflaig Apr 4, 2024
c3f1aa3
More robust error handling
nflaig Apr 4, 2024
3100d07
Formatting
nflaig Apr 4, 2024
127909d
Simplify err body message parser
nflaig Apr 4, 2024
facf88c
Revert error handling changes
nflaig Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 44 additions & 9 deletions packages/validator/src/util/externalSignerClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,20 @@ type Web3SignerSerializedRequest = {
signingRoot: RootHex;
};

enum MediaType {
json = "application/json",
}

/**
* Return public keys from the server.
*/
export async function externalSignerGetKeys(externalSignerUrl: string): Promise<string[]> {
const res = await fetch(`${externalSignerUrl}/api/v1/eth2/publicKeys`, {
method: "GET",
headers: {"Content-Type": "application/json"},
headers: {Accept: MediaType.json},
});

return handlerExternalSignerResponse<string[]>(res);
return handleExternalSignerResponse<string[]>(res);
}

/**
Expand Down Expand Up @@ -143,11 +147,14 @@ export async function externalSignerPostSignature(

const res = await fetch(`${externalSignerUrl}/api/v1/eth2/sign/${pubkeyHex}`, {
method: "POST",
headers: {"Content-Type": "application/json"},
headers: {
Accept: MediaType.json,
"Content-Type": MediaType.json,
},
body: JSON.stringify(requestObj),
});

const data = await handlerExternalSignerResponse<{signature: string}>(res);
const data = await handleExternalSignerResponse<{signature: string}>(res);
return data.signature;
}

Expand All @@ -157,20 +164,48 @@ export async function externalSignerPostSignature(
export async function externalSignerUpCheck(remoteUrl: string): Promise<boolean> {
const res = await fetch(`${remoteUrl}/upcheck`, {
method: "GET",
headers: {"Content-Type": "application/json"},
headers: {Accept: MediaType.json},
});

const data = await handlerExternalSignerResponse<{status: string}>(res);
const data = await handleExternalSignerResponse<{status: string}>(res);
return data.status === "OK";
}

async function handlerExternalSignerResponse<T>(res: Response): Promise<T> {
async function handleExternalSignerResponse<T>(res: Response): Promise<T> {
if (!res.ok) {
const errBody = await res.text();
throw Error(`${errBody}`);
throw Error(errBody ? getErrorMessage(errBody) : res.statusText);
}

const contentType = res.headers.get("content-type");
if (contentType === null) {
jeluard marked this conversation as resolved.
Show resolved Hide resolved
throw Error("No Content-Type header found in response");
}

const mediaType = contentType.split(";", 1)[0].trim().toLowerCase();

if (mediaType !== MediaType.json) {
throw Error(`Unsupported response media type: ${mediaType}`);
}

try {
return (await res.json()) as T;
jeluard marked this conversation as resolved.
Show resolved Hide resolved
} catch (e) {
throw Error(`Invalid json response: ${(e as Error).message}`);
}
}

return JSON.parse(await res.text()) as T;
function getErrorMessage(errBody: string): string {
try {
const errJson = JSON.parse(errBody) as {message: string};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea why neither Web3Signer nor Diva return proper errors as json but I made this a bit more future proof to handle potential json errors.

The spec does not define an error schema so we can't be sure how remote signers implement this.

Copy link
Contributor Author

@cnupy cnupy Apr 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to treat error response as json if it's not in the spec. Even if the error response is actually json, you are not guaranteed that there is a field named message (it could be msg, error or even errorMsg ...) and the result will be worse than if you had included the response content as plain text. You may also need to check the content-type of the error response before treating it as json. I know there is a try/catch block, but why do this if it's not in the spec?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair, we would have to check content type and response format is unclear. Just assumed sane format similar to beacon api. Although for errors is not as critical to check proper format and have fallback logic via try/catch.

Let's keep error handling as is, the Diva team is working on the spec will ask them about it and see if we can standardize this a bit better.

if (errJson.message) {
return errJson.message;
} else {
return errBody;
}
} catch (e) {
return errBody;
}
}

function serializerSignableMessagePayload(config: BeaconConfig, payload: SignableMessage): Record<string, unknown> {
Expand Down