Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Prevent access to external files/URLs via XML entity references.

  1. WIP - xmlvalidate implementation from TODO list

    Marcos Magueta <maguetamarcos@gmail.com> — 2025-12-06T23:38:24Z

    Hello!

I am likely one of the few people still using XML, but I noticed XSD schema
validation is still a TODO on postgresql, which I have some personal use
cases for.

In this patch I attempt to implement XMLVALIDATE with the already in use
libxml following a version I got my hands on of the SQL/XML standard of
2016.

In short, I had to add the ACCORDING word to comply with it and completely
ignored the version that was already in the src that fetches arbitrary
schemas (it refers to validations of dtds, which is another more
troublesome TODO).

I had problems running the regression tests on my machine, so I could only
test the feature by spawning a modified instance of postgresql and issuing
queries through psql, therefore I am marking it as WIP. If anyone can
assert the tests pass, I would be glad.

Also, this is my first patch, so I might have not followed standard
practices as best as I could, so please pay particular attention to that on
review.

Cheers,
Marcos Magueta.

  2. Re: WIP - xmlvalidate implementation from TODO list

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-07T07:34:22Z

    On Sun, 7 Dec 2025 at 04:38, Marcos Magueta <maguetamarcos@gmail.com> wrote:
>
> Hello!
>
> I am likely one of the few people still using XML, but I noticed XSD schema validation is still a TODO on postgresql, which I have some personal use cases for.
>
> In this patch I attempt to implement XMLVALIDATE with the already in use libxml following a version I got my hands on of the SQL/XML standard of 2016.
>
> In short, I had to add the ACCORDING word to comply with it and completely ignored the version that was already in the src that fetches arbitrary schemas (it refers to validations of dtds, which is another more troublesome TODO).
>
> I had problems running the regression tests on my machine, so I could only test the feature by spawning a modified instance of postgresql and issuing queries through psql, therefore I am marking it as WIP. If anyone can assert the tests pass, I would be glad.
>
> Also, this is my first patch, so I might have not followed standard practices as best as I could, so please pay particular attention to that on review.
>
> Cheers,
> Marcos Magueta.

HI!

1)
> + // Default case since nothing got returned
> + // out of the normal path for validation calls to libxml

PostgreSQL uses /**/ comments style.

2)
XML regression test suite fails, see attached. By the way, what are
your issues with running `make check` ?

3)
By the way, in [0] we have this

`
The function in PostgreSQL produces an “unimplemented” error, because
PostgreSQL does not have any implementation of the mechanism assumed
in the standard for registering schemas in advance, which is necessary
to address the security implications of a function that could refer to
schemas from arbitrary locations.
`

How does your patch resolve this? I did not find any change in this area

4)
Also I want to mention that without a doc, the patch is not in a
commitable shape.

5) I am a bit surprised by this grammar rule:

>  XMLVALIDATE '(' document_or_content a_expr ACCORDING TO XMLSCHEMA a_expr ')'

this allow a wide class of expressions accepted by parser, like

`SELECT xmlvalidate(DOCUMENT  (select oid from pg_class) ACCORDING TO
XMLSCHEMA (select 32)) AS is_valid FROM xml_validation_test;`

Is this expected? a_expr is way more than string constants and column
references..  If yes, the regression test that you added, does not
test this..


p.s. I failed to find in google SQL/XML standard of 2016. So, I cannot
double-check if this feature is compliant with it...

[0] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards

-- 
Best regards,
Kirill Reshke

  3. Re: WIP - xmlvalidate implementation from TODO list

    Marcos Magueta <maguetamarcos@gmail.com> — 2025-12-07T19:43:40Z

    Thank you for your kind review!

Before I continue with the implementation, I would like to address
your concerns and discuss it further and see if it's worth carrying
on.

1) Will do!

2) The issue is the following:
```
./../../src/test/regress/pg_regress --temp-instance=./tmp_check
--inputdir=. --bindir=     --dlpath=. --max-concurrent-tests=20
 --schedule=./parallel_schedule
# +++ regress check in src/test/regress +++
# initializing database system by copying initdb template
# could not exec "sh": No such file or directory
Bail out!# postmaster failed, examine
```

This is likely due to my setup on Nix. If any command assumes paths on
conventional *nix I am often in trouble. I am checking that with a
friend, but any insights are welcome. The out file I generated was
while being completely blindfolded.

By the way, the diff you sent is assuming global paths for some
reason, so I couldn't apply it without manually changing them.

3) From what I understand, that refers to ISO/IEC 9075-14:2016, chapter and
section 11.6, page 245:
 - Parts of the grammar that reference an URI: <XML valid according to
what> ::= <XML valid according to URI> | <XML valid according to identifier>
 - NO NAMESPACE, which unspecifies the qualification but can access
something through a LOCATI ON
 - ID which should allow access to a registered schema
So they amount to:
  ACCORDING TO XMLSCHEMA URI <uri> [LOCATION <uri>]
  ACCORDING TO XMLSCHEMA NO NAMESPACE [LOCATION <uri>]
  ACCORDING TO XMLSCHEMA ID <registered_schema_name>

What I did is rely on the protection mechanisms that are already
implemented to just side-step the issue of arbitrary retrieval.

`PgXmlErrorContext * pg_xml_init(PgXmlStrictness strictness)` starts
the xml error context preventing any attempt by libxml2 to load an
external entity (DTD, XSD from URL, local file, etc.) returns an empty
string instead. Check around the line 1420 of xml.c:

```
/*
 * Also, install an entity loader to prevent unwanted fetches of external
 * files and URLs.
 */
errcxt->saved_entityfunc = xmlGetExternalEntityLoader();
xmlSetExternalEntityLoader(xmlPgEntityLoader);
```

So since I am relying on a TEXT for the schema, there should be no
issues of that sort. It does however cut part of the grammar that
handles locations, which is part of the standard, and would require
this feature to be much bigger in scope...

4) I suppose you refer to doc/src/sgml/func/func-xml.sgml. Will do

5) Hmm my intent was to simply handle TEXT on the xmlschema portion,
so the expr rule on that side is indeed an oversight. Now about the
first argument, that is just following the pattern already specified
in other xml functions, like XMLPARSE and XMLSERIALIZE, which have the
same <XML Value Expression> specified in the grammar. So that might
have alraedy diverged from the standard a while back...

This is currently grammatically valid, for example:
```
select xmlparse(DOCUMENT  (select oid from pg_class limit 1));
ERROR:  invalid XML document
DETAIL:  line 1: Start tag expected, '<' not found
2619
^
```

As a summary, it does not fully implement schemas as first-class
objects, as that would require extra parts of the grammar specified in
the standard, so I capitulated to use schemas as provided text. That's
already safe in my understanding given the shielding in place. If we
are to implement the rest, I think other serious concerns would arise,
like role management, how to store schemas, etc. And when it comes
to that, is it worth all the trouble just for xml? I would like this
feature and I think the solution of relying on text is decent, since
the cost of complying 100% seems very high for low returns.

Em dom., 7 de dez. de 2025 às 04:34, Kirill Reshke <reshkekirill@gmail.com>
escreveu:

> On Sun, 7 Dec 2025 at 04:38, Marcos Magueta <maguetamarcos@gmail.com>
> wrote:
> >
> > Hello!
> >
> > I am likely one of the few people still using XML, but I noticed XSD
> schema validation is still a TODO on postgresql, which I have some personal
> use cases for.
> >
> > In this patch I attempt to implement XMLVALIDATE with the already in use
> libxml following a version I got my hands on of the SQL/XML standard of
> 2016.
> >
> > In short, I had to add the ACCORDING word to comply with it and
> completely ignored the version that was already in the src that fetches
> arbitrary schemas (it refers to validations of dtds, which is another more
> troublesome TODO).
> >
> > I had problems running the regression tests on my machine, so I could
> only test the feature by spawning a modified instance of postgresql and
> issuing queries through psql, therefore I am marking it as WIP. If anyone
> can assert the tests pass, I would be glad.
> >
> > Also, this is my first patch, so I might have not followed standard
> practices as best as I could, so please pay particular attention to that on
> review.
> >
> > Cheers,
> > Marcos Magueta.
>
> HI!
>
> 1)
> > + // Default case since nothing got returned
> > + // out of the normal path for validation calls to libxml
>
> PostgreSQL uses /**/ comments style.
>
> 2)
> XML regression test suite fails, see attached. By the way, what are
> your issues with running `make check` ?
>
> 3)
> By the way, in [0] we have this
>
> `
> The function in PostgreSQL produces an “unimplemented” error, because
> PostgreSQL does not have any implementation of the mechanism assumed
> in the standard for registering schemas in advance, which is necessary
> to address the security implications of a function that could refer to
> schemas from arbitrary locations.
> `
>
> How does your patch resolve this? I did not find any change in this area
>
> 4)
> Also I want to mention that without a doc, the patch is not in a
> commitable shape.
>
> 5) I am a bit surprised by this grammar rule:
>
> >  XMLVALIDATE '(' document_or_content a_expr ACCORDING TO XMLSCHEMA
> a_expr ')'
>
> this allow a wide class of expressions accepted by parser, like
>
> `SELECT xmlvalidate(DOCUMENT  (select oid from pg_class) ACCORDING TO
> XMLSCHEMA (select 32)) AS is_valid FROM xml_validation_test;`
>
> Is this expected? a_expr is way more than string constants and column
> references..  If yes, the regression test that you added, does not
> test this..
>
>
> p.s. I failed to find in google SQL/XML standard of 2016. So, I cannot
> double-check if this feature is compliant with it...
>
> [0] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards
>
> --
> Best regards,
> Kirill Reshke
>

  4. Re: WIP - xmlvalidate implementation from TODO list

    Marcos Magueta <maguetamarcos@gmail.com> — 2025-12-19T03:25:51Z

    Hello again!

I took some time to actually finish this feature. I think the answers
for the previous questions are now clearer. I checked the
initialization and the protections are indeed in place since commit
a4b0c0aaf093a015bebe83a24c183e10a66c8c39, which specifically states:

> Prevent access to external files/URLs via XML entity references.

> xml_parse() would attempt to fetch external files or URLs as needed to
> resolve DTD and entity references in an XML value, thus allowing
> unprivileged database users to attempt to fetch data with the privileges
> of the database server.  While the external data wouldn't get returned
> directly to the user, portions of it could be exposed in error messages
> if the data didn't parse as valid XML; and in any case the mere ability
> to check existence of a file might be useful to an attacker.
>
> The ideal solution to this would still allow fetching of references that
> are listed in the host system's XML catalogs, so that documents can be
> validated according to installed DTDs.  However, doing that with the
> available libxml2 APIs appears complex and error-prone, so we're not going
> to risk it in a security patch that necessarily hasn't gotten wide review.
> So this patch merely shuts off all access, causing any external fetch to
> silently expand to an empty string.  A future patch may improve this.

With that, the obvious affordance on the xmlvalidate implementation
was to not rely on external schema sources on the host
catalog. Therefore the implementation relies solely on expressions
that necessarily evaluate to a schema in plain text.

I added the requested documentation and a bunch of tests for each
scenario. I would appreciate another round of reviews whenever someone
has the time and patience.

At last, to nourish the curiosity: I had issues with make check, as
stated above on the e-mail thread. These got resolved when I changed
`execl` to `execlp` on `pg_regress.c`. I of course did not commit
such, but more people I know have had the very same issue while
relying on immutable package managers.

  5. Re: WIP - xmlvalidate implementation from TODO list

    Marcos Magueta <maguetamarcos@gmail.com> — 2025-12-31T20:26:55Z

    Hello again!

Is there any interest in this? I understand PostgreSQL has bigger fish to
fry, but I would like to at least know; in case this was just forgotten.

Regards!

Em sex., 19 de dez. de 2025 às 00:25, Marcos Magueta <
maguetamarcos@gmail.com> escreveu:

> Hello again!
>
> I took some time to actually finish this feature. I think the answers
> for the previous questions are now clearer. I checked the
> initialization and the protections are indeed in place since commit
> a4b0c0aaf093a015bebe83a24c183e10a66c8c39, which specifically states:
>
> > Prevent access to external files/URLs via XML entity references.
>
> > xml_parse() would attempt to fetch external files or URLs as needed to
> > resolve DTD and entity references in an XML value, thus allowing
> > unprivileged database users to attempt to fetch data with the privileges
> > of the database server.  While the external data wouldn't get returned
> > directly to the user, portions of it could be exposed in error messages
> > if the data didn't parse as valid XML; and in any case the mere ability
> > to check existence of a file might be useful to an attacker.
> >
> > The ideal solution to this would still allow fetching of references that
> > are listed in the host system's XML catalogs, so that documents can be
> > validated according to installed DTDs.  However, doing that with the
> > available libxml2 APIs appears complex and error-prone, so we're not
> going
> > to risk it in a security patch that necessarily hasn't gotten wide
> review.
> > So this patch merely shuts off all access, causing any external fetch to
> > silently expand to an empty string.  A future patch may improve this.
>
> With that, the obvious affordance on the xmlvalidate implementation
> was to not rely on external schema sources on the host
> catalog. Therefore the implementation relies solely on expressions
> that necessarily evaluate to a schema in plain text.
>
> I added the requested documentation and a bunch of tests for each
> scenario. I would appreciate another round of reviews whenever someone
> has the time and patience.
>
> At last, to nourish the curiosity: I had issues with make check, as
> stated above on the e-mail thread. These got resolved when I changed
> `execl` to `execlp` on `pg_regress.c`. I of course did not commit
> such, but more people I know have had the very same issue while
> relying on immutable package managers.
>