Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Apart from `Builder::add_file`, you can use:
- `Builder::add_directory` to add a single directory (just the directory, not any contents)
- `Builder::add_directory_all` to recursively add an on-filesystem directory

See `ArchiveBuilder::builder_with_policy` for policy knobs that
See `Builder::with_policy` for policy knobs that
can be changed during building.

Note that `tar-codec` does **not** perform compression for you.
Expand All @@ -61,7 +61,7 @@ TarArchive::new(reader)

Unlike encoding, decoding/extraction has two policy layers:

- Use `TarArchive::new_with_policy` to control various aspects of GNU or pax handling.
- Use `TarArchive::with_policy` to control various aspects of GNU or pax handling.
- Use `extract::ExtractPolicy` to control various aspects of how archives become
real paths on the host filesystem.

Expand Down
24 changes: 10 additions & 14 deletions crates/archive-trait/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,7 @@ impl<E> BuildFailure<E> {
/// The asynchronous methods on this trait are implementation hooks for
/// [`Builder`]. Archive construction callers must not invoke them directly;
/// doing so bypasses builder policy, collision tracking, and cancellation
/// poisoning. Use [`Self::builder`] or [`Self::builder_with_policy`] and then
/// the [`Builder`] APIs instead.
/// poisoning. Use [`Self::builder`] and then the [`Builder`] APIs instead.
///
/// Hook implementations must return [`BuildFailure::recoverable`] only when the
/// failed invocation wrote no output. Any failure after output may have begun
Expand All @@ -362,16 +361,6 @@ pub trait ArchiveBuilder: Sized {
}
}

/// Wraps this format writer in a builder using `policy`.
///
/// Implementors should not override this default implementation.
fn builder_with_policy(self, policy: BuilderPolicy) -> Builder<Self> {
Builder {
backend: self,
state: BuilderState::new(policy),
}
}

/// Writes any format-specific archive terminator or index.
async fn finish_archive(&mut self) -> Result<(), BuildFailure<Self::Error>>;

Expand Down Expand Up @@ -400,14 +389,21 @@ pub trait ArchiveBuilder: Sized {

/// A stateful format-neutral archive construction engine.
///
/// Create this wrapper with [`ArchiveBuilder::builder`] or
/// [`ArchiveBuilder::builder_with_policy`].
/// Create this wrapper with [`ArchiveBuilder::builder`].
pub struct Builder<B> {
backend: B,
state: BuilderState,
}

impl<B: ArchiveBuilder> Builder<B> {
/// Configures the policy used by this builder.
///
/// Call before adding any entries.
pub fn with_policy(mut self, policy: BuilderPolicy) -> Self {
self.state.policy = policy;
self
}

/// Adds one regular file from a [`FilePayload`].
///
/// If the payload ends before its declared size or returns an error, the
Expand Down
13 changes: 8 additions & 5 deletions crates/archive-trait/tests/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ async fn name_validation_supports_default_custom_and_disabled_policies() {
];

for (policy, path, accepted, context) in policies {
let mut builder = MockFormat::new().builder_with_policy(policy);
let mut builder = MockFormat::new().builder().with_policy(policy);
let result = builder
.add_file(path, b"".as_slice(), EntryMetadata::default())
.await;
Expand Down Expand Up @@ -543,7 +543,7 @@ async fn recursive_build_applies_symlink_policy() {
let preserve = BuilderPolicy::default().symlink_policy(SymlinkPolicy::Preserve);
let format = MockFormat::new();
let entries = format.entries();
let mut builder = format.builder_with_policy(preserve);
let mut builder = format.builder().with_policy(preserve);
builder
.add_directory_all(&source)
.await
Expand All @@ -564,7 +564,8 @@ async fn recursive_build_applies_symlink_policy() {
}));
assert!(matches!(
MockFormat::new()
.builder_with_policy(policy)
.builder()
.with_policy(policy)
.add_directory_all(&source)
.await,
Err(BuildError::Traversal(TraversalError::NameRejected {
Expand All @@ -576,7 +577,8 @@ async fn recursive_build_applies_symlink_policy() {
std::fs::remove_file(source.join("custom")).expect("custom link should be removed");
symlink(" leading", source.join("disabled")).expect("disabled-policy link should be created");
MockFormat::new()
.builder_with_policy(preserve.name_validator(None))
.builder()
.with_policy(preserve.name_validator(None))
.add_directory_all(&source)
.await
.expect("disabled validation should accept the link target");
Expand Down Expand Up @@ -631,7 +633,8 @@ async fn recursive_build_reports_non_utf8_and_unsupported_sources() {
.expect("non-UTF-8 symbolic link should be created");
assert!(matches!(
MockFormat::new()
.builder_with_policy(BuilderPolicy::default().symlink_policy(SymlinkPolicy::Preserve),)
.builder()
.with_policy(BuilderPolicy::default().symlink_policy(SymlinkPolicy::Preserve))
.add_directory_all(&source)
.await,
Err(BuildError::Traversal(
Expand Down
115 changes: 81 additions & 34 deletions crates/tar-codec/src/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use archive_trait::{
SpecialKind,
};
use tar_framing::{
ArchiveFormat, FrameError, PaxKeyword, PaxKind, PaxRecord, PaxValue, UstarKind,
ArchiveFormat, FrameError, PaxKeyword, PaxKind, PaxRecord, PaxValue, StreamPolicy, UstarKind,
logical::{MemberExtensions, MemberFrame, MemberPayload as FramingMemberPayload, TarReader},
};
use thiserror::Error;
Expand All @@ -31,28 +31,32 @@ pub struct TarArchive<R> {
impl<R> TarArchive<R> {
/// Creates an archive decoder from an uncompressed tar reader.
pub fn new(reader: R) -> Self {
Self::new_with_policy(reader, DecodePolicy::default())
}

/// Creates an archive decoder using `policy`.
pub fn new_with_policy(reader: R, policy: DecodePolicy) -> Self {
let mut reader = TarReader::new(reader);
reader.set_max_pax_extension_size(policy.pax_policy.max_extension_size);
reader.set_max_global_pax_extensions_size(policy.pax_policy.max_global_extensions_size);
reader.set_allow_all_nul_numeric_fields(policy.allow_all_nul_numeric_fields);
reader.set_max_gnu_extension_size(policy.max_gnu_extension_size);
Self {
reader,
policy,
reader: TarReader::new(reader),
policy: DecodePolicy::default(),
fused: false,
}
}

/// Configures the decoding policy used by this archive.
///
/// Call before reading any members.
pub fn with_policy(mut self, policy: DecodePolicy) -> Self {
let stream_policy = StreamPolicy::default()
.max_pax_extension_size(policy.pax_policy.max_extension_size)
.max_global_pax_extensions_size(policy.pax_policy.max_global_extensions_size)
.allow_all_nul_numeric_fields(policy.allow_all_nul_numeric_fields)
.max_gnu_extension_size(policy.max_gnu_extension_size);
self.reader = self.reader.with_policy(stream_policy);
self.policy = policy;
self
}
}

/// Controls tar compatibility and the feature subset member decoding may accept.
///
/// See each configuration API for its default.
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Debug)]
pub struct DecodePolicy {
allow_gnu: bool,
allow_all_nul_numeric_fields: bool,
Expand All @@ -63,25 +67,60 @@ pub struct DecodePolicy {
/// Controls pax compatibility and the feature subset member decoding may accept.
///
/// See each allow API for its default.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct PaxDecodePolicy {
max_extension_size: u64,
max_global_extensions_size: u64,
allow_non_utf8_pax_vendor_values: bool,
allow_global_pax_extensions: bool,
allow_unknown_pax_vendor_records: bool,
vendor_extension_policy: PaxVendorExtensionPolicy,
allow_duplicate_pax_records: bool,
allow_global_pax_member_metadata: bool,
}

/// Controls which vendor-namespaced pax records may be ignored during decoding.
#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub enum PaxVendorExtensionPolicy {
/// Reject every unknown vendor-namespaced pax record.
#[default]
RejectUnknown,
/// Ignore only records whose complete keywords appear in this allowlist.
///
/// Keywords include the vendor namespace, such as `Acme.attribute`.
Ignore(PaxVendorExtensionAllowlist),
/// Ignore every unknown vendor-namespaced pax record.
///
/// Unknown vendor semantics can affect the archive's intended contents.
AllowUnknown,
}

impl PaxVendorExtensionPolicy {
/// Ignores vendor records whose complete keywords appear in `keywords`.
///
/// Keywords include the vendor namespace, such as `Acme.attribute`.
pub fn ignore(keywords: impl IntoIterator<Item = &'static str>) -> Self {
Self::Ignore(PaxVendorExtensionAllowlist {
keywords: keywords.into_iter().collect(),
})
}
}

/// An opaque allowlist of vendor-namespaced pax record keywords.
///
/// Construct an allowlist with [`PaxVendorExtensionPolicy::ignore`].
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct PaxVendorExtensionAllowlist {
keywords: HashSet<&'static str>,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

NB: I don't think it's worth it for now, but in principle this should be something like HashSet<Namespaced> where Namespaced is:

struct Namespaced {
    namespace: Arc<str>,
    name: Arc<str>,
}

...and then we could reuse our PaxRecord::parse_namespaced machinery.

}

impl Default for PaxDecodePolicy {
fn default() -> Self {
Self {
max_extension_size: DEFAULT_MAX_PAX_EXTENSION_SIZE,
max_global_extensions_size: DEFAULT_MAX_GLOBAL_PAX_EXTENSIONS_SIZE,
allow_non_utf8_pax_vendor_values: true,
allow_global_pax_extensions: true,
allow_unknown_pax_vendor_records: false,
vendor_extension_policy: PaxVendorExtensionPolicy::default(),
allow_duplicate_pax_records: false,
allow_global_pax_member_metadata: false,
}
Expand Down Expand Up @@ -232,8 +271,8 @@ impl PaxDecodePolicy {
/// record value to be valid UTF-8. Vendor values remain exposed as opaque
/// bytes in either mode.
///
/// [`Self::allow_unknown_pax_vendor_records`] separately controls whether
/// decoding may ignore vendor records after they have been parsed.
/// [`Self::vendor_extension_policy`] separately controls whether decoding
/// may ignore vendor records after they have been parsed.
pub fn allow_non_utf8_pax_vendor_values(mut self, allow: bool) -> Self {
self.allow_non_utf8_pax_vendor_values = allow;
self
Expand All @@ -252,22 +291,23 @@ impl PaxDecodePolicy {
self
}

/// Configures whether unknown vendor-namespaced pax records may be accepted.
/// Configures which unknown vendor-namespaced pax records may be ignored.
///
/// When enabled, well-formed vendor-namespaced pax records do not cause a
/// decoding error. Their values are parsed structurally, but their semantics
/// are not interpreted.
/// [`PaxVendorExtensionPolicy::Ignore`] accepts only explicitly listed
/// complete keywords, while [`PaxVendorExtensionPolicy::AllowUnknown`]
/// accepts every vendor-namespaced record. Accepted values are parsed
/// structurally, but their semantics are not interpreted.
///
/// This can produce output that differs from the archive's intended
/// contents. For example, `GNU.sparse.*` records can change a member's
/// effective name, logical size, and mapping from stored payload bytes to
/// file contents; these semantics are ignored when this option is enabled.
///
/// **IMPORTANT**: Only enable this when silently ignoring unknown vendor
/// semantics is acceptable. Unknown vendor-namespaced pax records are
/// **forbidden by default**.
pub fn allow_unknown_pax_vendor_records(mut self, allow: bool) -> Self {
self.allow_unknown_pax_vendor_records = allow;
/// **IMPORTANT**: Only permit records whose ignored semantics are
/// acceptable. Unknown vendor-namespaced pax records are **forbidden by
/// default**.
pub fn vendor_extension_policy(mut self, policy: PaxVendorExtensionPolicy) -> Self {
self.vendor_extension_policy = policy;
self
}

Expand Down Expand Up @@ -317,7 +357,14 @@ impl PaxDecodePolicy {
value,
} = record
{
if !self.allow_unknown_pax_vendor_records {
let allowed = match &self.vendor_extension_policy {
PaxVendorExtensionPolicy::RejectUnknown => false,
PaxVendorExtensionPolicy::Ignore(allowed) => allowed
.keywords
.contains(format!("{vendor}.{name}").as_str()),
PaxVendorExtensionPolicy::AllowUnknown => true,
};
if !allowed {
return Err(DecodeError::policy_violation(
position,
DecodePolicyViolation::PaxVendorExtension {
Expand Down Expand Up @@ -485,10 +532,10 @@ impl<R: AsyncRead + Unpin> ArchiveTrait for TarArchive<R> {
&'a mut self,
) -> Result<Option<Member<Self::Payload<'a>>>, Self::Error> {
#[inline]
async fn decode_next_member<R: AsyncRead + Unpin>(
reader: &mut TarReader<R>,
policy: DecodePolicy,
) -> Result<Option<Member<TarMemberPayload<'_, R>>>, DecodeError> {
async fn decode_next_member<'a, R: AsyncRead + Unpin>(
reader: &'a mut TarReader<R>,
policy: &DecodePolicy,
) -> Result<Option<Member<TarMemberPayload<'a, R>>>, DecodeError> {
let Some(frame) = reader.next_frame().await? else {
return Ok(None);
};
Expand All @@ -500,7 +547,7 @@ impl<R: AsyncRead + Unpin> ArchiveTrait for TarArchive<R> {
return Ok(None);
}

let result = decode_next_member(&mut self.reader, self.policy).await;
let result = decode_next_member(&mut self.reader, &self.policy).await;
self.fused = !matches!(&result, Ok(Some(_)));
result
}
Expand Down
3 changes: 2 additions & 1 deletion crates/tar-codec/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub use archive_trait::{
};
pub use archive_trait::{builder, extract};
pub use decode::{
DecodeError, DecodePolicy, DecodePolicyViolation, PaxDecodePolicy, TarArchive, TarMemberPayload,
DecodeError, DecodePolicy, DecodePolicyViolation, PaxDecodePolicy, PaxVendorExtensionAllowlist,
PaxVendorExtensionPolicy, TarArchive, TarMemberPayload,
};
pub use encode::{EncodeError, TarEncoder};
2 changes: 1 addition & 1 deletion crates/tar-codec/tests/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ async fn recursive_encoding_frames_preserved_symlinks() {

let policy = BuilderPolicy::default().symlink_policy(SymlinkPolicy::Preserve);
let mut bytes = Vec::new();
let mut encoder = TarEncoder::new(&mut bytes).builder_with_policy(policy);
let mut encoder = TarEncoder::new(&mut bytes).builder().with_policy(policy);
encoder
.add_directory_all(&source)
.await
Expand Down
21 changes: 11 additions & 10 deletions crates/tar-codec/tests/members.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,9 @@ async fn all_nul_numeric_fields_are_policy_controlled() -> TestResult {
assert!(members.next().await?.is_none());
}

let mut members =
TarArchive::new_with_policy(bytes.as_slice(), strict_policy).members();
let mut members = TarArchive::new(bytes.as_slice())
.with_policy(strict_policy.clone())
.members();
assert!(
matches!(members.next().await, Err(DecodeError::Framing(_))),
"strict policy should reject an all-NUL {format:?} {field} field"
Expand Down Expand Up @@ -221,9 +222,9 @@ async fn advancing_drains_payload_and_applies_tar_policy() -> TestResult {
let mut archive = ArchiveBuilder::new();
archive.gnu("file", b'0', b"", "", 0o644);
let bytes = archive.finish();
let mut members =
TarArchive::new_with_policy(bytes.as_slice(), DecodePolicy::default().allow_gnu(false))
.members();
let mut members = TarArchive::new(bytes.as_slice())
.with_policy(DecodePolicy::default().allow_gnu(false))
.members();
assert!(matches!(
members.next().await,
Err(DecodeError::PolicyViolation { .. })
Expand All @@ -234,11 +235,11 @@ async fn advancing_drains_payload_and_applies_tar_policy() -> TestResult {
.pax(b'x', &pax_record(PaxKeyword::Comment, "metadata"))
.ustar("file", b'0', b"", "", 0o644);
let bytes = archive.finish();
let mut members = TarArchive::new_with_policy(
bytes.as_slice(),
DecodePolicy::default().pax_policy(PaxDecodePolicy::default().max_extension_size(1)),
)
.members();
let mut members = TarArchive::new(bytes.as_slice())
.with_policy(
DecodePolicy::default().pax_policy(PaxDecodePolicy::default().max_extension_size(1)),
)
.members();
assert!(matches!(members.next().await, Err(DecodeError::Framing(_))));
Ok(())
}
Expand Down
Loading