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

Don't crash OsIcon when osCpe or osTxt are undefined #1975

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

swaterkamp
Copy link
Member

Checklist:

@swaterkamp swaterkamp requested a review from a team February 19, 2020 16:12
@swaterkamp swaterkamp self-assigned this Feb 19, 2020
Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

From the patch view I can't really see the cause of the issue but I guess real issue is below the changed code. What's the return value of OperatingSystems.find('') now?

@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

Merging #1975 into gsa-9.0 will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           gsa-9.0    #1975      +/-   ##
===========================================
- Coverage    49.02%   49.01%   -0.01%     
===========================================
  Files         1067     1067              
  Lines        24958    24960       +2     
  Branches      7026     7053      +27     
===========================================
  Hits         12235    12235              
- Misses       11564    11566       +2     
  Partials      1159     1159
Impacted Files Coverage Δ
gsa/src/web/components/icon/osicon.js 56% <50%> (-4.87%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ddf90...6c861a8. Read the comment docs.

@swaterkamp
Copy link
Member Author

It now returns undefined, if it receives an empty string as name-param. The issue was that it crashed when name was undefined and .include() couldn't be called at all.

The real issue definitely lies deeper and has been discussed across several teams in the last days. With this small fix we just take care that whenever this kind of situation comes up again, GSA doesn't crash and at least allows the user to continue work.

@swaterkamp swaterkamp merged commit 0727251 into greenbone:gsa-9.0 Feb 19, 2020
@bjoernricks
Copy link
Contributor

Hmm after looking at the code and your changes again, I don't get how your PR fixes a problem. For me it doesn't really change the behavior.

@bjoernricks
Copy link
Contributor

The only case were it could crash is if osTxt is defined and not a string. In that case neither the old nor the new code works.

swaterkamp added a commit to swaterkamp/gsa that referenced this pull request Feb 20, 2020
Don't crash OsIcon when osCpe or osTxt is undefined
swaterkamp added a commit to swaterkamp/gsa that referenced this pull request Feb 20, 2020
Don't crash OsIcon when osCpe or osTxt is undefined
bjoernricks added a commit that referenced this pull request Feb 20, 2020
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.

2 participants