Opened 3 years ago

Closed 14 months ago

#19036 closed Bug (fixed)

Big base64-encoded files uploading

Reported by: anthony@… Owned by: nobody
Component: File uploads/storage Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm trying to upload image file using ajax & browser FileReader.readAsDataURL API. In most cases it fails with MultiPartParserError: Could not decode base64 data: Error('Incorrect padding',).
When django cut of header from 'multipart/form-data' part with encoded file data, it does not check length of the remaining data and pass it to decoder. Length of passed data is (FileUploadHandler chunk size) - (removed header length) which is not divides by 4 in most cases.
It is reproduced by testcase I attached. It is based on standard Django test_base64_upload test but reads 512Kb from /dev/urandom instead of short string.

Attachments (4)

big_base64_upload (970 bytes) - added by anonymous 3 years ago.
19036-test.diff (1.5 KB) - added by claudep 3 years ago.
Test case as a patch
19036-1.diff (2.5 KB) - added by claudep 3 years ago.
Decoding base64 by multiple of 4
19036-2.diff (2.5 KB) - added by johannesl 3 years ago.
Corrections for stable/1.5.x branch

Download all attachments as: .zip

Change History (14)

Changed 3 years ago by anonymous

comment:1 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Confirmed. Do you have any idea about the cleaner way to solve this?

Some ideas:

  1. force the stream to be a multiple of 4 by "ungetting" some bytes from the stream after we stripped off the headers
  2. only decode the base64-encoded file at the end of the upload (using the file_complete upload handler method)

Changed 3 years ago by claudep

Test case as a patch

comment:2 Changed 3 years ago by Sir Anthony

Not sure. I tried to make a patch by modifying MultiPartParser, where it must be processed in my opinion (variant 1), but was confused a little with streams there. I did not spent much time with this part of Django previously and do not realize full request processing routine in details yet. But I think, second variant is less acceptable because it will be returning to data processing after finishing and will require doubling of code (using low-level calls in higher-level abstractions) what will complicate the structure of handlers.

Changed 3 years ago by claudep

Decoding base64 by multiple of 4

comment:3 Changed 3 years ago by claudep

  • Has patch set

Just attached a patch that seems to work (testing welcome). My only worry is about memory management when doing chunk += over_chunk. Hopefully the Python implementation is smart and do not copy too much stuff in memory.

comment:4 Changed 3 years ago by claudep

  • Severity changed from Normal to Release blocker

Added blocker flag, as currently 3 of 4 base64 file uploads are doomed to fail.

Changed 3 years ago by johannesl

Corrections for stable/1.5.x branch

comment:5 Changed 3 years ago by johannesl

Regarding memory usage, this should be good enough for release. After 1.5, it might be possible to use base64.encode together with StringIO, or even adjusting read-sizes in streaming code.. I doubt the complexity it adds will be worth the performance gains though.

comment:6 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 3 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 2a67374b51c5705d5c64a5d7117ad8552e98b4bb:

Fixed #19036 -- Fixed base64 uploads decoding

Thanks anthony at adsorbtion.org for the report, and johannesl for
bringing the patch up-to-date.

comment:8 Changed 3 years ago by Claude Paroz <claude@…>

In fc379b48655a365f0dd51880a1f68a15811f903a:

[1.5.x] Fixed #19036 -- Fixed base64 uploads decoding

Thanks anthony at adsorbtion.org for the report, and johannesl for
bringing the patch up-to-date.
Backport of 2a67374b5 from master.

comment:9 Changed 14 months ago by m.zak@…

  • Resolution fixed deleted
  • Status changed from closed to new

This bug is present also in Django 1.4.13 which is mentioned on Django webpage as still supported.

comment:10 Changed 14 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed

The 1.4 branch is only receiving security fixes at this time.

Note: See TracTickets for help on using tickets.
Back to Top