diff --git a/config/application.rb b/config/application.rb index cc962e72650d5ebd7efe0fc4107b96c64e50f509..16920401814d3476dbab4158c0665823139f74cd 100644 --- a/config/application.rb +++ b/config/application.rb @@ -84,6 +84,7 @@ class Application < Rails::Application require_dependency Rails.root.join('lib/gitlab/middleware/path_traversal_check') require_dependency Rails.root.join('lib/gitlab/middleware/rack_multipart_tempfile_factory') require_dependency Rails.root.join('lib/gitlab/middleware/secure_headers') + require_dependency Rails.root.join('lib/gitlab/middleware/web_ide_extension_host_domain') require_dependency Rails.root.join('lib/gitlab/runtime') require_dependency Rails.root.join('lib/gitlab/patch/database_config') require_dependency Rails.root.join('lib/gitlab/patch/redis_cache_store') @@ -445,6 +446,9 @@ class Application < Rails::Application config.middleware.insert_after ActionDispatch::ShowExceptions, ::Gitlab::Middleware::JsonValidation + # Insert this early in the middleware stack to bypass ActionDispatch::HostAuthorization + config.middleware.unshift ::Gitlab::Middleware::WebIdeExtensionHostDomain + # Allow access to GitLab API from other domains config.middleware.insert_before Warden::Manager, Rack::Cors do headers_to_expose = %w[Link X-Total X-Total-Pages X-Per-Page X-Page X-Next-Page X-Prev-Page X-Gitlab-Blob-Id X-Gitlab-Commit-Id X-Gitlab-Content-Sha256 X-Gitlab-Encoding X-Gitlab-File-Name X-Gitlab-File-Path X-Gitlab-Last-Commit-Id X-Gitlab-Ref X-Gitlab-Size X-Request-Id ETag] @@ -523,24 +527,6 @@ class Application < Rails::Application methods: %i[get head] end end - - # Allow assets to be loaded to web-ide - # https://gitlab.com/gitlab-org/gitlab/-/issues/421177 - allow do - origins 'https://*.web-ide.gitlab-static.net' - resource '/assets/webpack/*', - credentials: false, - methods: %i[get head] - end - - # Allow assets to be loaded to web-ide - # https://gitlab.com/gitlab-org/gitlab/-/issues/421177 - allow do - origins 'https://*.web-ide.gitlab-static.net' - resource '/assets/vite/*', - credentials: false, - methods: %i[get head] - end end # Use caching across all environments diff --git a/lib/gitlab/middleware/web_ide_extension_host_domain.rb b/lib/gitlab/middleware/web_ide_extension_host_domain.rb new file mode 100644 index 0000000000000000000000000000000000000000..354f4e4302c3b595e552827c08ff75d288d850e6 --- /dev/null +++ b/lib/gitlab/middleware/web_ide_extension_host_domain.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Gitlab + module Middleware + class WebIdeExtensionHostDomain + def initialize(app) + @app = app + end + + def call(env) + # This middleware handles GET,OPTIONS requests to get CORS headers for static assets. + # The OPTIONS HTTP request is sent by Workhorse when it serves a static asset and + # the static asset route does not exist in Rails. For this type of request, the middleware + # returns early. + # This middleware also handles GET requests although this type request only happen in + # development or test environments when the sprockets pipeline is enabled. + request = ActionDispatch::Request.new(env) + + return @app.call(env) unless handles_request?(request) + + headers = get_cors_headers(request) + + return [204, headers, []] if request.method == 'OPTIONS' + + status, base_headers, body = @app.call(env) + + headers.merge!(base_headers) + + [status, headers, body] + end + + private + + def handles_request?(request) + %w[OPTIONS GET].include?(request.method) && request.path.start_with?('/assets/') + end + + def get_cors_headers(request) + origin_header = request.headers['Origin'] + + if origin_header.present? && + origin_header.match?(::WebIde::ExtensionMarketplace.origin_matches_extension_host_regexp) + { + 'Access-Control-Allow-Origin' => request.headers['Origin'], + 'Access-Control-Allow-Methods' => 'GET, HEAD, OPTIONS', + 'Vary' => 'Origin' + } + else + {} + end + end + end + end +end diff --git a/lib/web_ide/extension_marketplace.rb b/lib/web_ide/extension_marketplace.rb index c385842778ce5b0bab414bff2d3325d53cab91d5..fb367bb813979ee1b736505dc0bf95610dd433fb 100644 --- a/lib/web_ide/extension_marketplace.rb +++ b/lib/web_ide/extension_marketplace.rb @@ -51,5 +51,9 @@ def self.extension_host_domain def self.extension_host_domain_changed? extension_host_domain != DEFAULT_EXTENSION_HOST_DOMAIN end + + def self.origin_matches_extension_host_regexp + %r{^https://(?:v--|workbench-)?[a-z0-9]{30,56}\.#{Regexp.escape(extension_host_domain)}$} + end end end diff --git a/spec/lib/gitlab/middleware/web_ide_extension_host_domain_spec.rb b/spec/lib/gitlab/middleware/web_ide_extension_host_domain_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..a01c7e83630ee455f9ba58b5b35bfb461b38e595 --- /dev/null +++ b/spec/lib/gitlab/middleware/web_ide_extension_host_domain_spec.rb @@ -0,0 +1,187 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Middleware::WebIdeExtensionHostDomain, feature_category: :web_ide do + let(:app) { double(:app) } # rubocop:disable RSpec/VerifiedDoubles -- stubbed app + let(:middleware) { described_class.new(app) } + + let(:env) do + { + 'REQUEST_METHOD' => request_method, + 'PATH_INFO' => path_info, + 'HTTP_ORIGIN' => origin_header + } + end + + let(:request_method) { 'GET' } + let(:path_info) { '/assets/application.js' } + let(:origin_header) { nil } + let(:extension_host_domain) { 'cdn.web-ide.gitlab-static.net' } + + before do + allow(app).to receive(:call).and_return([200, { 'Content-Type' => 'application/javascript' }, ['OK']]) + allow(WebIde::ExtensionMarketplace).to receive(:origin_matches_extension_host_regexp).and_return( + %r{^https://(?:v--|workbench-)?[a-z0-9]{30,56}\.#{Regexp.escape(extension_host_domain)}$} + ) + end + + describe '#call' do + context 'when request is for assets path' do + let(:path_info) { '/assets/application.js' } + + shared_examples 'handles Origin header scenarios' do + context 'when Origin header matches extension host domain' do + let(:origin_header) { matching_origin } + + it 'includes CORS headers' do + status, headers, body = middleware.call(env) + + expect(status).to eq(expected_status) + expect(headers['Access-Control-Allow-Origin']).to eq(origin_header) + expect(headers['Access-Control-Allow-Methods']).to eq('GET, HEAD, OPTIONS') + expect(headers['Vary']).to eq('Origin') + expect(body).to eq(expected_body) + end + end + + context 'when Origin header does not match extension host domain' do + let(:origin_header) { 'https://evil.com' } + + it 'does not include CORS headers' do + status, headers, body = middleware.call(env) + + expect(status).to eq(expected_status) + expect(headers['Access-Control-Allow-Origin']).to be_nil + expect(headers['Access-Control-Allow-Methods']).to be_nil + expect(headers['Vary']).to be_nil + expect(body).to eq(expected_body) + end + end + + context 'when Origin header is not present' do + let(:origin_header) { nil } + + it 'does not include CORS headers' do + status, headers, body = middleware.call(env) + + expect(status).to eq(expected_status) + expect(headers['Access-Control-Allow-Origin']).to be_nil + expect(body).to eq(expected_body) + end + end + end + + context 'with OPTIONS request' do + let(:request_method) { 'OPTIONS' } + let(:matching_origin) { 'https://abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + let(:expected_status) { 204 } + let(:expected_body) { [] } + + include_examples 'handles Origin header scenarios' + + it 'does not pass the request to other middlewares' do + expect(app).not_to receive(:call) + + middleware.call(env) + end + end + + context 'with GET request' do + let(:request_method) { 'GET' } + let(:matching_origin) { 'https://v--abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + let(:expected_status) { 200 } + let(:expected_body) { ['OK'] } + + include_examples 'handles Origin header scenarios' + + it 'passes the request to other middlewares' do + expect(app).to receive(:call).with(env) + .and_return([200, { 'Content-Type' => 'application/javascript' }, ['OK']]) + + middleware.call(env) + end + end + end + + context 'when request is not for assets path' do + let(:path_info) { '/users/sign_in' } + let(:request_method) { 'GET' } + let(:origin_header) { 'https://abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + + it 'passes through to the app without handling' do + expect(app).to receive(:call).with(env).and_return([200, {}, ['OK']]) + + status, headers, body = middleware.call(env) + + expect(status).to eq(200) + expect(headers['Access-Control-Allow-Origin']).to be_nil + expect(body).to eq(['OK']) + end + end + + context 'when request method is not GET or OPTIONS' do + let(:path_info) { '/assets/application.js' } + let(:request_method) { 'POST' } + let(:origin_header) { 'https://abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + + it 'passes through to the app without handling' do + expect(app).to receive(:call).with(env).and_return([200, {}, ['OK']]) + + status, headers, body = middleware.call(env) + + expect(status).to eq(200) + expect(headers['Access-Control-Allow-Origin']).to be_nil + expect(body).to eq(['OK']) + end + end + + context 'when path does not start with /assets/' do + let(:request_method) { 'GET' } + + [ + '/api/v4/projects', + '/asset/file.js', # singular 'asset' + '/public/assets/file.js', # nested assets + '/' + ].each do |non_asset_path| + context "with path #{non_asset_path}" do + let(:path_info) { non_asset_path } + let(:origin_header) { 'https://abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + + it 'passes through to the app without handling' do + expect(app).to receive(:call).with(env) + + middleware.call(env) + end + end + end + end + + context 'when merging headers from app response' do + let(:path_info) { '/assets/application.js' } + let(:request_method) { 'GET' } + let(:origin_header) { 'https://abcdefghijklmnopqrstuvwxyz1234.cdn.web-ide.gitlab-static.net' } + + it 'preserves headers from the app response' do + app_headers = { + 'Content-Type' => 'application/javascript', + 'Cache-Control' => 'public, max-age=3600', + 'X-Custom-Header' => 'value' + } + allow(app).to receive(:call).with(env).and_return([200, app_headers, ['OK']]) + + status, headers, body = middleware.call(env) + + expect(status).to eq(200) + expect(headers['Content-Type']).to eq('application/javascript') + expect(headers['Cache-Control']).to eq('public, max-age=3600') + expect(headers['X-Custom-Header']).to eq('value') + expect(headers['Access-Control-Allow-Origin']).to eq(origin_header) + expect(headers['Access-Control-Allow-Methods']).to eq('GET, HEAD, OPTIONS') + expect(headers['Vary']).to eq('Origin') + expect(body).to eq(['OK']) + end + end + end +end diff --git a/spec/lib/web_ide/extension_marketplace_spec.rb b/spec/lib/web_ide/extension_marketplace_spec.rb index d1d12d5992de1adea6783d22ae8be8611ffc95bf..677aae505bf87681ad778d2e6188d76bd10da7b4 100644 --- a/spec/lib/web_ide/extension_marketplace_spec.rb +++ b/spec/lib/web_ide/extension_marketplace_spec.rb @@ -145,4 +145,59 @@ it { is_expected.to be(true) } end end + + describe '#origin_matches_extension_host_regexp' do + subject(:regexp) { described_class.origin_matches_extension_host_regexp } + + where(:extension_host_domain, :origin, :should_match) do + # matches valid origins with minimum length identifier + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234.web-ide.net' | true + # matches valid origins with maximum length identifier + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz123456789012345678901234567890.web-ide.net' | true + # matches origins with v-- prefix + 'web-ide.net' | 'https://v--abcdefghijklmnopqrstuvwxyz1234.web-ide.net' | true + # matches origins with workbench- prefix + 'web-ide.net' | 'https://workbench-abcdefghijklmnopqrstuvwxyz1234.web-ide.net' | true + # matches origins with port + 'web-ide.net:3443' | 'https://workbench-abcdefghijklmnopqrstuvwxyz1234.web-ide.net:3443' | true + + # invalid cases + # does not match origins with too short identifier + 'web-ide.net' | 'https://abc123.web-ide.net' | false + # does not match origins with too long identifier + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234567890123456789012345678901.web-ide.net' | false + # does not match origins with uppercase letters in identifier + 'web-ide.net' | 'https://ABCDEFGHIJKLMNOPQRSTUVWXYZ1234.web-ide.net' | false + # does not match origins with special characters in identifier + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz-123.web-ide.net' | false + # does not match http origins + 'web-ide.net' | 'http://abcdefghijklmnopqrstuvwxyz1234.web-ide.net' | false + # does not match origins without protocol + 'web-ide.net' | 'abcdefghijklmnopqrstuvwxyz1234.web-ide.net' | false + # does not match origins with wrong domain + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234.example.com' | false + # does not match origins with path + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234.web-ide.net/path' | false + # does not match origins with query parameters + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234.web-ide.net?param=value' | false + # does not match origins with fragment + 'web-ide.net' | 'https://abcdefghijklmnopqrstuvwxyz1234.web-ide.net#fragment' | false + end + + with_them do + before do + Gitlab::CurrentSettings.update!( + vscode_extension_marketplace_extension_host_domain: extension_host_domain + ) + end + + it 'matches valid origins and rejects invalid ones' do + if should_match + expect(origin).to match(regexp) + else + expect(origin).not_to match(regexp) + end + end + end + end end diff --git a/workhorse/_support/lint_last_known_acceptable.txt b/workhorse/_support/lint_last_known_acceptable.txt index 42cbe0784c03e8da58f920a5d07598920229857c..6df3e83b027fc15631f69b38c01f98fe64b4c20f 100644 --- a/workhorse/_support/lint_last_known_acceptable.txt +++ b/workhorse/_support/lint_last_known_acceptable.txt @@ -16,7 +16,7 @@ internal/imageresizer/image_resizer.go:369:13: Error return value of `file.Close cmd/gitlab-workhorse/main.go:118:6: Function 'buildConfig' has too many statements (41 > 40) (funlen) internal/healthcheck/puma_checker.go:105:32: Function 'Check' is too long (62 > 60) (funlen) internal/imageresizer/image_resizer.go:152:19: Function 'Inject' is too long (61 > 60) (funlen) -internal/upstream/routes.go:302:6: Function 'configureRoutes' is too long (305 > 60) (funlen) +internal/upstream/routes.go:292:6: Function 'configureRoutes' is too long (303 > 60) (funlen) cmd/gitlab-workhorse/main.go:210:1: cognitive complexity 26 of func `run` is high (> 20) (gocognit) internal/git/archive.go:67:1: cognitive complexity 23 of func `(*archive).Inject` is high (> 20) (gocognit) internal/transport/transport.go:147:1: cognitive complexity 59 of func `validateIPAddress` is high (> 20) (gocognit) @@ -63,4 +63,4 @@ internal/imageresizer/image_resizer.go:1:1: package-comments: should have a pack internal/imageresizer/image_resizer.go:33:6: exported: exported type Resizer should have comment or be unexported (revive) internal/imageresizer/image_resizer.go:144:1: exported: exported function NewResizer should have comment or be unexported (revive) internal/upstream/upstream.go:279:56: redefines-builtin-id: redefinition of the built-in type error (revive) -internal/upstream/routes.go:223:74: (*upstream).wsRoute - matchers always receives nil (unparam) +internal/upstream/routes.go:213:74: (*upstream).wsRoute - matchers always receives nil (unparam) diff --git a/workhorse/internal/staticpages/servefile.go b/workhorse/internal/staticpages/servefile.go index 98307556199828c880bf06174087894979218f92..1ea4f38ad9aa43e2706f9a4155d017fd7c0fd923 100644 --- a/workhorse/internal/staticpages/servefile.go +++ b/workhorse/internal/staticpages/servefile.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "time" @@ -75,6 +76,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun } }() + s.resolveCorsHeaders(r, w) s.setCacheHeaders(w, cache) s.logFileServed(r.Context(), file, w.Header().Get("Content-Encoding"), r.Method, r.RequestURI) @@ -84,6 +86,36 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun var errPathTraversal = errors.New("path traversal") +func (s *Static) resolveCorsHeaders(r *http.Request, w http.ResponseWriter) { + allowedMethods := []string{"OPTIONS", "GET"} + allowedHeaders := map[string]bool{ + "access-control-allow-origin": true, + "access-control-allow-methods": true, + "access-control-allow-headers": true, + "access-control-allow-credentials": true, + "vary": true, + } + hasOriginHeader := len(r.Header.Get("Origin")) > 0 + isAssetsPath := strings.HasPrefix(r.URL.Path, "/assets/") + + if isAssetsPath && hasOriginHeader && slices.Contains(allowedMethods, r.Method) { + optionsRequest := &http.Request{Method: "OPTIONS", URL: r.URL, Header: r.Header.Clone()} + httpResponse, _, _ := s.API.PreAuthorize(r.RequestURI, optionsRequest) + + if httpResponse == nil { + return + } + + for k, v := range httpResponse.Header { + if allowedHeaders[strings.ToLower(k)] { + w.Header()[k] = v + } + } + + defer func() { _ = httpResponse.Body.Close() }() + } +} + func (s *Static) getFile(prefix urlprefix.Prefix, path string) (string, error) { relativePath, err := s.validatePath(prefix.Strip(path)) if err != nil { diff --git a/workhorse/internal/staticpages/servefile_test.go b/workhorse/internal/staticpages/servefile_test.go index a867d82d68e0ed91c1b000392432c5f824cc318e..49564bf4d5a3c97ac7a194c4907c3cb084016067 100644 --- a/workhorse/internal/staticpages/servefile_test.go +++ b/workhorse/internal/staticpages/servefile_test.go @@ -9,9 +9,12 @@ import ( "path/filepath" "testing" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/testhelper" - "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/testhelper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/upstream/roundtripper" ) const ( @@ -157,3 +160,180 @@ func TestServingThePregzippedFile(t *testing.T) { func TestServingThePregzippedFileWithoutEncoding(t *testing.T) { testServingThePregzippedFile(t, false) } + +// testRailsServer creates a mock Rails server that returns the specified headers +func testRailsServer(t *testing.T, headers map[string]string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Set the headers that Rails would return + for k, v := range headers { + w.Header().Set(k, v) + } + w.WriteHeader(http.StatusOK) + })) +} + +// verifyCorsHeaders verifies that the expected CORS headers are present and no unexpected CORS headers are set +func verifyCorsHeaders(t *testing.T, w *httptest.ResponseRecorder, expectedHeaders map[string]string) { + t.Helper() + + // Verify expected CORS headers are present with correct values + for expectedKey, expectedValue := range expectedHeaders { + require.Equal(t, expectedValue, w.Header().Get(expectedKey), + "Expected CORS header %s to be %s", expectedKey, expectedValue) + } + + // Verify that headers NOT in expectedHeaders are NOT set + allCorsHeaders := []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Allow-Credentials", + "Vary", + } + for _, header := range allCorsHeaders { + if _, expected := expectedHeaders[header]; !expected { + require.Empty(t, w.Header().Get(header), + "Header %s should not be set", header) + } + } +} + +func TestResolveCorsHeaders(t *testing.T) { + dir := t.TempDir() + + // Create a test file in the assets directory + assetsDir := filepath.Join(dir, "assets") + err := os.MkdirAll(assetsDir, 0755) + require.NoError(t, err) + + fileContent := "STATIC ASSET" + err = os.WriteFile(filepath.Join(assetsDir, "test.js"), []byte(fileContent), 0600) + require.NoError(t, err) + + testCases := []struct { + desc string + path string + method string + hasOriginHeader bool + railsHeaders map[string]string + expectedHeaders map[string]string + }{ + { + desc: "CORS headers returned for GET request on /assets/ with Origin header", + path: "/assets/test.js", + method: "GET", + hasOriginHeader: true, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + "Access-Control-Allow-Methods": "GET, OPTIONS", + "Access-Control-Allow-Headers": "Content-Type", + "Access-Control-Allow-Credentials": "true", + "Vary": "Origin", + }, + expectedHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + "Access-Control-Allow-Methods": "GET, OPTIONS", + "Access-Control-Allow-Headers": "Content-Type", + "Access-Control-Allow-Credentials": "true", + "Vary": "Origin", + }, + }, + { + desc: "CORS headers returned for OPTIONS request on /assets/ with Origin header", + path: "/assets/test.js", + method: "OPTIONS", + hasOriginHeader: true, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + "Access-Control-Allow-Methods": "GET, OPTIONS", + }, + expectedHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + "Access-Control-Allow-Methods": "GET, OPTIONS", + }, + }, + { + desc: "No CORS headers when Origin header is missing", + path: "/assets/test.js", + method: "GET", + hasOriginHeader: false, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + }, + expectedHeaders: map[string]string{}, + }, + { + desc: "No CORS headers for POST request (not in allowed methods)", + path: "/assets/test.js", + method: "POST", + hasOriginHeader: true, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + }, + expectedHeaders: map[string]string{}, + }, + { + desc: "No CORS headers for non-assets path", + path: "/other/test.js", + method: "GET", + hasOriginHeader: true, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + }, + expectedHeaders: map[string]string{}, + }, + { + desc: "Only allowed CORS headers are copied", + path: "/assets/test.js", + method: "GET", + hasOriginHeader: true, + railsHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + "X-Custom-Header": "should-not-be-copied", + "Content-Type": "application/json", + }, + expectedHeaders: map[string]string{ + "Access-Control-Allow-Origin": "https://example.com", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + testhelper.ConfigureSecret() + + // Create a mock Rails server that returns the specified CORS headers + ts := testRailsServer(t, tc.railsHeaders) + defer ts.Close() + + // Create API client + backend := helper.URLMustParse(ts.URL) + rt := roundtripper.NewTestBackendRoundTripper(backend) + apiClient := api.NewAPI(backend, "123", rt) + + // Create request + httpRequest, err := http.NewRequest(tc.method, tc.path, nil) + require.NoError(t, err) + + if tc.hasOriginHeader { + httpRequest.Header.Set("Origin", "https://example.com") + } + + // Create static handler with API client + w := httptest.NewRecorder() + st := &Static{DocumentRoot: dir, API: apiClient} + st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) + + // For paths where the file exists, we get 200 + // For paths outside /assets or without origin, we get 200 but no CORS headers + // For valid cases with CORS, we should get 200 with CORS headers + if tc.path == "/assets/test.js" && (tc.method == "GET" || tc.method == "OPTIONS") { + require.Equal(t, 200, w.Code, "Request should succeed when file exists") + } + + // Verify CORS headers + verifyCorsHeaders(t, w, tc.expectedHeaders) + }) + } +} diff --git a/workhorse/internal/staticpages/static.go b/workhorse/internal/staticpages/static.go index 7cc74e91f9ff818fdef71828bbf6ed71d976c1ed..b07aa0a6a18f8c9c465109884e3a3a21ad271764 100644 --- a/workhorse/internal/staticpages/static.go +++ b/workhorse/internal/staticpages/static.go @@ -1,11 +1,16 @@ package staticpages -import "net/http" +import ( + "net/http" + + apipkg "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" +) // Static represents a package for serving static pages and handling errors. type Static struct { DocumentRoot string Exclude []string + API *apipkg.API } func setNoCacheHeaders(header http.Header) { diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index b0151686b219ac40c16414e55bb207fc0849eda0..59b3725cdde7b498eddb5eb296f21b808b559143 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -60,7 +60,6 @@ type routeOptions struct { tracing bool isGeoProxyRoute bool matchers []matcherFunc - allowOrigins *regexp.Regexp bodyLimit int64 bodyLimitMode bodylimit.Mode } @@ -122,12 +121,6 @@ func withGeoProxy() func(*routeOptions) { } } -func withAllowOrigins(pattern string) func(*routeOptions) { - return func(options *routeOptions) { - options.allowOrigins = compileRegexp(pattern) - } -} - func withBodyLimit(bodyLimit int64) func(*routeOptions) { return func(options *routeOptions) { options.bodyLimit = bodyLimit @@ -204,9 +197,6 @@ func (u *upstream) route(method string, metadata routeMetadata, handler http.Han // Add distributed tracing handler = tracing.Handler(handler, tracing.WithRouteIdentifier(metadata.regexpStr)) } - if options.allowOrigins != nil { - handler = corsMiddleware(handler, options.allowOrigins) - } if options.bodyLimit > 0 { handler = withBodyLimitContext(options.bodyLimit, options.bodyLimitMode, handler) @@ -301,7 +291,7 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg conf func configureRoutes(u *upstream) { api := u.APIClient - static := &staticpages.Static{DocumentRoot: u.DocumentRoot, Exclude: staticExclude} + static := &staticpages.Static{DocumentRoot: u.DocumentRoot, Exclude: staticExclude, API: u.APIClient} dependencyProxyInjector := dependencyproxy.NewInjector() // Build proxy with optional success tracking @@ -580,7 +570,6 @@ func configureRoutes(u *upstream) { assetsNotFoundHandler, ), withoutTracing(), // Tracing on assets is very noisy - withAllowOrigins("^https://.*\\.web-ide\\.gitlab-static\\.net$"), ), // Uploads @@ -671,7 +660,6 @@ func configureRoutes(u *upstream) { assetsNotFoundHandler, ), withoutTracing(), // Tracing on assets is very noisy - withAllowOrigins("^https://.*\\.web-ide\\.gitlab-static\\.net$"), ), // Don't define a catch-all route. If a route does not match, then we know @@ -690,23 +678,6 @@ func denyWebsocket(next http.Handler) http.Handler { }) } -func corsMiddleware(next http.Handler, allowOriginRegex *regexp.Regexp) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requestOrigin := r.Header.Get("Origin") - hasOriginMatch := allowOriginRegex.MatchString(requestOrigin) - hasMethodMatch := r.Method == "GET" || r.Method == "HEAD" || r.Method == "OPTIONS" - - if hasOriginMatch && hasMethodMatch { - w.Header().Set("Access-Control-Allow-Origin", requestOrigin) - // why: `Vary: Origin` is needed because allowable origin is variable - // https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#the_http_response_headers - w.Header().Set("Vary", "Origin") - } - - next.ServeHTTP(w, r) - }) -} - func allowedProxy(proxy http.Handler, dependencyProxyInjector *dependencyproxy.Injector, u *upstream) http.Handler { if u.CircuitBreakerConfig.Enabled { roundTripperCircuitBreaker := circuitbreaker.NewRoundTripper(u.RoundTripper, &u.CircuitBreakerConfig, u.rdb) diff --git a/workhorse/internal/upstream/routes_test.go b/workhorse/internal/upstream/routes_test.go index 695fee2df42f8924dd86f0cf6c81f073ecd35305..9a442759c65a07db87b6e5f9e7caf86592516653 100644 --- a/workhorse/internal/upstream/routes_test.go +++ b/workhorse/internal/upstream/routes_test.go @@ -20,24 +20,6 @@ import ( configRedis "gitlab.com/gitlab-org/gitlab/workhorse/internal/redis" ) -func TestStaticCORS(t *testing.T) { - path := "/assets/static.txt" - content := "local geo asset" - testhelper.SetupStaticFileHelper(t, path, content, testDocumentRoot) - - testCases := []testCaseRequest{ - {"With no origin, does not set cors headers", "GET", "/assets/static.txt", map[string]string{}, map[string]string{"Access-Control-Allow-Origin": ""}}, - {"With unknown origin, does not set cors headers", "GET", "/assets/static.txt", map[string]string{"Origin": "https://example.com"}, map[string]string{"Access-Control-Allow-Origin": ""}}, - {"With known origin, sets cors headers", "GET", "/assets/static.txt", map[string]string{"Origin": "https://123.cdn.web-ide.gitlab-static.net"}, map[string]string{"Access-Control-Allow-Origin": "https://123.cdn.web-ide.gitlab-static.net", "Vary": "Origin"}}, - {"With known origin HEAD, sets cors headers", "HEAD", "/assets/static.txt", map[string]string{"Origin": "https://123.cdn.web-ide.gitlab-static.net"}, map[string]string{"Access-Control-Allow-Origin": "https://123.cdn.web-ide.gitlab-static.net", "Vary": "Origin"}}, - {"With known origin OPTIONS, sets cors headers", "OPTIONS", "/assets/static.txt", map[string]string{"Origin": "https://123.cdn.web-ide.gitlab-static.net"}, map[string]string{"Access-Control-Allow-Origin": "https://123.cdn.web-ide.gitlab-static.net", "Vary": "Origin"}}, - {"With known origin POST, does not set cors headers", "POST", "/assets/static.txt", map[string]string{"Origin": "https://123.cdn.web-ide.gitlab-static.net"}, map[string]string{"Access-Control-Allow-Origin": ""}}, - {"With evil origin, does not set cors headers", "GET", "/assets/static.txt", map[string]string{"Origin": "https://123.cdn.web-ide.gitlab-static.net.evil.com"}, map[string]string{"Access-Control-Allow-Origin": ""}}, - } - - runTestCasesWithGeoProxyEnabledRequest(t, testCases) -} - func TestAdminGeoPathsWithGeoProxy(t *testing.T) { testCases := []testCase{ {"Regular admin/geo", "/admin/geo", "Geo primary received request to path /admin/geo"}, diff --git a/workhorse/internal/upstream/upstream_test.go b/workhorse/internal/upstream/upstream_test.go index e5751ed94f872195218d7bbfb34c57a3a284cf32..ba67f5051c80946d63de5ab604b8329b5c4cd136 100644 --- a/workhorse/internal/upstream/upstream_test.go +++ b/workhorse/internal/upstream/upstream_test.go @@ -39,14 +39,6 @@ type testCasePost struct { body io.Reader } -type testCaseRequest struct { - desc string - method string - path string - headers map[string]string - expectedHeaders map[string]string -} - func TestMain(m *testing.M) { // Secret should be configured before any Geo API poll happens to prevent // race conditions where the first API call happens without a secret path @@ -378,29 +370,6 @@ func runTestCasesPost(t *testing.T, ws *httptest.Server, testCases []testCasePos } } -func runTestCasesRequest(t *testing.T, ws *httptest.Server, testCases []testCaseRequest) { - t.Helper() - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - client := http.Client{} - request, err := http.NewRequest(tc.method, ws.URL+tc.path, nil) - require.NoError(t, err) - for key, value := range tc.headers { - request.Header.Set(key, value) - } - - resp, err := client.Do(request) - require.NoError(t, err) - defer resp.Body.Close() - - require.Equal(t, 200, resp.StatusCode, "response code") - for key, value := range tc.expectedHeaders { - require.Equal(t, resp.Header.Get(key), value, fmt.Sprint("response header ", key)) - } - }) - } -} - func runTestCasesWithGeoProxyEnabled(t *testing.T, testCases []testCase) { remoteServer := startRemoteServer(t) @@ -423,17 +392,6 @@ func runTestCasesWithGeoProxyEnabledPost(t *testing.T, testCases []testCasePost) runTestCasesPost(t, ws, testCases) } -func runTestCasesWithGeoProxyEnabledRequest(t *testing.T, testCases []testCaseRequest) { - remoteServer := startRemoteServer(t) - - geoProxyEndpointResponseBody := fmt.Sprintf(`{"geo_enabled":true,"geo_proxy_url":"%v"}`, remoteServer.URL) - railsServer := startRailsServer(t, &geoProxyEndpointResponseBody) - - ws, _ := startWorkhorseServer(t, railsServer.URL, true) - - runTestCasesRequest(t, ws, testCases) -} - func newUpstreamConfig(authBackend string) *config.Config { return &config.Config{ Version: "123",