mike-jumper commented on a change in pull request #511:
URL: https://github.com/apache/guacamole-client/pull/511#discussion_r601831956
##########
File path:
extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java
##########
@@ -106,6 +114,12 @@ public String create(GuacamoleConfiguration config) throws
GuacamoleException {
// Generate a name for the configuration.
String name = QCParser.getName(config);
+ // Purge configuration parameters that are either not allowed or are
+ // explicitly denied.
+ QCParser.purgeConfigParameters(config,
+ confService.getAllowedParameters(),
+ confService.getDeniedParameters());
Review comment:
Manually purging parameters after the `GuacamoleConfiguration` has been
created makes me uncomfortable. If this is ever accidentally omitted, then a
security issue appears. Can this be done at the time that the
`GuacamoleConfiguration` is built from the URL?
For example, if `QCParser` were concrete, with a constructor requiring a set
of allowed and denied parameter, that would ensure that all parsing of URLs has
to explicitly take the allow/deny bits into account, and any usage that
(unsafely) _ignores_ those bits would need to be pretty purposefully written to
ignore them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
