Skip to content

docs: clarify stdlib xml vs defusedxml usage at every import site - #1860

Open
jacalata wants to merge 2 commits into
developmentfrom
jac/xml-import-comments
Open

docs: clarify stdlib xml vs defusedxml usage at every import site#1860
jacalata wants to merge 2 commits into
developmentfrom
jac/xml-import-comments

Conversation

@jacalata

@jacalata jacalata commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Motivation

TSC uses both xml.etree.ElementTree (stdlib) and defusedxml.ElementTree
across the codebase, and the pattern isn't obvious from a fresh reading.
Frontier AI's recent security analysis of all OSS Salesforce repos flagged
these stdlib xml.etree.ElementTree imports as a potential XXE concern.
On investigation, every parsing path (from_response, from_xml inputs)
already uses defusedxml.fromstring for XXE safety; stdlib
xml.etree.ElementTree is retained only for building outbound request
bodies (Element / SubElement / tostring) and as a type annotation
for already-parsed inputs. defusedxml doesn't provide a builder
equivalent, so we can't simplify by using it everywhere.

Inline comments answer the security-analysis question at each import
site so future audits (Frontier AI or human) can confirm the intent
without re-tracing every usage, and so a well-meaning "swap stdlib for
defusedxml" cleanup doesn't accidentally break outbound XML building.

Behavior change

Docs only.

  • One inline comment on every import xml.etree.ElementTree ... line
    (21 files) explaining the intent.
  • Expanded comment on the defusedxml line in pyproject.toml describing
    the same split.

Test plan

  • Full suite passes, mypy + black --check clean.

🤖 Generated with Claude Code

@jacalata @claude
docs: clarify stdlib xml vs defusedxml usage at every import site
a1d0e29 
All XML parsing of server responses already uses defusedxml (safe against
XXE/entity expansion). Stdlib xml is retained only for building outbound
request bodies (no defusedxml equivalent) and for the ParseError exception
type (which defusedxml raises unchanged). Added inline comments at each
import site so future contributors don't replace these with defusedxml
unnecessarily, and updated pyproject.toml dependency comment to explain
the split.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Coverage

Coverage Report
FileStmtsMissCoverMissing
tableauserverclient
   __init__.py50100% 
   config.py150100% 
   datetime_helpers.py2511 96%
   exponential_backoff.py200100% 
   filesys_helpers.py310100% 
   namespace.py2633 88%
tableauserverclient/bin
   __init__.py20100% 
   _version.py358212212 41%
tableauserverclient/helpers
   __init__.py10100% 
   logging.py20100% 
   strings.py3111 97%
tableauserverclient/models
   __init__.py460100% 
   collection_item.py4177 83%
   column_item.py553232 42%
   connection_credentials.py351111 69%
   connection_item.py941414 85%
   custom_view_item.py1442121 85%
   data_acceleration_report_item.py5411 98%
   data_alert_item.py15844 97%
   data_freshness_policy_item.py1551515 90%
   database_item.py2073636 83%
   datasource_item.py3001212 96%
   dqw_item.py10455 95%
   exceptions.py40100% 
   extensions_item.py13244 97%
   extract_item.py4444 91%
   favorites_item.py6988 88%
   fileupload_item.py190100% 
   flow_item.py1491010 93%
   flow_run_item.py710100% 
   group_item.py8966 93%
   groupset_item.py4977 86%
   interval_item.py1823232 82%
   job_item.py1921010 95%
   linked_tasks_item.py7911 99%
   location_item.py2922 93%
   metric_item.py1291313 90%
   oidc_item.py6333 95%
   pagination_item.py3411 97%
   permissions_item.py1111212 89%
   project_item.py2073131 85%
   property_decorators.py1001818 82%
   reference_item.py2622 92%
   revision_item.py5911 98%
   schedule_item.py20966 97%
   server_info_item.py3777 81%
   site_item.py6361313 98%
   subscription_item.py10122 98%
   table_item.py1191818 85%
   tableau_auth.py612525 59%
   tableau_types.py2711 96%
   tag_item.py150100% 
   target.py60100% 
   task_item.py5622 96%
   user_item.py3231616 95%
   view_item.py2201616 93%
   virtual_connection_item.py6488 88%
   webhook_item.py6911 99%
   workbook_item.py3621616 96%
tableauserverclient/server
   __init__.py90100% 
   exceptions.py40100% 
   filter.py2911 97%
   pager.py3311 97%
   query.py1431515 90%
   request_factory.py1335195195 85%
   request_options.py38655 99%
   server.py1882323 88%
   sort.py60100% 
tableauserverclient/server/endpoint
   __init__.py350100% 
   auth_endpoint.py731010 86%
   custom_views_endpoint.py1521212 92%
   data_acceleration_report_endpoint.py210100% 
   data_alert_endpoint.py942323 76%
   databases_endpoint.py1113030 73%
   datasources_endpoint.py3233333 90%
   default_permissions_endpoint.py4433 93%
   dqw_endpoint.py451616 64%
   endpoint.py2592424 91%
   exceptions.py7966 92%
   extensions_endpoint.py310100% 
   favorites_endpoint.py942222 77%
   fileuploads_endpoint.py510100% 
   flow_runs_endpoint.py6299 85%
   flow_task_endpoint.py2122 90%
   flows_endpoint.py1985353 73%
   groups_endpoint.py12699 93%
   groupsets_endpoint.py7277 90%
   jobs_endpoint.py6799 87%
   linked_tasks_endpoint.py370100% 
   metadata_endpoint.py881414 84%
   metrics_endpoint.py5566 89%
   oidc_endpoint.py4211 98%
   permissions_endpoint.py4433 93%
   projects_endpoint.py1782424 87%
   resource_tagger.py1273535 72%
   schedules_endpoint.py1191111 91%
   server_info_endpoint.py361010 72%
   sites_endpoint.py1302727 79%
   subscriptions_endpoint.py561414 75%
   tables_endpoint.py1103636 67%
   tasks_endpoint.py6366 90%
   users_endpoint.py18388 96%
   views_endpoint.py15099 94%
   virtual_connections_endpoint.py1131010 91%
   webhooks_endpoint.py5499 83%
   workbooks_endpoint.py3382222 93%
TOTAL12065142488% 

@jacalata
jacalata requested a lite review from Copilot August 17, 2026 20:03
Copilot AI reviewed Aug 17, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR clarifies the project’s intended split between stdlib xml.etree.ElementTree (XML building/types) and defusedxml.ElementTree (XML parsing), adding inline comments at import/dependency sites to make future security audits and refactors less error-prone.

Changes:

  • Added inline comments on stdlib xml.etree.ElementTree (and related) imports to document “builder-only” usage intent.
  • Added an inline comment on ParseError imports/usages to document compatibility with defusedxml.
  • Expanded the defusedxml dependency comment in pyproject.toml to describe the parsing vs building split.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tableauserverclient/server/request_factory.py Annotates stdlib ElementTree import as intended for request XML building only.
tableauserverclient/server/endpoint/endpoint.py Annotates ParseError import as exception-type-only (shared with defusedxml).
tableauserverclient/models/workbook_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/webhook_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/virtual_connection_item.py Adds builder-only intent comment to stdlib Element import.
tableauserverclient/models/user_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/tag_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/site_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/server_info_item.py Adds comment clarifying stdlib xml import is only for ParseError typing/handling.
tableauserverclient/models/schedule_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/project_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/permissions_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/metric_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/location_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/groupset_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/flow_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/extract_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/datasource_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/data_freshness_policy_item.py Adds builder-only intent comment to stdlib ElementTree import.
tableauserverclient/models/collection_item.py Adds builder-only intent comment to stdlib Element import.
pyproject.toml Expands defusedxml dependency comment to document parsing-vs-building intent.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tableauserverclient/models/metric_item.py Outdated
Comment thread tableauserverclient/models/server_info_item.py Outdated
@jacalata @claude
fix: parse metric server responses with defusedxml, not stdlib ET
0c12c74 
metric_item.py's from_response() was parsing untrusted server response
bytes with xml.etree.ElementTree.fromstring, which is exactly the
scenario defusedxml exists to defend against. Switch to
defusedxml.ElementTree.fromstring so metric responses go through the
same hardened parser as every other from_response path in this package.

Also correct import-site comments to reflect actual usage:
- location_item, data_freshness_policy_item and 14 other models use
  stdlib ET only for type annotations (ET.Element hints, isinstance
  narrowing). Comment now reads "type annotation only; parsing uses
  defusedxml" instead of the inaccurate "building XML request bodies
  only".
- server_info_item.py used a bare `import xml` and reached into
  xml.etree.ElementTree.ParseError, which only worked because
  defusedxml happens to import xml.etree transitively. Replace with an
  explicit `from xml.etree.ElementTree import ParseError` and match the
  except clause. defusedxml.ElementTree.fromstring raises this same
  class for malformed input.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jacalata