Codeql : Fix to remove exception details from the response

[namanimsft]

…ls in the response

Issue describing the changes in this PR

resolves #issue_for_this_pr

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

• Backporting to the in-proc branch is not required
  • Otherwise: Link to backporting PR
• My changes do not require documentation changes
  • Otherwise: Documentation issue linked to PR
• My changes should not be added to the release notes for the next release
  • Otherwise: I've added my notes to release_notes.md
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• My changes do not require diagnostic events changes
  • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
• I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

[surgupta-msft]

Looks like on fork PRs, the tests pipelines are not running. I took changes to my branch and ran tests. All are passing.
https://azfunc.visualstudio.com/public/_build/results?buildId=193606&view=results
https://azfunc.visualstudio.com/internal/_build/results?buildId=193607&view=results

[jviau]

Blocking this PR for now. I am not too concerned about changing the error format for these APIs, but what we change it to is something worth discussing. I will talk to my team offline about this and get back to you.

[namanimsft]

[like] Nagarajan Mani reacted to your message:

________________________________ From: Surbhi Gupta ***@***.***> Sent: Monday, December 16, 2024 10:08:45 PM To: Azure/azure-functions-host ***@***.***> Cc: Nagarajan Mani ***@***.***>; Author ***@***.***> Subject: Re: [Azure/azure-functions-host] Codeql : Fix to remove exception details from the response (PR #10671) @surgupta-msft commented on this pull request.

________________________________ In src/WebJobs.Script.WebHost/Management/VirtualFileSystem.cs<#10671 (comment)>:

@@ -641,10 +640,43 @@ private IEnumerable<VfsStatEntry> GetDirectoryResponse(HttpRequest request, File

protected HttpResponseMessage CreateResponse(HttpStatusCode statusCode, object payload = null) { var response = new HttpResponseMessage(statusCode); - if (payload != null) + try + { + if (payload != null) + { + // Use safe serialization settings + var jsonSerializerSettings = new JsonSerializerSettings + { + NullValueHandling = NullValueHandling.Ignore, + DefaultValueHandling = DefaultValueHandling.Include, + Formatting = Formatting.None + }; + + // Check if the payload is a string or an exception + payload = payload is string ? payload as string : payload is Exception ? (payload as Exception).Message : payload; Thinking if exception name (example: System.InvaliOperationException) could be relevant here for customers as they used to get that before.

________________________________ In src/WebJobs.Script.WebHost/Management/VirtualFileSystem.cs<#10671 (comment)>:

{

- var content = payload is string ? payload as string : JsonConvert.SerializeObject(payload); - response.Content = new StringContent(content, Encoding.UTF8, "application/json"); + // Return a generic error message to avoid exposing sensitive details Previously for such exception cases, customers will get JsonSerializationException (with complete stack trace?) and no response. Now, customers will get InternalServerError in Http response. This looks change in API behavior to me if they depend on getting the exception instead of response. @jviau<https://github.com/jviau> what are your thoughts on this? — Reply to this email directly, view it on GitHub<#10671 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AP43SQC5W3FFN5YBX337WSL2F5FO3AVCNFSM6AAAAABTBC34LCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBXGM4TSMBZGQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[namanimsft]

[like] Nagarajan Mani reacted to your message:

…

________________________________ From: Surbhi Gupta ***@***.***> Sent: Monday, December 16, 2024 10:17:06 PM To: Azure/azure-functions-host ***@***.***> Cc: Nagarajan Mani ***@***.***>; Author ***@***.***> Subject: Re: [Azure/azure-functions-host] Codeql : Fix to remove exception details from the response (PR #10671) Looks like on fork PRs, the tests pipelines are not running. I took changes to my branch and ran tests. All are passing. https://azfunc.visualstudio.com/public/_build/results?buildId=193606&view=results https://azfunc.visualstudio.com/internal/_build/results?buildId=193607&view=results — Reply to this email directly, view it on GitHub<#10671 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AP43SQACURSZKG2KRSQHMW32F5GOFAVCNFSM6AAAAABTBC34LCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWHE3DCMRUGI>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[namanimsft]

[like] Nagarajan Mani reacted to your message:

________________________________ From: Jacob Viau ***@***.***> Sent: Monday, December 16, 2024 10:32:01 PM To: Azure/azure-functions-host ***@***.***> Cc: Nagarajan Mani ***@***.***>; Author ***@***.***> Subject: Re: [Azure/azure-functions-host] Codeql : Fix to remove exception details from the response (PR #10671) @jviau requested changes on this pull request. Blocking this PR for now. I am not too concerned about changing the error format for these APIs, but what we change it to is something worth discussing. I will talk to my team offline about this and get back to you.

________________________________ In src/WebJobs.Script.WebHost/Management/VirtualFileSystem.cs<#10671 (comment)>:

@@ -641,10 +640,43 @@ private IEnumerable<VfsStatEntry> GetDirectoryResponse(HttpRequest request, File

protected HttpResponseMessage CreateResponse(HttpStatusCode statusCode, object payload = null) { var response = new HttpResponseMessage(statusCode); - if (payload != null) + try + { + if (payload != null) + { + // Use safe serialization settings + var jsonSerializerSettings = new JsonSerializerSettings + { + NullValueHandling = NullValueHandling.Ignore, + DefaultValueHandling = DefaultValueHandling.Include, + Formatting = Formatting.None + }; + + // Check if the payload is a string or an exception + payload = payload is string ? payload as string : payload is Exception ? (payload as Exception).Message : payload; A switch expression would be more readable here payload = payload switch { Exception ex => ex.Message, default => payload, };

________________________________ In src/WebJobs.Script.WebHost/Management/VirtualFileSystem.cs<#10671 (comment)>:

+ {

+ if (payload != null) + { + // Use safe serialization settings + var jsonSerializerSettings = new JsonSerializerSettings + { + NullValueHandling = NullValueHandling.Ignore, + DefaultValueHandling = DefaultValueHandling.Include, + Formatting = Formatting.None + }; + + // Check if the payload is a string or an exception + payload = payload is string ? payload as string : payload is Exception ? (payload as Exception).Message : payload; + + // Sanitize the payload if it's an object + var content = payload is string ? payload as string : JsonConvert.SerializeObject(payload, jsonSerializerSettings); switch expression here as well.

________________________________ In src/WebJobs.Script.WebHost/Management/VirtualFileSystem.cs<#10671 (comment)>:

{

- var content = payload is string ? payload as string : JsonConvert.SerializeObject(payload); - response.Content = new StringContent(content, Encoding.UTF8, "application/json"); + // Return a generic error message to avoid exposing sensitive details Any uncaught exception in AspNetCore will result in an InternalServerError so the status code isn't changing. The body of the response is changing though. However, I think there may be some deeper discussion to be had with the error body. If we are changing the error body, what we change to is important. A service should have consistent error formats across its APIs. We should review if we already have a defined format and use that. If we don't have one we should discuss if defining one and moving this API to it is in scope. — Reply to this email directly, view it on GitHub<#10671 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AP43SQB3X24RPEPFYFFJ5A32F5IGDAVCNFSM6AAAAABTBC34LCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBXGQ3DSMZTGA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[jviau]

@namanimsft I am going to block this PR for now. I want to have a discussion with my team in the new year about error response schema.

Will update my review after the discussion.