Skip to content

Delay short-circuit in dry-run.rb if vulnerable#6258

Merged
jeffwidman merged 1 commit intomainfrom
tweak-dry-run-logic-for-security-updates
Dec 5, 2022
Merged

Delay short-circuit in dry-run.rb if vulnerable#6258
jeffwidman merged 1 commit intomainfrom
tweak-dry-run-logic-for-security-updates

Conversation

@jeffwidman
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman commented Dec 4, 2022

In #5950, this short-circuit was moved up to skip checking for the latest version if we already know we're on the latest version. (Which is awesome! It's a great speedup)

During review, it was moved below the case for a security update, when a dep is already up-to-date but also not vulnerable.

However, there's yet another case, which is where a dep is up-to-date and also vulnerable. I was surprised in
#6230 (comment) to see that it spits out no update needed as it's already up-to-date when I had explicitly passed in a security advisor marking the latest version as vulnerable. It wasn't clear to me if the dry-run was prematurely bailing before checking for vulnerable versions. That was for the pub ecosystem, where both
checker.lowest_resolvable_security_fix_version and checker.latest_resolvable_version return the same version.

The very next if block handles the case when the dep is vulnerable. So by moving this short-circuit below that, it will still short circuit for the common case where a version is not vulnerable and on latest.

For reference, here's pre-change run:

[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:26:35.101963 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:26:35.102107 #38484]  INFO -- : Cloning the flutter repo https://github.com/flutter/flutter.
I, [2022-12-04T22:27:40.833227 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:27:46.471869 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:39.504293 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:40.773354 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:29:41.511587 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:29:41.511704 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:29:42.735602 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:45.652134 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:46.743673 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'

And here's post-change run:

[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:44:40.544797 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:40.544922 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:41.954572 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:45.209362 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:46.159503 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:44:46.554930 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:46.555046 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:47.709661 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:51.640507 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:52.616051 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
 => there is no available non-vulnerable version
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'

Note that the output above is slightly confusing because it's not doing a security update only, but rather a normal update with a specified security advisory... so I think it's fine for debugging purposes that we get the information that there's no non-vulnerable version, and also that from a version-checking perspective there's no available update, regardless of vulnerability.

@jeffwidman jeffwidman requested a review from a team as a code owner December 4, 2022 22:58
@jeffwidman jeffwidman changed the title Delay short-circuit if vulnerable Delay short-circuit in dry-run.rb if vulnerable Dec 4, 2022
@jeffwidman jeffwidman force-pushed the tweak-dry-run-logic-for-security-updates branch from 32c269b to 52195d9 Compare December 5, 2022 17:14
In #5950, this
short-circuit was moved up to skip checking for the latest version if
we already know we're on the latest version.

During review, it was moved below the case for a security update, when a
dep is already up-to-date but also _not_ vulnerable.

However, there's yet another case, which is where a dep is up-to-date
and _also_ vulnerable. I was surprised in
#6230 (comment)
to see that it spits out `no update needed as it's already up-to-date`
when I had explicitly passed in a security advisor marking the latest
version as vulnerable. It wasn't clear to me if the `dry-run` was
prematurely bailing before checking for vulnerable versions. That was
for the `pub` ecosystem, where both
`checker.lowest_resolvable_security_fix_version` and
`checker.latest_resolvable_version` return the same version.

The very next `if` block handles the case when the dep _is_ vulnerable.
So by moving this short-circuit below that, it will still short circuit
for the common case where a version is not vulnerable and on latest.

For reference, here's pre-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:26:35.101963 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:26:35.102107 #38484]  INFO -- : Cloning the flutter repo https://github.com/flutter/flutter.
I, [2022-12-04T22:27:40.833227 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:27:46.471869 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:39.504293 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:40.773354 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:29:41.511587 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:29:41.511704 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:29:42.735602 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:45.652134 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:46.743673 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

And here's post-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:44:40.544797 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:40.544922 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:41.954572 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:45.209362 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:46.159503 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:44:46.554930 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:46.555046 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:47.709661 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:51.640507 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:52.616051 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
 => there is no available non-vulnerable version
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

Note that the output above is slightly confusing because it's _not_
doing a security update only, but rather a normal update with a
specified security advisory... so I think it's fine for debugging
purposes that we get the information that there's no non-vulnerable
version, and also that from a version-checking perspective there's no
available update, regardless of vulnerability.
@jeffwidman jeffwidman force-pushed the tweak-dry-run-logic-for-security-updates branch from 52195d9 to a0909c7 Compare December 5, 2022 17:56
@jeffwidman jeffwidman enabled auto-merge (rebase) December 5, 2022 17:57
@jeffwidman jeffwidman merged commit db729c1 into main Dec 5, 2022
@jeffwidman jeffwidman deleted the tweak-dry-run-logic-for-security-updates branch December 5, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants