Revert "DEV: enable cors to all cdn get requests from workbox. (#10684)" (#11076)

This reverts commit e3de45359f.

We need to improve out strategy by adding a cache breaker with this change ... some assets on CDNs and clients may have incorrect CORS headers which can cause stuff to break.

app/assets/javascripts/service-worker.js.erb

@@ -4,109 +4,24 @@ importScripts("<%= "#{Discourse.asset_host}#{Discourse.base_path}/javascripts/wo
 
 workbox.setConfig({
   modulePathPrefix: "<%= "#{Discourse.asset_host}#{Discourse.base_path}/javascripts/workbox" %>",
-  debug: <%= Rails.env.development? %>
+  debug: false
 });
 
 var authUrl = "<%= Discourse.base_path %>/auth/";
 
 var cacheVersion = "1";
-var discourseCacheName = "discourse-" + cacheVersion;
-var externalCacheName = "external-" + cacheVersion;
 
 // Cache all GET requests, so Discourse can be used while offline
-
 workbox.routing.registerRoute(
   function(args) {
-    return args.url.origin === location.origin && !args.url.pathname.startsWith(authUrl);
+    return !(args.url.origin === location.origin && args.url.pathname.startsWith(authUrl));
   }, // Match all except auth routes
   new workbox.strategies.NetworkFirst({ // This will only use the cache when a network request fails
-    cacheName: discourseCacheName,
+    cacheName: "discourse-" + cacheVersion,
     plugins: [
-      new workbox.cacheableResponse.Plugin({
-        statuses: [200] // opaque responses will return status code '0'
-      }), // for s3 secure media signed urls
       new workbox.expiration.Plugin({
         maxAgeSeconds: 7* 24 * 60 * 60, // 7 days
-        maxEntries: 250,
+        maxEntries: 500,
-        purgeOnQuotaError: true, // safe to automatically delete if exceeding the available storage
-      }),
-    ],
-  })
-);
-
-var cdnUrls = [];
-
-<% if GlobalSetting.try(:cdn_cors_enabled) %>
-cdnUrls = ["<%= "#{GlobalSetting.s3_cdn_url}" %>", "<%= "#{GlobalSetting.cdn_url}" %>"].filter(Boolean);
-
-if (cdnUrls.length > 0) {
-  var cdnCacheName = "cdn-" + cacheVersion;
-
-  var appendQueryStringPlugin = {
-    requestWillFetch: function (args) {
-      var request = args.request;
-
-      if (request.url.includes("avatar") || request.url.includes("emoji")) {
-        var url = new URL(request.url);
-        // Using this temporary query param to force browsers to redownload images from server.
-        url.searchParams.append('refresh', 'true');
-        return new Request(url.href, request);
-      }
-
-      return request;
-    }
-  };
-
-  workbox.routing.registerRoute(
-    function(args) {
-      var matching = cdnUrls.filter(
-        function(url) {
-          return args.url.href.startsWith(url);
-        }
-      );
-      return matching.length > 0;
-    }, // Match all cdn resources
-    new workbox.strategies.NetworkFirst({ // This will only use the cache when a network request fails
-      cacheName: cdnCacheName,
-      fetchOptions: {
-        mode: 'cors',
-        credentials: 'omit'
-      },
-      plugins: [
-        new workbox.expiration.Plugin({
-          maxAgeSeconds: 7* 24 * 60 * 60, // 7 days
-          maxEntries: 250,
-          purgeOnQuotaError: true, // safe to automatically delete if exceeding the available storage
-        }),
-        appendQueryStringPlugin
-      ],
-    })
-  );
-}
-<% end %>
-
-workbox.routing.registerRoute(
-  function(args) {
-    if (args.url.origin === location.origin) {
-      return false;
-    }
-
-    var matching = cdnUrls.filter(
-      function(url) {
-        return args.url.href.startsWith(url);
-      }
-    );
-    return matching.length === 0;
-  }, // Match all other external resources
-  new workbox.strategies.NetworkFirst({ // This will only use the cache when a network request fails
-    cacheName: externalCacheName,
-    plugins: [
-      new workbox.cacheableResponse.Plugin({
-        statuses: [200] // opaque responses will return status code '0'
-      }),
-      new workbox.expiration.Plugin({
-        maxAgeSeconds: 7* 24 * 60 * 60, // 7 days
-        maxEntries: 250,
         purgeOnQuotaError: true, // safe to automatically delete if exceeding the available storage
       }),
     ],

app/controllers/application_controller.rb

@@ -42,7 +42,6 @@ class ApplicationController < ActionController::Base
   before_action :preload_json
   before_action :add_noindex_header, if: -> { is_feed_request? || !SiteSetting.allow_index_in_robots_txt }
   before_action :check_xhr
-  before_action :block_cdn_requests
   after_action  :add_readonly_header
   after_action  :perform_refresh_session
   after_action  :dont_cache_page
@@ -673,19 +672,6 @@ class ApplicationController < ActionController::Base
     raise ApplicationController::RenderEmpty.new unless ((request.format && request.format.json?) || request.xhr?)
   end
 
-  def block_cdn_requests
-    raise Discourse::NotFound if Discourse.is_cdn_request?(request.env, request.method)
-  end
-
-  def apply_cdn_headers
-    Discourse.apply_cdn_headers(response.headers) if Discourse.is_cdn_request?(request.env, request.method)
-  end
-
-  def self.cdn_action(args = {})
-    skip_before_action :block_cdn_requests, args
-    before_action :apply_cdn_headers, args
-  end
-
   def self.requires_login(arg = {})
     @requires_login_arg = arg
   end

app/controllers/highlight_js_controller.rb

@@ -3,8 +3,6 @@
 class HighlightJsController < ApplicationController
   skip_before_action :preload_json, :redirect_to_login_if_required, :check_xhr, :verify_authenticity_token, only: [:show]
 
-  cdn_action only: [:show]
-
   def show
 
     no_cookies

app/controllers/static_controller.rb

@@ -7,8 +7,6 @@ class StaticController < ApplicationController
   skip_before_action :preload_json, only: [:brotli_asset, :cdn_asset, :enter, :favicon, :service_worker_asset]
   skip_before_action :handle_theme, only: [:brotli_asset, :cdn_asset, :enter, :favicon, :service_worker_asset]
 
-  cdn_action only: [:brotli_asset, :cdn_asset, :enter, :favicon, :service_worker_asset]
-
   PAGES_WITH_EMAIL_PARAM = ['login', 'password_reset', 'signup']
   MODAL_PAGES = ['password_reset', 'signup']
 

app/controllers/stylesheets_controller.rb

@@ -3,8 +3,6 @@
 class StylesheetsController < ApplicationController
   skip_before_action :preload_json, :redirect_to_login_if_required, :check_xhr, :verify_authenticity_token, only: [:show, :show_source_map, :color_scheme]
 
-  cdn_action only: [:show, :show_source_map, :color_scheme]
-
   def show_source_map
     show_resource(source_map: true)
   end

app/controllers/svg_sprite_controller.rb

@@ -3,8 +3,6 @@
 class SvgSpriteController < ApplicationController
   skip_before_action :preload_json, :redirect_to_login_if_required, :check_xhr, :verify_authenticity_token, only: [:show, :search, :svg_icon]
 
-  cdn_action only: [:show, :search, :svg_icon]
-
   requires_login except: [:show, :svg_icon]
 
   def show

app/controllers/theme_javascripts_controller.rb

@@ -13,8 +13,6 @@ class ThemeJavascriptsController < ApplicationController
 
   before_action :is_asset_path, :no_cookies, only: [:show]
 
-  cdn_action only: [:show]
-
   def show
     raise Discourse::NotFound unless last_modified.present?
     return render body: nil, status: 304 if not_modified?

app/controllers/uploads_controller.rb

@@ -10,8 +10,6 @@ class UploadsController < ApplicationController
 
   before_action :is_asset_path, only: [:show, :show_short, :show_secure]
 
-  cdn_action only: [:show, :show_short, :show_secure]
-
   SECURE_REDIRECT_GRACE_SECONDS = 5
 
   def create

app/controllers/user_avatars_controller.rb

@@ -4,8 +4,6 @@ class UserAvatarsController < ApplicationController
 
   skip_before_action :preload_json, :redirect_to_login_if_required, :check_xhr, :verify_authenticity_token, only: [:show, :show_letter, :show_proxy_letter]
 
-  cdn_action only: [:show, :show_letter, :show_proxy_letter]
-
   def refresh_gravatar
     user = User.find_by(username_lower: params[:username].downcase)
     guardian.ensure_can_edit!(user)

config/initializers/008-rack-cors.rb

@@ -25,19 +25,15 @@ class Discourse::Cors
     status, headers, body = @app.call(env)
     headers ||= {}
 
-    Discourse::Cors.apply_headers(cors_origins, env, headers)
+    Discourse::Cors.apply_headers(cors_origins, env, headers) if cors_origins
 
     [status, headers, body]
   end
 
   def self.apply_headers(cors_origins, env, headers)
-    request_method = env['REQUEST_METHOD']
+    origin = nil
-
-    if env['SCRIPT_NAME'] == "/assets" && Discourse.is_cdn_request?(env, request_method)
-      Discourse.apply_cdn_headers(headers)
-    elsif cors_origins
-      origin = nil
 
+    if cors_origins
       if origin = env['HTTP_ORIGIN']
         origin = nil unless cors_origins.include?(origin)
       end
@@ -52,6 +48,6 @@ class Discourse::Cors
   end
 end
 
-if GlobalSetting.enable_cors || GlobalSetting.cdn_url
+if GlobalSetting.enable_cors
   Rails.configuration.middleware.insert_before ActionDispatch::Flash, Discourse::Cors
 end

lib/discourse.rb

@@ -17,7 +17,6 @@ end
 
 module Discourse
   DB_POST_MIGRATE_PATH ||= "db/post_migrate"
-  REQUESTED_HOSTNAME ||= "REQUESTED_HOSTNAME"
 
   require 'sidekiq/exception_handler'
   class SidekiqExceptionHandler
@@ -910,24 +909,6 @@ module Discourse
   def self.is_parallel_test?
     ENV['RAILS_ENV'] == "test" && ENV['TEST_ENV_NUMBER']
   end
-
-  CDN_REQUEST_METHODS ||= ["GET", "HEAD", "OPTIONS"]
-
-  def self.is_cdn_request?(env, request_method)
-    return unless CDN_REQUEST_METHODS.include?(request_method)
-
-    cdn_hostnames = GlobalSetting.cdn_hostnames
-    return if cdn_hostnames.blank?
-
-    requested_hostname = env[REQUESTED_HOSTNAME] || env[Rack::HTTP_HOST]
-    cdn_hostnames.include?(requested_hostname)
-  end
-
-  def self.apply_cdn_headers(headers)
-    headers['Access-Control-Allow-Origin'] = '*'
-    headers['Access-Control-Allow-Methods'] = CDN_REQUEST_METHODS.join(", ")
-    headers
-  end
 end
 
 # rubocop:enable Style/GlobalVars

lib/middleware/enforce_hostname.rb

@@ -17,7 +17,6 @@ module Middleware
       allowed_hostnames = RailsMultisite::ConnectionManagement.current_db_hostnames
       requested_hostname = env[Rack::HTTP_HOST]
 
-      env[Discourse::REQUESTED_HOSTNAME] = requested_hostname
       env[Rack::HTTP_HOST] = allowed_hostnames.find { |h| h == requested_hostname } || Discourse.current_hostname
 
       @app.call(env)

lib/tasks/javascript.rake

@@ -135,10 +135,6 @@ def dependencies
       destination: 'workbox',
       public: true,
       skip_versioning: true
-    }, {
-      source: 'workbox-cacheable-response/build/.',
-      destination: 'workbox',
-      public: true
     }, {
       source: '@popperjs/core/dist/umd/popper.js'
     }, {

package.json

@@ -33,7 +33,6 @@
     "pikaday": "1.8.0",
     "resumablejs": "1.1.0",
     "spectrum-colorpicker": "1.8.0",
-    "workbox-cacheable-response": "^4.3.1",
     "workbox-core": "^4.3.1",
     "workbox-expiration": "^4.3.1",
     "workbox-routing": "^4.3.1",

public/javascripts/workbox/workbox-cacheable-response.dev.js

@@ -1,200 +0,0 @@
-this.workbox = this.workbox || {};
-this.workbox.cacheableResponse = (function (exports, WorkboxError_mjs, assert_mjs, getFriendlyURL_mjs, logger_mjs) {
-  'use strict';
-
-  try {
-    self['workbox:cacheable-response:4.3.1'] && _();
-  } catch (e) {} // eslint-disable-line
-
-  /*
-    Copyright 2018 Google LLC
-
-    Use of this source code is governed by an MIT-style
-    license that can be found in the LICENSE file or at
-    https://opensource.org/licenses/MIT.
-  */
-  /**
-   * This class allows you to set up rules determining what
-   * status codes and/or headers need to be present in order for a
-   * [`Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response)
-   * to be considered cacheable.
-   *
-   * @memberof workbox.cacheableResponse
-   */
-
-  class CacheableResponse {
-    /**
-     * To construct a new CacheableResponse instance you must provide at least
-     * one of the `config` properties.
-     *
-     * If both `statuses` and `headers` are specified, then both conditions must
-     * be met for the `Response` to be considered cacheable.
-     *
-     * @param {Object} config
-     * @param {Array<number>} [config.statuses] One or more status codes that a
-     * `Response` can have and be considered cacheable.
-     * @param {Object<string,string>} [config.headers] A mapping of header names
-     * and expected values that a `Response` can have and be considered cacheable.
-     * If multiple headers are provided, only one needs to be present.
-     */
-    constructor(config = {}) {
-      {
-        if (!(config.statuses || config.headers)) {
-          throw new WorkboxError_mjs.WorkboxError('statuses-or-headers-required', {
-            moduleName: 'workbox-cacheable-response',
-            className: 'CacheableResponse',
-            funcName: 'constructor'
-          });
-        }
-
-        if (config.statuses) {
-          assert_mjs.assert.isArray(config.statuses, {
-            moduleName: 'workbox-cacheable-response',
-            className: 'CacheableResponse',
-            funcName: 'constructor',
-            paramName: 'config.statuses'
-          });
-        }
-
-        if (config.headers) {
-          assert_mjs.assert.isType(config.headers, 'object', {
-            moduleName: 'workbox-cacheable-response',
-            className: 'CacheableResponse',
-            funcName: 'constructor',
-            paramName: 'config.headers'
-          });
-        }
-      }
-
-      this._statuses = config.statuses;
-      this._headers = config.headers;
-    }
-    /**
-     * Checks a response to see whether it's cacheable or not, based on this
-     * object's configuration.
-     *
-     * @param {Response} response The response whose cacheability is being
-     * checked.
-     * @return {boolean} `true` if the `Response` is cacheable, and `false`
-     * otherwise.
-     */
-
-
-    isResponseCacheable(response) {
-      {
-        assert_mjs.assert.isInstance(response, Response, {
-          moduleName: 'workbox-cacheable-response',
-          className: 'CacheableResponse',
-          funcName: 'isResponseCacheable',
-          paramName: 'response'
-        });
-      }
-
-      let cacheable = true;
-
-      if (this._statuses) {
-        cacheable = this._statuses.includes(response.status);
-      }
-
-      if (this._headers && cacheable) {
-        cacheable = Object.keys(this._headers).some(headerName => {
-          return response.headers.get(headerName) === this._headers[headerName];
-        });
-      }
-
-      {
-        if (!cacheable) {
-          logger_mjs.logger.groupCollapsed(`The request for ` + `'${getFriendlyURL_mjs.getFriendlyURL(response.url)}' returned a response that does ` + `not meet the criteria for being cached.`);
-          logger_mjs.logger.groupCollapsed(`View cacheability criteria here.`);
-          logger_mjs.logger.log(`Cacheable statuses: ` + JSON.stringify(this._statuses));
-          logger_mjs.logger.log(`Cacheable headers: ` + JSON.stringify(this._headers, null, 2));
-          logger_mjs.logger.groupEnd();
-          const logFriendlyHeaders = {};
-          response.headers.forEach((value, key) => {
-            logFriendlyHeaders[key] = value;
-          });
-          logger_mjs.logger.groupCollapsed(`View response status and headers here.`);
-          logger_mjs.logger.log(`Response status: ` + response.status);
-          logger_mjs.logger.log(`Response headers: ` + JSON.stringify(logFriendlyHeaders, null, 2));
-          logger_mjs.logger.groupEnd();
-          logger_mjs.logger.groupCollapsed(`View full response details here.`);
-          logger_mjs.logger.log(response.headers);
-          logger_mjs.logger.log(response);
-          logger_mjs.logger.groupEnd();
-          logger_mjs.logger.groupEnd();
-        }
-      }
-
-      return cacheable;
-    }
-
-  }
-
-  /*
-    Copyright 2018 Google LLC
-
-    Use of this source code is governed by an MIT-style
-    license that can be found in the LICENSE file or at
-    https://opensource.org/licenses/MIT.
-  */
-  /**
-   * A class implementing the `cacheWillUpdate` lifecycle callback. This makes it
-   * easier to add in cacheability checks to requests made via Workbox's built-in
-   * strategies.
-   *
-   * @memberof workbox.cacheableResponse
-   */
-
-  class Plugin {
-    /**
-     * To construct a new cacheable response Plugin instance you must provide at
-     * least one of the `config` properties.
-     *
-     * If both `statuses` and `headers` are specified, then both conditions must
-     * be met for the `Response` to be considered cacheable.
-     *
-     * @param {Object} config
-     * @param {Array<number>} [config.statuses] One or more status codes that a
-     * `Response` can have and be considered cacheable.
-     * @param {Object<string,string>} [config.headers] A mapping of header names
-     * and expected values that a `Response` can have and be considered cacheable.
-     * If multiple headers are provided, only one needs to be present.
-     */
-    constructor(config) {
-      this._cacheableResponse = new CacheableResponse(config);
-    }
-    /**
-     * @param {Object} options
-     * @param {Response} options.response
-     * @return {boolean}
-     * @private
-     */
-
-
-    cacheWillUpdate({
-      response
-    }) {
-      if (this._cacheableResponse.isResponseCacheable(response)) {
-        return response;
-      }
-
-      return null;
-    }
-
-  }
-
-  /*
-    Copyright 2018 Google LLC
-
-    Use of this source code is governed by an MIT-style
-    license that can be found in the LICENSE file or at
-    https://opensource.org/licenses/MIT.
-  */
-
-  exports.CacheableResponse = CacheableResponse;
-  exports.Plugin = Plugin;
-
-  return exports;
-
-}({}, workbox.core._private, workbox.core._private, workbox.core._private, workbox.core._private));
-//# sourceMappingURL=workbox-cacheable-response.dev.js.map

public/javascripts/workbox/workbox-cacheable-response.prod.js

@@ -1,2 +0,0 @@
-this.workbox=this.workbox||{},this.workbox.cacheableResponse=function(t){"use strict";try{self["workbox:cacheable-response:4.3.1"]&&_()}catch(t){}class s{constructor(t={}){this.t=t.statuses,this.s=t.headers}isResponseCacheable(t){let s=!0;return this.t&&(s=this.t.includes(t.status)),this.s&&s&&(s=Object.keys(this.s).some(s=>t.headers.get(s)===this.s[s])),s}}return t.CacheableResponse=s,t.Plugin=class{constructor(t){this.i=new s(t)}cacheWillUpdate({response:t}){return this.i.isResponseCacheable(t)?t:null}},t}({});
-//# sourceMappingURL=workbox-cacheable-response.prod.js.map

spec/requests/application_controller_spec.rb

@@ -737,17 +737,6 @@ RSpec.describe ApplicationController do
       end
     end
 
-    context "cdn requests" do
-      before do
-        GlobalSetting.stubs(:cdn_url).returns("https://www.example.com/")
-      end
-
-      it "should block the dynamic routes" do
-        get "/"
-        expect(response.status).to eq(404)
-      end
-    end
-
     context "set_locale_from_accept_language_header enabled" do
       context "accept-language header differs from default locale" do
         before do

spec/requests/static_controller_spec.rb

@@ -116,25 +116,6 @@ describe StaticController do
         File.delete(file_path)
       end
     end
-
-    it 'has correct cors headers for brotli assets' do
-      begin
-        assets_path = Rails.root.join("public/assets")
-
-        FileUtils.mkdir_p(assets_path)
-
-        file_path = assets_path.join("test.js.br")
-        File.write(file_path, 'fake brotli file')
-        GlobalSetting.stubs(:cdn_url).returns("https://www.example.com/")
-
-        get "/brotli_asset/test.js"
-
-        expect(response.status).to eq(200)
-        expect(response.headers["Access-Control-Allow-Origin"]).to match("*")
-      ensure
-        File.delete(file_path)
-      end
-    end
   end
 
   context '#cdn_asset' do

yarn.lock

@@ -3078,13 +3078,6 @@ wordwrap@^1.0.0:
   resolved "https://registry.yarnpkg.com/wordwrap/-/wordwrap-1.0.0.tgz#27584810891456a4171c8d0226441ade90cbcaeb"
   integrity sha1-J1hIEIkUVqQXHI0CJkQa3pDLyus=
 
-workbox-cacheable-response@^4.3.1:
-  version "4.3.1"
-  resolved "https://registry.yarnpkg.com/workbox-cacheable-response/-/workbox-cacheable-response-4.3.1.tgz#f53e079179c095a3f19e5313b284975c91428c91"
-  integrity sha512-Rp5qlzm6z8IOvnQNkCdO9qrDgDpoPNguovs0H8C+wswLuPgSzSp9p2afb5maUt9R1uTIwOXrVQMmPfPypv+npw==
-  dependencies:
-    workbox-core "^4.3.1"
-
 workbox-core@^4.3.1:
   version "4.3.1"
   resolved "https://registry.yarnpkg.com/workbox-core/-/workbox-core-4.3.1.tgz#005d2c6a06a171437afd6ca2904a5727ecd73be6"