Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http\RequestFactory: added workaround for exceeded post_max_size #793

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fabik
Copy link
Contributor

@fabik fabik commented Sep 30, 2012

@fabik fabik closed this Sep 30, 2012
@fabik fabik reopened this Sep 30, 2012
@fprochazka
Copy link
Contributor

Chtělo by to nějaký testík :)

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Nevíš, jak na to? Ten test by asi musel běžet na webserveru, protože do php://input se zapisovat nedá.

@fprochazka
Copy link
Contributor

Možná by se dal dočasně nahradit protokol php:// za vlastní, pak by to šlo.

A nebo udělat setter, který by nastavoval vstupní cestu, pro účely testování.

private $inputStream = 'php://input';
public function setInputStream($stream)
{
    $this->inputStream = $stream;
}

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Tohle funguje.

class StringStream
{
    /** @var string */
    public static $string;

    /** @var int */
    protected $offset = 0;



    public function stream_open()
    {
        return true;
    }



    public function stream_read($length) 
    {
        $buffer = (string) substr(self::$string, $this->offset, $length);
        $this->offset += strlen($buffer);
        return $buffer;
    }
}

stream_wrapper_unregister('php');
stream_wrapper_register('php', 'StringStream');
StringStream::$string = '...';

@lm
Copy link
Contributor

lm commented Oct 1, 2012

PHP se chová správně když takový request neparsuje, ty data nejsou kompletní, pokud by ten fileinput byl ve formuláři na začátku, tak z toho nepůjdou dostat data žádná.

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Testoval jsem to u sebe, fileinput byl první a přesto se data zachránila, zvláštní.

Mám takový pocit, že browser tam ta ostatní data posílá ještě před souborem.

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Nevíte někdo, jak nejlépe porovnávat objekty FileUpload?

. "--BOUNDARY--\r\n",
array('first' => 'ok', 'second' => "ok\r\nok\r\n"),
array()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Přidal bych sem na konec, jako prevenci chyb

stream_wrapper_restore('php')

Nette totiž v shutdown funkcích ještě něco zpracovává (byť ne zrovna tohle, ale mohlo by)

@fprochazka
Copy link
Contributor

Assert::equal() by mělo porovnávat pouze hodnoty, ne?

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Pravda, to mě nenapadlo.

@fprochazka
Copy link
Contributor

Tohle by ale mohl být problém

http://php.net/manual/en/wrappers.php.php#refsect2-wrappers.php-unknown-unknown-descriptiop

"php://input is not available with enctype="multipart/form-data""

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

V případě, že velikost requestu překročí LimitRequestBody, tak tam php://input je. (Aspoň mi to takto funguje.)

@fprochazka
Copy link
Contributor

Skoro to vypadá na bug. A navíc by se to k PHP snad nemělo ani dostat - apache, nginx

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Aspoň je to užitečnej bug. Možná by se mohlo automaticky přidávat UploadControlům validační pravidlo pro omezení velikosti souboru podle upload_max_filesize, aby to zamítla javascriptová validace. (Což funguje za předpokladu, že administrátor nastaví upload_max_filesize < LimitRequestBody.)

Mohl bys to prosím na nginxu otestovat?

@lm
Copy link
Contributor

lm commented Oct 1, 2012

Testoval jsem to u sebe, fileinput byl první a přesto se data zachránila, zvláštní.

Zkoušel jsem to a Chrome, IE, FF, Opera zachovala pořadí tak jak bylo ve vygenerovaném HTML, takže file byl před text inputy.

Mohl bys to prosím na nginxu otestovat?

Nginx vrátí rovnou 413 Request entity too large, php to ke zpracování nedá - podle http://httpd.apache.org/docs/2.0/mod/core.html#limitrequestbody by se měl apache zachovat stejně.

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Mně to funguje:
Apache/2.2.22 (Ubuntu)
Chromium 20.0.1132.47 Ubuntu 12.04 (144678): předtím a potom
Firefox 15.0.1 (Ubuntu): předtím a potom

Ale zvláštní je, že když se na to podívám wiresharkem, tak se pořadí zachovává.

@lm
Copy link
Contributor

lm commented Oct 1, 2012

A jak vlastně v takovém případě vypadá obsah php://input?

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Aha, tak už vím, v čem je chyba. Neměl jsem tak nastavený LimitRequestBody, ale jenom post_max_size, vlastně se jedná o defaultní nastavení.

Když nastavím LimitRequestBody, tak mně to vyhodí chybu 413.

@fprochazka
Copy link
Contributor

Takže to můžeme uzavřít? :)

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

No stále to má jisté uplatnění - v případě, že mám jenom nízký post_max_size, ale nelimituje mě LimitRequestBody (což je defaultní konfigurace), tak to funguje.

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Javascriptová validace (ae268bb) je asi lepší, ale zase ji nepodporují všechny prohlížeče. Nejúčinnější by byla asi kombinace obojího.

Ideální by teda bylo dávat do všech UploadControlů další validační pravidlo, které by ověřovalo, že velikost nepřevyšuje upload_max_filesize a do všech Formů pravidlo, že velikost všech dat nepřevyšuje post_max_size.

Ještě by tu mohla být jedna možnost, jak zajistit, aby nedocházelo ke ztrátě dat - uložit všechna pole formuláře někam do cookie nebo html5 storage a při dalším requestu je v případě chyby načíst.

@fprochazka
Copy link
Contributor

To už moc nesouvisí se server-side Nette :)

Ale jinak máš pravdu, že když je nastavena jen jedna direktiva, tak to pomůže a možná by stálo za to, to i podporovat. Jenže tohle je dost nestandardní situace a je to hloupost admina, že něco takového dopustí.

@fabik
Copy link
Contributor Author

fabik commented Oct 1, 2012

Důležité je, aby uživatel nepřišel o svá data a byl srozumitelně informován o tom, že soubor je moc velký.

Jinak, tohle nastavení mi nepřipadá moc nestandardní. Na všech serverech, co používám, tak to je zrovna takhle - není nastavený LimitRequestBody, ale post_max_size ano. A je to tak i ve standardní konfiguraci apache a php.

@dg
Copy link
Member

dg commented Apr 16, 2013

Tohle je věc, kterou bych ve frameworku rád vyřešil. A ano, důležité je, aby uživatel byl srozumitelně informován o tom, že soubor je moc velký.

Nicméně parsování inputu na straně PHP bych se určitě vyhnul, to je balancování na hraně nedefinovaného chování PHP a prohlížečů. Asi bych to vyřešil jen tím, že by formulář situaci dokázal detekovat a automaticky zobrazit odpovídající chybovou hlášku.

File API na straně JavaScriptu je už dostupné 60 % používaných prohlížečů.(http://caniuse.com/fileapi), mohli bychom tedy validaci pro velikost doplňovat automaticky. Ale není to tak jednoduché (http://php.vrana.cz/velikost-nahravanych-souboru.php) a také je třeba počítat s tím, že uživatel může souborů nahrávat více.

@fabik
Copy link
Contributor Author

fabik commented Apr 17, 2013

To JavaScriptové řešení vypadá nejrozumněji, nicméně pro těch zbylých 40 % uživatelů je to z hlediska UX docela fail, když sice dostanou srozumitelnou chybovou hlášku, ale přijdou o všechny vyplněné hodnoty. Framework by se mohl alespoň pokusit ta data zachránit.

Sice se v dokumentaci píše, že ten php://input stream není dostupný, ale ve skutečnosti v něm při překročení post_max_size ta data zůstanou (zatím na všech serverech, kde jsem to zkoušel, to tak fungovalo). Myslím si, že když to bude ověřovat přítomnost php://input, tak by v tom neměl být problém.

Pro případ, že by php://input dostupný nebyl, tak by se ještě mělo přidat to ověření do formulářů, aby se aspoň zobrazila ta chybová hláška.

@dg dg removed this from the 2.2 milestone Jun 19, 2014
@dg dg force-pushed the master branch 2 times, most recently from 991ba1a to e23de7a Compare August 26, 2014 15:04
@dg dg force-pushed the master branch 2 times, most recently from 489cca2 to 0b969cd Compare May 10, 2018 20:14
@dg dg force-pushed the master branch 3 times, most recently from 09a7d92 to b9698a8 Compare February 10, 2019 23:23
@dg dg force-pushed the master branch 3 times, most recently from 5a8c108 to 3aa3147 Compare February 26, 2019 14:39
@dg dg force-pushed the master branch 2 times, most recently from 5feee0e to 3fc1e40 Compare January 5, 2021 15:23
@dg dg force-pushed the master branch 5 times, most recently from 688f189 to 1bc9d13 Compare January 13, 2023 04:18
@dg dg force-pushed the master branch 2 times, most recently from 7215ae6 to 71b2047 Compare February 12, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants