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

Test client redirect resets path info in 0.15 #1498

Closed
sekrause opened this issue Apr 1, 2019 · 3 comments
Closed

Test client redirect resets path info in 0.15 #1498

sekrause opened this issue Apr 1, 2019 · 3 comments
Labels
Milestone

Comments

@sekrause
Copy link
Contributor

@sekrause sekrause commented Apr 1, 2019

Pull request #1402 changes the redirect handling in werkzeug.test.Client and seems to reuse the environ from the first request to follow the redirect. This leads to problems when the application modifies the environ in-place.

I have a multi-tenant application which resolves the tenant from the first path element in a middleware with werkzeug.wsgi.pop_path_info() before passing the request to a Flask application. This behavior is recommended by the Flask documentation: http://flask.pocoo.org/docs/0.12/patterns/appdispatch/#dispatch-by-path

However, combining pop_path_info() and redirects will breaks tests with Werkzeug >= 0.15 because when Werkzeug makes the second request following the redirct, it will reuse an environ where the first path element is already gone.

Here is a test application which demonstrates this problem:

from flask import Flask, redirect, url_for
from werkzeug.test import Client
from werkzeug.wrappers import BaseResponse
from werkzeug.wsgi import pop_path_info

app = Flask(__name__)

@app.route("/first")
def first():
    return redirect(url_for('.second'))

@app.route("/second")
def second():
    return b'hello'

client = Client(app, BaseResponse)
r = client.get('/first', follow_redirects=True)
assert r.data == b'hello'

def middleware(environ, start_response):
    print('Before pop_path_info(): %s' % environ['PATH_INFO'])
    pop_path_info(environ)
    print('After pop_path_info(): %s' % environ['PATH_INFO'])
    return app(environ, start_response)

client = Client(middleware, BaseResponse)
r = client.get('/tenant/first', follow_redirects=True)
assert r.data == b'hello'
@davidism davidism changed the title Werkzeug >= 0.15 breaks tests which use redirects and modify the environ Test client redirect resets path info in 0.15 Apr 1, 2019
@davidism
Copy link
Member

@davidism davidism commented Apr 1, 2019

Related to #1491

@davidism
Copy link
Member

@davidism davidism commented Apr 1, 2019

I think we need to make a copy of the original environ to pass to the app.

@amercader
Copy link

@amercader amercader commented Jul 15, 2020

This makes perfect sense but I'd like to offer another scenario when werkzeug re-using the already processed environ to handle redirects is desirable.

In my case we have a small middleware that pops the HTTP_HOST key to prevent open redirects to external sites on certain endpoints. But because the changes are applied to the copy of the environ, once werkzeug handles the redirects the effect of the middleware can not be tested:

from flask import Flask, redirect, url_for                                                                                                                                                                                                                                                                           
from werkzeug.test import Client                                                                                                                                                                                                                                                                                     
from werkzeug.wrappers import BaseResponse                                                                                                                                                                                                                                                                           
                                                                                                                                                                                                                                                                        
                                                                                                                                                                                                                                                                                                                     
app = Flask(__name__)                                                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                                                     
@app.route("/first")                                                                                                                                                                                                                                                                                                 
def first():                                                                                                                                                                                                                                                                                                         
    return redirect(url_for('.second'))                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                     
@app.route("/second")                                                                                                                                                                                                                                                                                                
def second():                                                                                                                                                                                                                                                                                                        
    return b'hello'                                                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                     
def middleware(environ, start_response):                                                                                                                                                                                                                                                                             
    print('Before environ pop HTTP_HOST: %s' % environ['HTTP_HOST'])                                                                                                                                                                                                                                                 
    environ.pop('HTTP_HOST', None)                                                                                                                                                                                                                                                                                   
    print('After environ pop HTTP_HOST: %s' % environ.get('HTTP_HOST'))                                                                                                                                                                                                                                              
    return app(environ, start_response)                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                     
client = Client(middleware, BaseResponse)                                                                                                                                                                                                                                                                            
                                                                                                                                                                                                                                                                                                                     
headers = {'Host': 'example.com'}                                                                                                                                                                                                                                                                                    
r = client.get('/first', follow_redirects=True, headers=headers)                                                                                                                                                                                                                                                     
assert r.data == b'hello'  

This will raise a RuntimeError: Following external redirects is not supported. exception, although the middleware was called.

Would a persist_environ_on_redirects flag or similar to allow this scenario make sense? Or if you can think of an alternative way of testing this that would be very welcome.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants