appservice: Fix site files by avoiding kudu redirect #1537
Conversation
alexweininger
force-pushed
the
alex/aqua-mosquito
branch
from
c728c0a
to
0fe0506
Compare
appservice/src/siteFiles.ts
Outdated
| * @param path - Do not include leading slash. Include trailing slash if path represents a folder. | ||
| */ | ||
| export function createSiteFilesHref(site: ParsedSite, path: string): string { | ||
| return `${site.kuduUrl}/api/vfs/${path}?api-version=2022=03-01` |
There was a problem hiding this comment.
I'm pretty sure that Kudu doesn't have an api-version. That was my mistake when migrating over Kudu calls (because I was thinking it was an ARM call)
appservice/src/siteFiles.ts
Outdated
| @@ -80,7 +82,7 @@ async function getFsResponse(context: IActionContext, site: ParsedSite, filePath | |||
| try { | |||
| return await client.sendRequest(createPipelineRequest({ | |||
| method: 'GET', | |||
| url: `${site.id}/hostruntime/admin/vfs/${filePath}/?api-version=2022-03-01` | |||
| url: href, | |||
There was a problem hiding this comment.
Might make sense to just have href be assigned url to match the property.
There was a problem hiding this comment.
Ok, I'm assigning the href property to url when creating the folder/file tree items. So now it's known as url everywhere.
This should be correct. |
| try { | ||
| if (site.isFunctionApp) { | ||
| const linuxHome: string = '/home'; | ||
| if (site.isLinux && !filePath.startsWith(linuxHome)) { |
There was a problem hiding this comment.
Oh yeah, are you still considering this special case?
There was a problem hiding this comment.
We aren't using the path anymore so this isn't needed. I'm assuming the href returned by the API is always correct. Do you know how to test this case?
There was a problem hiding this comment.
I verified it works with a Linux Function App. Not sure how else to test it.
There was a problem hiding this comment.
That works for me. I honestly don't remember the full context of why we had this in the first place.
Browsing site files was broken during the migration to Track 2 SDK in #1450. Detailed explanation below.
Related extension issue: microsoft/vscode-azureappservice#2544
Problem
The App Service VFS API endpoints are in the format
/api/vfs/${path}?api-version=2022=03-01wherepathis a path to a file or folder. If you make a request for a folder, but omit the trailing slash the API will correct this by redirecting the request and adding the slash onto the redirect location.So
/api/vfs/thisIsAFolder?api-version=2022=03-01will be redirected to/api/vfs/thisIsAFolder/?api-version=2022=03-01(see the added slash).Sounds great right? Wrong! The redirection causes a 401 error. We recently migrated to the Track 2 client. The Track 2 client redirection policy purposefully removes the authentication header from the redirected request. This is a security fix and not a bug according to the SDK team.
I haven't checked, but I'm assuming Track 1 client didn't have this security patch so it just worked even with the redirections.
Solution
Technically we could circumvent the default redirect policy, however A. it's a security risk, and B. a lot of work.
Instead, we now form requests to the App Service VFS API such that no redirect is needed. We can do so by ensuring we include or exclude trailing slashes based on if the path points to a folder or file. I also found that we were including an extra leading slash which also resulted in a redirect.
To do this, I refactored how the site files and site file tree items worked. Instead of tracking file paths to make requests, I'm just storing the
hrefproperty that the API returns which in my testing is always correct. This makes it really easy for us and means I was able to delete some code. I don't know why we didn't use this property originally, maybe it's new.I tested this extensively in both App Service and Functions extensions. This impacts the Logs feature too which I also tested. Afaik Logs and Files are the only thing these changes impact.
It'd be awesome if we could mock the Kudu APIs or maybe just test against a live App Service resource. I'm super open to brainstorming ways we can refactor this stuff to make it more testable to prevent this sort of bug in the future.