Fix Flow Error Handling #17519

connorwinston · 2023-02-15T00:49:52Z

Description

Fixes #15291, Fixes #14415, Closes #16166

This validates that the result of an operation is valid serializable JSON, fixes errors being swallowed because they are enumerable, fixes the request operations error being inconclusive, and adds the ability for operations to throw JSON string errors and them to be parsed before being pushed as an error.

Type of Change

Requirements Checklist

New / updated tests are included
All tests are passing locally
Performed a self-review of the submitted code

If adding a new feature:

Documentation was added/updated. PR:

This partially reverts commit c7e7671.

…circular

api/src/flows.ts

…circular

br41nslug

Works like a charm! But from a security standpoint I am wondering about whether we should dump the entire error stack here 🤔 Perhaps we should only persist the error message itself.

connorwinston · 2023-02-21T19:35:28Z

Works like a charm! But from a security standpoint I am wondering about whether we should dump the entire error stack here 🤔 Perhaps we should only persist the error message itself.

I was curious about that too, but thought that was the intended behavior since the error was just dumped into it before...it just couldn't stringify it because it was enumerable. Which resulted in inconsistent errors because Axios errors some keys are enumerable and some are not. Etc. @rijkvanzanten do you want the full errors returned?

…circular

paescuj · 2023-03-02T12:12:02Z

Works like a charm! But from a security standpoint I am wondering about whether we should dump the entire error stack here 🤔 Perhaps we should only persist the error message itself.

Are you worried about the paths? In this case we could use https://github.com/sindresorhus/clean-stack to clean up the stack.

Edit: Let's not block the pull request by this concern and rather track it separately 👍

br41nslug · 2023-03-02T12:16:12Z

@paescuj The path i would consider unintended information disclosure yeah but it is also not very readable in this format when all you really need is TypeError: Converting circular structure to JSON

paescuj · 2023-03-02T12:20:37Z

@paescuj The path i would consider unintended information disclosure yeah but it is also not very readable in this format when all you really need is TypeError: Converting circular structure to JSON

Agree very much! I'm going to create a separate issue for this 👍

…circular

connorwinston · 2023-03-06T04:33:35Z

@br41nslug Not every Operation throws an instance of error, though when it errors. For example, the webhook/request operation. This PR also fixed returning the request instead of the result. The only way to do that with Axios was to make some custom error throwing and catching as a string and then parsing it. Your change breaks the changes made to that. Additionally, in almost every other place, we return errors as JSON objects; however, your change returns a string which may confuse people using flows?

br41nslug · 2023-03-06T09:22:37Z

Thanks @connorsimply I may have been a little overzealous on the friday fix there. Have updated it to only apply in the case of an actual Error and use the existing logic otherwise

connorwinston · 2023-03-06T16:28:42Z

@br41nslug @paescuj Are we good then to merge now? Tested your changes and they LGTM!

br41nslug · 2023-03-06T17:09:27Z

@connorsimply Yep I think so

api/src/operations/request/index.ts

packages/shared/src/utils/is-valid-json.ts

api/src/flows.ts

packages/shared/src/utils/is-valid-json.test.ts

api/src/flows.ts

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

api/src/flows.ts

connorwinston · 2023-03-13T04:44:28Z

@paescuj Is this clear to be merged now?

api/src/flows.ts

paescuj · 2023-03-13T10:50:00Z

@paescuj Is this clear to be merged now?

Looks good from my side now 👍 Glad if you could take a final look at it again! 😃

connorwinston

LGTM!

…circular

* Validate Operation Result is Serializable * Enumerate Flow Error Object so it gets logged * Add ability for error to be a JSON string and parse it * Make the request operation throw useful error * Revert DockerCompose in "Validate Operation Result is Serializable" This partially reverts commit c7e7671. * Fix Typescript Errors * Move isValidJSON to Shared Util and add Tests * return the error message excluding stack trace * allow for non-exception errors * Apply suggestions from code review Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch> * Clean-up after wrong suggestion * Clean-up processing of error data * Use content-type json if body is object * Reformat error data check --------- Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com> Co-authored-by: Brainslug <br41nslug@users.noreply.github.com> Co-authored-by: Brainslug <tim@brainslug.nl> Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

connorwinston added 7 commits February 14, 2023 16:57

Validate Operation Result is Serializable

c7e7671

Enumerate Flow Error Object so it gets logged

a20f3c6

Add ability for error to be a JSON string and parse it

7d760a6

Make the request operation throw useful error

e65e3a0

Revert DockerCompose in "Validate Operation Result is Serializable"

ffab9e0

This partially reverts commit c7e7671.

Fix Typescript Errors

a2f38c4

Merge branch 'main' into connor/eng-200-run-script-return-value-with-…

daedaad

…circular

connorwinston requested review from a team, rijkvanzanten, br41nslug and Nitwel and removed request for a team February 16, 2023 20:24

connorwinston added the 🍰 Bug label Feb 16, 2023

connorwinston self-assigned this Feb 16, 2023

connorwinston marked this pull request as ready for review February 16, 2023 20:25

br41nslug requested changes Feb 20, 2023

View reviewed changes

api/src/flows.ts Outdated Show resolved Hide resolved

rijkvanzanten and others added 2 commits February 20, 2023 12:04

Merge branch 'main' into connor/eng-200-run-script-return-value-with-…

4881c46

…circular

Move isValidJSON to Shared Util and add Tests

56d1f0d

br41nslug reviewed Feb 21, 2023

View reviewed changes

Merge branch 'main' into connor/eng-200-run-script-return-value-with-…

4483847

…circular

paescuj added the API label Feb 28, 2023

paescuj requested review from br41nslug and a team and removed request for a team March 2, 2023 12:20

br41nslug and others added 2 commits March 3, 2023 12:19

Merge branch 'main' into connor/eng-200-run-script-return-value-with-…

0c24dab

…circular

return the error message excluding stack trace

39760a7

allow for non-exception errors

0f2af5d

br41nslug approved these changes Mar 6, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

paescuj requested changes Mar 6, 2023

View reviewed changes

Apply suggestions from code review

278e272

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

paescuj reviewed Mar 7, 2023

View reviewed changes

api/src/flows.ts Outdated Show resolved Hide resolved

Clean-up after wrong suggestion

0891f1f

Clean-up processing of error data

be0331a

br41nslug reviewed Mar 13, 2023

View reviewed changes

api/src/flows.ts Outdated Show resolved Hide resolved

paescuj added 2 commits March 13, 2023 11:19

Use content-type json if body is object

1174c93

Reformat error data check

cd9f4a1

paescuj approved these changes Mar 13, 2023

View reviewed changes

connorwinston commented Mar 15, 2023

View reviewed changes

Merge branch 'main' into connor/eng-200-run-script-return-value-with-…

a798e6a

…circular

connorwinston mentioned this pull request Mar 15, 2023

Fix JSON.stringify issue with circular ref objects #16166

Closed

9 tasks

connorwinston enabled auto-merge (squash) March 15, 2023 08:28

connorwinston merged commit 34acd97 into main Mar 15, 2023
5 checks passed

connorwinston deleted the connor/eng-200-run-script-return-value-with-circular branch March 15, 2023 08:29

licitdev added this to the Next Release milestone Mar 15, 2023

evanf mentioned this pull request Apr 13, 2023

No error code is returned when the Update Item Operation fails #18161

Closed

connorwinston mentioned this pull request Apr 13, 2023

Only show message on just request/exec flow operations #18167

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Flow Error Handling #17519

Fix Flow Error Handling #17519

connorwinston commented Feb 15, 2023 •

edited

br41nslug left a comment

connorwinston commented Feb 21, 2023

paescuj commented Mar 2, 2023 •

edited

br41nslug commented Mar 2, 2023

paescuj commented Mar 2, 2023

connorwinston commented Mar 6, 2023

br41nslug commented Mar 6, 2023

connorwinston commented Mar 6, 2023 •

edited

br41nslug commented Mar 6, 2023

This comment was marked as outdated.

connorwinston commented Mar 13, 2023

paescuj commented Mar 13, 2023

connorwinston left a comment

Fix Flow Error Handling #17519

Fix Flow Error Handling #17519

Conversation

connorwinston commented Feb 15, 2023 • edited

Description

Type of Change

Requirements Checklist

br41nslug left a comment

Choose a reason for hiding this comment

connorwinston commented Feb 21, 2023

paescuj commented Mar 2, 2023 • edited

br41nslug commented Mar 2, 2023

paescuj commented Mar 2, 2023

connorwinston commented Mar 6, 2023

br41nslug commented Mar 6, 2023

connorwinston commented Mar 6, 2023 • edited

br41nslug commented Mar 6, 2023

This comment was marked as outdated.

connorwinston commented Mar 13, 2023

paescuj commented Mar 13, 2023

connorwinston left a comment

Choose a reason for hiding this comment

connorwinston commented Feb 15, 2023 •

edited

paescuj commented Mar 2, 2023 •

edited

connorwinston commented Mar 6, 2023 •

edited