-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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 package_manager for Composer v1 deprecation warning and unsupported error #10716
base: main
Are you sure you want to change the base?
Changes from all commits
f8574d7
497b364
7080fe4
3cf8f8d
f2a6f05
807a368
c116452
e678bc2
f88b2e6
e9e8d4a
95ebef7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,13 @@ | |
module Dependabot | ||
module Composer | ||
module Helpers | ||
V1 = "1" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as a follow up; can this file be strongly typed please? |
||
V2 = "2" | ||
# If we are updating a project with no lock file then the default should be the newest version | ||
DEFAULT = V2 | ||
|
||
# From composers json-schema: https://getcomposer.org/schema.json | ||
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]+)*$} | ||
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]++)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]++)*$} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated quantifiers from + to ++ to prevent backtracking and improve performance, addressing potential ReDoS vulnerabilities flagged by CodeQL. Resolved CodeQL: #10716 (comment) |
||
# From https://github.com/composer/composer/blob/b7d770659b4e3ef21423bd67ade935572913a4c1/src/Composer/Repository/PlatformRepository.php#L33 | ||
PLATFORM_PACKAGE_REGEX = / | ||
^(?:php(?:-64bit|-ipv6|-zts|-debug)?|hhvm|(?:ext|lib)-[a-z0-9](?:[_.-]?[a-z0-9]+)* | ||
|
@@ -18,15 +23,35 @@ module Helpers | |
FAILED_GIT_CLONE = /^Failed to clone (?<url>.*?)/ | ||
|
||
def self.composer_version(composer_json, parsed_lockfile = nil) | ||
v1_unsupported = Dependabot::Experiments.enabled?(:composer_v1_unsupported_error) | ||
|
||
# If the parsed lockfile has a plugin API version, we return either V1 or V2 | ||
# based on the major version of the lockfile. | ||
if parsed_lockfile && parsed_lockfile["plugin-api-version"] | ||
version = Composer::Version.new(parsed_lockfile["plugin-api-version"]) | ||
return version.canonical_segments.first == 1 ? "1" : "2" | ||
else | ||
return "1" if composer_json["name"] && composer_json["name"] !~ COMPOSER_V2_NAME_REGEX | ||
return "1" if invalid_v2_requirement?(composer_json) | ||
return version.canonical_segments.first == 1 ? V1 : V2 | ||
end | ||
|
||
# Check if the composer name does not follow the Composer V2 naming conventions. | ||
# This happens if "name" is present in composer.json but doesn't match the required pattern. | ||
composer_name_invalid = composer_json["name"] && composer_json["name"] !~ COMPOSER_V2_NAME_REGEX | ||
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved
Hide resolved
|
||
|
||
# If the name is invalid returns the fallback version. | ||
if composer_name_invalid | ||
return v1_unsupported ? V2 : V1 | ||
end | ||
|
||
# Check if the composer.json file contains "require" entries that don't follow | ||
# either the platform package naming conventions or the Composer V2 name conventions. | ||
invalid_v2 = invalid_v2_requirement?(composer_json) | ||
|
||
# If there are invalid requirements returns fallback version. | ||
if invalid_v2 | ||
return v1_unsupported ? V2 : V1 | ||
end | ||
|
||
"2" | ||
# If no conditions are met return V2 by default. | ||
V2 | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explanation for changes made in |
||
|
||
def self.dependency_url_from_git_clone_error(message) | ||
|
@@ -43,6 +68,8 @@ def self.dependency_url_from_git_clone_error(message) | |
end | ||
end | ||
|
||
# Checks if the "require" key in composer.json contains invalid packages | ||
# that don't match either platform package patterns or Composer V2 naming rules. | ||
def self.invalid_v2_requirement?(composer_json) | ||
return false unless composer_json.key?("require") | ||
|
||
|
@@ -52,6 +79,7 @@ def self.invalid_v2_requirement?(composer_json) | |
end | ||
private_class_method :invalid_v2_requirement? | ||
|
||
# Removes user credentials from a given dependency URL for security reasons. | ||
def self.clean_dependency_url(dependency_url) | ||
return dependency_url unless URI::DEFAULT_PARSER.regexp[:ABS_URI].match?(dependency_url) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
# typed: strong | ||
# frozen_string_literal: true | ||
|
||
require "sorbet-runtime" | ||
require "dependabot/package_manager" | ||
require "dependabot/composer/version" | ||
|
||
module Dependabot | ||
module Composer | ||
PACKAGE_MANAGER = "composer" | ||
|
||
# Keep versions in ascending order | ||
SUPPORTED_COMPOSER_VERSIONS = T.let([Version.new("2")].freeze, T::Array[Dependabot::Version]) | ||
|
||
DEPRECATED_COMPOSER_VERSIONS = T.let([ | ||
Version.new("1") | ||
].freeze, T::Array[Dependabot::Version]) | ||
|
||
class PackageManager < PackageManagerBase | ||
extend T::Sig | ||
|
||
sig { params(version: T.any(String, Dependabot::Version)).void } | ||
def initialize(version) | ||
@version = T.let(Version.new(version), Dependabot::Version) | ||
@name = T.let(PACKAGE_MANAGER, String) | ||
@deprecated_versions = T.let(DEPRECATED_COMPOSER_VERSIONS, T::Array[Dependabot::Version]) | ||
@supported_versions = T.let(SUPPORTED_COMPOSER_VERSIONS, T::Array[Dependabot::Version]) | ||
end | ||
|
||
sig { override.returns(String) } | ||
attr_reader :name | ||
|
||
sig { override.returns(Dependabot::Version) } | ||
attr_reader :version | ||
|
||
sig { override.returns(T::Array[Dependabot::Version]) } | ||
attr_reader :deprecated_versions | ||
|
||
sig { override.returns(T::Array[Dependabot::Version]) } | ||
attr_reader :supported_versions | ||
|
||
sig { override.returns(T::Boolean) } | ||
def deprecated? | ||
return false if unsupported? | ||
|
||
# Check if the feature flag for Composer v1 deprecation warning is enabled. | ||
return false unless Dependabot::Experiments.enabled?(:composer_v1_deprecation_warning) | ||
|
||
deprecated_versions.include?(version) | ||
end | ||
|
||
sig { override.returns(T::Boolean) } | ||
def unsupported? | ||
# Check if the feature flag for Composer v1 unsupported error is enabled. | ||
return false unless Dependabot::Experiments.enabled?(:composer_v1_unsupported_error) | ||
|
||
supported_versions.all? { |supported| supported > version } | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
# typed: false | ||
# frozen_string_literal: true | ||
|
||
require "dependabot/composer/package_manager" | ||
require "dependabot/package_manager" | ||
require "spec_helper" | ||
|
||
RSpec.describe Dependabot::Composer::PackageManager do | ||
let(:package_manager) { described_class.new(version) } | ||
|
||
describe "#initialize" do | ||
context "when version is a String" do | ||
let(:version) { "2" } | ||
|
||
it "sets the version correctly" do | ||
expect(package_manager.version).to eq(Dependabot::Version.new(version)) | ||
end | ||
|
||
it "sets the name correctly" do | ||
expect(package_manager.name).to eq(Dependabot::Composer::PACKAGE_MANAGER) | ||
end | ||
|
||
it "sets the deprecated_versions correctly" do | ||
expect(package_manager.deprecated_versions).to eq(Dependabot::Composer::DEPRECATED_COMPOSER_VERSIONS) | ||
end | ||
|
||
it "sets the supported_versions correctly" do | ||
expect(package_manager.supported_versions).to eq(Dependabot::Composer::SUPPORTED_COMPOSER_VERSIONS) | ||
end | ||
end | ||
|
||
context "when version is a Dependabot::Version" do | ||
let(:version) { Dependabot::Version.new("2") } | ||
|
||
it "sets the version correctly" do | ||
expect(package_manager.version).to eq(version) | ||
end | ||
|
||
it "sets the name correctly" do | ||
expect(package_manager.name).to eq(Dependabot::Composer::PACKAGE_MANAGER) | ||
end | ||
|
||
it "sets the deprecated_versions correctly" do | ||
expect(package_manager.deprecated_versions).to eq(Dependabot::Composer::DEPRECATED_COMPOSER_VERSIONS) | ||
end | ||
|
||
it "sets the supported_versions correctly" do | ||
expect(package_manager.supported_versions).to eq(Dependabot::Composer::SUPPORTED_COMPOSER_VERSIONS) | ||
end | ||
end | ||
end | ||
|
||
describe "SUPPORTED_COMPOSER_VERSIONS" do | ||
it "is in ascending order" do | ||
expect(Dependabot::Composer::SUPPORTED_COMPOSER_VERSIONS) | ||
.to eq(Dependabot::Composer::SUPPORTED_COMPOSER_VERSIONS.sort) | ||
end | ||
end | ||
|
||
describe "#deprecated?" do | ||
before do | ||
allow(Dependabot::Experiments).to receive(:enabled?) | ||
.with(:composer_v1_deprecation_warning) | ||
.and_return(feature_flag_deprecation_enabled) | ||
allow(Dependabot::Experiments).to receive(:enabled?) | ||
.with(:composer_v1_unsupported_error) | ||
.and_return(feature_flag_unsupported_enabled) | ||
end | ||
|
||
context "when feature flag `composer_v1_deprecation_warning` is enabled and version is deprecated" do | ||
let(:version) { "1" } | ||
let(:feature_flag_deprecation_enabled) { true } | ||
let(:feature_flag_unsupported_enabled) { false } | ||
|
||
it "returns true" do | ||
expect(package_manager.deprecated?).to be true | ||
end | ||
end | ||
|
||
context "when feature flag `composer_v1_deprecation_warning` is disabled" do | ||
let(:version) { "1" } | ||
let(:feature_flag_deprecation_enabled) { false } | ||
let(:feature_flag_unsupported_enabled) { false } | ||
|
||
it "returns false" do | ||
expect(package_manager.deprecated?).to be false | ||
end | ||
end | ||
|
||
context "when version is unsupported and takes precedence" do | ||
let(:version) { "0.9" } | ||
let(:feature_flag_deprecation_enabled) { true } | ||
let(:feature_flag_unsupported_enabled) { true } | ||
|
||
it "returns false, as unsupported takes precedence" do | ||
expect(package_manager.deprecated?).to be false | ||
end | ||
end | ||
end | ||
|
||
describe "#unsupported?" do | ||
before do | ||
allow(Dependabot::Experiments).to receive(:enabled?) | ||
.with(:composer_v1_unsupported_error) | ||
.and_return(feature_flag_unsupported_enabled) | ||
end | ||
|
||
context "when feature flag `composer_v1_unsupported_error` is enabled and version is unsupported" do | ||
let(:version) { "0.9" } | ||
let(:feature_flag_unsupported_enabled) { true } | ||
|
||
it "returns true" do | ||
expect(package_manager.unsupported?).to be true | ||
end | ||
end | ||
|
||
context "when feature flag `composer_v1_unsupported_error` is disabled" do | ||
let(:version) { "0.9" } | ||
let(:feature_flag_unsupported_enabled) { false } | ||
|
||
it "returns false" do | ||
expect(package_manager.unsupported?).to be false | ||
end | ||
end | ||
|
||
context "when feature flag is enabled and version is supported" do | ||
let(:version) { "2" } | ||
let(:feature_flag_unsupported_enabled) { true } | ||
|
||
it "returns false" do | ||
expect(package_manager.unsupported?).to be false | ||
end | ||
end | ||
end | ||
|
||
describe "#raise_if_unsupported!" do | ||
before do | ||
allow(Dependabot::Experiments).to receive(:enabled?) | ||
.with(:composer_v1_unsupported_error) | ||
.and_return(feature_flag_enabled) | ||
end | ||
|
||
context "when feature flag is enabled and version is unsupported" do | ||
let(:version) { "0.9" } | ||
let(:feature_flag_enabled) { true } | ||
|
||
it "raises a ToolVersionNotSupported error" do | ||
expect { package_manager.raise_if_unsupported! }.to raise_error(Dependabot::ToolVersionNotSupported) | ||
end | ||
end | ||
|
||
context "when feature flag is disabled" do | ||
let(:version) { "0.9" } | ||
let(:feature_flag_enabled) { false } | ||
|
||
it "does not raise an error" do | ||
expect { package_manager.raise_if_unsupported! }.not_to raise_error | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the follow up; let's consider making this strong typing too