Marcinkiewicz/issue invalid url construct #2374
Conversation
…string. Handle exception thrown by new URL() when the incorrect parameter is being passed, instead of allowing the exception to bubble up.
change/@microsoft-teams-js-cbfc373a-83b3-4276-bde7-0cb028ecc6f6.json
Outdated
Show resolved
Hide resolved
| shouldProcessMessageLogger('Message has an invalid origin of %s', messageOrigin); | ||
| } | ||
| return isOriginValid; | ||
| } catch (_) { |
There was a problem hiding this comment.
Can you please provide an example of a 'malformed' URL (except an empty URL) that is causing the exception? If it exists, then is there a reason why we are handling it here but not in the validateOrigin() method itself? Thank you.
There was a problem hiding this comment.
Fwiw, I believe that messageOriginURL being set here is fine since otherwise validateOrigin input parameter would need to be modified to a less strict type (it is currently expecting a URL type)
@marcinkiewicz appending to the above suggestion, would you be willing to add a unit test to communications.spec.ts that tests for this specific scenario that you are seeing.
There was a problem hiding this comment.
I am curious to hear if it's something other than an empty string that is causing a problem here. FWIW I don't think this can be handled inside of validateOrigin unless we change a lot about how that function works because I think the exception is being thrown from the URL constructor itself. We'd have to change the validateOrigin function to take an arbitrary string.
Unit test would be nice!
There was a problem hiding this comment.
Concern is not about an empty URL, as I mentioned it as an exception in my previous message. It is expected that we should check for a non-empty string before calling the validateOrigin() method and I understand that changing the function signature is not a good option for this scenario.
The issue lies with other 'malformed' URLs mentioned in the PR description. If there are no such malformed URLs other than an empty one, could we please update the PR description to avoid confusion?
Also, If we are only addressing the empty URL scenario, then the call to validateOrigin() should not be within a try block. Placing it inside a try block could potentially mask underlying issues in the validateOrigin() method. It is important to ensure this method's integrity, as it might be used in other locations as well.
I am curious to know about the malformed URLs, if any, that necessitate this try block. Otherwise, we should handle the empty URL check separately and ensure that validateOrigin() is enough to handle any valid URL scenarios.
If handling all edge cases is not feasible, we should catch the exception inside the validateOrigin() method itself. This way, all callers won't need to wrap their calls in try and catch blocks to achieve the same result.
There was a problem hiding this comment.
I agree with the point about hiding potential issues in the validateOrigin() method if it throws an error. The malformed URL could be a relative URL (not containing host). Our telemetry does not include this detail, so I had to assume both possibilities - empty and malformed URL, since both scenarios are causing the URL constructor to throw.
Based on the concern of the validateOrigin(), I will update the PR to only capture URL creation in the try-catch.
…hub.com/OfficeDev/microsoft-teams-library-js into marcinkiewicz/issue-invalidUrlConstruct
marcinkiewicz
dismissed stale reviews from jadahiya-MSFT and TrevorJoelHarris
via
aad0318
marcinkiewicz
requested review from
TrevorJoelHarris,
jadahiya-MSFT and
baljesingh
| try { | ||
| messageOriginURL = new URL(messageOrigin); | ||
| } catch (_) { | ||
| shouldProcessMessageLogger('Message has an invalid origin of %s', messageOrigin); |
There was a problem hiding this comment.
Can we get a unit test added to communication.spec.ts that hits this catch block?
For more information about how to contribute to this repo, visit this page.
Description
While creating URL from empty or malformed string, we would see Failed to construct 'URL': Invalid URL while validating the message origin. Add extra check to include this case and return invalid origin response instead of populating the TypeError exception.
Main changes in the PR:
communication.tsfile with the exception handling