Skip to content

simplify/robustify WebSocketHttpHeaders.java getSecWebSocketProtocol() ? #37282

Description

@jonenst

Hi, while browsing the code, I was suprised by the logic in https://github.com/spring-projects/spring-framework/blob/main/spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java#L162

	public List<String> getSecWebSocketProtocol() {
		List<String> values = get(SEC_WEBSOCKET_PROTOCOL);
		if (CollectionUtils.isEmpty(values)) {
			return Collections.emptyList();
		}
		else if (values.size() == 1) {
			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);
		}
		else {
			return values;
		}
	}

it would make the code "fail" when request comes with
protocol: foo
protocol: bar, baz

return ["foo", "bar, baz"] instead of ["foo", "bar", "baz"]

Also unify the quote handling (even though the rfc explicitly forbids quotes for sec-websocket-protocol, but at least this would be uniform. but even better would maybe be to reject or just ignore quotes ?) :
protocol: "foo", "bar" returns ["foo", "bar"]
vs
protocol: "foo"
protocol: "bar"
returns [""foo"", ""bar""]

It looks like it's the only place in the whole codebase with this extra check for size() == 1

$ git grep -C 3 "size() == 1" | grep -i AsList
spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java-			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);

seems like it was iterated on in 55dae61#diff-e2d6218e6585f8b7e32682f6f8f7aed22dbd76d862ec92e86e0902243889556aL462-R471 and

It's obviously an edge case, but unless I missed somthing, it becomes simpler/more robust to just change it to

	public List<String> getSecWebSocketProtocol() {
			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);
	}

like in all methods in the parent class HttpHeaders.java (e.g. https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/HttpHeaders.java#L706

	public List<String> getAccessControlAllowHeaders() {
		return getValuesAsList(ACCESS_CONTROL_ALLOW_HEADERS);
	}

I just saw this in passing, just though it'd mention it (obviously I'm leaving all the hard work of knowing what to do to you...). Feel free to close if appropriate

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions