Skip to content

MLE-34177 fix multipart header and parameter injection in document write - #150

Open
RitaChen609 wants to merge 2 commits into
developfrom
MLE-34177-multipart-header-and-parameter-injection
Open

RitaChen609 wants to merge 2 commits into
developfrom
MLE-34177-multipart-header-and-parameter-injection

Conversation

@RitaChen609

Copy link
Copy Markdown

Summary

Document write paths built multipart Content-Disposition headers with unescaped f-strings. Caller-supplied values could inject extra disposition parameters, extra header lines, or a blank line that ends the part's headers early. Examples are a URI containing \r\n, or a repair value containing ;.

Security review finding F1. CWE-93, CWE-113, CWE-74.
CVSS 3.1: AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:L/A:N = 4.3 (Medium). This assumes an application that passes untrusted input into Document.

Root cause

  • Metadata part: to_metadata_request_field rendered filename={self.uri} raw and unquoted. urllib3 also appends its own escaped filename="...", so the header carried two filename parameters, and the first one was injectable.
  • Content part: _make_content_disposition appended extension, directory, repair, extract, version_id and temporal_document as raw ;key=value.
  • Not affected: in the content part, the URI was already safe, because urllib3 escapes the name/filename it renders itself.

Changes (marklogic/documents.py)

  • Metadata part: the raw filename={uri} is removed. The disposition is now attachment; category=metadata, and the filename comes only from urllib3's escaped filename="...", the same as the content part.
  • URI: a write raises ValueError if the URI contains control characters (\x00-\x1f, \x7f). Validation happens only on the write path, so reading documents is unchanged.
  • repair: only full or none are accepted, as already documented.
  • extension, directory, extract, version_id, temporal_document: these raise ValueError if they contain control characters, ; or ".
    • version_id may be wrapped in one pair of quotes, so a raw ETag such as "12345" still works. The existing test_optimistic_locking relies on this.

Behavior change

Values that were previously sent unvalidated now raise ValueError before any request is made:

  • a repair value other than full or none;
  • a URI containing control characters;
  • a parameter value containing ;, " or control characters.

Legitimate URIs that contain ; or " still work.

Copilot AI balanced review requested due to automatic review settings November 6, 2026 19:12

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.

🟡 Changes recommended

Quoted version IDs ending in a backslash can escape the emitted header’s closing quote, allowing URI content to inject separate disposition parameters.

1 open issue
What changed in this PR

This PR hardens Document multipart writes against header and parameter injection while preserving read behavior and support for quoted version IDs.

Changes:

  • Removes the raw metadata filename parameter in favor of urllib3’s escaped filename.
  • Validates write-path URIs, disposition parameters, and supported repair values.
  • Adds injection regression tests and integration tests for valid document writes.
File Description
tests/​test_write_documents_injection.py Tests rejected inputs, multipart rendering, and valid write scenarios.
marklogic/​documents.py Adds write-time validation and removes unsafe metadata filename rendering.

Review effort: Balanced

Comment thread marklogic/documents.py
Comment on lines +39 to +40
if name == "versionId" and len(value) >= 2 and value[0] == value[-1] == '"':
to_check = value[1:-1]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants