From 022d235e87e28b9fec8aba48ad1bdc308a44830d Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Mon, 6 Dec 2021 14:46:11 -0800 Subject: [PATCH] changes to allow cors allowed origin and max age to be configurable (#1177) * allowed cors origin and max_age to be configurable * e2e test edit * moved comment * feedback changes * e2e test edit * remove extra line at end of file Co-authored-by: JR Conlin --- Cargo.lock | 5 +- Cargo.toml | 1 + config/local.example.toml | 4 ++ src/server/mod.rs | 26 ++++----- src/server/test.rs | 9 ++-- src/settings.rs | 70 +++++++++++++++++++++++++ src/web/mod.rs | 4 ++ tools/integration_tests/run.py | 11 ++-- tools/integration_tests/test_storage.py | 31 ++++++++++- 9 files changed, 137 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c4d7d0c1..8fa2a041 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1322,9 +1322,9 @@ dependencies = [ [[package]] name = "http" -version = "0.2.4" +version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "527e8c9ac747e28542699a951517aa9a6945af506cd1f2e1b53a576c17b6cc11" +checksum = "1323096b05d41827dadeaee54c9981958c0f94e670bc94ed80037d1a7b8b186b" dependencies = [ "bytes 1.0.1", "fnv", @@ -2976,6 +2976,7 @@ dependencies = [ "hkdf", "hmac", "hostname", + "http", "jsonwebtoken", "lazy_static", "log", diff --git a/Cargo.toml b/Cargo.toml index d081e3d7..eda0a69c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ hex = "0.4.3" hostname = "0.3.1" hkdf = "0.11" hmac = "0.11" +http = "0.2.5" log = { version = "0.4", features = ["max_level_debug", "release_max_level_info"] } mime = "0.3" num_cpus = "1" diff --git a/config/local.example.toml b/config/local.example.toml index 3044c7cb..a1bfbef1 100644 --- a/config/local.example.toml +++ b/config/local.example.toml @@ -23,3 +23,7 @@ tokenserver.fxa_email_domain = "api-accounts.stage.mozaws.net" tokenserver.fxa_metrics_hash_secret = "INSERT_SECRET_KEY_HERE" tokenserver.fxa_oauth_server_url = "https://oauth.stage.mozaws.net" tokenserver.test_mode_enabled = false + +# cors settings +# cors_allowed_origin = "localhost" +# cors_max_age = 86400 diff --git a/src/server/mod.rs b/src/server/mod.rs index f8c339fc..849dfbae 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -2,7 +2,6 @@ use std::{sync::Arc, time::Duration}; -use actix_cors::Cors; use actix_web::{ dev, http::header::LOCATION, http::StatusCode, middleware::errhandlers::ErrorHandlers, web, App, HttpRequest, HttpResponse, HttpServer, @@ -64,7 +63,7 @@ pub struct Server; #[macro_export] macro_rules! build_app { - ($syncstorage_state: expr, $tokenserver_state: expr, $secrets: expr, $limits: expr) => { + ($syncstorage_state: expr, $tokenserver_state: expr, $secrets: expr, $limits: expr, $cors: expr) => { App::new() .data($syncstorage_state) .data($tokenserver_state) @@ -76,12 +75,7 @@ macro_rules! build_app { .wrap(middleware::weave::WeaveTimestamp::new()) .wrap(middleware::sentry::SentryWrapper::default()) .wrap(middleware::rejectua::RejectUA::default()) - // Followed by the "official middleware" so they run first. - // actix is getting increasingly tighter about CORS headers. Our server is - // not a huge risk but does deliver XHR JSON content. - // For now, let's be permissive and use NGINX (the wrapping server) - // for finer grained specification. - .wrap(Cors::permissive()) + .wrap($cors) .service( web::resource(&cfg_path("/info/collections")) .route(web::get().to(handlers::get_collections)), @@ -174,7 +168,7 @@ macro_rules! build_app { #[macro_export] macro_rules! build_app_without_syncstorage { - ($state: expr, $secrets: expr) => { + ($state: expr, $secrets: expr, $cors: expr) => { App::new() .data($state) .data($secrets) @@ -189,7 +183,7 @@ macro_rules! build_app_without_syncstorage { // not a huge risk but does deliver XHR JSON content. // For now, let's be permissive and use NGINX (the wrapping server) // for finer grained specification. - .wrap(Cors::permissive()) + .wrap($cors) .service( web::resource("/1.0/{application}/{version}") .route(web::get().to(tokenserver::handlers::get_tokenserver_result)), @@ -228,6 +222,7 @@ macro_rules! build_app_without_syncstorage { impl Server { pub async fn with_settings(settings: Settings) -> Result { + let settings_copy = settings.clone(); let metrics = metrics::metrics_from_opts(&settings)?; let host = settings.host.clone(); let port = settings.port; @@ -267,7 +262,8 @@ impl Server { syncstorage_state, tokenserver_state.clone(), Arc::clone(&secrets), - limits + limits, + settings_copy.build_cors() ) }); @@ -285,13 +281,17 @@ impl Server { pub async fn tokenserver_only_with_settings( settings: Settings, ) -> Result { + let settings_copy = settings.clone(); let host = settings.host.clone(); let port = settings.port; let secrets = Arc::new(settings.master_secret); let tokenserver_state = tokenserver::ServerState::from_settings(&settings.tokenserver)?; - let server = HttpServer::new(move || { - build_app_without_syncstorage!(Some(tokenserver_state.clone()), Arc::clone(&secrets)) + build_app_without_syncstorage!( + Some(tokenserver_state.clone()), + Arc::clone(&secrets), + settings_copy.build_cors() + ) }); let server = server diff --git a/src/server/test.rs b/src/server/test.rs index 7c66340b..1ea8b372 100644 --- a/src/server/test.rs +++ b/src/server/test.rs @@ -94,7 +94,8 @@ macro_rules! init_app { get_test_state(&$settings).await, None::, Arc::clone(&SECRETS), - limits + limits, + $settings.build_cors() )) .await } @@ -216,7 +217,8 @@ where get_test_state(&settings).await, None::, Arc::clone(&SECRETS), - limits + limits, + settings.build_cors() )) .await; @@ -256,7 +258,8 @@ async fn test_endpoint_with_body( get_test_state(&settings).await, None::, Arc::clone(&SECRETS), - limits + limits, + settings.build_cors() )) .await; let req = create_request(method, path, None, Some(body)).to_request(); diff --git a/src/settings.rs b/src/settings.rs index 4006085c..aec26503 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1,7 +1,9 @@ //! Application settings objects and initialization use std::{cmp::min, env}; +use actix_cors::Cors; use config::{Config, ConfigError, Environment, File}; +use http::method::Method; use serde::{de::Deserializer, Deserialize, Serialize}; use url::Url; @@ -9,6 +11,10 @@ use crate::db::spanner::models::MAX_SPANNER_LOAD_SIZE; use crate::error::ApiError; use crate::tokenserver::settings::Settings as TokenserverSettings; use crate::web::auth::hkdf_expand_32; +use crate::web::{ + X_LAST_MODIFIED, X_VERIFY_CODE, X_WEAVE_BYTES, X_WEAVE_NEXT_OFFSET, X_WEAVE_RECORDS, + X_WEAVE_TIMESTAMP, X_WEAVE_TOTAL_BYTES, X_WEAVE_TOTAL_RECORDS, +}; static DEFAULT_PORT: u16 = 8000; @@ -86,6 +92,12 @@ pub struct Settings { /// Settings specific to Tokenserver pub tokenserver: TokenserverSettings, + + /// Cors Settings + pub cors_allowed_origin: Option, + pub cors_max_age: Option, + pub cors_allowed_methods: Option>, + pub cors_allowed_headers: Option>, } impl Default for Settings { @@ -114,6 +126,10 @@ impl Default for Settings { spanner_emulator_host: None, disable_syncstorage: false, tokenserver: TokenserverSettings::default(), + cors_allowed_origin: None, + cors_allowed_methods: None, + cors_allowed_headers: None, + cors_max_age: None, } } } @@ -177,6 +193,29 @@ impl Settings { s.set_default("tokenserver.fxa_metrics_hash_secret", "secret")?; s.set_default("tokenserver.test_mode_enabled", false)?; + // Set Cors defaults + s.set_default( + "cors_allowed_headers", + Some(vec![ + "Authorization", + "Content-Type", + "UserAgent", + X_LAST_MODIFIED, + X_WEAVE_TIMESTAMP, + X_WEAVE_NEXT_OFFSET, + X_WEAVE_RECORDS, + X_WEAVE_BYTES, + X_WEAVE_TOTAL_RECORDS, + X_WEAVE_TOTAL_BYTES, + X_VERIFY_CODE, + "TEST_IDLES", + ]), + )?; + s.set_default( + "cors_allowed_methods", + Some(vec!["DELETE", "GET", "POST", "PUT"]), + )?; + // Merge the config file if supplied if let Some(config_filename) = filename { s.merge(File::with_name(config_filename))?; @@ -272,6 +311,37 @@ impl Settings { .unwrap_or_else(|_| "".to_owned()); format!("http://{}:{} ({}) {}", self.host, self.port, db, quota) } + + pub fn build_cors(&self) -> Cors { + // Followed by the "official middleware" so they run first. + // actix is getting increasingly tighter about CORS headers. Our server is + // not a huge risk but does deliver XHR JSON content. + // For now, let's be permissive and use NGINX (the wrapping server) + // for finer grained specification. + let mut cors = Cors::default(); + + if let Some(allowed_origin) = &self.cors_allowed_origin { + cors = cors.allowed_origin(allowed_origin); + } + + if let Some(allowed_methods) = &self.cors_allowed_methods { + let mut methods = vec![]; + for method_string in allowed_methods { + let method = Method::from_bytes(method_string.as_bytes()).unwrap(); + methods.push(method); + } + cors = cors.allowed_methods(methods); + } + if let Some(allowed_headers) = &self.cors_allowed_headers { + cors = cors.allowed_headers(allowed_headers); + } + + if let Some(max_age) = &self.cors_max_age { + cors = cors.max_age(*max_age); + } + + cors + } } /// Server-enforced limits for request payloads. diff --git a/src/web/mod.rs b/src/web/mod.rs index 472e421f..e9a266b9 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -11,6 +11,10 @@ pub static X_LAST_MODIFIED: &str = "x-last-modified"; pub static X_WEAVE_TIMESTAMP: &str = "x-weave-timestamp"; pub static X_WEAVE_NEXT_OFFSET: &str = "x-weave-next-offset"; pub static X_WEAVE_RECORDS: &str = "x-weave-records"; +pub static X_WEAVE_BYTES: &str = "x-weave-bytes"; +pub static X_WEAVE_TOTAL_RECORDS: &str = "x-weave-total-records"; +pub static X_WEAVE_TOTAL_BYTES: &str = "x-weave-total-bytes"; +pub static X_VERIFY_CODE: &str = "x-verify-code"; // Known DockerFlow commands for Ops callbacks pub const DOCKER_FLOW_ENDPOINTS: [&str; 4] = [ diff --git a/tools/integration_tests/run.py b/tools/integration_tests/run.py index 14b553b6..a2c8051d 100644 --- a/tools/integration_tests/run.py +++ b/tools/integration_tests/run.py @@ -37,9 +37,9 @@ if __name__ == "__main__": ) def start_server(): - the_server_subprocess = subprocess.Popen(target_binary, - shell=True, - env=os.environ) + the_server_subprocess = subprocess.Popen( + target_binary, shell=True, env=os.environ + ) # TODO we should change this to watch for a log message on startup # to know when to continue instead of sleeping for a fixed amount @@ -47,7 +47,10 @@ if __name__ == "__main__": return the_server_subprocess - os.environ.setdefault('SYNC_MASTER_SECRET', 'secret0') + os.environ.setdefault("SYNC_MASTER_SECRET", "secret0") + os.environ.setdefault("SYNC_CORS_MAX_AGE", "555") + os.environ.setdefault("SYNC_CORS_ALLOWED_ORIGIN", "localhost") + the_server_subprocess = start_server() atexit.register(lambda: terminate_process(the_server_subprocess)) res = run_live_functional_tests(TestStorage, sys.argv) diff --git a/tools/integration_tests/test_storage.py b/tools/integration_tests/test_storage.py index bf429fcd..87a54e39 100644 --- a/tools/integration_tests/test_storage.py +++ b/tools/integration_tests/test_storage.py @@ -16,6 +16,7 @@ consider it a bug. import unittest2 + import re import json import time @@ -29,6 +30,7 @@ import contextlib import simplejson from pyramid.interfaces import IAuthenticationPolicy +from webtest.app import AppError from test_support import StorageFunctionalTestCase import tokenlib @@ -1746,8 +1748,8 @@ class TestStorage(StorageFunctionalTestCase): self.assertTrue("max_request_bytes" in limits) endpoint = self.root + "/storage/xxx_col2?batch=true" - # There are certain obvious constraints on these limits, - # violations of which would be very confusing for clients. +# There are certain obvious constraints on these limits, +# violations of which would be very confusing for clients. # # self.assertTrue( # limits['max_request_bytes'] > limits['max_post_bytes'] @@ -2245,3 +2247,28 @@ class TestStorage(StorageFunctionalTestCase): testEmptyCommit("application/newlines", "\n", status=400) testEmptyCommit("application/newlines", "{}", status=400) testEmptyCommit("application/newlines", "[]", status=400) + + def test_cors_settings_are_set(self): + + res = self.app.options( + self.root + "/__heartbeat__", + headers={ + "Access-Control-Request-Method": "GET", + "Origin": "localhost", + "Access-Control-Request-Headers": "Content-Type", + }, + ) + + self.assertEqual( + int(res.headers["access-control-max-age"]), 555 + ) + self.assertEqual( + res.headers["access-control-allow-origin"], "localhost" + ) + + # PATCH is not a default allowed method, so request should return 405 + def test_patch_is_not_allowed(self): + collection = self.root + "/storage/xxx_col1" + with self.assertRaises(AppError) as error: + self.app.patch_json(collection) + self.assertIn("405", error)