Address code review feedback

- Fix unsafe type assertions in GetObjectHandler and HeadObjectHandler
  when getting identity from context (properly handle type assertion failure)
- Extract getConditionContextValue helper to eliminate duplicated logic
  between EvaluateConditions and EvaluateConditionsLegacy
- Ensure consistent handling of missing condition keys (always return
  empty slice)

weed/s3api/policy_engine/conditions.go

@@ -709,6 +709,27 @@ func GetConditionEvaluator(operator string) (ConditionEvaluator, error) {
 // ExistingObjectTagPrefix is the prefix for S3 policy condition keys
 const ExistingObjectTagPrefix = "s3:ExistingObjectTag/"
 
+// getConditionContextValue resolves the value(s) for a condition key.
+// For s3:ExistingObjectTag/<key> conditions, it looks up the tag in objectEntry.
+// For other condition keys, it looks up the value in contextValues.
+func getConditionContextValue(key string, contextValues map[string][]string, objectEntry map[string][]byte) []string {
+	if strings.HasPrefix(key, ExistingObjectTagPrefix) {
+		tagKey := key[len(ExistingObjectTagPrefix):]
+		metadataKey := s3_constants.AmzObjectTaggingPrefix + tagKey
+		if objectEntry != nil {
+			if tagValue, exists := objectEntry[metadataKey]; exists {
+				return []string{string(tagValue)}
+			}
+		}
+		return []string{}
+	}
+
+	if vals, exists := contextValues[key]; exists {
+		return vals
+	}
+	return []string{}
+}
+
 // EvaluateConditions evaluates all conditions in a policy statement
 // objectEntry is the object's metadata from entry.Extended (can be nil)
 func EvaluateConditions(conditions PolicyConditions, contextValues map[string][]string, objectEntry map[string][]byte) bool {
@@ -724,29 +745,7 @@ func EvaluateConditions(conditions PolicyConditions, contextValues map[string][]
 		}
 
 		for key, value := range conditionMap {
-			var contextVals []string
-
-			// Handle s3:ExistingObjectTag/<tag-key> condition keys
-			// These refer to tags that already exist on the object
-			if strings.HasPrefix(key, ExistingObjectTagPrefix) {
-				// Extract tag value from entry.Extended using the tag prefix
-				tagKey := key[len(ExistingObjectTagPrefix):]
-				metadataKey := s3_constants.AmzObjectTaggingPrefix + tagKey
-				if objectEntry != nil {
-					if tagValue, exists := objectEntry[metadataKey]; exists {
-						contextVals = []string{string(tagValue)}
-					}
-				}
-				// If tag doesn't exist, contextVals remains empty
-			} else {
-				// Regular condition key lookup
-				var exists bool
-				contextVals, exists = contextValues[key]
-				if !exists {
-					contextVals = []string{}
-				}
-			}
-
+			contextVals := getConditionContextValue(key, contextValues, objectEntry)
 			if !conditionEvaluator.Evaluate(value.Strings(), contextVals) {
 				return false // If any condition fails, the whole condition block fails
 			}
@@ -777,25 +776,7 @@ func EvaluateConditionsLegacy(conditions map[string]interface{}, contextValues m
 		}
 
 		for key, value := range conditionMapTyped {
-			var contextVals []string
-
-			// Handle s3:ExistingObjectTag/<tag-key> condition keys
-			if strings.HasPrefix(key, ExistingObjectTagPrefix) {
-				tagKey := key[len(ExistingObjectTagPrefix):]
-				metadataKey := s3_constants.AmzObjectTaggingPrefix + tagKey
-				if objectEntry != nil {
-					if tagValue, exists := objectEntry[metadataKey]; exists {
-						contextVals = []string{string(tagValue)}
-					}
-				}
-			} else {
-				var exists bool
-				contextVals, exists = contextValues[key]
-				if !exists {
-					contextVals = []string{}
-				}
-			}
-
+			contextVals := getConditionContextValue(key, contextValues, objectEntry)
 			if !conditionEvaluator.Evaluate(value, contextVals) {
 				return false // If any condition fails, the whole condition block fails
 			}

weed/s3api/s3api_object_handlers.go

@@ -636,7 +636,17 @@ func (s3a *S3ApiServer) GetObjectHandler(w http.ResponseWriter, r *http.Request)
 
 	// Re-check bucket policy with object entry for tag-based conditions (e.g., s3:ExistingObjectTag)
 	if objectEntryForSSE != nil {
-		identity, _ := s3_constants.GetIdentityFromContext(r).(*Identity)
+		identityRaw := s3_constants.GetIdentityFromContext(r)
+		var identity *Identity
+		if identityRaw != nil {
+			var ok bool
+			identity, ok = identityRaw.(*Identity)
+			if !ok {
+				glog.Errorf("GetObjectHandler: unexpected identity type in context for %s/%s", bucket, object)
+				s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
+				return
+			}
+		}
 		principal := buildPrincipalARN(identity)
 		if errCode, _ := s3a.checkPolicyWithEntry(r, bucket, object, string(s3_constants.ACTION_READ), principal, objectEntryForSSE.Extended); errCode != s3err.ErrNone {
 			s3err.WriteErrorResponse(w, r, errCode)
@@ -2198,7 +2208,17 @@ func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request
 	}
 
 	// Re-check bucket policy with object entry for tag-based conditions (e.g., s3:ExistingObjectTag)
-	identity, _ := s3_constants.GetIdentityFromContext(r).(*Identity)
+	identityRaw := s3_constants.GetIdentityFromContext(r)
+	var identity *Identity
+	if identityRaw != nil {
+		var ok bool
+		identity, ok = identityRaw.(*Identity)
+		if !ok {
+			glog.Errorf("HeadObjectHandler: unexpected identity type in context for %s/%s", bucket, object)
+			s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
+			return
+		}
+	}
 	principal := buildPrincipalARN(identity)
 	if errCode, _ := s3a.checkPolicyWithEntry(r, bucket, object, string(s3_constants.ACTION_READ), principal, objectEntryForSSE.Extended); errCode != s3err.ErrNone {
 		s3err.WriteErrorResponse(w, r, errCode)