diff --git a/backend/internal/application/application_test.go b/backend/internal/application/application_test.go index 170e33c..2ce419b 100644 --- a/backend/internal/application/application_test.go +++ b/backend/internal/application/application_test.go @@ -508,7 +508,7 @@ func TestApplicationComposesPasswordLoginAndLogoutHandlers(t *testing.T) { logoutResponse := httptest.NewRecorder() app.Handler().ServeHTTP(logoutResponse, httptest.NewRequest(http.MethodPost, "http://app.test/api/auth/logout", nil)) - if logoutResponse.Code != http.StatusTemporaryRedirect || logoutResponse.Header().Get("Location") != "http://app.test/auth/login?loggedOut=1" { + if logoutResponse.Code != http.StatusTemporaryRedirect || logoutResponse.Header().Get("Location") != "https://public.example.test/auth/login?loggedOut=1" { t.Fatalf("logout response = %d location=%q", logoutResponse.Code, logoutResponse.Header().Get("Location")) } if got := len(logoutResponse.Result().Cookies()); got != identity.CookieMaxChunks { diff --git a/backend/internal/assets/oss.go b/backend/internal/assets/oss.go index 5321d8c..cb50f08 100644 --- a/backend/internal/assets/oss.go +++ b/backend/internal/assets/oss.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "log" "net/url" "path" "strings" @@ -77,10 +78,12 @@ func (s *OSS) Put(ctx context.Context, key string, body io.Reader, size int64, c } err := s.client.Put(ctx, OSSPutRequest{Endpoint: s.config.Endpoint, Bucket: s.config.Bucket, Key: key, Body: body, Size: size, ContentType: contentType}) if err != nil { + logOSSOperationFailure("put", err) return StoredObject{}, mapOSSError(err) } if s.config.PublicRead { if err = s.client.SetACL(ctx, OSSACLRequest{Endpoint: s.config.Endpoint, Bucket: s.config.Bucket, Key: key, ACL: OSSACLPublicRead}); err != nil { + logOSSOperationFailure("set_acl", err) return StoredObject{}, mapOSSError(err) } } @@ -92,6 +95,9 @@ func (s *OSS) Read(ctx context.Context, key string) (Blob, error) { } blob, err := s.client.Get(ctx, OSSObjectRequest{Endpoint: s.config.Endpoint, Bucket: s.config.Bucket, Key: key}) if err != nil { + if !isMissingOSS(err) { + logOSSOperationFailure("get", err) + } return Blob{}, mapOSSError(err) } return blob, nil @@ -104,8 +110,50 @@ func (s *OSS) Delete(ctx context.Context, key string) error { if isMissingOSS(err) { return nil } + if err != nil { + logOSSOperationFailure("delete", err) + } return mapOSSError(err) } + +type ossOperationDiagnostic struct { + Operation string + Status int + Code string + ErrorClass string +} + +func diagnoseOSSOperation(operation string, err error) ossOperationDiagnostic { + diagnostic := ossOperationDiagnostic{Operation: operation, ErrorClass: "client"} + var ossErr *OSSError + if errors.As(err, &ossErr) { + diagnostic.Status = ossErr.Status + diagnostic.Code = ossErr.Code + if ossErr.Status > 0 || ossErr.Code != "" { + diagnostic.ErrorClass = "service" + } else { + diagnostic.ErrorClass = "transport" + } + return diagnostic + } + if errors.Is(err, context.Canceled) { + diagnostic.ErrorClass = "canceled" + } else if errors.Is(err, context.DeadlineExceeded) { + diagnostic.ErrorClass = "timeout" + } + return diagnostic +} + +func logOSSOperationFailure(operation string, err error) { + diagnostic := diagnoseOSSOperation(operation, err) + log.Printf( + "zhinian-api OSS operation failed operation=%s status=%d code=%q errorClass=%s", + diagnostic.Operation, + diagnostic.Status, + diagnostic.Code, + diagnostic.ErrorClass, + ) +} func mapOSSError(err error) error { if err == nil { return nil diff --git a/backend/internal/assets/oss_http.go b/backend/internal/assets/oss_http.go index 294d973..fe2fa69 100644 --- a/backend/internal/assets/oss_http.go +++ b/backend/internal/assets/oss_http.go @@ -4,16 +4,20 @@ import ( "context" "crypto/hmac" "crypto/sha1" + "crypto/tls" "encoding/base64" "encoding/xml" "errors" "fmt" "io" + "log" + "net" "net/http" "net/url" "path" "sort" "strings" + "syscall" "time" ) @@ -144,12 +148,80 @@ func (c *OSSHTTPClient) do(req *http.Request) (*http.Response, error) { if err == nil { return response, nil } + diagnostic := diagnoseOSSTransport(req, err) + log.Printf( + "zhinian-api OSS transport request failed method=%s endpointClass=%s errorClass=%s", + diagnostic.Method, + diagnostic.EndpointClass, + diagnostic.ErrorClass, + ) if ctxErr := req.Context().Err(); ctxErr != nil { return nil, ctxErr } return nil, &OSSError{Err: errors.New("OSS transport request failed")} } +type ossTransportDiagnostic struct { + Method string + EndpointClass string + ErrorClass string +} + +func diagnoseOSSTransport(req *http.Request, err error) ossTransportDiagnostic { + diagnostic := ossTransportDiagnostic{ErrorClass: classifyOSSTransportError(err)} + if req == nil { + diagnostic.Method = "unknown" + diagnostic.EndpointClass = "unknown" + return diagnostic + } + diagnostic.Method = req.Method + host := strings.ToLower(req.URL.Hostname()) + switch { + case strings.Contains(host, "-internal.") || strings.HasSuffix(host, "-internal"): + diagnostic.EndpointClass = "internal" + case strings.HasSuffix(host, ".aliyuncs.com"): + diagnostic.EndpointClass = "public" + default: + diagnostic.EndpointClass = "custom" + } + return diagnostic +} + +func classifyOSSTransportError(err error) string { + if errors.Is(err, context.Canceled) { + return "canceled" + } + if errors.Is(err, context.DeadlineExceeded) { + return "timeout" + } + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + if dnsErr.IsTimeout { + return "dns_timeout" + } + return "dns" + } + var tlsErr *tls.CertificateVerificationError + if errors.As(err, &tlsErr) { + return "tls_certificate" + } + switch { + case errors.Is(err, syscall.ECONNRESET): + return "connection_reset" + case errors.Is(err, syscall.ECONNREFUSED): + return "connection_refused" + case errors.Is(err, syscall.ENETUNREACH): + return "network_unreachable" + case errors.Is(err, syscall.EHOSTUNREACH): + return "host_unreachable" + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return "timeout" + } + return "transport" +} + func ossObjectURL(endpoint, bucket, key, subresource string) (*url.URL, error) { endpoint = strings.TrimSpace(endpoint) if !strings.Contains(endpoint, "://") { diff --git a/backend/internal/assets/oss_http_test.go b/backend/internal/assets/oss_http_test.go index 329f32d..d521f0f 100644 --- a/backend/internal/assets/oss_http_test.go +++ b/backend/internal/assets/oss_http_test.go @@ -4,9 +4,12 @@ import ( "bytes" "context" "errors" + "fmt" "io" "net/http" + "net/url" "strings" + "syscall" "testing" "time" ) @@ -148,6 +151,28 @@ func TestOSSHTTPClientPropagatesContextCancellation(t *testing.T) { } } +func TestOSSTransportDiagnosticIsClassifiedAndDoesNotExposeRequestData(t *testing.T) { + request, err := http.NewRequest(http.MethodPut, "https://private-bucket.oss-cn-guangzhou-internal.aliyuncs.com/uploads/private-file.png?credential=secret", nil) + if err != nil { + t.Fatal(err) + } + request.Header.Set("Authorization", "OSS access-key:private-signature") + diagnostic := diagnoseOSSTransport(request, &url.Error{ + Op: "Put", + URL: request.URL.String(), + Err: syscall.ECONNRESET, + }) + if diagnostic.Method != http.MethodPut || diagnostic.EndpointClass != "internal" || diagnostic.ErrorClass != "connection_reset" { + t.Fatalf("diagnostic = %#v", diagnostic) + } + serialized := fmt.Sprintf("%+v", diagnostic) + for _, secret := range []string{"private-bucket", "private-file", "credential", "access-key", "private-signature"} { + if strings.Contains(serialized, secret) { + t.Fatalf("diagnostic leaks %q: %s", secret, serialized) + } + } +} + func TestNewOSSHTTPClientRejectsMissingDependencies(t *testing.T) { for _, test := range []struct { id, secret string diff --git a/backend/internal/assets/oss_test.go b/backend/internal/assets/oss_test.go index 8445c43..d10898a 100644 --- a/backend/internal/assets/oss_test.go +++ b/backend/internal/assets/oss_test.go @@ -4,7 +4,10 @@ import ( "bytes" "context" "errors" + "fmt" "io" + "log" + "strings" "testing" ) @@ -93,3 +96,61 @@ func TestOSSRejectsUnsafeKeysAndIncompleteConfiguration(t *testing.T) { } } } + +func TestOSSOperationDiagnosticIncludesStageAndSafeServiceError(t *testing.T) { + diagnostic := diagnoseOSSOperation("set_acl", &OSSError{ + Status: 403, + Code: "AccessDenied", + Err: errors.New("private-key private-object secret response"), + }) + if diagnostic.Operation != "set_acl" || diagnostic.Status != 403 || diagnostic.Code != "AccessDenied" || diagnostic.ErrorClass != "service" { + t.Fatalf("diagnostic = %#v", diagnostic) + } + serialized := fmt.Sprintf("%+v", diagnostic) + for _, secret := range []string{"private-key", "private-object", "secret response"} { + if strings.Contains(serialized, secret) { + t.Fatalf("diagnostic leaks %q: %s", secret, serialized) + } + } +} + +func TestOSSPutFailureLogsStageWithoutSensitiveDetails(t *testing.T) { + var output bytes.Buffer + previousOutput, previousFlags := log.Writer(), log.Flags() + log.SetOutput(&output) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(previousOutput) + log.SetFlags(previousFlags) + }) + + client := &ossClientStub{errorToReturn: &OSSError{ + Status: 403, + Code: "AccessDenied", + Err: errors.New("access-key private-object secret response"), + }} + store, err := NewOSS(OSSConfig{ + Endpoint: "https://oss-cn-guangzhou.aliyuncs.com", + Bucket: "private-bucket", + PublicBaseURL: "https://private-bucket.oss-cn-guangzhou.aliyuncs.com", + PublicRead: true, + }, client) + if err != nil { + t.Fatal(err) + } + if _, err = store.Put(context.Background(), "uploads/private-file.png", bytes.NewReader([]byte("png")), 3, "image/png"); err == nil { + t.Fatal("Put error = nil") + } + + got := output.String() + for _, expected := range []string{"operation=put", "status=403", `code="AccessDenied"`, "errorClass=service"} { + if !strings.Contains(got, expected) { + t.Fatalf("log %q does not contain %q", got, expected) + } + } + for _, secret := range []string{"access-key", "private-object", "secret response", "private-bucket", "private-file"} { + if strings.Contains(got, secret) { + t.Fatalf("log leaks %q: %s", secret, got) + } + } +} diff --git a/backend/internal/httpapi/auth_logout.go b/backend/internal/httpapi/auth_logout.go index 829cbde..2dd10ac 100644 --- a/backend/internal/httpapi/auth_logout.go +++ b/backend/internal/httpapi/auth_logout.go @@ -38,7 +38,11 @@ func (handler *authLogoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Reque for _, write := range identity.ClearSessionCookies(secure) { http.SetCookie(w, transportCookie(write)) } - w.Header().Set("Location", logoutLocation(requestURL)) + redirectBaseURL := strings.TrimSpace(handler.config.PublicBaseURL) + if redirectBaseURL == "" { + redirectBaseURL = requestURL + } + w.Header().Set("Location", logoutLocation(redirectBaseURL)) w.WriteHeader(http.StatusTemporaryRedirect) } diff --git a/backend/internal/httpapi/auth_logout_test.go b/backend/internal/httpapi/auth_logout_test.go index df479ef..29c0be7 100644 --- a/backend/internal/httpapi/auth_logout_test.go +++ b/backend/internal/httpapi/auth_logout_test.go @@ -64,6 +64,20 @@ func TestAuthLogoutUsesRequestOriginAndExplicitCookieSecurity(t *testing.T) { } } +func TestAuthLogoutUsesPublicBaseURLBehindDevelopmentProxy(t *testing.T) { + handler := httpapi.NewAuthLogoutHandler(httpapi.LogoutConfig{PublicBaseURL: "http://127.0.0.1:3000"}) + request := httptest.NewRequest(http.MethodPost, "http://127.0.0.1:8080/api/auth/logout", nil) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + + if response.Code != http.StatusTemporaryRedirect { + t.Fatalf("status = %d, want %d", response.Code, http.StatusTemporaryRedirect) + } + if got := response.Header().Get("Location"); got != "http://127.0.0.1:3000/auth/login?loggedOut=1" { + t.Fatalf("Location = %q", got) + } +} + func TestAuthLogoutMethodAndPathSemantics(t *testing.T) { handler := httpapi.NewAuthLogoutHandler(httpapi.LogoutConfig{}) tests := []struct {