Accept only 4xx and 5xx statuses in workspace transfer errors

Related to #1252

The workspaces auth middleware renders a transfer error with the status code it carries (auth.go#L203). A transfer error reaches the workspace host in the __gitlab_workspace_transfer_error query parameter and then in the gitlab-workspace-error cookie, and TransferError.IsValid (auth_extra.go#L16-22) checked only that its URL names the request's host. The handler therefore wrote whatever status the transfer error held, a success or a redirect included, and WriteHeader panicked on a missing status or any other code outside 100-999, which closed the connection.

As defense in depth, http_status_code now carries a (buf.validate.field).uint32 rule that accepts only 400 to 599, and original_url a (buf.validate.field).string.uri rule. kas itself only writes 400, 404 and 500 into a transfer error. h.marshaler validates a transfer error when it reads one, so one that fails a rule takes the path a host mismatch already takes: from the query parameter it redirects to OAuth without setting the error cookie, and from the cookie it is answered with 400 Something went wrong and the cookie is deleted.

Notes for the review:

  • auth.pb.go, build/protobreaking/wire.txtpb and the BUILD.bazel entry come from make regenerate-proto (Bazel 9.3.1).
  • validation_test.go covers both rules, and two middleware tests send the statuses 0, 200, 302, 99 and 1000 through the query parameter and through the cookie.
  • One effect of uri: an original URL whose query holds a character RFC 3986 does not allow, such as |, which browsers send unescaped, now fails validation, so an OAuth error for it is answered with a 500.
Tests on master and on this branch

On master 7b70fad26 with only the test changes of this MR, the FAIL lines and the panic values:

$ go test -count=1 -run '^(TestValidation_Valid|TestValidation_Invalid|TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError|TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth)$' ./internal/module/workspaces/server/
--- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError (0.01s)
    --- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError/0 (0.00s)
        Panic value: invalid WriteHeader code 0
    --- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError/200 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError/302 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError/99 (0.00s)
        Panic value: invalid WriteHeader code 99
    --- FAIL: TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError/1000 (0.00s)
        Panic value: invalid WriteHeader code 1000
--- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth/0 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth/200 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth/302 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth/99 (0.00s)
    --- FAIL: TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth/1000 (0.00s)
--- FAIL: TestValidation_Invalid (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_http_status_code:_must_be_greater_than_or_equal_to_400_and_less_than_or_equal_to_599 (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_http_status_code:_must_be_greater_than_or_equal_to_400_and_less_than_or_equal_to_599#01 (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_http_status_code:_must_be_greater_than_or_equal_to_400_and_less_than_or_equal_to_599#02 (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_http_status_code:_must_be_greater_than_or_equal_to_400_and_less_than_or_equal_to_599#03 (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_http_status_code:_must_be_greater_than_or_equal_to_400_and_less_than_or_equal_to_599#04 (0.00s)
    --- FAIL: TestValidation_Invalid/validation_error:_original_url:_must_be_a_valid_URI (0.00s)
FAIL
FAIL    gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces/server    0.025s
FAIL

On this branch:

$ go test -race -count=1 ./internal/module/workspaces/...
?       gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces   [no test files]
ok      gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces/agentw    2.948s
?       gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces/rpc       [no test files]
ok      gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces/server    1.321s

$ go test -race -count=50 -run '^(TestValidation_Valid|TestValidation_Invalid|TestAuthHandler_Middleware_HandleErrorCookie_StatusNotAnError_ReturnsError|TestAuthHandler_Middleware_HandleTransferError_StatusNotAnError_RedirectsToOAuth)$' ./internal/module/workspaces/server/
ok      gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/module/workspaces/server    1.661s

$ go build ./... && go vet ./internal/module/workspaces/...
$ golangci-lint fmt --diff ./internal/module/workspaces/...
$ golangci-lint run ./internal/module/workspaces/...
$ ./build/deadcode.sh v0.52.0
$ go run gitlab.com/gitlab-org/labkit/v2/cmd/validate-log-fields@v2.65.1 .
No deprecated logging field usages found.

golangci-lint is the custom build from .custom-gcl.yml at v2.15.0; Go 1.27.3, linux/amd64. make regenerate-proto ran under Bazel 9.3.1, and a second run on this branch changes nothing.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading