204 and 304 must not have a body, see https://tools.ietf.org/html/rfc7230#section-3.3 #25835

Merged
merged 1 commit into from Aug 18, 2016

Projects

None yet

7 participants

@butonic
Member
butonic commented Aug 17, 2016

The activity app always sets content, even for a for a 304 response, which makes firewalls cry:

http_process_state_prepend - Invalid action:0x109010 Server sends too much data.

Instead of fixing the app I chose to make the appframework do a sanity check in case another app misbehaves.

backport to oc9 requested.

cc @felixboehm

@butonic butonic 204 and 304 must not have a body, see https://tools.ietf.org/html/rfc…
e927e0d
@butonic butonic added this to the 9.2 milestone Aug 17, 2016
@mention-bot

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @DeepDiver1975 and @MorrisJobke to be potential reviewers

@nickvergessen
Contributor

Well this basically means that OCS v2 is not compatible with the RFC in this case, because OCS always expects their own body.

@DeepDiver1975
Member

This is only for v2 - right?
In this case this is accepable.

@guruz
Contributor
guruz commented Aug 17, 2016

👍

@MorrisJobke MorrisJobke commented on the diff Aug 17, 2016
lib/public/AppFramework/Http/JSONResponse.php
* @since 6.0.0
* @throws \Exception If data could not get encoded
*/
public function render() {
- $response = json_encode($this->data, JSON_HEX_TAG);
- if($response === false) {
- throw new \Exception(sprintf('Could not json_encode due to invalid ' .
- 'non UTF-8 characters in the array: %s', var_export($this->data, true)));
+ if ($this->getStatus() === Http::STATUS_NO_CONTENT
+ || $this->getStatus() === Http::STATUS_NOT_MODIFIED
+ ) {
+ $response = null;
@MorrisJobke
MorrisJobke Aug 17, 2016 Member

Maybe also log that the content was truncated. Otherwise this is weird behavior to get the body just deleted. Debug should be enough.

@butonic
butonic Aug 18, 2016 Member

I don't want to add a dependency on the logger. The Response class is too important and should still function if the logging breaks so the user gets a response ... I have seen that kind of problem too often.

@PVince81
Collaborator

From reading the log, tests passed except OCI that timed out.

@PVince81
Collaborator

👍

@PVince81 PVince81 merged commit f9e927d into master Aug 18, 2016

3 of 4 checks passed

continuous-integration/jenkins/pr This commit cannot be built
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the nobodyifnocontent branch Aug 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment