Anchor and IFrame: validate URL schemes against the application's own configuration, re-checking on attach - #25185
Conversation
UrlUtil.isSafeUrl relied on VaadinService.getCurrent(), which is only defined while a session is locked. A component tree that is created in a background thread and only attached to the UI later on was therefore validated against the framework default instead of the application's own safeUrlSchemes configuration. In the versions where the default is a wildcard, this silently skipped validation altogether. Anchor and IFrame now register a one-shot attach listener when a URL is set while the component isn't attached. Attaching is the point where the right configuration is guaranteed to be known, and also the point where the value would first be sent to the browser. The value is cleared before the exception is thrown, so an unsafe URL isn't sent to the client even if the application catches the exception. Methods that replace the value without validating it cancel the pending check. The check at set time now resolves the configuration from the component's own UI when it is attached, and Page#open uses the UI it belongs to, so neither depends on the thread local when a better source is available.
The Sonar quality gate reported 67% coverage on new code because the validateUrlOnAttach logic in UrlUtil was only exercised through the Anchor and IFrame tests in flow-html-components, which don't contribute to the coverage report of flow-server. UrlUtilTest now tests the helper and the component-aware isSafeUrl overload directly, and IFrameTest covers attaching an already validated src and the deprecated stream resource setter. Take the URL to validate as a value instead of a supplier, since every method that changes the value either schedules a new check or cancels the pending one. This also removes the redundant isDebugEnabled guard around a parameterized log call and a src null check in IFrame that cannot be reached, as setting a null src fails before the check is scheduled.
Anchor and IFrame had the same structure for validating a URL and for scheduling and canceling the check that is repeated on attach, and each of them repeated the strings used for the error message. UrlUtil now offers validateUrl, which checks the URL against the best available configuration, throws with a consistent message and schedules the check to be repeated on attach when needed, and cancelUrlValidation for the methods that replace the value without validating it. The pending registration is stored as component data, so the components no longer need a field and the related null handling. Removing a component event listener is now idempotent regardless of whether it happens through the returned registration or through ComponentEvent#unregisterListener, as both now use the same registration instance. This removes the need for tracking elsewhere whether a self-removing listener has already been fired. Deprecate the URL check that relies on the current thread having a locked session, and log at warning level when it falls back to the default schemes, since nothing will re-check the URL in that case. The component-aware checks stay silent when a check on attach is scheduled. The tests now build their UI from MockUI and the shared mock service and session instead of hand-written mocks.
totally-not-ai
Bot
changed the title
Legioth
changed the title
| */ | ||
| public static void validateUrl(Component component, String type, String url, | ||
| String unsafeMethod, SerializableRunnable urlClearer) { | ||
| validateUrl(component, type, url, unsafeMethod); |
There was a problem hiding this comment.
I guess this means that there's a possibility of falling back to the defaults if the component is not attached and there's no current service. That would lead to redundant errors in case the application's own configuration is more lenient than the default configuration.
There was a problem hiding this comment.
Right, and that would have been a real regression risk. The fallback to the defaults is gone: a component that is not attached is no longer checked at all, and the check happens when it is attached, which is also the point where the value would first be sent to the browser. So a scheme the application has explicitly allowed is never rejected.
| // the application wasn't available, so it has to be repeated once the | ||
| // component knows which application it belongs to | ||
| cancelUrlValidation(component, type); | ||
| if (component.getUI().isEmpty()) { |
There was a problem hiding this comment.
This is redundant if validation succeeded based on the current service. There is a very hypothetical case of configuring a detached component under one current service and then attaching the component to the UI of another service.
I wonder if the appropriate approach would be to never check the current service in any other variant than the old deprecated one?
There was a problem hiding this comment.
Went with that: the current service is now only consulted by the deprecated isSafeUrl(String). The component variants read the configuration solely from the component own UI, so there is nothing redundant left - either the component is attached and the check happens right away, or it is not and the check is deferred to attach. That also removes the wrong-application case you describe. The visible trade-off is that setting an unsafe URL on a detached component now throws when it is attached rather than immediately; the setter Javadoc says so, and the constructors point at the setter.
| // Unregister before validating so that the same value isn't | ||
| // reported again if the application catches the exception and | ||
| // attaches the component another time | ||
| event.unregisterListener(); |
There was a problem hiding this comment.
Explicit unregister seems redundant since the following line of code will also do the same as a side effect.
There was a problem hiding this comment.
Removed. cancelUrlValidation on the next line removes the listener through the stored registration, and that is idempotent, so the explicit unregister added nothing.
| private static VaadinService findService(Component component) { | ||
| VaadinService service = component.getUI().map(UI::getSession) | ||
| .map(VaadinSession::getService).orElse(null); | ||
| if (service == null) { |
There was a problem hiding this comment.
This is not needed if the above orElse is replaced with orElseGet. Though on the other hand, I called the fallback to getCurrent() into question in another comment so maybe the if should just be removed?
There was a problem hiding this comment.
The if is gone along with the getCurrent() fallback. The lookup is now just component.getUI().map(UI::getSession).map(VaadinSession::getService).map(...getUrlSafeSchemes()) returning an Optional, and an empty result is what makes the caller defer the check to attach instead of guessing.
| AtomicBoolean cleared = new AtomicBoolean(); | ||
| UrlUtil.validateUrlOnAttach(component, "href", "setUnsafeHref(String)", | ||
| "http://example.com", () -> cleared.set(true)); | ||
| // Safe according to the framework default, so this doesn't throw yet |
There was a problem hiding this comment.
We don't want the opposite. If the application's own configuration is more lenient, then this shouldn't fail due to the framework default either.
There was a problem hiding this comment.
Agreed - that test asserted the wrong thing and is replaced. There are now tests that a detached component is not checked against the framework defaults (custom:foo passes and stays when the application allows custom) and that a current service is not used for it either. The component tests that expected an immediate throw for javascript: on a detached component now expect it on attach instead.
The check no longer falls back to VaadinService.getCurrent() or to the framework defaults. Both of those can differ from the configuration of the application that the component ends up in, which means that a URL using a scheme that the application has explicitly allowed could be rejected, and that a check based on the current service was either redundant with the check on attach or based on the wrong application. A component that isn't attached is therefore not checked at all until it is attached, which is also the point where the value would first be sent to the browser. As a consequence, setting a URL with an unsafe scheme on a detached component throws when the component is attached rather than immediately, which the Javadoc of the setters now describes. Also drop the explicit unregister in the deferred check, as canceling the validation on the next line removes the listener anyway, and let the service lookup be a plain Optional now that there is no fallback.
totally-not-ai
Bot
changed the title
|
2 participants
Fixes #25086.
UrlUtil.isSafeUrl(String)resolved the allowed URL schemes fromVaadinService.getCurrent(), which is only available while a session is locked on the current thread. A component tree built in a background thread and attached to the UI later was therefore validated against the framework defaults rather than the application's ownvaadin.urlSafeSchemesconfiguration — and in versions where that default is a wildcard, validation was silently skipped altogether. The same fallback could also reject a URL whose scheme the application had explicitly allowed.What changed
Validation is now tied to the component's own UI.
AnchorandIFrameresolve the safe schemes from theVaadinServiceof the UI they are attached to. There is no fallback toVaadinService.getCurrent()or to the framework defaults, since either can differ from the configuration of the application the component ends up in.Unvalidated URLs are re-checked on attach. If a URL is set while the component isn't attached, a one-shot attach listener is registered. Attach is the point where the correct configuration is guaranteed to be known, and also the point where the value would first be sent to the browser. The value is cleared before the exception is thrown, so an unsafe URL is not sent to the client even if the application catches the exception. Methods that replace the value without validating it (
setUnsafeHref,removeHref, the stream-resource andDownloadHandlersetters, …) cancel any pending check.As a behavioral consequence, setting a URL with an unsafe scheme on a detached component now throws when the component is attached rather than immediately. The Javadoc of the setters and constructors describes this.
Bookkeeping moved into
UrlUtil.AnchorandIFramehad duplicated structure for validating a URL, scheduling and canceling the attach-time re-check, and building the error message.UrlUtil.validateUrl(...)now handles all of it, andUrlUtil.cancelUrlValidation(...)serves the methods that bypass validation. The pending registration is stored as component data, so the components need neither a field nor the related null handling.Page#openuses the UI it belongs to instead of the thread local.UrlUtil.isSafeUrl(String)is deprecated because it relies on the current thread having a locked session, and it now logs at warning level when it falls back to the default schemes — nothing will re-check the URL in that case. The component-aware paths stay silent when a check on attach is scheduled instead.ComponentEventBus: removing a component event listener is now idempotent regardless of whether it happens through the returnedRegistrationor throughComponentEvent#unregisterListener, since both use the same registration instance. This removes the need to track elsewhere whether a self-removing listener has already fired.Tests
AnchorTestandIFrameTestbuild their UI fromMockUIand the shared mock service and session instead of hand-written mocks, and cover setting URLs while detached, the deferred throw on attach, and cancellation.UrlUtilTestexercisesvalidateUrland the component-aware overload directly fromflow-server, since the previous coverage came only fromflow-html-componentsand did not count toward theflow-serverquality gate.PageTestcoversPage#openresolving the configuration from its own UI.API Changes
com.vaadin.flow.internal.UrlUtil