Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

404 on ce_download/-s #8375

Closed
asaage opened this issue Jun 20, 2016 · 21 comments
Closed

404 on ce_download/-s #8375

asaage opened this issue Jun 20, 2016 · 21 comments
Assignees
Labels
Milestone

Comments

@asaage
Copy link

asaage commented Jun 20, 2016

shouldn't the download/-s Elements fire a 404 if the file-parameter is not valid?

Reason is: google indexed some downloads which the customer now deleted because they are obsolete but the google-SeachConsole refuses to kick them out of the index because they still respond with 200.

try the following:

http://demo.contao.org/en/file-elements.html?file=files/contaodemo/media/content-images/DSC_5276.jpg -> 200

http://demo.contao.org/en/file-elements.html?file=files/contaodemo/media/content-images/DSC_5276invalid.jpg -> 200

Im not shure what the best-practice is when handling invalid get-parameters though.

@leofeyer
Copy link
Member

shouldn't the download/-s Elements fire a 404 if the file-parameter is not valid?

Nope: #4632, #5683 and #5178.

@fritzmg
Copy link
Contributor

fritzmg commented Jun 20, 2016

@leofeyer in all diesen Fällen geht es aber nicht darum, ob die Datei überhaupt existiert. Auch in #5178 ging es nicht darum, ob die Datei generell existiert, sondern nur darum, ob die angeforderte Datei in der Liste der selektierten Download Dateien ist. Da musste das Senden des 404 headers entfernt werden, und das ist auch ok.

Das hat aber noch nichts damit zu tun, ob die Datei generell existiert. In so einem Fall sollte ein 404 header gesendet werden. Und soweit ich das sehe, würde das in keinem der genannten Fälle ein Problem machen.

@fritzmg
Copy link
Contributor

fritzmg commented Jun 20, 2016

Ganzheitlich würde dies das Problem aber dennoch nicht lösen. Angenommen man entfernt das Download(s) Inhaltselement wieder von einer Seite, dann würden URLs mit ?file=… wiederum keinen 404 header senden.

@leofeyer
Copy link
Member

Eine mögliche Lösung wäre es, einen Event-Listener auf kernel.response zu registrieren, der bei Vorhandensein des ?file= parameters eine 404-Exception wirft. Wenn die Datei existiert, würde bereits vorher eine File-Response geschickt.

@contao/developers /cc

@Toflar
Copy link
Member

Toflar commented Jun 20, 2016

Ich finde nicht, nein. Ein 404 ist für eine Seite die nicht gefunden wurde und hat nichts mit einem behandelten oder nicht behandelten Query-Parameter zu tun. Wenn der Query-Parameter nicht passt, soll er einfach ignoriert werden, das ist in meinen Augen das absolut richtige Verhalten. Ich will definitiv keinen 404 hier.

@asaage
Copy link
Author

asaage commented Jun 20, 2016

Wenn der Query-Parameter nicht passt, soll er einfach ignoriert werden

So kenne ich das für gewöhnlich auch.
Ich hatte deshalb auch schonmal mit dem Gedanken gespielt anstelle der Links mit query-parameter post-forms zu verwenden.
Das würde nochmal andere Möglichkeiten ins Spiel bringen (z.B. wirkliche Verhinderung von Hotlinking oder Zusammengefasster Download von ce_downloads als zip-Datei)
Wie gesagt - war nur ein Gedankenspiel, was ich aus verschiedenen Gründen wieder verworfen hatte aber vielleicht kann man's doch nochmal aufgreifen.

@fritzmg
Copy link
Contributor

fritzmg commented Jun 20, 2016

Oder man macht es in Zukunft als URL Parameter? Also zB. wenn auf der Seite foo.html ein Download Inhaltselement ist und man dann Dateien per foo/file/DSC132456.jpg runterladen kann - ist aber nicht so leicht in Contao möglich, schätze ich.

@ausi
Copy link
Member

ausi commented Jun 22, 2016

Eine mögliche Lösung wäre es, einen Event-Listener auf kernel.response zu registrieren, der bei Vorhandensein des ?file= parameters eine 404-Exception wirft.

Ich denke nicht, dass das optimal wäre. Was ist wenn ich auf einer Seite den file-Parameter für etwas anderes benutze?

@Toflar
Copy link
Member

Toflar commented Jun 22, 2016 via email

@leofeyer
Copy link
Member

leofeyer commented Jun 23, 2016

Also z.B. ?file=foo.pdf&cid=42?

@Toflar
Copy link
Member

Toflar commented Jun 23, 2016 via email

@leofeyer leofeyer added this to the 4.3.0 milestone Jul 15, 2016
@leofeyer
Copy link
Member

Wir sollten hierüber nochmal diskutieren, denn die Lösung mit dem cid=42 ist ein BC-Break IMO. Alle bisherigen Download-Links würden dadurch ungültig.

@leofeyer leofeyer removed this from the 4.3.0 milestone Oct 28, 2016
@fritzmg
Copy link
Contributor

fritzmg commented Oct 28, 2016

Bei nicht vorhanden sein des cid Parameters, würde doch einfach nur das alte Verhalten wieder da sein, oder nicht? Also zB

if (!\Input::get('cid') || \Input::get('cid') == $this->id)
{
    …
}

@leofeyer
Copy link
Member

So könnte man es implementieren, aber das würde das oben angesprochene Google-Problem ja nicht lösen.

@fritzmg
Copy link
Contributor

fritzmg commented Oct 28, 2016

Naja, für alte URLs nicht. Für neue URLs kann das spezifizierte Inhaltselement ja dann den 404 Header senden.

// soll jetzt überhaupt ein 404 Header gesendet werden in so einem Fall?

@leofeyer
Copy link
Member

Initiale Problembeschreibung:

Reason is: google indexed some downloads which the customer now deleted because they are obsolete but the google-SeachConsole refuses to kick them out of the index because they still respond with 200.

@asaage
Copy link
Author

asaage commented Oct 28, 2016

what about something like that?

<div class="ce_download">
  <form method="post">
     <input type="hidden" value="filerefence">
     <input type="hidden" value="cid">
     <input type="submit" value="filename size etc..">
  </form>
</div>

and on submit pass the download as stream.

It would shurely be a BC Break too but this approach would work with no queryParameters at all and it prevents google or other sites to link to the file directly. (if you want that you could still use ce_hyperlink with the filepicker)

@bekanntmacher
Copy link
Contributor

Kann man nicht den 200er lassen und:

@asaage
Copy link
Author

asaage commented Dec 9, 2016

In diesem Zusammenhang ist mir noch aufgefallen, dass der Browser einen anderen mime-type erwartet als er bekommt. Ist zwar nicht weiter dramatisch aber vielleicht lässt sich das in Folgeüberlegungen miteinbeziehen. Für mich wäre das ein weiters Indiz, dass die Lösung mit Queryparametern nicht ganz astrein ist.
image

@leofeyer
Copy link
Member

leofeyer commented Apr 12, 2018

Bei nicht vorhanden sein des cid Parameters, würde doch einfach nur das alte Verhalten wieder da sein, oder nicht? Also zB

if (!\Input::get('cid') || \Input::get('cid') == $this->id)
{
   …
}

Wie am 12. April auf Mumble besprochen, wollen wir diese Lösung, selbst wenn sie das Problem nur für zukünftige und nicht für bestehende URLs lösen kann.

@leofeyer leofeyer added this to the 4.6.0 milestone Apr 12, 2018
@leofeyer leofeyer self-assigned this Apr 27, 2018
leofeyer added a commit to contao/core-bundle that referenced this issue Apr 27, 2018
@leofeyer
Copy link
Member

Implementiert in contao/core-bundle@399bfe6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants