Fixes #1276 Added a referer header when new issues are created

[nj4710]

r? @karlcow

[nj4710]

Ping @karlcow

[karlcow]

@nj4710 Look at all my comments.

Your current pull request will not work.

[karlcow]

@nj4710 and sorry I had forgottent to actually submit the review (written two days ago).

[nj4710]

we construct a string to check against depending on the domain contest.

I did not understand this approach

For url parsing, we will do something like this:

from urlparse import urlparse
x = urlparse(request.referer)
if x.path == ('/issues/new'):
    flash(number,'thanks')

For string splitting, something like this:

x = (request.referrer)
a = x.split("/")
if ('issues') in a and ('new') in a:
  flash(number,'thanks')`

How do we determine which method is best for security purposes? @karlcow

[karlcow]

@nj4710 to make code example readable on github, you need to use this syntax

```python
if blah:
    foobar()
```

(fixing your markup in your previous comment.)

About

we construct a string to check against depending on the domain contest.

What I was saying is that we have 3 domains currently.

• http://localhost:5000
• https://staging.webcompat.com
• https://webcompat.com

So the referer when it exists will be DOMAIN/issues/new where DOMAIN is one of the 3.

For the proposal with urlparse. This is a good proposal but did you test it with all circumstances?

>>> from urlparse import urlparse
>>> referer_url = 'https://webcompat.com/issues/new'
>>> url_path = urlparse(referer_url).path
>>> url_path.startswith('/issues/new')
True
>>> # Now let's see when request.referrer is None (most common case)
... referer_url = None
>>> url_path = urlparse(referer_url).path
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urlparse.py", line 143, in urlparse
    tuple = urlsplit(url, scheme, allow_fragments)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urlparse.py", line 182, in urlsplit
    i = url.find(':')
AttributeError: 'NoneType' object has no attribute 'find'

What it says is that you can't parse an URL which is None.

[nj4710]

The None case will be present in all the three cases.
I like the URL parsing approach better.
What I want to know is when can request.referrer be none e.g. if the user manually entered the URL?

To handle this case:

if request.referrer is not None:
        x = urlparse(request.referrer)
        if x.path == ('/issues/new'): 
            flash(number, 'thanks')

Please correct me if I am wrong

[karlcow]

The Referer HTTP Header can be None in many circumstances. When the person decides to enter directly the URL or if the browser itself blocks this information through an addon/extension. Also the CSP policy of a Web site can forbid referrers (not the case currently for us)

[karlcow]

About your python code, we are getting closer. :)

>>> if not None:
...    print "here"
... else:
...    print "not here"
... 
here

So to be more pythonic 😁

• 4 spaces,
• None in if is equivalent to False,
• parenthesis not necessary
• be explicit about variable names

if not request.referrer:
    url_path = urlparse(request.referrer).path
    if url_path == '/issue/new': 
        flash(number, 'thanks')

But once you did that, still run the tests before sending the commit. To make sure we do not break anything.

Thanks @nj4710

[nj4710]

Shouldn't it be
if request.referrer: since we do not want it to be None?

I ran the nosetests -v command. There were no errors but while running functional tests only 60/72 tests ran.

[karlcow]

@nj4710 sorry about this, but you were right about

if request.referrer is not None:
    url_path = urlparse(request.referrer).path
    if url_path == '/issue/new': 
        flash(number, 'thanks')

This is the correct way. Thanks.

Could you commit the change, then rebase.
Thanks.
And I will test on my side.

[nj4710]

I am still not sure about functional tests. r? @karlcow

[karlcow]

@nj4710 did you try to run them? Or did you have difficulties running them?
(asking because that would help us to improve our manual when it's difficult.)
https://github.com/webcompat/webcompat.com/blob/master/CONTRIBUTING.md#running-tests

[nj4710]

@karlcow I ran them. Only 62 tests ran.

[karlcow]

so testing locally this branch.
@nj4710 Below there's nothing wrong with what you did. Just the way we currently handle things for now and we might need to fix. Sometimes things are more complicated than they appear from the distance. 🤓

Creating an issue from the home page and I don't get the flash message. Ooops. Let's explore.

The request headers to http://localhost:5000/issues/555 (the issue I just created) is

GET /issues/555 HTTP/1.1
Host: localhost:5000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://localhost:5000/
Cookie: session=eyJjc3JmX3Rva2VuIjp7IiBiIjoiWm1SbVpqa3hNRGMzTm1ZeU5tRmxZamsyTVdReFpESTRObVppT0RkaE9Ua3pOVEF6T0dKbVpnPT0ifX0.C87ycQ.Spw9n6LxYC9fxa6JsZzUu4tIgtw
Connection: keep-alive
Upgrade-Insecure-Requests: 1

aka Referer: http://localhost:5000/ is not having the right path. so I wondered why?

The previous request is our HTTP POST.

The request is correct and has the correct referer Referer: http://localhost:5000/ because the POST has been made from the home page.

POST /issues/new HTTP/1.1
Host: localhost:5000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://localhost:5000/
Content-Type: multipart/form-data; boundary=---------------------------8963752337807573261063294912
Content-Length: 1392
Cookie: session=eyJjc3JmX3Rva2VuIjp7IiBiIjoiWm1SbVpqa3hNRGMzTm1ZeU5tRmxZamsyTVdReFpESTRObVppT0RkaE9Ua3pOVEF6T0dKbVpnPT0ifX0.C87ycQ.Spw9n6LxYC9fxa6JsZzUu4tIgtw
Connection: keep-alive
Upgrade-Insecure-Requests: 1

but the interesting part is the response from the server which is:

HTTP/1.0 302 FOUND
Content-Type: text/html; charset=utf-8
Content-Length: 229
Location: http://localhost:5000/issues/555
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Frame-Options: DENY
Content-Security-Policy-Report-Only: default-src 'none'; connect-src 'self'; font-src 'self'; img-src 'self' https://www.google-analytics.com https://*.githubusercontent.com data:; manifest-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline' https://www.google-analytics.com https://api.github.com; style-src 'self' 'unsafe-inline'; report-uri /csp-report
Server: Werkzeug/0.10.4 Python/2.7.10
Date: Tue, 11 Apr 2017 23:05:18 GMT

We get a weird HTTP/1.0. I need to understand why. It should be HTTP/1.1
And we send back a 302 Found and Location: http://localhost:5000/issues/555, instead of a 201 Created

So we could send a 201 Created with a Location: header and if it's not working for the redirection (We need to test), we could still send a Content-Location: with the issue URI (to test too).

[karlcow]

Let's explore a bit more. I modified the anonymous POST on /issues/new to be

    elif form.get('submit-type') == PROXY_REPORT:
        response = report_issue(form, proxy=True).json()
        return ('Created', 201,
                {'Location': url_for('show_issue',
                                     number=response.get('number'))})

the HTTP Response is

HTTP/1.0 201 CREATED
Location: http://localhost:5000/issues/556
Content-Type: text/html; charset=utf-8
Content-Length: 7
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Frame-Options: DENY
Content-Security-Policy-Report-Only: default-src 'none'; connect-src 'self'; font-src 'self'; img-src 'self' https://www.google-analytics.com https://*.githubusercontent.com data:; manifest-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline' https://www.google-analytics.com https://api.github.com; style-src 'self' 'unsafe-inline'; report-uri /csp-report
Server: Werkzeug/0.10.4 Python/2.7.10
Date: Wed, 12 Apr 2017 00:24:37 GMT

without redirection. It stays with the word Created for the body. So on 201 Created There is no redirection and it is sending an HTTP/1.0 again. Adding a Content-Location doesn't change anything for the body.

[karlcow]

1. POST /issues/new done from the home page
2. 302 + location
3. The browser applies the redirection but the initial request was made from the homepage. This is happening on the browser side so not sure we can do anything about it.

So my initial idea in using referer was I guess flawed.
I think for now we need to keep the solution of @miketaylr (Unfortunately 😉 )

@nj4710 I will close the PR. You did an amazing job, but there was an issue I had not foreseen. Sorry about that, but it's part of dev too. Discovering things when implementing.

[miketaylr]

I think for now we need to keep the solution of @miketaylr (Unfortunately 😉 )

🔥