Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade match check in WebSocketHandlerMapping does not work if handler mapped to "/*" #34503

Closed
cn19 opened this issue Feb 27, 2025 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@cn19
Copy link

cn19 commented Feb 27, 2025

I register a websocket with path '/*' and also called webSocketHandlerMapping.setWebSocketUpgradeMatch(true) in my program. When I send a common http request without Upgrade header, WebSocketHandlerMapping aslo returned defaultHandler. I think webSocketUpgradeMatch should take effect for all handler include defaultHandler and getHandler() should return null when webSocketUpgradeMatch=true and no Upgrade header present.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 27, 2025
@rstoyanchev
Copy link
Contributor

The defaultHandler is by design what we call when there is no match. It's not clear why you have it set, if you don't want it to be used?

Also, the webSocketUpgradeMatch flag is in effect only for handlers of type WebSocketHttpRequestHandler, i.e. where we know the intent is to handle the request as a WebSocket upgrade. So this wouldn't apply to the default handler unless it is also a WebSocketHttpRequestHandler, which seems unlikely.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Mar 6, 2025
@cn19
Copy link
Author

cn19 commented Mar 7, 2025

I believe that logically, WebSocketHandlerMapping should only return handlers that meet the conditions, and webSocketUpgradeMatch, as a property of WebSocketHandlerMapping, should apply to all returned handlers.

In my program, I added a WebSocketHandler with the path set to "/*" and webSocketUpgradeMatch set to true. After starting the application, Spring Boot added the defaultHandler for WebSocketHandlerMapping. However, when handling a regular HTTP request (without the Upgrade header), the defaultHandler was returned. I think WebSocketHandlerMapping should return null in this case, allowing the process to proceed to the next WebSocketHandlerMapping or return a 404 status.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 7, 2025
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 11, 2025

defaultHandler is meant to use as a fallback option, to send an alternative response when the request doesn't match to the expected URL paths. If you mean to start a WebSocket connection regardless of the URL path, then just map the handler to "/*" rather than relying on a default handler, especially if that's the same handler.

In any case, there is no easy way to do what you're requesting since the overriding getDefaultHandler does not provide access to the request.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2025
@rstoyanchev rstoyanchev added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Mar 11, 2025
@cn19
Copy link
Author

cn19 commented Mar 11, 2025

I am indeed registering the handler on the "/" path, and then Spring treats it as the defaultHandler. In other words, Spring considers "/" as the defaultHandler and does not perform any other condition matching.

org.springframework.web.servlet.handler.AbstractUrlHandlerMapping#registerHandler(java.lang.String, java.lang.Object)

		Object mappedHandler = this.handlerMap.get(urlPath);
		if (mappedHandler != null) {
			if (mappedHandler != resolvedHandler) {
				throw new IllegalStateException(
						"Cannot map " + getHandlerDescription(handler) + " to URL path [" + urlPath +
						"]: There is already " + getHandlerDescription(mappedHandler) + " mapped.");
			}
		}
		else {
			if (urlPath.equals("/")) {
				if (logger.isTraceEnabled()) {
					logger.trace("Root mapping to " + getHandlerDescription(handler));
				}
				setRootHandler(resolvedHandler);
			}
			else if (urlPath.equals("/*")) {
				if (logger.isTraceEnabled()) {
					logger.trace("Default mapping to " + getHandlerDescription(handler));
				}
				setDefaultHandler(resolvedHandler);
			}
			else {
				this.handlerMap.put(urlPath, resolvedHandler);
				if (getPatternParser() != null) {
					this.pathPatternHandlerMap.put(getPatternParser().parse(urlPath), resolvedHandler);
				}
				if (logger.isTraceEnabled()) {
					logger.trace("Mapped [" + urlPath + "] onto " + getHandlerDescription(handler));
				}
			}
		}

@rstoyanchev
Copy link
Contributor

That's a good point. The treatment of "/*" as a default handler ends up preventing WebSocketHandlerMapping from doing what it's trying to do.

@rstoyanchev rstoyanchev reopened this Mar 11, 2025
@rstoyanchev rstoyanchev added type: bug A general bug and removed status: declined A suggestion or change that we don't feel we should currently apply labels Mar 11, 2025
@rstoyanchev rstoyanchev self-assigned this Mar 11, 2025
@rstoyanchev rstoyanchev changed the title WebSocketHandlerMapping return defaultHandler when webSocketUpgradeMatch is true. Upgrade match check in WebSocketHandlerMapping does not work if handler mapped to "/*" Mar 11, 2025
@rstoyanchev rstoyanchev added this to the 6.2.4 milestone Mar 11, 2025
@rstoyanchev
Copy link
Contributor

I am changing the target to 7.0 since the change has some potential to affect existing applications. In the mean time, as a workaround, you could use a different pattern that achieves the same outcome, e.g. "/{path}", "/?*", etc.

@rstoyanchev rstoyanchev modified the milestones: 6.2.4, 7.0.0-M3 Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants