Fix bug where astropy writes XML files that it cannot read - #20469
Open
jdavies-st wants to merge 6 commits into
Open
jdavies-st wants to merge 6 commits into
jdavies-st wants to merge 6 commits into
Conversation
- Check text against the characters XML allows before escaping it, so writing a field name, attribute, description or table cell containing one of them raises ValueError. Before, most were written out verbatim, producing a file that astropy could not read back. A null was deleted by the escaper wherever it appeared, and in description text a vertical tab or form feed became a space, so those altered the data with no error at all. - Reject bad XML characters in XMLWriter._flush() before textwrap.fill() runs and whenever xml_cleaning_method() has replaced the escaper, since none of the wrapped text or the alternate xml cleaning methods (bleach.clean, or the "none" option) would notice. - Name the attribute, element, or row and column in the error. The C table writer cannot say which cell it was on, so _write_tabledata() locates it after the failure, which costs nothing unless a write has already failed, and reports it identically to the Python table writer. - Fix a null byte in bytes input being deleted rather than written out. The list of characters the escaper replaces ends in a marker entry that uses a null, and a real null in the input matched that marker and was replaced with nothing. The loops now check for the end of the list before comparing. - Leave bytes input escaped but unchecked, since its text encoding is not known, and say so in the docstrings. These functions are for text. - Only the TABLEDATA serialization format is affected, since it stores values as XML text. BINARY and BINARY2 store them base64 encoded, so these characters are representable there and are still written. - Add unit tests to check all these cases.
Contributor
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
- PyUnicode_GET_LENGTH, PyUnicode_KIND, PyUnicode_DATA and PyUnicode_READ are not part of the limited API. - Use PyUnicode_GetLength and PyUnicode_ReadChar instead for the saem behavior.
pllim
requested review from
ManonMarchand,
gilleslandais,
stvoutsin and
tomdonaldson
September 23, 2026 15:01
2 tasks
stvoutsin
reviewed
Sep 24, 2026
| ``ValueError`` if a field name, attribute, description or table cell contains | ||
| a character that XML does not allow, instead of writing a file that cannot be | ||
| read back. Nulls, which were previously dropped without warning, are also | ||
| refused. The ``BINARY`` and ``BINARY2`` formats are unaffected, since they |
Contributor
There was a problem hiding this comment.
Minor nit, but I think BINARY2 and BINARY2 are unaffected in terms of the cell data, but the field names, units, UCDs and descriptions of VOTables with binary serialized data will still go through XMLWriter so a bad character would still raise an exception, so perhaps worth slightly rewording that sentence to clarify that?
Contributor
Author
There was a problem hiding this comment.
Doh! Yes, exactly. And I ran a small test to confirm. Only the cell data is exempt in BINARY and BINARY2. I have updated the changelog to
Writing a VOTable now raises a ``ValueError`` if a field name,
description or table cell contains a character that XML does not allow,
instead of writing a file that cannot be read back. Nulls, which were
previously dropped without warning, are also refused. Cell data is exempt in
the ``BINARY`` and ``BINARY2`` serialization formats, which store values
base64 encoded rather than as XML text; the names, units, UCDs and
descriptions of such a table are still written as XML and are still checked.
jdavies-st
force-pushed
the
bugfix-votable-xml-roundtrip
branch
from
September 24, 2026 21:20
d1a0abe to
e2ab3e8
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
XML forbids certain characters from appearing anywhere in a document. Astropy enforces this when reading, because
expatdoes, but not when writing. So astropy would happily write files it could not read back, and in some cases quietly alter the data instead.This PR checks characters on the way out. A field name, attribute, description or table cell containing one now raises
ValueErrorinstead of producing a broken file.What users will see
ValueError, naming the character and where it was: the attribute, the element, or the row and column. Previously most were written verbatim and the file could not be parsed afterwards.TABLEDATAserialization is affected, because it stores values as XML text.BINARYandBINARY2base64 encode them, so these characters are representable there and are still written.Implementation note
The check piggybacks on the escaper/sanitizer that already walks every value on its way out, rather than adding a second pass, so it costs nothing measurable.
This follows #20439, which fixed a similar class of problem for XML tokens. It does not address
check_token()and its callers silently discarding what the check returns; that should be a separate PR. And that likely requires care, as readers need to not raise, but the writer should probably warn.Fixes #20457
AI Disclosure
Claude Opus 5.5 was used to investigate, iterate and write fixes in this PR. I have carefully reviewed, modified where needed and tested the changes.
Merge method