Skip to content

Commit f73c4a5

Browse files
UzlopakUzlopak
committed
fix: remove sha1 algorithm
1 parent 6a6aaa4 commit f73c4a5

File tree

7 files changed

+64
-148
lines changed

7 files changed

+64
-148
lines changed

‎src/node/sign.ts‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,17 @@
11
import { createHmac } from "crypto";
2-
import { Algorithm, type SignOptions } from "../types";
32
import { VERSION } from "../version";
43

54
export async function sign(
6-
options: SignOptions | string,
5+
secret: string | Buffer,
76
payload: string,
87
): Promise<string> {
9-
const { secret, algorithm } =
10-
typeof options === "object"
11-
? {
12-
secret: options.secret,
13-
algorithm: options.algorithm || Algorithm.SHA256,
14-
}
15-
: { secret: options, algorithm: Algorithm.SHA256 };
16-
178
if (!secret || !payload) {
189
throw new TypeError(
1910
"[@octokit/webhooks-methods] secret & payload required for sign()",
2011
);
2112
}
2213

23-
if (!Object.values(Algorithm).includes(algorithm as Algorithm)) {
24-
throw new TypeError(
25-
`[@octokit/webhooks] Algorithm ${algorithm} is not supported. Must be 'sha1' or 'sha256'`,
26-
);
27-
}
28-
29-
return `${algorithm}=${createHmac(algorithm, secret)
30-
.update(payload)
31-
.digest("hex")}`;
14+
return `sha256=${createHmac("sha256", secret).update(payload).digest("hex")}`;
3215
}
3316

3417
sign.VERSION = VERSION;

‎src/node/verify.ts‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
1-
import { timingSafeEqual } from "crypto";
1+
import { createHmac, timingSafeEqual } from "crypto";
22
import { Buffer } from "buffer";
33

4-
import { sign } from "./sign";
54
import { VERSION } from "../version";
6-
import { getAlgorithm } from "../utils";
5+
import { isValidSignaturePrefix } from "../utils";
76

87
export async function verify(
9-
secret: string,
8+
secret: string | Buffer,
109
eventPayload: string,
1110
signature: string,
1211
): Promise<boolean> {
@@ -16,17 +15,19 @@ export async function verify(
1615
);
1716
}
1817

19-
const signatureBuffer = Buffer.from(signature);
20-
const algorithm = getAlgorithm(signature);
21-
22-
const verificationBuffer = Buffer.from(
23-
await sign({ secret, algorithm }, eventPayload),
24-
);
18+
if (isValidSignaturePrefix(signature) === false) {
19+
return false;
20+
}
21+
const signatureBuffer = Buffer.from(signature.slice(7), "hex");
2522

26-
if (signatureBuffer.length !== verificationBuffer.length) {
23+
if (signatureBuffer.length !== 32) {
2724
return false;
2825
}
2926

27+
const verificationBuffer = Buffer.from(
28+
createHmac("sha256", secret).update(eventPayload).digest(),
29+
);
30+
3031
// constant time comparison to prevent timing attacks
3132
// https://stackoverflow.com/a/31096242/206879
3233
// https://en.wikipedia.org/wiki/Timing_attack

‎src/types.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
export enum Algorithm {
2-
SHA1 = "sha1",
3-
SHA256 = "sha256",
4-
}
5-
6-
export type AlgorithmLike = Algorithm | "sha1" | "sha256";
7-
81
export type SignOptions = {
92
secret: string;
10-
algorithm?: AlgorithmLike;
113
};

‎src/utils.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1-
export const getAlgorithm = (signature: string) => {
2-
return signature.startsWith("sha256=") ? "sha256" : "sha1";
1+
export const isValidSignaturePrefix = (signature: string) => {
2+
return (
3+
signature.length === 71 &&
4+
signature[0] === "s" &&
5+
signature[1] === "h" &&
6+
signature[2] === "a" &&
7+
signature[3] === "2" &&
8+
signature[4] === "5" &&
9+
signature[5] === "6" &&
10+
signature[6] === "="
11+
);
312
};

‎src/web.ts‎

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
import { Algorithm, type AlgorithmLike, type SignOptions } from "./types";
2-
import { getAlgorithm } from "./utils";
1+
import { type SignOptions } from "./types";
32

43
const enc = new TextEncoder();
54

@@ -21,24 +20,15 @@ function UInt8ArrayToHex(signature: ArrayBuffer) {
2120
.join("");
2221
}
2322

24-
function getHMACHashName(algorithm: AlgorithmLike) {
25-
return (
26-
{
27-
[Algorithm.SHA1]: "SHA-1",
28-
[Algorithm.SHA256]: "SHA-256",
29-
} as { [key in Algorithm]: string }
30-
)[algorithm];
31-
}
32-
33-
async function importKey(secret: string, algorithm: AlgorithmLike) {
23+
async function importKey(secret: string) {
3424
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HmacImportParams
3525
return crypto.subtle.importKey(
3626
"raw", // raw format of the key - should be Uint8Array
3727
enc.encode(secret),
3828
{
3929
// algorithm details
4030
name: "HMAC",
41-
hash: { name: getHMACHashName(algorithm) },
31+
hash: { name: "SHA-256" },
4232
},
4333
false, // export = false
4434
["sign", "verify"], // what this key can do
@@ -50,25 +40,19 @@ export async function sign(options: SignOptions | string, payload: string) {
5040
typeof options === "object"
5141
? {
5242
secret: options.secret,
53-
algorithm: options.algorithm || Algorithm.SHA256,
43+
algorithm: "sha256",
5444
}
55-
: { secret: options, algorithm: Algorithm.SHA256 };
45+
: { secret: options, algorithm: "sha256" };
5646

5747
if (!secret || !payload) {
5848
throw new TypeError(
5949
"[@octokit/webhooks-methods] secret & payload required for sign()",
6050
);
6151
}
6252

63-
if (!Object.values(Algorithm).includes(algorithm as Algorithm)) {
64-
throw new TypeError(
65-
`[@octokit/webhooks] Algorithm ${algorithm} is not supported. Must be 'sha1' or 'sha256'`,
66-
);
67-
}
68-
6953
const signature = await crypto.subtle.sign(
7054
"HMAC",
71-
await importKey(secret, algorithm),
55+
await importKey(secret),
7256
enc.encode(payload),
7357
);
7458

@@ -86,11 +70,10 @@ export async function verify(
8670
);
8771
}
8872

89-
const algorithm = getAlgorithm(signature);
9073
return await crypto.subtle.verify(
9174
"HMAC",
92-
await importKey(secret, algorithm),
93-
hexToUInt8Array(signature.replace(`${algorithm}=`, "")),
75+
await importKey(secret),
76+
hexToUInt8Array(signature.replace(`sha256=`, "")),
9477
enc.encode(eventPayload),
9578
);
9679
}

‎test/sign.test.ts‎

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -35,46 +35,24 @@ describe("sign", () => {
3535
);
3636
});
3737

38-
test("sign({secret, algorithm}) throws with invalid algorithm", async () => {
39-
await expect(() =>
40-
// @ts-expect-error
41-
sign({ secret, algorithm: "sha2" }, eventPayload),
42-
).rejects.toThrow(
43-
"[@octokit/webhooks] Algorithm sha2 is not supported. Must be 'sha1' or 'sha256'",
44-
);
45-
});
46-
47-
describe("with eventPayload as string", () => {
48-
describe("returns expected sha1 signature", () => {
38+
describe("with secret as Buffer", () => {
39+
describe("returns expected sha256 signature", () => {
4940
test("sign(secret, eventPayload)", async () => {
50-
const signature = await sign(secret, JSON.stringify(eventPayload));
51-
expect(signature).toBe(
52-
"sha256=4864d2759938a15468b5df9ade20bf161da9b4f737ea61794142f3484236bda3",
41+
const signature = await sign(
42+
Buffer.from(secret),
43+
JSON.stringify(eventPayload),
5344
);
54-
});
55-
56-
test("sign({secret}, eventPayload)", async () => {
57-
const signature = await sign({ secret }, JSON.stringify(eventPayload));
5845
expect(signature).toBe(
5946
"sha256=4864d2759938a15468b5df9ade20bf161da9b4f737ea61794142f3484236bda3",
6047
);
6148
});
62-
63-
test("sign({secret, algorithm: 'sha1'}, eventPayload)", async () => {
64-
const signature = await sign(
65-
{ secret, algorithm: "sha1" },
66-
JSON.stringify(eventPayload),
67-
);
68-
expect(signature).toBe("sha1=d03207e4b030cf234e3447bac4d93add4c6643d8");
69-
});
7049
});
50+
});
7151

52+
describe("with eventPayload as string", () => {
7253
describe("returns expected sha256 signature", () => {
73-
test("sign({secret, algorithm: 'sha256'}, eventPayload)", async () => {
74-
const signature = await sign(
75-
{ secret, algorithm: "sha256" },
76-
JSON.stringify(eventPayload),
77-
);
54+
test("sign(secret, eventPayload)", async () => {
55+
const signature = await sign(secret, JSON.stringify(eventPayload));
7856
expect(signature).toBe(
7957
"sha256=4864d2759938a15468b5df9ade20bf161da9b4f737ea61794142f3484236bda3",
8058
);

‎test/verify.test.ts‎

Lines changed: 22 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,7 @@ function toNormalizedJsonString(payload: object) {
1010

1111
const eventPayload = toNormalizedJsonString({ foo: "bar" });
1212
const secret = "mysecret";
13-
const signatureSHA1 = "sha1=640c0ea7402a3f74e1767338fa2dba243b1f2d9c";
14-
const signatureSHA256 =
13+
const signature =
1514
"sha256=e3eccac34c43c7dc1cbb905488b1b81347fcc700a7b025697a9d07862256023f";
1615

1716
describe("verify", () => {
@@ -51,69 +50,40 @@ describe("verify", () => {
5150
);
5251
});
5352

54-
test("verify(secret, eventPayload, signatureSHA1) returns true for correct signature", async () => {
55-
const signatureMatches = await verify(secret, eventPayload, signatureSHA1);
53+
test("verify(secret, eventPayload, signature) returns true for correct signature", async () => {
54+
const signatureMatches = await verify(secret, eventPayload, signature);
5655
expect(signatureMatches).toBe(true);
5756
});
5857

59-
test("verify(secret, eventPayload, signatureSHA1) returns false for incorrect signature", async () => {
60-
const signatureMatches = await verify(secret, eventPayload, "foo");
61-
expect(signatureMatches).toBe(false);
62-
});
63-
64-
test("verify(secret, eventPayload, signatureSHA1) returns false for correct secret", async () => {
65-
const signatureMatches = await verify("foo", eventPayload, signatureSHA1);
66-
expect(signatureMatches).toBe(false);
67-
});
68-
69-
test("verify(secret, eventPayload, signatureSHA1) returns true if eventPayload contains special characters (#71)", async () => {
70-
// https://github.com/octokit/webhooks.js/issues/71
71-
const signatureMatchesLowerCaseSequence = await verify(
72-
"development",
73-
toNormalizedJsonString({
74-
foo: "Foo\n\u001b[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001b[0m\u001b[2K",
75-
}),
76-
"sha1=82a91c5aacc9cdc2eea893bc828bd03d218df79c",
77-
);
78-
expect(signatureMatchesLowerCaseSequence).toBe(true);
79-
const signatureMatchesUpperCaseSequence = await verify(
80-
"development",
81-
toNormalizedJsonString({
82-
foo: "Foo\n\u001B[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001B[0m\u001B[2K",
83-
}),
84-
"sha1=82a91c5aacc9cdc2eea893bc828bd03d218df79c",
85-
);
86-
expect(signatureMatchesUpperCaseSequence).toBe(true);
87-
const signatureMatchesEscapedSequence = await verify(
88-
"development",
89-
toNormalizedJsonString({
90-
foo: "\\u001b",
91-
}),
92-
"sha1=bdae4705bdd827d026bb227817ca025b5b3a6756",
58+
test("verify(secret, eventPayload, signature) returns true for secret provided as Buffer", async () => {
59+
const signatureMatches = await verify(
60+
Buffer.from(secret),
61+
eventPayload,
62+
signature,
9363
);
94-
expect(signatureMatchesEscapedSequence).toBe(true);
64+
expect(signatureMatches).toBe(true);
9565
});
9666

97-
test("verify(secret, eventPayload, signatureSHA256) returns true for correct signature", async () => {
67+
test("verify(secret, eventPayload, signature) returns false for incorrect signature", async () => {
9868
const signatureMatches = await verify(
9969
secret,
10070
eventPayload,
101-
signatureSHA256,
71+
"sha256=xxxccac34c43c7dc1cbb905488b1b81347fcc700a7b025697a9d07862256023f",
10272
);
103-
expect(signatureMatches).toBe(true);
73+
expect(signatureMatches).toBe(false);
10474
});
10575

106-
test("verify(secret, eventPayload, signatureSHA256) returns false for incorrect signature", async () => {
76+
test("verify(secret, eventPayload, signature) returns false for incorrect signature", async () => {
10777
const signatureMatches = await verify(secret, eventPayload, "foo");
10878
expect(signatureMatches).toBe(false);
10979
});
11080

111-
test("verify(secret, eventPayload, signatureSHA256) returns false for incorrect secret", async () => {
112-
const signatureMatches = await verify("foo", eventPayload, signatureSHA256);
81+
test("verify(secret, eventPayload, signature) returns false for incorrect secret", async () => {
82+
const signatureMatches = await verify("foo", eventPayload, signature);
11383
expect(signatureMatches).toBe(false);
11484
});
11585

116-
test("verify(secret, eventPayload, signatureSHA256) returns true if eventPayload contains special characters (#71)", async () => {
86+
test("verify(secret, eventPayload, signature) returns true if eventPayload contains special characters (#71)", async () => {
11787
// https://github.com/octokit/webhooks.js/issues/71
11888
const signatureMatchesLowerCaseSequence = await verify(
11989
"development",
@@ -147,31 +117,31 @@ describe("verifyWithFallback", () => {
147117
expect(verifyWithFallback).toBeInstanceOf(Function);
148118
});
149119

150-
test("verifyWithFallback(secret, eventPayload, signatureSHA256, [bogus]) returns true", async () => {
120+
test("verifyWithFallback(secret, eventPayload, signature, [bogus]) returns true", async () => {
151121
const signatureMatches = await verifyWithFallback(
152122
secret,
153123
eventPayload,
154-
signatureSHA256,
124+
signature,
155125
["foo"],
156126
);
157127
expect(signatureMatches).toBe(true);
158128
});
159129

160-
test("verifyWithFallback(bogus, eventPayload, signatureSHA256, [secret]) returns true", async () => {
130+
test("verifyWithFallback(bogus, eventPayload, signature, [secret]) returns true", async () => {
161131
const signatureMatches = await verifyWithFallback(
162132
"foo",
163133
eventPayload,
164-
signatureSHA256,
134+
signature,
165135
[secret],
166136
);
167137
expect(signatureMatches).toBe(true);
168138
});
169139

170-
test("verify(bogus, eventPayload, signatureSHA256, [bogus]) returns false", async () => {
140+
test("verify(bogus, eventPayload, signature, [bogus]) returns false", async () => {
171141
const signatureMatches = await verifyWithFallback(
172142
"foo",
173143
eventPayload,
174-
signatureSHA256,
144+
signature,
175145
["foo"],
176146
);
177147
expect(signatureMatches).toBe(false);

0 commit comments

Comments
 (0)