fix: cap the BYTEA read-path preview at 10 KB, sniff MIME from magic bytes (#87) - #107
Merged
Merged
Conversation
…bytes (#87) The BYTEA read path base64-encoded the entire byte array into the BLOB:<size>:<mime>:<b64> wire format, with no cap and a hardcoded application/octet-stream MIME. The builtin driver truncates the read/preview path to the first 10,240 bytes (MAX_BLOB_PREVIEW_SIZE) while still reporting the true total_size in the header, and sniffs MIME from the (possibly truncated) content's magic bytes. Large BYTEA columns produced much larger responses in this plugin than in the builtin, slowing the data grid and inflating transfers -- and the hardcoded MIME meant the UI couldn't preview image/PDF blobs the builtin's sniffed MIME allows. Extracted a shared src/utils/blob.rs (encode_blob for the truncated read/preview path, encode_blob_full for the untruncated file-export path) matching the builtin's drivers/common/blob.rs exactly, including sniffing MIME from the truncated preview slice specifically (not the full data) to match the builtin's encode_blob byte-for-byte. Both extract.rs's Type::BYTEA scalar-column arm and its extract_simple_kind_from_bytes (the array-element / composite-field decode path) now use the shared encode_blob; handlers/blob.rs's fetch_blob_as_data_url uses encode_blob_full, replacing its own duplicate copy of that function. Found and fixed a second, previously undetected instance of the same bug while auditing extract.rs for every BYTEA decode path: extract_simple_kind_from_bytes had its own independently-drifted, untruncated copy of the BYTEA arm, reached via array elements (bytea[]) and composite fields rather than top-level scalar columns. The builtin has exactly one BYTEA decode path shared by all three (Kind::Simple routes array/composite elements through the same simple::extract_or_null the scalar path uses); the plugin had drifted into two independent copies. Both are now fixed. TDD: added unit tests to utils/blob_tests.rs (encode_blob/encode_blob_full, including the 20 KB-input/10 KB-preview case the issue specifically requested) and a new bytea_array_element_... test in extract_tests.rs for the array-element path. Confirmed both the scalar and array-element tests fail against the pre-fix code (20480 vs expected 10240) before the fix, pass after. Verified end-to-end against a live PostgreSQL instance: a 20 KB BYTEA value's read-path response reports the true size (20480) but truncates the base64 payload to exactly the first 10,240 bytes; the underlying stored data in PostgreSQL is confirmed untouched (still 20480 bytes) -- only the read-path response is capped; fetch_blob_as_data_url (export path) still returns the complete, untruncated payload; and PNG magic bytes are correctly sniffed to image/png on the read path (previously always application/octet-stream). Filed #106 for a related, out-of-scope finding: extract.rs's binary_blob_wrapper! macro (used for internal planner-statistics types like pg_mcv_list) has its own separate untruncated encoding that should also move to the new shared encode_blob, but is low-severity (those types are essentially never queried directly) and left for a follow-up.
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
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
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.
Summary
BLOB:<size>:<mime>:<b64>wire format, with no cap and a hardcodedapplication/octet-streamMIME type. The builtin driver truncates the read/preview path to the first 10,240 bytes (MAX_BLOB_PREVIEW_SIZE) while still reporting the truetotal_sizein the header, and sniffs MIME from the (possibly truncated) content's magic bytes.src/utils/blob.rs(encode_blobfor the truncated read/preview path,encode_blob_fullfor the untruncated file-export path) matching the builtin'sdrivers/common/blob.rsexactly — including sniffing MIME from the truncated preview slice specifically, not the full data, to match the builtin'sencode_blobbyte-for-byte.extract.rs'sType::BYTEAscalar-column arm andextract_simple_kind_from_bytes(the array-element / composite-field decode path) now use the sharedencode_blob;handlers/blob.rs'sfetch_blob_as_data_urlusesencode_blob_full, replacing its own duplicate copy of that function.Second finding, fixed in this PR: while auditing every BYTEA decode path in
extract.rs, foundextract_simple_kind_from_byteshad its own independently-drifted, untruncated copy of the BYTEA arm — reached via array elements (bytea[]) and composite fields rather than top-level scalar columns. The builtin has exactly one BYTEA decode path shared by all three (Kind::Simpleroutes array/composite elements through the samesimple::extract_or_nullthe scalar path uses); this repo had drifted into two independent copies. Both are now fixed and share the sameencode_blob.Third, out-of-scope finding — filed as #106:
extract.rs'sbinary_blob_wrapper!macro (used for internal planner-statistics types likepg_mcv_list) has its own separate untruncated encoding whose doc comment incorrectly claims it matches the builtin. Low severity (those types are essentially never queried directly), so left as a follow-up rather than bundled here.Fixes #87.
Test plan
utils/blob_tests.rs(encode_blob/encode_blob_full, including the 20 KB-input/10 KB-preview case the issue specifically requested) and a newbytea_array_element_...test inextract_tests.rsfor the array-element path. Confirmed both the scalar-path and array-element-path tests fail against the pre-fix code (20480 vs expected 10240) before the fix, pass after.cargo test --lib— 320 passedcargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— clean20480) but truncates the base64 payload to exactly the first 10,240 bytes, matching the real data's first 10,240 bytesoctet_lengthstill20480) — only the read-path response is capped, never the actual datafetch_blob_as_data_url(the file-export path) still returns the complete, untruncated 20,480-byte payloadimage/pngon the read path (previously alwaysapplication/octet-stream)