Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add defensive checks to prevent inscrutable error messages #4913

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add defensive checks to prevent inscrutable error messages #4913

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions go_modules/lib/dependabot/go_modules/resolvability_errors.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,19 @@ module ResolvabilityErrors
GITHUB_REPO_REGEX = %r{github.com/[^:@]*}

def self.handle(message, goprivate:)
# TODO: currently this matches last. Instead, if more than one match, and they
# aren't identical, then don't try to be clever with GitDependenciesNotReachable
# but instead raise DependencyFileNotResolvable and report the whole error.
# This would have resulted in a more obvious error message for #4625
Copy link
Member Author

@jeffwidman jeffwidman Mar 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wasn't 100% sure on this... I could see a case where a multi-line error string may have multiple URLs, and the last one is indeed the proper one to match on...

But given that GitDependenciesNotReachable tells the user that Dependabot can't reach the repo and to update their access creds, that's a fairly opinionated solution...

IMO I'd only want to report that if 110% sure that's the issue, otherwise surface the whole error message and let the user diagnose the issue.

(I assume DependencyFileNotResolvable reports the message to the end user... if it just hides it in logs that only Dependabot devs can see, then that's not so helpful either.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see these cases as most useful to raise GitDependenciesNotReachable.

  1. You have a direct dependency on a private github repo but Dependabot doesn't have access to it.
  2. You have an indirect dependency on a github private repo (from another direct private github dependency) but Dependabot doesn't have access to it.

Do we have examples of what multiple URLs look like in this error message handy? I imagine this could get tricky if you had a failing indirect dependency on another host that came from one of your direct github dependencies.

mod_path = message.scan(GITHUB_REPO_REGEX).last
if mod_path
# TODO: if mod_path doesn't look like a URL, don't continue, but instead raise
# DependencyFileNotResolvable and report the whole error.
# This would have resulted in a more obvious error message for #4625
# How to implement this though?
# * Ruby has no built-in URL parsing, and no great alternatives in https://stackoverflow.com/q/1805761/770425...
# Not sure what Dependabot team policy is on using 3rd-party gems?
# Alternatively a basic sanity check of "it should not contain whitespace" may suffice for now... ??
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jurre @mctofu any feedback on implementing this? Maybe there's a gem that's already being used elsewhere in another check that could be leveraged here? Otherwise, given that this is a simple defensive check, a regex match on any whitespace (especially spaces and newlines) is probably sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect a URL here or just the module path? If it's the path we could use some of the basic character checks from https://pkg.go.dev/golang.org/x/mod/module#CheckImportPath / https://pkg.go.dev/golang.org/x/mod/module#CheckPath as a start? Agree we just need a simple defensive check rather than a robust validation here.

raise Dependabot::DependencyFileNotResolvable, message unless mod_path

# Module not found on github.com - query for _any_ version to know if it
Expand Down
Loading