Add new zip archive comment check

[alinpahontu2912]

Add check for comment size in zip archive

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Adds validation for ZipArchive.Comment so oversized comments are rejected rather than silently truncated, and introduces a new resource string for the exception message.

Changes:

• ZipArchive.Comment now throws ArgumentException when the encoded comment exceeds ushort.MaxValue bytes.
• Adds SR.CommentTooLong resource string used by the new exception.

Reviewed changes

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

File	Description
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs	Validates encoded archive comment length and throws when it exceeds the EOCD limit.
src/libraries/System.IO.Compression/src/Resources/Strings.resx	Adds a new localized error string for oversized archive comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/libraries/System.IO.Compression/src/Resources/Strings.resx:181

• The new resource string is incorrect/misleading: the ZIP comment length field is a 16-bit byte count (max 65,535 bytes), not “2^16 bits” (which would be 8,192 bytes). Consider updating the message to be accurate and actionable.

    <value>Comments cannot require more than 2^16 bits.</value>

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs:222

• This changes ZipArchive.Comment from silently truncating (per the previous remarks and setter behavior) to throwing when the encoded comment is too long. That’s a behavioral breaking change for a public API; it likely needs explicit compat/breaking-change consideration (and potentially a different approach, e.g., keep truncation or gate the throw behind a switch).

                byte[] encodedComment = ZipHelper.GetEncodedTruncatedBytesFromString(value, EntryNameAndCommentEncoding, 0 /* No truncation */, out _);

                if (encodedComment.Length > ZipEndOfCentralDirectoryBlock.ZipFileCommentMaxLength)
                {
                    throw new ArgumentException(SR.CommentTooLong, nameof(Comment));
                }

src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs:1152

• The new tests only cover ASCII length ==/>(ushort.MaxValue) characters. Since the production check is on encoded byte length, it would be good to add coverage for a multi-byte UTF-8 character pushing the encoded comment over the 65,535-byte limit (even when the character count is below ushort.MaxValue).

            // ALettersUShortMaxValue is exactly ushort.MaxValue ASCII characters, so its encoded
            // length is exactly the maximum allowed archive comment length.
            using MemoryStream ms = new MemoryStream();
            using ZipArchive archive = new ZipArchive(ms, ZipArchiveMode.Create, leaveOpen: true);

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[alinpahontu2912]

@copilot this causes a breaking change since previously the comment was silently truncated. Now an error is thrown

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs:211

• This changes a public API’s behavior from silently truncating an oversized comment (via GetEncodedTruncatedBytesFromString(..., ZipFileCommentMaxLength, ...)) to throwing ArgumentException at set-time. That’s a potentially breaking behavioral change for callers relying on truncation; it likely needs explicit compatibility/breaking-change consideration (and documentation) before merging.

        /// <summary>
        /// Gets or sets the optional archive comment.
        /// </summary>
        /// <remarks>
        /// The comment encoding is determined by the <c>entryNameEncoding</c> parameter of the <see cref="ZipArchive(Stream,ZipArchiveMode,bool,Encoding?)"/> constructor.
        /// </remarks>
        /// <exception cref="ArgumentException">The encoded comment exceeds <see cref="ushort.MaxValue"/> bytes.</exception>
        [AllowNull]

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite