Skip to content

Normalize non-ASCII errors in Base64 bytes loaders - #456

Open
binggao1230 wants to merge 4 commits into
reagento:developfrom
binggao1230:fix-strict-base64-loading
Open

binggao1230 wants to merge 4 commits into
reagento:developfrom
binggao1230:fix-strict-base64-loading

Conversation

@binggao1230

@binggao1230 binggao1230 commented Jul 16, 2026 •

Copy link
Copy Markdown

Problem

The bytes-like Base64 loaders encode string input as ASCII before decoding. Non-ASCII input therefore leaks UnicodeEncodeError instead of using the provider's ValueLoadError contract.

Fix

Convert UnicodeEncodeError to ValueLoadError("Bad base64 string", data) in the shared bytes loader. This covers bytes, bytearray, BytesIO, and IO[bytes] loaders. Padding behavior is unchanged.

Validation

  • Python 3.11 concrete-provider tests: 306 passed
  • Targeted public-seam matrix: 24 passed across result type, coercion, and debug-trail combinations
  • Mutation check: removing the Unicode handler makes all 24 targeted cases fail
  • Ruff 0.14.14 and git diff --check: passed

@binggao1230

Copy link
Copy Markdown
Author

The Coverage job failed before reading any coverage data: its first request, GET https://api.github.com/repos/reagento/adaptix, returned HTTP 503 with GitHub's Unicorn HTML page. All seven test jobs that produced the coverage artifacts passed. I attempted to rerun only the failed job, but fork contributors do not have the required repository permission.

@sonarqubecloud

Copy link
Copy Markdown

@zhPavel

zhPavel commented Sep 6, 2026

Copy link
Copy Markdown
Member

@binggao1230 sorry for the long delay. Thanks for your interest in the project. Catching the UnicodeEncodeError exception looks like a very important improvement, but I still can't quite see the benefit of the additional padding checks. Does extra padding actually cause any problems during decoding?

@binggao1230

Copy link
Copy Markdown
Author

You are right to ask. I rechecked this, and there is no decoded-byte corruption in these cases. AAA= and AAA== both decode to the same two bytes; AAAA, AAAA=, and AAAA== all decode to the same three bytes; and padding-only strings decode to empty bytes. The additional check therefore only enforces a canonical representation instead of fixing the decoded result. RFC 4648 also permits decoders to ignore excess terminal padding, so without a project requirement for canonical input, I do not think that part has enough benefit. I inferred stricter validation from the existing alphabet and missing-padding checks, but that is not a concrete decoding problem. I can remove the padding checks, related tests, and changelog wording, and keep the UnicodeEncodeError to ValueLoadError fix only.

@zhPavel

zhPavel commented Sep 8, 2026

Copy link
Copy Markdown
Member

@binggao1230 remove exceeded validaton, please

@binggao1230 binggao1230 changed the title Reject malformed Base64 padding in bytes loaders Normalize non-ASCII errors in Base64 bytes loaders Sep 12, 2026
@binggao1230

Copy link
Copy Markdown
Author

Removed the excess-padding validation, its tests, and the padding-specific changelog wording. The PR now only converts non-ASCII input from UnicodeEncodeError to ValueLoadError; the 24-case public-seam matrix and all 306 concrete-provider tests pass on the signed follow-up commit.

@zhPavel

zhPavel commented Sep 30, 2026

Copy link
Copy Markdown
Member

@binggao1230 Sorry, I cannot merge PR with failed tests. Can you fix them?

@sonarqubecloud

sonarqubecloud Bot commented Oct 5, 2026

Copy link
Copy Markdown

@binggao1230

Copy link
Copy Markdown
Author

Fixed the patch-level ParamSpec bound-source expectation on signed commit 67eb34f. The new run is fully green across CPython 3.10-3.14, PyPy 3.10/3.11, lint, coverage, SonarCloud, and docs.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/adaptix/_internal/morphing
  concrete_provider.py
Project Total  

This report was generated by python-coverage-comment-action

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.

2 participants