From 2d5dd68ab4c7146e88eb00f39c39aa1e54b29117 Mon Sep 17 00:00:00 2001 From: Vishal Tak Date: Sun, 7 May 2023 11:58:18 +0530 Subject: [PATCH 1/4] Add pre and post flatten validations for devfile Certain validations need to be performed before flattening the devfile. For example, to check if the devfile has a parent or not. If we perform this validation after flattening the devfile, that can be an security issue since the flattening process can request the content of that unsafe URL. --- .../workspaces/create/devfile_processor.rb | 4 +++- .../workspaces/create/devfile_validator.rb | 11 ++++++---- .../create/devfile_validator_spec.rb | 20 +++++++++++++++---- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/ee/lib/remote_development/workspaces/create/devfile_processor.rb b/ee/lib/remote_development/workspaces/create/devfile_processor.rb index 9bfd00e399abc4..d8efb5ffa13477 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_processor.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_processor.rb @@ -9,9 +9,11 @@ class DevfileProcessor WORKSPACE_VOLUME = 'gl-workspace-data' def process(devfile:, editor:, project:, workspace_root:) + devfile_validator = DevfileValidator.new + devfile_validator.pre_flatten_validate(devfile: devfile) flattened_devfile_yaml = Devfile::Parser.flatten(devfile) flattened_devfile = YAML.safe_load(flattened_devfile_yaml) - DevfileValidator.new.validate(flattened_devfile: flattened_devfile) + devfile_validator.post_flatten_validate(flattened_devfile: flattened_devfile) flattened_devfile = add_workspace_volume(flattened_devfile: flattened_devfile, volume_name: WORKSPACE_VOLUME) flattened_devfile = add_editor( diff --git a/ee/lib/remote_development/workspaces/create/devfile_validator.rb b/ee/lib/remote_development/workspaces/create/devfile_validator.rb index ed3b636fbd7aa4..963b26203161d3 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_validator.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_validator.rb @@ -21,8 +21,11 @@ class DevfileValidator # Currently, we only support 'exec' and 'apply' for validation SUPPORTED_COMMAND_TYPES = %w[exec apply].freeze - def validate(flattened_devfile:) - validate_parent(flattened_devfile: flattened_devfile) + def pre_flatten_validate(devfile:) + validate_parent(devfile: devfile) + end + + def post_flatten_validate(flattened_devfile:) validate_components( flattened_devfile: flattened_devfile, restricted_prefix: RESTRICTED_PREFIX, @@ -39,8 +42,8 @@ def validate(flattened_devfile:) private - def validate_parent(flattened_devfile:) - raise ArgumentError, _('Inheriting from parent is not yet supported') if flattened_devfile['parent'] + def validate_parent(devfile:) + raise ArgumentError, _('Inheriting from parent is not yet supported') if devfile['parent'] end def validate_components(flattened_devfile:, restricted_prefix:, unsupported_component_types:) diff --git a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb index fe83b4d8488aa6..5f01201a6e206f 100644 --- a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb @@ -16,11 +16,11 @@ context 'for devfiles containing no violations' do # noinspection RubyResolve it 'does not raises an error' do - expect { subject.validate(flattened_devfile: devfile) }.not_to raise_error + expect { subject.post_flatten_validate(flattened_devfile: devfile) }.not_to raise_error end end - context 'for devfiles containing violations' do + context 'for devfiles containing post flatten violations' do using RSpec::Parameterized::TableSyntax # rubocop:disable Layout/LineLength @@ -41,13 +41,25 @@ 'example.invalid-no-components-devfile.yaml' | "No components present in the devfile" 'example.invalid-attributes-editor-injector-absent-devfile.yaml' | "No component has 'gl/inject-editor' attribute" 'example.invalid-attributes-editor-injector-multiple-devfile.yaml' | "Multiple components([\"tooling-container\", \"tooling-container-2\"]) have 'gl/inject-editor' attribute" - 'example.invalid-unsupported-parent-inheritance-devfile.yaml' | "Inheriting from parent is not yet supported" end # rubocop:enable Layout/LineLength with_them do # noinspection RubyResolve it 'raises an error' do - expect { subject.validate(flattened_devfile: devfile) }.to raise_error(ArgumentError, error_str) + expect { subject.post_flatten_validate(flattened_devfile: devfile) }.to raise_error(ArgumentError, error_str) + end + end + end + + context 'for devfiles containing pre flatten violations' do + using RSpec::Parameterized::TableSyntax + + where(:devfile_name, :error_str) do + 'example.invalid-unsupported-parent-inheritance-devfile.yaml' | "Inheriting from parent is not yet supported" + end + with_them do + it 'raises an error' do + expect { subject.pre_flatten_validate(devfile: devfile) }.to raise_error(ArgumentError, error_str) end end end -- GitLab From c52fe8f684fb54f50b6a0866effdc173daa07ac7 Mon Sep 17 00:00:00 2001 From: Vishal Tak Date: Mon, 8 May 2023 13:37:22 +0530 Subject: [PATCH 2/4] Add validation for dedicated pod property We do not support this property yet in the devfile. It is better to not accept such a devfile rather than do so and not act on it. --- .../workspaces/create/devfile_validator.rb | 10 ++++++++++ ...rted-component-container-dedicated-pod-devfile.yaml | 9 +++++++++ .../workspaces/create/devfile_validator_spec.rb | 1 + 3 files changed, 20 insertions(+) create mode 100644 ee/spec/fixtures/remote_development/example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml diff --git a/ee/lib/remote_development/workspaces/create/devfile_validator.rb b/ee/lib/remote_development/workspaces/create/devfile_validator.rb index 963b26203161d3..7e5454813dac6d 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_validator.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_validator.rb @@ -46,6 +46,7 @@ def validate_parent(devfile:) raise ArgumentError, _('Inheriting from parent is not yet supported') if devfile['parent'] end + # rubocop:disable Metrics/CyclomaticComplexity def validate_components(flattened_devfile:, restricted_prefix:, unsupported_component_types:) components = flattened_devfile['components'] @@ -86,6 +87,14 @@ def validate_components(flattened_devfile:, restricted_prefix:, unsupported_comp # since we have only validate each component type if they are present. # rubocop:disable Style/Next unless container.nil? + if container['dedicatedPod'] + error_str = format( + "Property 'dedicatedPod' of component '%s' is not yet supported", + component_name + ) + raise ArgumentError, _(error_str) + end + next if container['endpoints'].nil? container['endpoints'].map do |endpoint| @@ -102,6 +111,7 @@ def validate_components(flattened_devfile:, restricted_prefix:, unsupported_comp # rubocop:enable Style/Next end end + # rubocop:enable Metrics/CyclomaticComplexity def validate_commands(flattened_devfile:, restricted_prefix:, supported_command_types:) commands = flattened_devfile['commands'] diff --git a/ee/spec/fixtures/remote_development/example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml new file mode 100644 index 00000000000000..5d1bac0741bed8 --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml @@ -0,0 +1,9 @@ +--- +schemaVersion: 2.2.0 +components: + - name: example + attributes: + gl/inject-editor: true + container: + image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo + dedicatedPod: true diff --git a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb index 5f01201a6e206f..b05f5c66bd91c2 100644 --- a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb @@ -41,6 +41,7 @@ 'example.invalid-no-components-devfile.yaml' | "No components present in the devfile" 'example.invalid-attributes-editor-injector-absent-devfile.yaml' | "No component has 'gl/inject-editor' attribute" 'example.invalid-attributes-editor-injector-multiple-devfile.yaml' | "Multiple components([\"tooling-container\", \"tooling-container-2\"]) have 'gl/inject-editor' attribute" + 'example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml' | "Property 'dedicatedPod' of component 'example' is not yet supported" end # rubocop:enable Layout/LineLength with_them do -- GitLab From 505eec2c44b061c50611e94ed5ad4064f9bd15db Mon Sep 17 00:00:00 2001 From: Vishal Tak Date: Mon, 8 May 2023 14:15:51 +0530 Subject: [PATCH 3/4] Add validations for starterProjects and projects in the devfile We do not support devfile properties starterProjects and projects yet. It is better to not accept such a devfile than to do so and not act on it. --- .../workspaces/create/devfile_validator.rb | 9 ++++++++- ...le.invalid-unsupported-projects-devfile.yaml | 17 +++++++++++++++++ ...id-unsupported-starter-projects-devfile.yaml | 17 +++++++++++++++++ .../workspaces/create/devfile_validator_spec.rb | 4 +++- locale/gitlab.pot | 8 +++++++- 5 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 ee/spec/fixtures/remote_development/example.invalid-unsupported-projects-devfile.yaml create mode 100644 ee/spec/fixtures/remote_development/example.invalid-unsupported-starter-projects-devfile.yaml diff --git a/ee/lib/remote_development/workspaces/create/devfile_validator.rb b/ee/lib/remote_development/workspaces/create/devfile_validator.rb index 7e5454813dac6d..22261bf25e634e 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_validator.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_validator.rb @@ -26,6 +26,7 @@ def pre_flatten_validate(devfile:) end def post_flatten_validate(flattened_devfile:) + validate_projects(flattened_devfile: flattened_devfile) validate_components( flattened_devfile: flattened_devfile, restricted_prefix: RESTRICTED_PREFIX, @@ -43,7 +44,13 @@ def post_flatten_validate(flattened_devfile:) private def validate_parent(devfile:) - raise ArgumentError, _('Inheriting from parent is not yet supported') if devfile['parent'] + raise ArgumentError, _("Inheriting from 'parent' is not yet supported") if devfile['parent'] + end + + def validate_projects(flattened_devfile:) + raise ArgumentError, _("'starterProjects' is not yet supported") if flattened_devfile['starterProjects'] + + raise ArgumentError, _("'projects' is not yet supported") if flattened_devfile['projects'] end # rubocop:disable Metrics/CyclomaticComplexity diff --git a/ee/spec/fixtures/remote_development/example.invalid-unsupported-projects-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-projects-devfile.yaml new file mode 100644 index 00000000000000..57db7a7cb6291d --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-projects-devfile.yaml @@ -0,0 +1,17 @@ +--- +schemaVersion: 2.2.0 +projects: + - name: example + git: + checkoutFrom: + remote: origin + revision: main + remotes: + origin: https://example.com/path/to/repository + subDir: app +components: + - name: example + attributes: + gl/inject-editor: true + container: + image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo diff --git a/ee/spec/fixtures/remote_development/example.invalid-unsupported-starter-projects-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-starter-projects-devfile.yaml new file mode 100644 index 00000000000000..3f1e8fabf5acac --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-starter-projects-devfile.yaml @@ -0,0 +1,17 @@ +--- +schemaVersion: 2.2.0 +starterProjects: + - name: example + git: + checkoutFrom: + remote: origin + revision: main + remotes: + origin: https://example.com/path/to/repository + subDir: app +components: + - name: example + attributes: + gl/inject-editor: true + container: + image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo diff --git a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb index b05f5c66bd91c2..9804f9733f6ded 100644 --- a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb @@ -42,6 +42,8 @@ 'example.invalid-attributes-editor-injector-absent-devfile.yaml' | "No component has 'gl/inject-editor' attribute" 'example.invalid-attributes-editor-injector-multiple-devfile.yaml' | "Multiple components([\"tooling-container\", \"tooling-container-2\"]) have 'gl/inject-editor' attribute" 'example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml' | "Property 'dedicatedPod' of component 'example' is not yet supported" + 'example.invalid-unsupported-starter-projects-devfile.yaml' | "'starterProjects' is not yet supported" + 'example.invalid-unsupported-projects-devfile.yaml' | "'projects' is not yet supported" end # rubocop:enable Layout/LineLength with_them do @@ -56,7 +58,7 @@ using RSpec::Parameterized::TableSyntax where(:devfile_name, :error_str) do - 'example.invalid-unsupported-parent-inheritance-devfile.yaml' | "Inheriting from parent is not yet supported" + 'example.invalid-unsupported-parent-inheritance-devfile.yaml' | "Inheriting from 'parent' is not yet supported" end with_them do it 'raises an error' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 92495b6eadb9a5..c19518c7faff96 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1319,6 +1319,12 @@ msgstr "" msgid "'%{value}' days of inactivity must be greater than or equal to 90" msgstr "" +msgid "'projects' is not yet supported" +msgstr "" + +msgid "'starterProjects' is not yet supported" +msgstr "" + msgid "(%d closed)" msgid_plural "(%d closed)" msgstr[0] "" @@ -23671,7 +23677,7 @@ msgstr "" msgid "Inherited:" msgstr "" -msgid "Inheriting from parent is not yet supported" +msgid "Inheriting from 'parent' is not yet supported" msgstr "" msgid "Initial default branch name" -- GitLab From 441c3ce4910a9480ec400251127ffd7731d4df97 Mon Sep 17 00:00:00 2001 From: Vishal Tak Date: Tue, 9 May 2023 11:49:14 +0530 Subject: [PATCH 4/4] Add validations for devfile event types We do not support any other event type except preStart yet. It is better to be explicit about it and not accept such devfile than to do and not act on it. --- .../workspaces/create/devfile_validator.rb | 8 +++++++- ...prefix-event-type-prestart-name-devfile.yaml | 4 +--- ...supported-event-type-poststart-devfile.yaml} | 1 - ...unsupported-event-type-poststop-devfile.yaml | 15 +++++++++++++++ ...-unsupported-event-type-prestop-devfile.yaml | 17 +++++++++++++++++ .../workspaces/create/devfile_validator_spec.rb | 5 +++-- 6 files changed, 43 insertions(+), 7 deletions(-) rename ee/spec/fixtures/remote_development/{example.invalid-restricted-prefix-event-type-poststart-name-devfile.yaml => example.invalid-unsupported-event-type-poststart-devfile.yaml} (95%) create mode 100644 ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststop-devfile.yaml create mode 100644 ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-prestop-devfile.yaml diff --git a/ee/lib/remote_development/workspaces/create/devfile_validator.rb b/ee/lib/remote_development/workspaces/create/devfile_validator.rb index 22261bf25e634e..40ad556f937775 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_validator.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_validator.rb @@ -153,8 +153,14 @@ def validate_events(flattened_devfile:, restricted_prefix:) events = flattened_devfile['events'] return if events.nil? - # Ensure no event starts with restricted_prefix events.map do |event_type, event_type_events| + # Ensure no event type other than "preStart" are allowed + unless event_type == 'preStart' + error_str = format("Event type '%s' is not yet supported", event_type) + raise ArgumentError, _(error_str) + end + + # Ensure no event starts with restricted_prefix event_type_events.each do |event| if event.downcase.start_with?(restricted_prefix) error_str = format("Event '%s' of type '%s' starts with '%s'", event, event_type, restricted_prefix) diff --git a/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-prestart-name-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-prestart-name-devfile.yaml index 9a731c506ccf29..8f16fe96aff6b1 100644 --- a/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-prestart-name-devfile.yaml +++ b/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-prestart-name-devfile.yaml @@ -8,10 +8,8 @@ components: image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo commands: - id: example - exec: + apply: component: example - commandLine: mvn clean - workingDir: /projects/spring-petclinic events: preStart: - example diff --git a/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-poststart-name-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststart-devfile.yaml similarity index 95% rename from ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-poststart-name-devfile.yaml rename to ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststart-devfile.yaml index e035558b538de0..6381db0e4672b2 100644 --- a/ee/spec/fixtures/remote_development/example.invalid-restricted-prefix-event-type-poststart-name-devfile.yaml +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststart-devfile.yaml @@ -15,4 +15,3 @@ commands: events: postStart: - example - - gl-example diff --git a/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststop-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststop-devfile.yaml new file mode 100644 index 00000000000000..36ae8df0172cb2 --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-poststop-devfile.yaml @@ -0,0 +1,15 @@ +--- +schemaVersion: 2.2.0 +components: + - name: example + attributes: + gl/inject-editor: true + container: + image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo +commands: + - id: example + apply: + component: example +events: + postStop: + - example diff --git a/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-prestop-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-prestop-devfile.yaml new file mode 100644 index 00000000000000..acff86fa2fc2fc --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-unsupported-event-type-prestop-devfile.yaml @@ -0,0 +1,17 @@ +--- +schemaVersion: 2.2.0 +components: + - name: example + attributes: + gl/inject-editor: true + container: + image: quay.io/mloriedo/universal-developer-image:ubi8-dw-demo +commands: + - id: example + exec: + component: example + commandLine: mvn clean + workingDir: /projects/spring-petclinic +events: + preStop: + - example diff --git a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb index 9804f9733f6ded..1d76c67af4f7b5 100644 --- a/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/devfile_validator_spec.rb @@ -30,9 +30,7 @@ 'example.invalid-restricted-prefix-command-name-devfile.yaml' | "Command id 'gl-example' starts with 'gl-'" 'example.invalid-restricted-prefix-component-container-endpoint-name-devfile.yaml' | "Endpoint name 'gl-example' of component 'example' starts with 'gl-'" 'example.invalid-restricted-prefix-component-name-devfile.yaml' | "Component name 'gl-example' starts with 'gl-'" - 'example.invalid-restricted-prefix-event-type-poststart-name-devfile.yaml' | "Event 'gl-example' of type 'postStart' starts with 'gl-'" 'example.invalid-restricted-prefix-event-type-prestart-name-devfile.yaml' | "Event 'gl-example' of type 'preStart' starts with 'gl-'" - 'example.invalid-restricted-prefix-event-type-prestop-name-devfile.yaml' | "Event 'gl-example' of type 'preStop' starts with 'gl-'" 'example.invalid-restricted-prefix-variable-name-devfile.yaml' | "Variable name 'gl-example' starts with 'gl-'" 'example.invalid-restricted-prefix-variable-name-with-underscore-devfile.yaml' | "Variable name 'gl_example' starts with 'gl_'" 'example.invalid-unsupported-component-type-image-devfile.yaml' | "Component type 'image' is not yet supported" @@ -44,6 +42,9 @@ 'example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml' | "Property 'dedicatedPod' of component 'example' is not yet supported" 'example.invalid-unsupported-starter-projects-devfile.yaml' | "'starterProjects' is not yet supported" 'example.invalid-unsupported-projects-devfile.yaml' | "'projects' is not yet supported" + 'example.invalid-unsupported-event-type-poststart-devfile.yaml' | "Event type 'postStart' is not yet supported" + 'example.invalid-unsupported-event-type-prestop-devfile.yaml' | "Event type 'preStop' is not yet supported" + 'example.invalid-unsupported-event-type-poststop-devfile.yaml' | "Event type 'postStop' is not yet supported" end # rubocop:enable Layout/LineLength with_them do -- GitLab