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

Fixing the content-type check for XML. #725

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

karelhusa
Copy link

Fixing the content-type check for XML. Checking with endsWith did not work for content-types with charset (e.g. "text/xml;charset=UTF-8").

… work for content-types with charset (e.g. "text/xml;charset=UTF-8").
@rob-a-robertson
Copy link

Would love to see this merged.

@juniorug
Copy link

There is one check failed, due to the code conflict.
I'd like to see this PR merged too :)

@@ -628,7 +628,7 @@ public void setEditable(boolean enabled) {

@Override
public int getSupportScoreForContentType(String contentType ) {
return contentType.toLowerCase().endsWith("xml")? 2 : 0;
return contentType.toLowerCase().contains("xml")? 2 : 0;

Choose a reason for hiding this comment

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

Isn't that dangerous, since it will also include content types that are not necessarily supported?
For example: application/ vnd.openxmlformats-officedocument. wordprocessingml.document which is the MIME-Type of a MS Word .docx file.

Copy link
Author

Choose a reason for hiding this comment

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

@wilk-polarny This is a score to indicate which tab to choose when the response comes. You can switch to XML tab manually anyway. So, this is not a security measure against an invalid content. The main purpose is to open xml tab when the xml has come in response. The current issue is that the hint is not working.

Choose a reason for hiding this comment

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

I agree that the current proposal is sufficient, but if the hold-up is because of how open-ended it is, we could tighten it up by checking to see if it ends in "xml" or if it contains "/xml;". A regular expression such as the following would work: (?:(xml$)|(/xml;.+)) It is unclear to me--is the "WhiteSource Configuration Change" issue still outstanding?

Copy link
Author

Choose a reason for hiding this comment

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

@dmccarty-incomm The issue is that the maintainers do not process the merge requests. Whitesource validation failure is a minor issue which can be resolved easilly as well.

Choose a reason for hiding this comment

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

How about split by ";" and check it?

text/xml;charset=UTF-8

[0] text/xml  -> endsWith xml -> stop checking
[1] charset=UTF-8

@wilk-polarny
Copy link

I'd also love to see this "fixed", since the current behavior in 5.7.0 is super annoying.

Wouldn't it be even better if the scoring logic would only analyze the media-type directive and completely omit the charset and boundary directives?

Also matching against known/supported media-types would be beneficial, since the string xml is also part of other media types that aren't necessarily supported by the view.

@karelhusa
Copy link
Author

There is one check failed, due to the code conflict. I'd like to see this PR merged too :)

The conflict is actually caused by a malformed whitesource file. That's another issue to be resolved.

@ChristophKronberger
Copy link

I'd also love to see this "fixed", since the current behavior in 5.7.0 is super annoying.

Wouldn't it be even better if the scoring logic would only analyze the media-type directive and completely omit the charset and boundary directives?

Also matching against known/supported media-types would be beneficial, since the string xml is also part of other media types that aren't necessarily supported by the view.

Did your suggested soloution in #760

Zipped jar file.
Archiv s jarem.
@ntt77
Copy link

ntt77 commented Jan 23, 2024

let merge it and release a fix immediately.
why would a bug like this exists and no one fix???

@croensch
Copy link

agree

@lhanson
Copy link

lhanson commented Feb 29, 2024

At this point I think the ideal path forward would be for somebody to maintain a community-supported fork where fixes don't rot in MRs for years at a time.

@ChristophKronberger
Copy link

@lhanson

totally agree with you.
Had the same thought few Months ago, even created a Logo and called it soapmen.

But then i didnt make the repo public since i fear the legal department from SmartBear.

who knows what all is a „registered trademark“

if we find some motivated maintainers, we could discuss this further.

@lhanson
Copy link

lhanson commented Feb 29, 2024

@ChristophKronberger that's a fair concern.

Five years ago I'd have been much more motivated, but thankfully I believe my days of using SOAP are coming to an end.

@wilk-polarny
Copy link

since i fear the legal department from SmartBear

Shouldn't the project license, albeit a weird choice (EUPL) allow for that?

@ChristophKronberger
Copy link

since i fear the legal department from SmartBear

Shouldn't the project license, albeit a weird choice (EUPL) allow for that?

Yes but only for the source code.

i think the graphics like the Logo and names and some other things are trademarks.

Legal Protection: This Licence does not grant permission to use the trade names, trademarks, service marks, or names of the Licensor, except as required for reasonable and customary use in describing the origin of the Work and reproducing the content of the copyright notice.

https://www.soapui.org/open-source/soapui-license/

I think it should be possible but we would defently have to change these protected things.

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.

10 participants