v.in.wfs: improve error message for ServiceException

[pesekon2]

the current one said that the downloaded XML cannot be found (not really what was going on) - this one reports the meaningful content of the service exception report instead

[pesekon2]

Any idea why does the Python Code Quality check complain scripts/v.in.wfs/v.in.wfs.py:230:16: N817 CamelCase ElementTre imported as acronym ET?

It is imported this way everywhere around the code (and it is the common way). If we are going to change it here to comply with the quality check, we should change it everywhere in order not to use inconsistent aliases.

[echoix]

Do we really use defused xml here? I don't remember seeing it in the deps listed and installed.

And for your question on why it "fails", it's related to the default settings of:

grass/pyproject.toml

Lines 394 to 397 in 8b5c6f9

	[tool.ruff.lint.flake8-import-conventions.extend-aliases]
	# Declare a custom aliases, checked with rule ICN001
	"grass.script" = "gs"

Listed here: https://docs.astral.sh/ruff/settings/#lintflake8-import-conventions

Where "xml.etree.ElementTree": "ET"}. Thus, it is unexpected, as by convention, ET would be the python's library ElementTree.

I'm not saying that using defusedxml is wrong, you are right that this code should be the right thing. There are multiple places where we use the Python library's xml parser where we are warned, with bandit, CodeQL, and the Python docs, that it is not enough and should be using an external library like defusedxml.

• See the top red box of: https://docs.python.org/3/library/xml.html#module-xml
• The description here: https://docs.python.org/3/library/xml.html#the-defusedxml-package
• https://docs.astral.sh/ruff/rules/suspicious-xml-expat-builder-usage/
• A discussion linked from a ruff discussion https://discuss.python.org/t/status-of-defusedxml-and-recommendation-in-docs/34762/20

[echoix]

That last discussion makes me think it might not be as pressing as I initially thought, and maybe not as much as an official recommendation, but a single (but valid) opinion at a time.

[pesekon2]

And for your question on why it "fails", it's related to the default settings of:

grass/pyproject.toml

Lines 394 to 397 in 8b5c6f9

	[tool.ruff.lint.flake8-import-conventions.extend-aliases]
	# Declare a custom aliases, checked with rule ICN001
	"grass.script" = "gs"

Listed here: https://docs.astral.sh/ruff/settings/#lintflake8-import-conventions

Where "xml.etree.ElementTree": "ET"}. Thus, it is unexpected, as by convention, ET would be the python's library ElementTree.

Cool, thanks for the explanation.

As for the use of decodexml -- I fully agree with you. I originally used just the native Python xml package. I switched to defusedxml in e2d4ef1 because github-advanced-security bot complained. What do you think would be the best solution? Shall we just ignore the warning and switch back to xml (which we use in multiple other places)? Because it is true that defusedxml is not between the dependencies - I encountered it in so many places in other places I actually thought it is already in native Python...

[echoix]

I think you could ignore the warning like we did with the others of existing code. We'd need to take a position on this at one point, where would it be discussed though? It's not something to fall on your shoulders and this PR.

[pesekon2]

I think you could ignore the warning like we did with the others of existing code.

The commit introducing defusedxml reverted.

We'd need to take a position on this at one point, where would it be discussed though? It's not something to fall on your shoulders and this PR.

Shall I open an issue for that? Or shall we have an internal discussion to avoid discussing vulnerabilities openly and bringing more attention to them (which we have already done, ehm)?

[echoix]

We'd need to take a position on this at one point, where would it be discussed though? It's not something to fall on your shoulders and this PR.

Shall I open an issue for that? Or shall we have an internal discussion to avoid discussing vulnerabilities openly and bringing more attention to them (which we have already done, ehm)?

You could if you'd want, what are our channels for these kinds of internal discussions?

[pesekon2]

what are our channels for these kinds of internal discussions?

I do not know if we have any official channel/way to handle this. The last security meeting was called internally by a meeting just through e-mails.

[nilason]

Wouldn't it be possible to use defusedxml if available otherwise use the core xml, they seem to have identical API? If often used in code, we could also consider creating a wrapper.

[echoix]

Wouldn't it be possible to use defusedxml if available otherwise use the core xml, they seem to have identical API? If often used in code, we could also consider creating a wrapper.

Indeed, if I understand correctly, they copied the entire stdlib (concerning xml), and adapted from there. At least that's how it started