Skip to content

Add trusted options to XML functions#2656

Open
GuntherRademacher wants to merge 3 commits into
BaseXdb:mainfrom
GuntherRademacher:trusted-xml-parsing
Open

Add trusted options to XML functions#2656
GuntherRademacher wants to merge 3 commits into
BaseXdb:mainfrom
GuntherRademacher:trusted-xml-parsing

Conversation

@GuntherRademacher
Copy link
Copy Markdown
Member

This PR adds support for the XQuery 4.0 trusted option to fn:doc, fn:doc-available, fn:parse-xml, and fn:parse-xml-fragment.

It also renames xsi-schema-location to use-xsi-schema-location (this is a breaking change).

External-resource features are now protected on two levels:

  • Perm.CREATE is required for options that may trigger external access (dtd, dtd-validation, xinclude, and use-xsi-schema-location with active XSD validation)
  • the fn-level trusted option decides whether this access is actually allowed, otherwise FODC0016 is raised

dtd-validation may now trigger external DTD loading independently of dtd.

The implementation-defined default for omitted fn-level trusted is controlled by FNXMLTRUSTED. It defaults to true for backwards-compatible behavior. The QT3 test driver sets FNXMLTRUSTED to false to test the spec default.

@ChristianGruen
Copy link
Copy Markdown
Member

Some feedback:

  • Maybe we need to spend more thoughts on the MainOptions.trusted flag. I think we must at least adopt its value from a parent MainOptions instance (see the resolver variable).
  • trusted must generally be inherited, and false must not be overriden with true.
  • The user management should determine the static trust property: If the current BaseX permission is ADMIN/CREATE, trusted should be true, otherwise false (for example for a client with READ-only permissions).
  • The spec says for fn:json-doc and other functions: “The ability to access external resources depends on whether the calling code is ·trusted.”. Is it planned to tackle this separately?
  • We may need to check whether we want a trusted option for fn:parse-xml-fragment (not provided by the spec).
  • We can rename FNXMLTRUSTED to TRUSTED.

@GuntherRademacher
Copy link
Copy Markdown
Member Author

Thank you for your comments!

  • Maybe we need to spend more thoughts on the MainOptions.trusted flag. I think we must at least adopt its value from a parent MainOptions instance (see the resolver variable).

Not adopting trusted from a parent instance would create a difference, when a parent had set it to false. The default is true, and instances with false are short-lived: they arise in QueryResources.create and FnParseXmlFragment.parse, and they are passed only to parsers, where AFAICS they never serve as parents in copy constructors.
However it seems indeed cleaner to set trusted on constructor level, so I have changed this accordingly, and additionally made it private with a getter (6501414).

  • trusted must generally be inherited, and false must not be overriden with true.

For the same argument as above, this does not currently happen. Yet it feels safer to have it inherited, so the change covers this as well.

  • The user management should determine the static trust property: If the current BaseX permission is ADMIN/CREATE, trusted should be true, otherwise false (for example for a client with READ-only permissions).

The implementation in this PR assumes that checking for Perm.CREATE serves as a way to take the static property into account. Primary external content (e.g. the document to be parsed by fn:doc) is assumed to be guarded by the functions being blocked with a Perm.CREATE tag. Extra external content (entities, xinclude), requires both Perm.CREATE (via Docs.check) and trusted, independently.

  • The spec says for fn:json-doc and other functions: “The ability to access external resources depends on whether the calling code is ·trusted.”. Is it planned to tackle this separately?

Not yet. The functions with a statement like this are:

  • fn:doc
  • fn:collection
  • fn:uri-collection
  • fn:unparsed-text
  • fn:unparsed-text-lines
  • fn:unparsed-text-available
  • fn:unparsed-binary
  • fn:html-doc
  • fn:json-doc
  • fn:csv-doc

All of them are tagged Perm.CREATE, with the exception of fn:collection and fn:uri-collection. These internally require Perm.READ, which seems a bit inconsistent, given the fact that fn:doc wants Perm.CREATE. Also their ability to access extra external resources cannot be controlled by a trusted option.

It might be necessary to change them to

  • require Perm.CREATE,
  • support option trusted.

Should I go ahead and make these changes in the context of this PR?

  • We may need to check whether we want a trusted option for fn:parse-xml-fragment (not provided by the spec).

As fn:parse-xml-fragment does not read DTDs or resolve xinclude, it does not need a trusted option (and the implementation does not provide one).

  • We can rename FNXMLTRUSTED to TRUSTED.

I am hesitant about renaming, because of the overloading of the word trusted.

The spec uses trusted for both the static property and the options. But the static property controls access to any external resources, while the trusted options for fn:doc and fn:parse-xml only relate to extra external resources. In contrast, the trusted option of fn:load-xquery-module relates to the static property of the module.
For making this distinction more obvious, I would rather use a different option name for the trusted options that are added here. Is there a chance to find a new name for them in the spec, which we could then use for the default property as well?

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