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

Offer an option to specify a cache for conditionally fetching public keys #81

Merged
merged 42 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
b844d07
feat: implement cache + conditional request support
francisfuzz Sep 12, 2024
5425c02
fix: run lint:fix and commit changes
francisfuzz Sep 12, 2024
373e252
test: include assertion for cache
francisfuzz Sep 12, 2024
9a48b50
fix: update test name for setup
francisfuzz Sep 12, 2024
54b514b
docs: leave TODO
francisfuzz Sep 12, 2024
4d06267
fix: update signature to return complete cache
francisfuzz Sep 12, 2024
94c111e
refactor: define VerificationKeysCache type
francisfuzz Sep 12, 2024
d421896
refactor: drop export
francisfuzz Sep 12, 2024
73ae790
fix: use key, not keyId
francisfuzz Sep 12, 2024
e62932a
fix: use standardized id value
francisfuzz Sep 12, 2024
029c584
fix: lint
francisfuzz Sep 12, 2024
6fdeb95
refactor: use existing type
francisfuzz Sep 12, 2024
d10e983
fix: use explicit assertions in typescript tests
francisfuzz Sep 12, 2024
8cda7cc
refactor: de-dupe VerifyRequestInterface
francisfuzz Sep 12, 2024
cd20041
fix: remove unused dep
francisfuzz Sep 12, 2024
58f4606
test: return cache when 304
francisfuzz Sep 12, 2024
d62dd94
Update index.d.ts
francisfuzz Sep 13, 2024
4828168
refactor: scope constant for reuse
francisfuzz Sep 13, 2024
81a6f33
feat: return cache key in .verifyAndParseRequest
francisfuzz Sep 13, 2024
7a740cc
Merge branch 'main' into 9-cache
francisfuzz Sep 16, 2024
6322126
docs: update verifyRequestByKeyId example
francisfuzz Sep 16, 2024
693ca29
docs: update fetchVerificationKeys
francisfuzz Sep 16, 2024
5397672
docs: update verifyAndParseRequest
francisfuzz Sep 16, 2024
2cb7b3a
docs: update verifyRequestByKeyId
francisfuzz Sep 16, 2024
73f15e4
fix: drop brackets
francisfuzz Sep 16, 2024
903cdfe
fix: remove newline
francisfuzz Sep 16, 2024
c7b600e
test: assert cache arg is acceptable
francisfuzz Sep 16, 2024
c4320eb
fix: remove extra backticks
francisfuzz Sep 16, 2024
fb4f95c
test: add test for cache verification
francisfuzz Sep 16, 2024
ec40132
fix: add newline back
francisfuzz Sep 16, 2024
8389fc2
fix: remove example script
francisfuzz Sep 16, 2024
f9707c5
fix: add newline between section
francisfuzz Sep 16, 2024
5fa5d20
docs: correct true method name
francisfuzz Sep 16, 2024
15ec544
docs: annotate isValidRequest
francisfuzz Sep 16, 2024
13fbe9e
docs: update constants
francisfuzz Sep 16, 2024
a975a31
test: update mock etag values
francisfuzz Sep 16, 2024
861f2e3
Merge branch 'main' into 9-cache
francisfuzz Sep 17, 2024
2cca2a9
Merge branch 'main' into 9-cache
gr2m Sep 18, 2024
659a108
test: adapt latest changes from `main` to conditional request APIs
gr2m Sep 18, 2024
47c473e
style: prettier
gr2m Sep 18, 2024
e71f3f1
use lowercase header, use same etag value as in other tests
gr2m Sep 18, 2024
d3ebeb5
Update lib/verification.js
gr2m Sep 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,14 @@ import { request } from "@octokit/request";
// verification types

type RequestInterface = typeof request;
export type VerificationKeysCache = {
id: string;
keys: VerificationPublicKey[];
};
type RequestOptions = {
request?: RequestInterface;
token?: string;
cache?: VerificationKeysCache;
};
export type VerificationPublicKey = {
key_identifier: string;
Expand All @@ -14,11 +19,11 @@ export type VerificationPublicKey = {
};

interface VerifyRequestInterface {
(rawBody: string, signature: string, key: string): Promise<boolean>;
(rawBody: string, signature: string, key: string): boolean;
}

interface FetchVerificationKeysInterface {
(requestOptions?: RequestOptions): Promise<VerificationPublicKey[]>;
(requestOptions?: RequestOptions): Promise<VerificationKeysCache>;
}

interface VerifyRequestByKeyIdInterface {
Expand All @@ -27,7 +32,10 @@ interface VerifyRequestByKeyIdInterface {
signature: string,
keyId: string,
requestOptions?: RequestOptions,
): Promise<boolean>;
): Promise<{
isValid: boolean;
cache: VerificationKeysCache;
}>;
}

// response types
Expand Down
11 changes: 9 additions & 2 deletions index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
prompt,
PromptResult,
getFunctionCalls,
VerificationKeysCache,
} from "./index.js";

const token = "";
Expand All @@ -32,7 +33,10 @@ export async function verifyRequestByKeyIdTest(
keyId: string,
) {
const result = await verifyRequestByKeyId(rawBody, signature, keyId);
expectType<boolean>(result);
expectType<{
isValid: boolean;
cache: { id: string; keys: VerificationPublicKey[] };
}>(result);

// @ts-expect-error - first 3 arguments are required
verifyRequestByKeyId(rawBody, signature);
Expand Down Expand Up @@ -76,13 +80,16 @@ export async function verifyRequestTest(

export async function fetchVerificationKeysTest() {
const result = await fetchVerificationKeys();
expectType<VerificationPublicKey[]>(result);
expectType<{ id: string; keys: VerificationPublicKey[] }>(result);

// accepts a token argument
await fetchVerificationKeys({ token });

// accepts a request argument
await fetchVerificationKeys({ request });

// accepts a cache argument
await fetchVerificationKeys({ cache: { id: "test", keys: [] } });
}

export function createAckEventTest() {
Expand Down
4 changes: 2 additions & 2 deletions lib/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ export function transformPayloadForOpenAICompatibility(payload) {

/** @type {import('..').VerifyAndParseRequestInterface} */
export async function verifyAndParseRequest(body, signature, keyID, options) {
const isValidRequest = await verifyRequestByKeyId(
const { isValid } = await verifyRequestByKeyId(
body,
signature,
keyID,
options
);

return {
isValidRequest,
isValidRequest: isValid,
payload: parseRequestBody(body),
};
}
Expand Down
37 changes: 28 additions & 9 deletions lib/verification.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { createVerify } from "node:crypto";

import { request as defaultRequest } from "@octokit/request";

/** @type {import('..').VerifyRequestByKeyIdInterface} */
/** @type {import('..').VerifyRequestInterface} */
export async function verifyRequest(rawBody, signature, key) {
// verify arguments
assertValidString(rawBody, "Invalid payload");
Expand All @@ -23,17 +23,35 @@ export async function verifyRequest(rawBody, signature, key) {

/** @type {import('..').FetchVerificationKeysInterface} */
export async function fetchVerificationKeys(
{ token = "", request = defaultRequest } = { request: defaultRequest }
{ token = "", request = defaultRequest, cache = { id: "", keys: [] } } = { request: defaultRequest }
) {
const { data } = await request("GET /meta/public_keys/copilot_api", {
headers: token
try {
const headers = token
? {
Authorization: `token ${token}`,
}
: {},
});
: {};

if (cache.id) headers["If-None-Match"] = cache.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try if the changes work? I found the etag response and If-None-Match request header to be tricky in the past, there is some prefix that needs to be encoded ... just want to make sure we test the code in real-life before we merge the PR. If you run into any trouble let me know I can look what I did in other projects

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just thinking about test running the changes. Will report back! 🫡

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Example script:

import { request } from "@octokit/request";
import { fetchVerificationKeys } from "./index.js";

async function main() {
    const token = process.env.GITHUB_TOKEN;
    if (!token) throw new Error("GITHUB_TOKEN is not defined");

    const r = request.defaults({
        headers: {
            Authorization: `token ${token}`,
        }
    });

    const { id, keys } = await fetchVerificationKeys({ token });
    const cache = { id, keys };

    const firstRateLimitCheck = await r("GET /rate_limit");
    const firstRateLimitRemaining = firstRateLimitCheck.data.resources.core.remaining;
    console.log("Rate limit after fetching keys:", firstRateLimitRemaining);

    const response = await fetchVerificationKeys({ token, cache });

    const secondRateLimitCheck = await r("GET /rate_limit");
    const secondRateLimitRemaining = secondRateLimitCheck.data.resources.core.remaining;
    console.log("Rate limit after fetching keys from cache:", secondRateLimitRemaining);
}

main().catch(console.error);

Running it:

@francisfuzz  /workspaces/preview-sdk.js (9-cache) $ node example.js 
Rate limit after fetching keys: 4953
Rate limit after fetching keys from cache: 4953


const response = await request("GET /meta/public_keys/copilot_api", {
headers,
});

if (response.status === 304) {
return cache;
}

const cacheId = response.headers.etag || "";
return { id: cacheId, keys: response.data.public_keys };
} catch (error) {
if (error.status === 304) {
return cache;
}

throw error;
}

return data.public_keys;
}

/** @type {import('..').VerifyRequestByKeyIdInterface} */
Expand All @@ -49,7 +67,7 @@ export async function verifyRequestByKeyId(
assertValidString(keyId, "Invalid keyId");

// receive valid public keys from GitHub
const keys = await fetchVerificationKeys(requestOptions);
const { id, keys } = await fetchVerificationKeys(requestOptions);

// verify provided key Id
const publicKey = keys.find((key) => key.key_identifier === keyId);
Expand All @@ -67,7 +85,8 @@ export async function verifyRequestByKeyId(
throw keyNotFoundError;
}

return verifyRequest(rawBody, signature, publicKey.key);
const isValid = await verifyRequest(rawBody, signature, publicKey.key);
return { isValid, cache: { id: id, keys } };
}

function assertValidString(value, message) {
Expand Down
74 changes: 64 additions & 10 deletions test/verification.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ test("verifyRequestByKeyId()", async (t) => {
return fetch(url, opts);
}

const publicKeys = [
{
key: CURRENT_PUBLIC_KEY,
key_identifier: KEY_ID,
is_current: true,
},
];

mockAgent.disableNetConnect();
const mockPool = mockAgent.get("https://api.github.com");
mockPool
Expand All @@ -44,13 +52,7 @@ test("verifyRequestByKeyId()", async (t) => {
.reply(
200,
{
public_keys: [
{
key: CURRENT_PUBLIC_KEY,
key_identifier: KEY_ID,
is_current: true,
},
],
public_keys: publicKeys,
},
{
headers: {
Expand All @@ -67,7 +69,7 @@ test("verifyRequestByKeyId()", async (t) => {
request: testRequest,
});

t.deepEqual(result, true);
t.deepEqual(result, { isValid: true, cache: { id: "", keys: publicKeys } });
});

test("verifyRequestByKeyId() - invalid arguments", async (t) => {
Expand Down Expand Up @@ -132,7 +134,7 @@ test("verifyRequest() - invalid", async (t) => {
t.deepEqual(result, false);
});

test("fetchVerificationKeys()", async (t) => {
test("fetchVerificationKeys() - without cache", async (t) => {
const mockAgent = new MockAgent();
function fetchMock(url, opts) {
opts ||= {};
Expand Down Expand Up @@ -180,5 +182,57 @@ test("fetchVerificationKeys()", async (t) => {
request: testRequest,
});

t.deepEqual(result, publicKeys);
t.deepEqual(result, { id: "", keys: publicKeys });
});

test("fetchVerificationKeys() - with cache", async (t) => {
const mockAgent = new MockAgent();
function fetchMock(url, opts) {
opts ||= {};
opts.dispatcher = mockAgent;
return fetch(url, opts);
}

const publicKeys = [
{
key: "<key 1>",
key_identifier: "<key-id 1>",
is_current: true,
},
{
key: "<key 2>",
key_identifier: "<key-id 2>",
is_current: true,
},
];

mockAgent.disableNetConnect();
const mockPool = mockAgent.get("https://api.github.com");
mockPool
.intercept({
method: "get",
path: `/meta/public_keys/copilot_api`,
})
.reply(
304,
{},
{
headers: {
"content-type": "application/json",
"x-request-id": "<request-id>",
},
},
);
const testRequest = defaultRequest.defaults({
request: { fetch: fetchMock },
});

const cache = { id: "etag-value", keys: publicKeys };

const result = await fetchVerificationKeys({
request: testRequest,
cache,
});

t.deepEqual(result, cache);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add another test which sends the fetchVerificationKeys() request twice to simulate real-life behavior? The first one should respond with 200 and the body and a etag header. And the 2nd responds with a 304.

I'm not sure how undici is working with mocking, defining different replies for the same request can be tricky. We can also file a follow up issue and take care of it later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took a stab at this in fb4f95c

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me update the etag to be something that looks like what we'd expect from the GitHub API - please hold!

});