Add custom Map Styles support#950
Conversation
dgraf-gh
left a comment
dgraf-gh
left a comment
There was a problem hiding this comment.
First off, thank you @FreshImmuc for all the effort you put into this. It’s some awesome work! I really like the general idea and the way the core map styles are implemented.
However, I’m having a hard time with the SafeHttpClient and URLSignature and all the security related components. It feels like they might be over-complicating the solution. To be specific:
- Maintenance: It adds a lot of complexity to the long-term management and maintenance of the code.
- Contributor Friction: It makes adding new styles much more difficult because a user has to understand all these specific restrictions first.
- Architecture: It feels like it’s trying to solve a problem that would be better handled at the network level rather than directly within the app.
- Trust: It gives the impression that we’re trying to protect the app from its own users. If we’ve reached a point where we can’t trust them, we likely have bigger problems than this to solve.
In the end, I’m not comfortable pulling this in as it stands because it adds too much complexity to an already complicated problem. That said, the underlying solution for the map styles looks great! I’d be more than happy to review this again once the security-based elements are streamlined or resolved.
Thanks again for the hard work!
| access_log /var/log/nginx/access.log main; | ||
|
|
||
| server_tokens off; | ||
| # Public fallback dns can leak internal hostnames if this proxy is ever used with untrusted upstream names. |
There was a problem hiding this comment.
I do not get this or do not understand the reasoning around this. Could you explain the threat level which shall be addressed here?
From my perspective, I would not leave this in since if you need this kind of protection (then 1.1.1.1 and 8.8.8.8 is also questionable - why should I trust them), maybe setting this on the docker level would be the better solution.
There was a problem hiding this comment.
Why does this exists? I do not get the usecase, this also applies to all the other security related stuff. The SafeHttpClient and the RemoteTileUrlValidator
| return messageSource.getMessage(messageKey, null, defaultMessage, LocaleContextHolder.getLocale()); | ||
| } | ||
|
|
||
| public String translateWithDefault(String messageKey, String defaultMessage, Object... args) { |
There was a problem hiding this comment.
Lets remove this method, the default messages if nothing is found should be in messages.properties so it falls back to english
|
|
||
| private URI parseHttpUrl(String value, String fieldName) { | ||
| if (!StringUtils.hasText(value)) { | ||
| throw new IllegalArgumentException(message("js.map.settings.dialog.map-styles.error-url-required", fieldName + " is required.", fieldName)); |
There was a problem hiding this comment.
Here you could define the error messages without the js. prefix. Since they are validated on the server.
Prefixing them with js. pulls them into the frontend automatically so the can be used by the t() function
| return StringUtils.hasText(value) ? value.trim() : null; | ||
| } | ||
|
|
||
| private String label(String key) { |
There was a problem hiding this comment.
This should not be done when persisting the value, since there is no way of reverting this when the user changes his language
There was a problem hiding this comment.
In general here are my points:
- do not validate input here in that class but rather in the calling class.
- do not mangle with user input here like trimming, replacing stuff with default values. This leads to confusion since we store different data than the user expected.
| } | ||
|
|
||
| byte[] bytes = address.getAddress(); | ||
| if (bytes.length == 4) { |
There was a problem hiding this comment.
That is to clever for me. What does this block do exactly?
# Conflicts: # src/main/resources/static/js/map-renderer.js
2 participants
Hey, firstly, it's an honor to contribute to this project. I'm using it every day and love it! :)
I didn't upgrade because of the removed custom tiles support, so i added it back.
Now to the things added. Basically, added full custom map tiles support:
Backend stuff:
Hope everything looks good, tried to keep it as optimized as possible :)