Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Parse xml rejection message (unescape) #1970

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

swaterkamp
Copy link
Member

Error messages like "Failed to find task 'foo'" are now handled correctly. This concerned xml gsad_response and action_result messages.

Checklist:

@swaterkamp swaterkamp requested review from sarahd93 and a team February 17, 2020 16:46
@swaterkamp swaterkamp self-assigned this Feb 17, 2020
@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #1970 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1970   +/-   ##
=======================================
  Coverage   50.62%   50.62%           
=======================================
  Files        1065     1065           
  Lines       25371    25371           
  Branches     7144     7171   +27     
=======================================
  Hits        12844    12844           
  Misses      11378    11378           
  Partials     1149     1149
Impacted Files Coverage Δ
gsa/src/gmp/http/transform/fastxml.js 100% <100%> (ø) ⬆️

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 88d6030...61d7bb3. Read the comment docs.

@bjoernricks
Copy link
Contributor

I am really not sure why this should be necessary. If this is necessary for the rejection/error case it must be necessary for the success case too. So some other thing is wrong here.

@bjoernricks
Copy link
Contributor

Any by the way transform/xml.js is the old transform which shouldn't be used anymore. transform/fastxml.js should be the default now. IMHO transform/xml.js can be removed completely because it is unused. Any if it is still used this is a mistake and should be changed asap.

@bjoernricks
Copy link
Contributor

Any by the way transform/xml.js is the old transform which shouldn't be used anymore. transform/fastxml.js should be the default now. IMHO transform/xml.js can be removed completely because it is unused. Any if it is still used this is a mistake and should be changed asap.

Ok this is wrong. transform.js contains the common code for the old (xml2js) and the current (fastxml) transforms. But I am still not sure why the new escaping is necessary.

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.

Looking at the code the escaping should already happen here https://github.com/greenbone/gsa/blob/master/gsa/src/gmp/http/transform/fastxml.js#L33 Could you double check why it is still necessary? Maybe we need to use some escaping libraries like he which is also used in the examples of fast-xml-parser?

@swaterkamp
Copy link
Member Author

It was necessary, because we didn't hand PARSER_OPTIONS to the parse() method for xml rejections. For the success case, we did that already and it worked fine.
updating PR...

@bjoernricks
Copy link
Contributor

It was necessary, because we didn't hand PARSER_OPTIONS to the parse() method for xml rejections. For the success case, we did that already and it worked fine.

😞 stupid mistake ...

@swaterkamp
Copy link
Member Author

Indeed ;) I hardly found it, because it was such a small thing, but now everything should be fine.

@bjoernricks
Copy link
Contributor

Thanks so much for taking care!

@bjoernricks bjoernricks merged commit 40768ad into greenbone:master Feb 18, 2020
@swaterkamp swaterkamp deleted the parseRejection branch February 20, 2020 11:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants