Skip to content

Conversation

@vbrandl
Copy link

@vbrandl vbrandl commented Jun 20, 2017

If a webserver or reverse proxy itself also sets the required headers,
the check will yield a false negative since the header will be set twice
(once from nextcloud, once from the webserver).

From
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader:
If there are multiple response headers with the same name, then their values are returned as a single concatenated string, where each value is separated from the previous one by a pair of comma and space.

So in case the header is set twice, getResponseHeader will return
something like SAMEORIGIN, SAMEORIGIN and the equals check will fail
and therefore result in a false negative. Using String.indexOf() to
search for a substring will prevent this.

If a webserver or reverse proxy itself also sets the required headers,
the check will yield a false negative since the header will be set twice
(once from nextcloud, once from the webserver).

From
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader:
`If there are multiple response headers with the same name, then their
values are returned as a single concatenated string, where each value is
separated from the previous one by a pair of comma and space`.

So in case the header is set twice, `getResponseHeader` will return
something like `SAMEORIGIN, SAMEORIGIN` and the equals check will fail
and therefore result in a false negative. Using String.indexOf() to
search for a substring will prevent this.
@mention-bot
Copy link

@vbrandl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @rullzer and @MorrisJobke to be potential reviewers.

Forgot the `-` sign
Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

I consider this expected behaviour. The web server should not set these headers as they could collide with Nextclouds configuration.

@vbrandl
Copy link
Author

vbrandl commented Jun 21, 2017

Ok I get that. And what about a more differentiated error message in case the header is set twice? Something like The header XXXX is set more than once. Maybe your webserver is setting the header, too. Consider turning it off on your webserver. Since myself and also some users on the IRC seemed to be confused by the error message.

@florianschroen
Copy link

florianschroen commented Jun 21, 2017

I had the same behavior. (my Apache sets some security headers if they're unset; some default security).
This results in multiple headers of the same name.

/etc/apache2/conf-enabled/security.conf

# Apache's Default:
#Header set X-XSS-Protection: "1; mode=block"
# my choice:
Header always setifempty X-XSS-Protection: "1; mode=block"
$ curl -Is https://cloud.*** | grep X-XSS
X-XSS-Protection: 1; mode=block
X-XSS-Protection: 1; mode=block

in nextcloud/.htaccess replace:
Header set X-XSS-Protection "1; mode=block"
with:
Header always setifempty X-XSS-Protection "1; mode=block"
and the problem is solved.

$ curl -Is https://cloud.*** | grep X-XSS
X-XSS-Protection: 1; mode=block

The problem on this solution is just the changed checksum which results in an error on integrity-check.

If default is not ok, value could be changed in apache vhost with:
Header edit X-XSS-Protection: 1; mode=othervalue
(overrides apache default, .htaccess does not set value)

@vbrandl
Copy link
Author

vbrandl commented Jun 21, 2017

I'm using nginx and I just have to disable the headers for the warning to disappear. I just think that there is a difference between 'header is not set' and 'header is set twice' and that this difference should be made clear by showing different messages to the user.

@MorrisJobke
Copy link
Member

There was a different approach also merged in via #4856. Also the text states that the configured header does not equal SAMEORIGIN. Another indicator that the admin could look into. Maybe it's also good to show the current one and the expected one. I would appreciate a PR doing this more that this fuzzy check for the header, because it would also mean that "abcSAMEORIGINdef" would succeed.

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.

5 participants