Fix: Token with sprite and glyphs #192
Conversation
so we can run unit tests
gavinr-maps
changed the title
|
Tested and working thanx Gavin! |
|
Just checking in on the progress of this? |
There was a problem hiding this comment.
@gavinr-maps generally I think this looks good. I'm a little cautious of the case where someone could craft a malicious style with a glyph or token URL pointed to their server which we would then append the token to thereby giving the token to an attacker.
I think the only way around this would be to confirm that the url of the style and the URL and the URL of the sprite/glyph are on the same domain if they are I think we can consider the token safe to use. Otherwise the user will need to append the token manually with the style option.
Co-authored-by: Patrick Arlt <[email protected]>
Co-authored-by: Patrick Arlt <[email protected]>
Co-authored-by: Patrick Arlt <[email protected]>
Co-authored-by: Patrick Arlt <[email protected]>
|
@patrickarlt I have updated it to check the domain per your comment. Please review. |
| @@ -142,6 +142,10 @@ function loadStyleFromUrl (styleUrl, options, callback) { | |||
| request(styleUrl, params, callback); | |||
| } | |||
|
|
|||
| function isSameTLD (url1, url2) { | |||
| return (new URL(url1)).hostname === (new URL(url2)).hostname; | |||
There was a problem hiding this comment.
I do have some mild concerns about this because new URL will throw errors on relative paths. In general I think this is ok because we check if they are relative in formatStyle. Ideally I would love to use new URL() to resolve the paths
There was a problem hiding this comment.
I do have some mild concerns about this because
new URLwill throw errors on relative paths. In general I think this is ok because we check if they are relative informatStyle.
I agree - I think it's not ideal but ok in this case since we're checking for relative in the parent function.
Ideally I would love to use
new URL()to resolve the paths
I'm not following here - are you suggesting a code change for this PR or just a general future idea?
# Conflicts: # src/EsriLeafletVector.js
|
@gavinr-maps this LGTM! Feel free to merge and release it. |
|
This was released in v4.2.0. |
This PR puts in a fix to properly add the token for sprites and glyphs, as pointed out in #188 and #186.
formatStylefunction inUtil.js. To do this, I had to export that function. So that's what I did first: ba913e7@mstiglingh can you please test this out if it fixes your use case.