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.

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

Resolve Vulnerabilities #1855

Merged
merged 7 commits into from
Feb 1, 2024
Merged

Resolve Vulnerabilities #1855

merged 7 commits into from
Feb 1, 2024

Conversation

EmilyZhang777
Copy link
Collaborator

@EmilyZhang777 EmilyZhang777 commented Jan 30, 2024

This PR resolves the following vulnerabilities:

  • bump search-core's version to 2.5.1 to include the latest vulnerabilities fix
  • remove usages of insecure document method
  • bump the version of http-cache-semantics (to 4.1.1), get-func-name (to 2.0.2) and terser (to 5.14.2) to resolve vulnerabilities to uncontrolled resource consumption and inefficient regular expression complexity

J=VULN-37771, VULN-37772, VULN-37773, VULN-38373, VULN-38391, VULN-38401
TEST=auto

Added new tests for the chang. Ran npm run test and npm run acceptance.

@EmilyZhang777 EmilyZhang777 requested a review from a team January 30, 2024 19:36
@coveralls
Copy link

coveralls commented Jan 30, 2024

Coverage Status

coverage: 62.02% (+0.6%) from 61.411%
when pulling c4ee117 on dev/vulnerabilities
into dc3b171 on hotfix/v1.16.6.

package.json Outdated Show resolved Hide resolved
tests/setup/enzymeadapter.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
const searchInputEl = wrapper.find('.js-yxt-FilterOptions-filter');
expect(searchInputEl).toHaveLength(1);

// empty input
Copy link
Contributor

Choose a reason for hiding this comment

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

can we split each of these cases into separate tests? right now, if one case fails, we won't run any cases after it. it would also make it easier to debug if it fails in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think they fit better in a single test as the initial config settings are the same only the input in the UI changed. Ack

Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to reuse the config settings, you could pull out the config variable. or you could put all the tests into a describe block and do all the setup in the beforeEach

@@ -448,21 +448,21 @@ export default class FilterOptionsComponent extends Component {
if (!filter) {
filterOption.classList.remove('hiddenSearch');
filterOption.classList.remove('displaySearch');
labelEl.innerHTML = labelText;
labelEl.textContent = labelText;
Copy link
Contributor

Choose a reason for hiding this comment

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

in case you haven't already, it would be good to spin up this component locally and test that the text still appears as expected. you should be able to just serve and navigate to the acceptance test page for it and try things out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The jest test I added already covers what I changed. Ack

@EmilyZhang777 EmilyZhang777 merged commit ff87d8b into hotfix/v1.16.6 Feb 1, 2024
12 of 13 checks passed
@EmilyZhang777 EmilyZhang777 deleted the dev/vulnerabilities branch February 1, 2024 19:54
This was referenced Feb 9, 2024
likimmy added a commit that referenced this pull request Feb 9, 2024
### Fixes
- Address and resolve vulnerabilities (#1855)
- Change search bar icons used within button component from divs to spans (#1857)
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.

None yet

3 participants