Added exception for security update error handling.

[sachin-sandhu]

What are you trying to accomplish?

Related with Sentry issue 👇

When there is Security update related exception in NPM and YARN ecosystem, the error response is generic ("No files were updated!") which is not very useful in Sentry analysis. The fix creates a newer exception class to better isolate the issues related with Security Update failures. All security update generic issues will be logged under a new category SecurityUpdateError which would be useful in metric and analysis until a permanent fix is found.

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

All NPM and YARN security update issues will be logged under new exception "Security Update Error, No files were updated!" in Sentry

Checklist

• [x] I have run the complete test suite to ensure all tests and linters pass.
• [x] I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
• [x] I have written clear and descriptive commit messages.
• [x] I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
• [x] I have ensured that the code is well-documented and easy to understand.

[jurre]

What exactly is the problem that we're trying to solve here?

[abdulapopoola]

What exactly is the problem that we're trying to solve here?

The problem is to separate security update failures due to transitive dependencies (because DG doesn't currently sent that information) into its own error bucket so we can actually track its impact. The current bucket is a catch-all and is masking several errors.

[jurre]

Thanks for clarifying @abdulapopoola

I think the current implementation would make this even worse, we'd now get a single catch all masking any error that happens here.

[jurre]

@sachin-sandhu let's start off with adding some tests around this that show that only the right errors are going into this new bucket, and that unrelated errors are not going into it

[sachin-sandhu]

@abdulapopoola , I did some more debugging on core issue. :dependabot: already has support for transitive dependencies updates for NPM and YARN. So it should be able to handle the updates well in both security updates and regular updates. However there are several reasons why this might fail. One possible reason is out of the sync package and package-lock files. This might happen if user has checked in package.json or package-lock.json out of sync to solution (manual version fix, different users checking in files etc.). Another issue is that upgrading security packages will break changes in some cases (confirmed via npm audit for this repo) and :dependabot: security update will fail as well as standalone running npm audit fix --force also failed.

This might happen when you were using dependencies versions which when updated via security update will break your solution. To test this ,I ran NPM standalone on repos causing the security update issue. I cleared the particular dependency content from package-lock.json so that NPM CLI can recreate the dependency tree with non breaking change. Similarly I removed similar content from repo's package-lock.json https://github.com/dsp-testing/6133_3/commit/6f11e1cc1350b347796a605fdd54f73001b4f16b , Then :dependabot: was able to generate a security update successfully (link: https://github.com/dsp-testing/6133_3/security/dependabot/2). This can happen with both transitive and main dependencies. Recreating a package-lock.json is not advisable from user's perspective as it might break the solution.

I think for now we can just separate out security update issue in a separate exception category and close the ticket.

[sachin-sandhu]

@jurre @landongrindheim ,

Edit: For better context, we are only moving the "NoChangeError" raised exceptions to new exception class SecurityUpdateError. That way we can isolate which security updates are failing , right now , there is no metric to do it apart from analyzing the trace from each issue. Then we can start isolating the issues which are causing security update to fail and work on fixes.

We are moving security update failures to new exception class (both direct and transitive) for now. let me know if this approach works , I tried to capture the "No File update" exception earlier with exception object

rescue Dependabot::NpmAndYarn::FileUpdater::NoChangeError => e
        handle_security_update_failure_error(e, dependency)
rescue StandardError => e

but it throws issues in dependabot-updater docker test cases. So only viable way i could find was rescue -> check for exception message -> handle.

[jeffwidman]

The problem you're trying to solve makes complete sense... essentially we want the capability to filter out Security errors.

But I think it might be more productive to have some preliminary design proposal and discussion internally with the team before you spend too much time on writing code/tests and making them pass. It'd help clarify some of the haziness at the edges...

A few questions off the top of my head--mostly jotting down notes to myself to discuss with you tomorrow on a call:

1. Is there a reason the custom error is created here and not alongside the existing set of "common" errors? https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/errors.rb
2. Are security errors only the "no non-vulnerable version found"? Ie, are this type of error exclusive bucket separate from our other errors?
3. Or do these include existing job errors such GitDependencyNotReachable but happening as part of a security job... do we want to be able to filter those out too? Because if so, then we essentially don't want a completely new/exclusive error, but rather a property/attribute of some kind on the existing errors that we can filter on in Sentry to say "give me all errors of type X, but exclude the ones triggered by a SecurityJob"...

Slightly related:

• https://github.com/dependabot/dependabot-core/pull/9923

[sachin-sandhu]

Hi @abdulapopoola , I had a discussion with @jeffwidman on this on wednesday. Jeff proposed idea that instead of putting a new exception type for security updates, a better way would be to include a meta tag with sentry logs to isolate exceptions in sentry itself. so for example, should be able to filter the exceptions like this

going a level further, additional meta tag regarding which dependency failed the update can be included. I like this idea, (regardless of how to implement) because I consistently see same named dependencies coming in failed jobs in security job updates. for example, I have cataloged security updates failures and see these 4 dependencies failing on regular basis

| semver        | unknown_error |
| ansi-regex    | unknown_error |
| elliptic      | unknown_error |
| y18n          | unknown_error |

This will allow to make more informed decisions on exceptions . Let me know if this seems a feasible idea to you.

[abdulapopoola]

Hi @abdulapopoola , I had a discussion with @jeffwidman on this on wednesday. Jeff proposed idea that instead of putting a new exception type for security updates, a better way would be to include a meta tag with sentry logs to isolate exceptions in sentry itself. so for example, should be able to filter the exceptions like this

going a level further, additional meta tag regarding which dependency failed the update can be included. I like this idea, (regardless of how to implement) because I consistently see same named dependencies coming in failed jobs in security job updates. for example, I have cataloged security updates failures and see these 4 dependencies failing on regular basis

| semver        | unknown_error |
| ansi-regex    | unknown_error |
| elliptic      | unknown_error |
| y18n          | unknown_error |

This will allow to make more informed decisions on exceptions . Let me know if this seems a feasible idea to you.

Sounds great, yes, please try it out and thanks @jeffwidman for suggesting it. Here is an example PR showing how to add new data to Sentry.

[jeffwidman]

going a level further, additional meta tag regarding which dependency failed the update can be included. I like this idea, (regardless of how to implement) because I consistently see same named dependencies coming in failed jobs in security job updates. for example, I have cataloged security updates failures and see these 4 dependencies failing on regular basis

@sachin-sandhu Oh, very nice! I hadn't even thought of that.

If it were me, I'd probably limit the scope of your initial attempt to just "job trigger: security update vs version update" (or whatever is an appropriate name, I haven't thought too much about it) and ensure it works how we want. And then do a follow-on PR adding the dependency names.

My experience overall with observability tools is sometimes they either don't work well or charge a lot of money when dealing with high cardinality tags. And the dependency names will for sure be somewhat high cardinality... so we may run into other issues there... Although now that I think on it, users/orgs are probably even higher cardinality and as @abdulapopoola pointed above we currently send those to Sentry.

[sachin-sandhu]

@abdulapopoola , @jeffwidman , I'm working on the fix, we happen to have security_updates_only attribute in jobs update which is true in case :dependabot: does a security update. I think we can use this one to identify security update jobs and send to Sentry.

[jeffwidman]

@sachin-sandhu can this be closed as your alternative approach is now merged?

• https://github.com/dependabot/dependabot-core/pull/10120

[sachin-sandhu]

sentry security-updates tag was added instead. closing the PR.