Skip to content

feat: encode store name + check for fetch #73

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

Merged
merged 3 commits into from
Oct 19, 2023
Merged

feat: encode store name + check for fetch #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef22a1c
feat: encode store name
eduardoboucas Oct 19, 2023
e9dd764
feat: throw error when `fetch` is not available
eduardoboucas Oct 19, 2023
231386e
feat: validate store name
eduardoboucas Oct 19, 2023
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
16 changes: 10 additions & 6 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,22 @@ export interface ClientOptions {
export class Client {
private apiURL?: string
private edgeURL?: string
private fetch?: Fetcher
private fetch: Fetcher
private siteID: string
private token: string

constructor({ apiURL, edgeURL, fetch, siteID, token }: ClientOptions) {
this.apiURL = apiURL
this.edgeURL = edgeURL
this.fetch = fetch
this.fetch = fetch ?? globalThis.fetch
this.siteID = siteID
this.token = token

if (!this.fetch) {
throw new Error(
'Netlify Blobs could not find a `fetch` client in the global scope. You can either update your runtime to a version that includes `fetch` (like Node.js 18.0.0 or above), or you can supply your own implementation using the `fetch` property.',
)
}
}

private async getFinalRequest(storeName: string, key: string, method: string, metadata?: Metadata) {
Expand Down Expand Up @@ -63,8 +69,7 @@ export class Client {
apiHeaders[METADATA_HEADER_EXTERNAL] = encodedMetadata
}

const fetch = this.fetch ?? globalThis.fetch
const res = await fetch(apiURL, { headers: apiHeaders, method })
const res = await this.fetch(apiURL, { headers: apiHeaders, method })

if (res.status !== 200) {
throw new Error(`${method} operation has failed: API returned a ${res.status} response`)
Expand Down Expand Up @@ -102,8 +107,7 @@ export class Client {
options.duplex = 'half'
}

const fetch = this.fetch ?? globalThis.fetch
const res = await fetchAndRetry(fetch, url, options)
const res = await fetchAndRetry(this.fetch, url, options)

if (res.status === 404 && method === HTTPMethod.GET) {
return null
Expand Down
67 changes: 67 additions & 0 deletions src/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,4 +1120,71 @@ describe(`getStore`, () => {
'The `getStore` method requires the name of the store as a string or as the `name` property of an options object',
)
})

test('Throws when the name of the store starts with the `deploy:` prefix', async () => {
const { fetch } = new MockFetch()

globalThis.fetch = fetch

expect(() =>
getStore({
name: 'deploy:foo',
token: apiToken,
siteID,
}),
).toThrowError('Store name cannot start with the string `deploy:`, which is a reserved namespace')

const context = {
siteID,
token: apiToken,
}

env.NETLIFY_BLOBS_CONTEXT = Buffer.from(JSON.stringify(context)).toString('base64')

expect(() => getStore('deploy:foo')).toThrowError(
'Store name cannot start with the string `deploy:`, which is a reserved namespace',
)
})

test('Throws when there is no `fetch` implementation available', async () => {
// @ts-expect-error Assigning a value that doesn't match the type.
globalThis.fetch = undefined

const context = {
siteID,
token: apiToken,
}

env.NETLIFY_BLOBS_CONTEXT = Buffer.from(JSON.stringify(context)).toString('base64')

expect(() => getStore('production')).toThrowError(
'Netlify Blobs could not find a `fetch` client in the global scope. You can either update your runtime to a version that includes `fetch` (like Node.js 18.0.0 or above), or you can supply your own implementation using the `fetch` property.',
)
})

test('URL-encodes the store name', async () => {
const mockStore = new MockFetch()
.get({
headers: { authorization: `Bearer ${apiToken}` },
response: new Response(JSON.stringify({ url: signedURL })),
url: `https://api.netlify.com/api/v1/sites/${siteID}/blobs/${key}?context=%2Fwhat%3F`,
})
.get({
response: new Response(value),
url: signedURL,
})

globalThis.fetch = mockStore.fetch

const blobs = getStore({
name: '/what?',
token: apiToken,
siteID,
})

const string = await blobs.get(key)
expect(string).toBe(value)

expect(mockStore.fulfilled).toBeTruthy()
})
})
9 changes: 8 additions & 1 deletion src/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,14 @@ export class Store {

constructor(options: StoreOptions) {
this.client = options.client
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to scope options.name as well? I'm wondering what happens when name is set dynamically based on user input, and some attacker gets this to be deploy:foo. Then they can access a different store. If we also prefixed options.name, that wouldn't be possible

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the attack vector there? If you set the store name to deploy:<something>, you end up scoping your store to one of your deploys, which is basically the same as supplying a deploy ID in the constructor. It's not like you'll gain access to someone else's deploy?

I would be more in favour of adding a validation rule that prevents you from getting a store that starts with deploy:, because that's a reserved scope. Not that anything happens if you use it, but because it isn't the right flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the attack vector there?

Imagine a site that stores some deploy-specific assets in deploy:id (don't know why, it's contrived), and then customer-specific assets in a store called :customer_id. Now a customer signs up with the slug deploy:foo, and 💥 their store clashes with the deploy-specific assets.

I would be more in favour of adding a validation rule that prevents you from getting a store that starts with `deploy:``

Yes, that sounds like a good fix!

Copy link
Member Author

Choose a reason for hiding this comment

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

Imagine a site that stores some deploy-specific assets in deploy:id (don't know why, it's contrived), and then customer-specific assets in a store called :customer_id. Now a customer signs up with the slug deploy:foo, and 💥 their store clashes with the deploy-specific assets.

With this PR, a store can't be called :customer_id, because we're URL-encoding it to %3Acustomer_id.

Copy link
Contributor

Choose a reason for hiding this comment

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

:customer_id was meant to be a placeholder for the customer ID

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I've understood your example, but either way checking for the deploy: prefix is something we should do.

Done in 231386e.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also URL-encoded the deploy ID for good measure. It shouldn't ever be needed, because deploy IDs are alphanumerical, but it just prevents someone doing weird things from triggering a request to a malformed URL.

this.name = 'deployID' in options ? `deploy:${options.deployID}` : options.name

if ('deployID' in options) {
this.name = `deploy:${encodeURIComponent(options.deployID)}`
} else if (options?.name.startsWith('deploy:')) {
throw new Error('Store name cannot start with the string `deploy:`, which is a reserved namespace')
} else {
this.name = encodeURIComponent(options.name)
}
}

async delete(key: string) {
Expand Down