Skip to content
This repository has been archived by the owner on Dec 2, 2024. It is now read-only.

Update Cucumber in Gemfile.lock #294

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Update Cucumber in Gemfile.lock #294

merged 1 commit into from
Oct 21, 2022

Conversation

juniortaeza
Copy link
Contributor

@juniortaeza juniortaeza commented Oct 4, 2022 •
edited
Loading

Desired Outcome

Resolves Snyk's medium severity security issue for potential HTML injection on outdated dependencies.

Implemented Changes

Updates dependencies in Gemfile.lock, default Cucumber version in Gemfile, and runtime error causes from test scripts.

Connected Issue/Story

CyberArk internal issue link: CONJSE-1518

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: insert issue ID
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@juniortaeza juniortaeza requested a review from a team as a code owner October 4, 2022 16:55
@juniortaeza juniortaeza force-pushed the update-branch branch 2 times, most recently from 9fa0c21 to f0578c8 Compare October 11, 2022 14:31
@juniortaeza juniortaeza force-pushed the update-branch branch 2 times, most recently from c8a3ad1 to 0a95287 Compare October 14, 2022 17:23
@szh
Copy link
Contributor

szh commented Oct 14, 2022

@andytinkham Is there a way to verify this is fixed before merging?

@andytinkham
Copy link
Contributor

@andytinkham Is there a way to verify this is fixed before merging?

Not 100% conclusively, but running snyk test on the main branch lists multiple issues in cucumber and snyk test on update-branch does not, so I'm pretty confident it will.

jtuttle
jtuttle reviewed Oct 19, 2022
View reviewed changes
features/support/world.rb
@@ -6,6 +6,11 @@
require 'uri'
require 'securerandom'

require_relative 'cf_helper'
require_relative 'http_helper'
require_relative 'conjur_helper'
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird to me that you should have to require these as cucumber (I believe) usually loads all your helper files for you. It may be related to this breaking change in v4. In any case I think this change is fine. If it is related to the v4 change and this is all it takes to use the new loading scheme, then that's great.

Nitpick, but could you please remove the extra spaces below these require_relatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good! i'm confident this new loading scheme was necessary as RuntimeErrors would pop up without it – which is still weird as it seemingly looks like Ruby does do the auto-loading for you

@juniortaeza
Copy link
Contributor Author

@andytinkham Is there a way to verify this is fixed before merging?

Not 100% conclusively, but running snyk test on the main branch lists multiple issues in cucumber and snyk test on update-branch does not, so I'm pretty confident it will.

glad to hear it!

john-odonnell
john-odonnell approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@juniortaeza juniortaeza merged commit abf681f into main Oct 21, 2022
@juniortaeza juniortaeza deleted the update-branch branch October 21, 2022 16:05
@gl-johnson gl-johnson mentioned this pull request Nov 30, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jtuttle jtuttle jtuttle left review comments

@john-odonnell john-odonnell john-odonnell approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@juniortaeza @szh @andytinkham @jtuttle @john-odonnell