ghoshp83 commented on issue #3973:
URL:
https://github.com/apache/iceberg-python/issues/3973#issuecomment-5731739102
Both code paths are as described — `LocationProvider.__init__` takes
`write.data.path` / `write.metadata.path` verbatim, and `purge_table` follows
the `file_path` entries in the manifests into `delete_data_files`.
Before writing anything I checked this against `SECURITY-THREAT-MODEL.md`,
and I
think the document already answers part of the question:
> Containment of what a writer commits — that manifest entries and write
paths
> stay inside the table's location — is enforced by the catalog, not by this
> client. A report showing that Iceberg Python follows a path the catalog
> accepted is a hardening opportunity here and a containment question there.
So a change here would be hardening, not a fix to a client-side containment
guarantee, since the document says there is not one. That framing matters for
what the change should look like, and it leaves three questions I do not
think a
contributor should answer unilaterally:
1. **Should either path check be on by default?** The issue notes that
redirecting writes is the documented purpose of `write.data.path` and
`write.metadata.path`. A containment check that is on by default would
break
that documented use for everyone relying on it, so I would expect this to
be
opt-in — but that is your call, not mine.
2. **Warn or raise?** For `purge_table` in particular, raising turns a
partially-completed purge into a hard failure with files already deleted,
which may be worse than logging and continuing. A warning that names the
out-of-location path may be the more useful behaviour.
3. **What counts as "contained"?** Prefix matching on the location string is
the
cheap answer and is wrong in both directions: `s3://bucket/table` prefix
matches `s3://bucket/table-backup`, and it does not handle differing
schemes
for the same store (`s3://` vs `s3a://`), trailing slashes, case, or
percent
encoding. Doing this properly means a normalising comparison, and it is
worth
agreeing on the rule before it is written.
My suggestion, if it is useful: treat the two halves separately. The
`purge_table` side is the one with real blast radius, since it deletes with
the
caller's credentials, and a default-on warning there would be a small change
that costs nobody anything. The `LocationProvider` side is the one where a
check
conflicts with a documented feature, and I would leave it opt-in or leave it
alone.
Happy to implement whichever shape you would like — I would just rather agree
the behaviour first than send a PR that presumes an answer to the three
questions above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
