From 40189d40f67df43ca72e55679155b5449b865504 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 28 Aug 2024 15:04:28 +0200 Subject: [PATCH] PB-934: Removed origin check Allow request if no origin/referer is set. If an origin is set, then returns correct CORS headers. --- app/app.py | 69 +++++++-------------------------- tests/unit_tests/test_qrcode.py | 46 ++++++++-------------- 2 files changed, 30 insertions(+), 85 deletions(-) diff --git a/app/app.py b/app/app.py index a267326..130f75c 100644 --- a/app/app.py +++ b/app/app.py @@ -5,7 +5,6 @@ from werkzeug.exceptions import HTTPException from flask import Flask -from flask import abort from flask import g from flask import request @@ -36,64 +35,24 @@ def log_route(): route_logger.info('%s %s', request.method, request.path) -# Reject request from non allowed origins -@app.before_request -def validate_origin(): - # The Origin headers is automatically set by the browser and cannot be changed by the javascript - # application. Unfortunately this header is only set if the request comes from another origin. - # Sec-Fetch-Site header is set to `same-origin` by most of the browser except by Safari ! - # The best protection would be to use the Sec-Fetch-Site and Origin header, however this is - # not supported by Safari. Therefore we added a fallback to the Referer header for Safari. - sec_fetch_site = request.headers.get('Sec-Fetch-Site', None) - origin = request.headers.get('Origin', None) - referrer = request.headers.get('Referer', None) - - logger.debug( - "Validate origin: sec_fetch_site=%s, origin=%s, referrer=%s", - sec_fetch_site, - origin, - referrer - ) - - if origin is not None: - if is_domain_allowed(origin): - return - logger.error('Origin=%s does not match %s', origin, ALLOWED_DOMAINS_PATTERN) - abort(403, 'Permission denied') - - # BGDIINF_SB-3115: Apparently IOS 16 has a bug and set Sec-Fetch-Site=cross-site even if the - # request is originated (same origin and/or referrer) from the same site ! Therefore to avoid - # issue on IOS we first checks the referrer before checking Sec-Fetch-Site even if this not - # correct. - if referrer is not None: - if is_domain_allowed(referrer): - return - logger.error('Referer=%s does not match %s', referrer, ALLOWED_DOMAINS_PATTERN) - abort(403, 'Permission denied') - - if sec_fetch_site is not None: - if sec_fetch_site in ['same-origin', 'same-site']: - return - logger.error('Sec-Fetch-Site=%s is not allowed', sec_fetch_site) - abort(403, 'Permission denied') - - logger.error('Referer and/or Origin and/or Sec-Fetch-Site headers not set') - abort(403, 'Permission denied') - - # Add CORS Headers to all request @app.after_request def add_cors_header(response): - # Do not add CORS header to internal /checker endpoint. - if request.endpoint == 'checker': - return response - - response.headers['Access-Control-Allow-Origin'] = request.host_url + # Only add CORS header if an origin or referer header is present in request + # is a host part of our whitelist + cors_origin = None if 'Origin' in request.headers and is_domain_allowed(request.headers['Origin']): - response.headers['Access-Control-Allow-Origin'] = request.headers['Origin'] - response.headers['Vary'] = 'Origin' - response.headers['Access-Control-Allow-Methods'] = 'GET, HEAD, OPTIONS' - response.headers['Access-Control-Allow-Headers'] = '*' + cors_origin = request.headers['Origin'] + + if 'Referer' in request.headers and is_domain_allowed(request.headers['Referer']): + cors_origin = request.headers['Referer'] + + if cors_origin: + response.headers['Access-Control-Allow-Origin'] = cors_origin + response.headers['Vary'] = 'Origin' + response.headers['Access-Control-Allow-Methods'] = 'GET, HEAD, OPTIONS' + response.headers['Access-Control-Allow-Headers'] = '*' + return response diff --git a/tests/unit_tests/test_qrcode.py b/tests/unit_tests/test_qrcode.py index 52cc1d4..89b6d89 100644 --- a/tests/unit_tests/test_qrcode.py +++ b/tests/unit_tests/test_qrcode.py @@ -46,6 +46,11 @@ def assertCors(self, response): # pylint: disable=invalid-name self.assertIn('Access-Control-Allow-Headers', response.headers) self.assertEqual(response.headers['Access-Control-Allow-Headers'], '*') + def assertNoCors(self, response): # pylint: disable=invalid-name + self.assertNotIn('Access-Control-Allow-Origin', response.headers) + self.assertNotIn('Access-Control-Allow-Methods', response.headers) + self.assertNotIn('Access-Control-Allow-Headers', response.headers) + def test_checker(self): response = self.app.get(url_for('checker'), headers=self.valid_origin_header) self.assertEqual(response.status_code, 200) @@ -54,21 +59,6 @@ def test_checker(self): self.assertEqual(response.json, {"message": "OK", "success": True, "version": APP_VERSION}) def test_generate_errors(self): - response = self.app.get(url_for('generate_get')) - self.assertEqual(response.status_code, 403, msg="ORIGIN must be set") - self.assertCors(response) - self.assertIn('Cache-Control', response.headers, msg="Cache control header missing") - self.assertIn( - 'max-age=', response.headers['Cache-Control'], msg="Cache Control max-age not set" - ) - self.assertEqual(response.content_type, "application/json") - self.assertEqual( - response.json, { - "error": { - "code": 403, "message": "Permission denied" - }, "success": False - } - ) response = self.app.post(url_for('generate_get'), headers=self.valid_origin_header) self.assertEqual(response.status_code, 405, msg="POST method is not allowed") self.assertCors(response) @@ -102,11 +92,15 @@ def test_generate_errors(self): } ) - def test_referer_check(self): - response = self.app.get(url_for('generate_get'), headers={'Referer': 'not allowed'}) - self.assertEqual( - response.status_code, 403, msg="Non allowed Referer did not returned an HTTP 403" + def test_no_origin_allowed(self): + response = self.app.get( + url_for('generate_get'), + query_string={'url': 'https://some_random_domain/test'}, ) + self.assertEqual(response.status_code, 200) + self.assertNoCors(response) + + def test_referer_check(self): response = self.app.get( url_for('generate_get'), query_string={'url': 'https://some_random_domain/test'}, @@ -115,6 +109,7 @@ def test_referer_check(self): self.assertEqual( response.status_code, 200, msg="Allowed Referer did not returned an HTTP 200" ) + self.assertCors(response) def test_generate_url_domain_restriction(self): response = self.app.get( @@ -157,20 +152,12 @@ def test_generate_origin_not_allowed(self, headers): query_string={'url': 'https://some_random_domain/test'}, headers=headers ) - self.assertEqual(response.status_code, 403, msg="Domain restriction not applied") - self.assertCors(response) + self.assertNoCors(response) self.assertIn('Cache-Control', response.headers, msg="Cache control header missing") self.assertIn( 'max-age=', response.headers['Cache-Control'], msg="Cache Control max-age not set" ) - self.assertEqual(response.content_type, "application/json") - self.assertEqual( - response.json, { - "error": { - "code": 403, "message": "Permission denied" - }, "success": False - } - ) + self.assertEqual(response.status_code, 200) @params( {'Origin': 'map.geo.admin.ch'}, @@ -183,7 +170,6 @@ def test_generate_origin_not_allowed(self, headers): { 'Origin': 'http://localhost', 'Sec-Fetch-Site': 'cross-site' }, - {'Sec-Fetch-Site': 'same-origin'}, {'Referer': 'https://map.geo.admin.ch'}, ) def test_generate_origin_allowed(self, headers):