feat: decrypt password-protected OOXML when a password is supplied (#102) - #130
Open
Mr-Neutr0n wants to merge 5 commits into
Open
feat: decrypt password-protected OOXML when a password is supplied (#102)#130Mr-Neutr0n wants to merge 5 commits into
Mr-Neutr0n wants to merge 5 commits into
Conversation
…recrawl#96) Word treats every w:num referencing one w:abstractNum as a single logical list; startOverride exists precisely to break away from it. Counters were keyed per numId instead, so documents whose outline is split across several instances of one abstract restarted at every switch: headings came out as Section 1, 2, 1(!), 1.1(!), 3 ... exactly the reported multiple-section-1 / 4.x-to-1.1 symptom in firecrawl#96. Reapplying the paragraph style clears it because it drops the direct numPr pointing at the second instance, collapsing the document back to one numId. Counters are now keyed by the resolved abstract definition. Entering a list through an instance that carries lvlOverride restarts exactly its overridden levels (nested w:lvl or startOverride); everything else keeps counting. The real-world text.docx fixture shows why this direction is right: consecutive items use numId 5 then numId 4 and the source itself labels them four, five -- previously rendered as IV, I. Snapshot updates: handmade-numbering's "Two-one independent counter" now continues the shared sequence (5.) rather than restarting (1.), and the style-referenced paragraph follows the merged sequence (8.). The override-driven cases (10., 7)) still restart as before. If the intent was that bare shared instances stay independent in anydoc despite Word, this is easy to flip behind a flag -- but Word parity is what firecrawl#96 asks for.
…irecrawl#102) A password-protected .docx/.xlsx/.pptx was always ConvertError::Encrypted with no way to hand over a password, so ingestion pipelines that receive the password alongside the file had no path through the library, CLI, or bindings. - to_markdown_with_password / to_markdown_bytes_with_password decrypt an encrypted OOXML package via office-crypto before conversion; plaintext files ignore the argument, and every resource limit applies to the decrypted zip unchanged. No password, empty password, or wrong password: Encrypted, exactly as today. Legacy RC4 formats stay Encrypted. office-crypto does not check the EncryptionInfo verifier, so a wrong password yields noise with an Ok status - the zip-signature gate on the payload is the cheap honest wrong-password detection. - CLI: -p/--password with ANYDOC_PASSWORD env fallback (argv leaks into shell history and ps). - Node, Python and wasm bindings gain an optional trailing password arg; old signatures keep working. Fixture: agile SHA-512 encrypted .docx from office-crypto's MIT test suite (password testPassword), annotated --errors for the no-password corpus sweep. New integration tests cover convert-with-password, no/wrong/empty password rejection, and plaintext-with-password passthrough. Fixes firecrawl#102 This change was prepared with AI assistance under human direction and review.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wasm/src/lib.rs">
<violation number="1" location="wasm/src/lib.rs:105">
P3: `toMarkdownBytes` now accepts `password`, but the WASM API documentation still exposes only the two-argument form. Document the third optional argument, including how to pass it when a format is omitted, so OOXML decryption is discoverable.</violation>
</file>
<file name="src/package/crypto.rs">
<violation number="1" location="src/package/crypto.rs:18">
P2: Decryption inflates the whole encrypted payload into one Vec with no size cap, and the caller copies the entire input file first (`bytes.to_vec()`). The 512 MiB archive budget in `limits.rs` only applies afterward, when parts are read from the decrypted zip, so an encrypted OOXML carrying a heavily-compressed payload can exhaust memory during `decrypt_ooxml` before any `ResourceLimit` fires. Cap the decrypted buffer size (e.g. reject when it exceeds `MAX_TOTAL_BYTES`) before trusting it.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| /// plaintext there is nothing else useful to say, and that is the error | ||
| /// callers already handle. | ||
| pub fn decrypt_ooxml(bytes: Vec<u8>, password: &str) -> Result<Vec<u8>, ConvertError> { | ||
| let plain = office_crypto::decrypt_from_bytes(bytes, password).map_err(|e| { |
There was a problem hiding this comment.
P2: Decryption inflates the whole encrypted payload into one Vec with no size cap, and the caller copies the entire input file first (bytes.to_vec()). The 512 MiB archive budget in limits.rs only applies afterward, when parts are read from the decrypted zip, so an encrypted OOXML carrying a heavily-compressed payload can exhaust memory during decrypt_ooxml before any ResourceLimit fires. Cap the decrypted buffer size (e.g. reject when it exceeds MAX_TOTAL_BYTES) before trusting it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/package/crypto.rs, line 18:
<comment>Decryption inflates the whole encrypted payload into one Vec with no size cap, and the caller copies the entire input file first (`bytes.to_vec()`). The 512 MiB archive budget in `limits.rs` only applies afterward, when parts are read from the decrypted zip, so an encrypted OOXML carrying a heavily-compressed payload can exhaust memory during `decrypt_ooxml` before any `ResourceLimit` fires. Cap the decrypted buffer size (e.g. reject when it exceeds `MAX_TOTAL_BYTES`) before trusting it.</comment>
<file context>
@@ -0,0 +1,50 @@
+/// plaintext there is nothing else useful to say, and that is the error
+/// callers already handle.
+pub fn decrypt_ooxml(bytes: Vec<u8>, password: &str) -> Result<Vec<u8>, ConvertError> {
+ let plain = office_crypto::decrypt_from_bytes(bytes, password).map_err(|e| {
+ log::debug!("OOXML decryption failed: {e}");
+ ConvertError::Encrypted
</file context>
| pub fn to_markdown_bytes( | ||
| bytes: &[u8], | ||
| format: Option<Format>, | ||
| password: Option<String>, |
There was a problem hiding this comment.
P3: toMarkdownBytes now accepts password, but the WASM API documentation still exposes only the two-argument form. Document the third optional argument, including how to pass it when a format is omitted, so OOXML decryption is discoverable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wasm/src/lib.rs, line 105:
<comment>`toMarkdownBytes` now accepts `password`, but the WASM API documentation still exposes only the two-argument form. Document the third optional argument, including how to pass it when a format is omitted, so OOXML decryption is discoverable.</comment>
<file context>
@@ -99,8 +99,17 @@ pub fn format_from_path(path: &str) -> Option<Format> {
+pub fn to_markdown_bytes(
+ bytes: &[u8],
+ format: Option<Format>,
+ password: Option<String>,
+) -> Result<String, JsValue> {
+ anydoc::to_markdown_bytes_with_password(
</file context>
Cubic review follow-ups on firecrawl#129, both real: - last_num is now Option<u64>: a document may legally carry numId=18446744073709551615 (xsd:int clamps were only applied to start values), and a u64::MAX sentinel would have swallowed the instance switch that triggers an override restart. - Overridden levels restart on their first USE under the entering instance (fresh_overrides) instead of eagerly marking pending bits at the switch. Eager marking let a following plain instance consume a restart it never earned: n1(1), n2[startOverride](unused at L0), n3 plain restarted to 1 instead of continuing at 2. Regression test walks nested lists and asserts the continuation.
Cubic review round on firecrawl#130, all addressed: - CLI: a path conversion with a password but no --format dropped the password entirely; such conversions now go through toMarkdownBytes with content detection. -p/--password without a value is a usage error instead of silently falling back to the environment variable. - CLI: the asset pass re-read the still-encrypted container after a successful password-protected Markdown conversion and died with Encrypted. The container is now decrypted once, up front, so Markdown and assets see the same plaintext. - Library: to_markdown_with_password no longer fails with Unsupported before decrypting when an encrypted file has no recognizable extension; unresolved formats flow through to the byte-level entry point. - crypto: the decrypted payload is bounded by MAX_TOTAL_BYTES before any part is read, and is validated by actually opening it as an archive — a wrong password whose noise happens to start with PK now still ends in Encrypted instead of Malformed. - is_encrypted_ooxml / decrypt_ooxml are re-exported for embedders. - Python binding doc: removed the duplicated summary line. - Carries the firecrawl#129 per-instance override-restart fix so the two branches do not conflict.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- crypto: wrong-password validation now runs before the size policy, so oversized noise still ends in Encrypted rather than ResourceLimit; the decrypted-in-memory budget becomes its own named constant with a note on why it is stricter than the plaintext part-read budget. - cli.js: a password-bearing path without --format keeps the extension fallback (formatFromPath) so signature-less CSV behaves exactly as without a password. - convert.rs: format detection moved after decryption, mirroring to_markdown_with_password - an encrypted file with an unrecognized extension no longer dies with Unsupported pre-decrypt. - numbering.rs: dropped the stale u64::MAX sentinel sentence from the last_num doc comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #102
Summary
Password-protected
.docx/.xlsx/.pptxconvert when the caller supplies a password, instead of always ending inEncrypted. Plaintext files are untouched; without a password nothing changes.What changed
to_markdown_with_password(path, password)andto_markdown_bytes_with_password(bytes, format, password): an encrypted OOXML package (OLE container withEncryptionInfo/EncryptedPackage, detected by the same shapeprobe_olealready classifies) is decrypted viaoffice-crypto0.3 and converted as its plaintext zip, so all existing resource limits apply unchanged. No password / empty password / wrong password →Encrypted, exactly as today. Legacy RC4 binary formats (.doc/.ppt/.xls) stay out of scope.Okstatus), so the decrypted payload's zip signature (PK) gates success. Documented insrc/package/crypto.rs.-p/--password, withANYDOC_PASSWORDenv fallback since argv leaks into shell history andps.passwordargument (toMarkdownBytes(bytes, format?, password?),to_markdown_bytes(data, format=None, password=None)); existing signatures keep working.Fixture provenance
tests/fixtures/encrypted/agile-sha512-docx--errors.docxis an agile SHA-512 encrypted.docxtaken from office-crypto's MIT-licensed test suite (passwordtestPassword). It carries the corpus sweep's--errorsannotation for the no-password path;tests/fixtures/docx/detection invariants are unaffected.Verification
tests/encrypted.rs, 5 cases): convert-with-password returns the expected body; no/wrong/empty password →Encrypted; plaintext file with a stray password converts identically.ANYDOC_PASSWORD=testPasswordthe fixture converts; without it,error: document is encrypted.This change was prepared with AI assistance under human direction and review.
Summary by cubic
Decrypts password-protected OOXML when a password is supplied and fixes DOCX list numbering to continue across instances that share an abstract, only restarting overridden levels. Previously encrypted OOXML always returned Encrypted; wrong/empty passwords still do, while plaintext files are unchanged.
to_markdown_with_password(path, password)andto_markdown_bytes_with_password(bytes, format, password)decrypt then convert; exportsis_encrypted_ooxmlanddecrypt_ooxml. Validates the decrypted payload by opening it as a zip, rejects wrong passwords before applying size limits, enforces a 512 MB decrypted-package budget, and then applies the usual package limits unchanged.-p/--passwordwithANYDOC_PASSWORDfallback; a missing--passwordvalue is a usage error. Password-bearing path conversions route through byte-level conversion after decryption, using content detection with the path extension as the fallback; assets come from the same decrypted bytes.passwordin Node (toMarkdownBytes(bytes, format?, password?)), Python (to_markdown_bytes(data, format=None, password=None)), and wasm; existing call sites continue to work.lvlOverride/startOverridelevels restart, and restarts trigger on first use under the overriding instance.Written for commit 7a4cac6. Summary will update on new commits.