Skip to content

Sanitize file names in Dependabot::DependencyFile initializer#5986

Closed
mattt wants to merge 1 commit intomainfrom
mattt/fix-npm-path-dependencies
Closed

Sanitize file names in Dependabot::DependencyFile initializer#5986
mattt wants to merge 1 commit intomainfrom
mattt/fix-npm-path-dependencies

Conversation

@mattt
Copy link
Copy Markdown
Contributor

@mattt mattt commented Oct 27, 2022

Most package ecosystems have code to write dependency files into a temporary directory. Here's a representative example from npm_and_yarn:

def write_temporary_dependency_files
write_lock_files
File.write(".npmrc", npmrc_content)
package_files.each do |file|
path = file.name
FileUtils.mkdir_p(Pathname.new(path).dirname)
File.write(file.name, prepared_package_json_content(file))
end
end

You can find a few dozen instances of this pattern in our codebase.

When file.name has a leading /, those calls to FileUtils#mkdir_p and File.write write to the root directory. This causes an Errno::EACCES. For example, when Dependabot runs an update job for a JavaScript project with a local path dependency it produces the error "Permission denied @ rb_sysopen - /package.json".

This PR attempts to address this problem by sanitizing Dependabot::DependencyFile names in the initializer. With this change, any file names with an absolute path are translated into relative paths that will be written into temporary directories instead of attempting to be written to system root.

@mattt mattt requested a review from a team as a code owner October 27, 2022 18:39
@mattt mattt requested a review from jakecoffman October 27, 2022 18:39
@mattt mattt force-pushed the mattt/fix-npm-path-dependencies branch from 26ea184 to cfb5f6d Compare October 27, 2022 19:06
@jakecoffman
Copy link
Copy Markdown
Member

This has been an issue with the file_fetchers (#5866, #5869), but it manifests in the update_checker when it tries to write the files out. I'm wondering if it's causing other subtle issues, and if we sanitize here we might see more issues elsewhere that will be harder to diagnose.

Also if users are checking in absolute paths we should just be rejecting the job since we can't see some of the dependencies involved, the update could be invalid.

So I think we need to dig down into each of these and see where we're going wrong in the file_fetcher rather than fix it up later.

@mattt
Copy link
Copy Markdown
Contributor Author

mattt commented Oct 27, 2022

@jakecoffman Yeah, looking at the test failures makes me think this isn't the right approach. I'll take another pass starting from the file fetcher to see if I can figure out a more targeted solution. Thanks for taking a look!

@mattt mattt closed this Oct 27, 2022
@mattt mattt deleted the mattt/fix-npm-path-dependencies branch October 27, 2022 20:11
@jeffwidman
Copy link
Copy Markdown
Member

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