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

fix: don't mistake paths for protocols #6212

Merged
merged 14 commits into from
Nov 28, 2023
Merged

fix: don't mistake paths for protocols #6212

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
40250e8
fix: dont mistake paths for protocols
Skn0tt Nov 27, 2023
146cb80
fix: find a fix where query params are still removed
Skn0tt Nov 27, 2023
de73e77
fix: remove image cdn test
Skn0tt Nov 27, 2023
cb5a139
Update tests/integration/commands/dev/v2-api.test.ts
Skn0tt Nov 27, 2023
a39ca0b
Update src/lib/functions/registry.ts
Skn0tt Nov 27, 2023
f205690
refactor: rename to urlpath
Skn0tt Nov 27, 2023
225e790
Merge branch 'v2-func-edge-case' of github.com:netlify/cli into v2-fu…
Skn0tt Nov 27, 2023
11a54b7
refactor: typescriptify
Skn0tt Nov 27, 2023
5024ba8
refactor: some more ts
Skn0tt Nov 27, 2023
01171e6
refactor: even more tsc
Skn0tt Nov 27, 2023
9f5b9a2
Update src/lib/functions/registry.ts
Skn0tt Nov 27, 2023
9bf539b
fix: remove ts-expect-error directive
Skn0tt Nov 27, 2023
d4a8a24
Merge branch 'main' into v2-func-edge-case
Skn0tt Nov 27, 2023
e5e8c08
Merge branch 'main' into v2-func-edge-case
Skn0tt Nov 28, 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
4 changes: 2 additions & 2 deletions src/lib/functions/registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ export class FunctionsRegistry {
async getFunctionForURLPath(url, method) {
Copy link
Member

Choose a reason for hiding this comment

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

The parameter should probably be named to urlPath, as it will now break if it's a fully qualified URL? Alternatively, we can keep it as url and check whether if it's a full URL or a URL path, adjusting accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in f205690

// We're constructing a URL object just so that we can extract the path from
// the incoming URL. It doesn't really matter that we don't have the actual
// local URL with the correct port.
const urlPath = new URL(url, 'http://localhost').pathname
Copy link
Contributor Author

@Skn0tt Skn0tt Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

here's an example for why this broke: Screenshot 2023-11-27 at 16 23 40

Copy link
Member

Choose a reason for hiding this comment

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

There is nothing ensuring that urlPath is indeed a URL path and not a full URL, right? I have a feeling that this is precisely why we had this logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've played around with it a bit more, and it seems like we had this in place to remove query parameters from the path. 146cb80 implements a fix that still does that

// local URL with the correct port.)
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
const urlPath = new URL(`http://localhost${url}`).pathname
const defaultURLMatch = urlPath.match(DEFAULT_FUNCTION_URL_EXPRESSION)

if (defaultURLMatch) {
Expand Down
5 changes: 5 additions & 0 deletions tests/integration/commands/dev/v2-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ describe.runIf(gte(version, '18.13.0'))('v2 api', () => {
expect(await response.text()).toBe(`With literal path: ${url}`)
})

test<FixtureTestContext>('edge case: double slash isnt mistaken as protocol', async ({ devServer }) => {
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
const response = await fetch(`http://localhost:${devServer.port}//something`)
expect(response.status).toBe(404)
})

test<FixtureTestContext>('doesnt run form logic on paths matching function', async ({ devServer }) => {
const url = `http://localhost:${devServer.port}/products`
await fetch(url, { method: 'POST' })
Expand Down
Loading