New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override defaults with URL #674

Merged
merged 1 commit into from Oct 17, 2016

Conversation

Projects
None yet
4 participants
@MaxLeiter
Member

MaxLeiter commented Oct 5, 2016

My attempt at a client-side alternative to #634

I need to replace getQueryString() with http://stackoverflow.com/questions/2090551/parse-query-string-in-javascript/13419367#13419367, which should fix channels

Closes #605

@@ -259,6 +259,16 @@ $(function() {
return msg;
}
// http://stackoverflow.com/a/1099670
function getQueryParam(name) {
var url = location.href;

This comment has been minimized.

@maxpoulin64

maxpoulin64 Oct 5, 2016

Member

Use location.search instead to avoid the need to parse it. See http://stackoverflow.com/a/13419367/1739985 for a complete solution.

This comment has been minimized.

@MaxLeiter

MaxLeiter Oct 5, 2016

Member

Changed to your link

windows.on("show", "#connect", function() {
var url_config_items = ["name", "host", "password", "nick", "username", "realname", "channels"];
url_config_items.forEach(function(e) {
var key = getQueryParam(e);

This comment has been minimized.

@maxpoulin64

maxpoulin64 Oct 5, 2016

Member

Have your query param extractor return an object to avoid parsing the URL again and again needlessly.

@astorije

This comment has been minimized.

Member

astorije commented Oct 6, 2016

👏 👏
💯 for the initiative! I'll take a look to the code when I can be stable, in a few days. Thanks!

@astorije astorije self-assigned this Oct 6, 2016

@astorije astorije referenced this pull request Oct 6, 2016

Closed

Override defaults via URL #634

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Oct 7, 2016

I think this is now ready for review

// Possible parameters: name, host, port, password, tls, nick, username, realname, join
for (var key in params) {
var value = params[key].toLowerCase();
var element = $("input[name=" + key + "]");

This comment has been minimized.

@xPaw

xPaw Oct 7, 2016

Member

Limit this selector to connect window, not global.

This comment has been minimized.

@MaxLeiter
var value = params[key].toLowerCase();
var element = $("input[name=" + key + "]");
if (value === "true" || value === "false") {

This comment has been minimized.

@xPaw

xPaw Oct 7, 2016

Member

I think you should be checking the type of input instead for safety.

This comment has been minimized.

@MaxLeiter

@MaxLeiter MaxLeiter changed the title from Begin work on overriding defaults in URL to Override defaults with URL Oct 7, 2016

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Oct 8, 2016

This is done except for tests. I haven't really written tests before, and am a little lost on where to place them - do I need to make a new client/ subdirectory in tests/?

var params = parseQuery();
// Possible parameters: name, host, port, password, tls, nick, username, realname, join
for (var key in params) {
var value = params[key].toLowerCase();

This comment has been minimized.

@xPaw

xPaw Oct 8, 2016

Member

I think you need to do something like .replace(/[^a-z]/g, '') here to disallow possible abuse and escapes from the jquery selector.

var value = params[key].toLowerCase();
var element = $("#connect input[name=" + key + "]");
if (element.is("input[type='checkbox']")) {

This comment has been minimized.

@xPaw

xPaw Oct 8, 2016

Member

.is(":checkbox") is simpler.

@xPaw

xPaw approved these changes Oct 8, 2016

@astorije

This comment has been minimized.

Member

astorije commented Oct 9, 2016

Am I right assuming this feature is only useful/applicable in public mode? If so, should we check it there?

Also, what's the expected/actual behaviors when trying to set a value in a locked or hidden network?

@xPaw

This comment has been minimized.

Member

xPaw commented Oct 9, 2016

Am I right assuming this feature is only useful/applicable in public mode? If so, should we check it there?

Technically it still works in private mode too, I see no reason to add an explicit check for it.

Also, what's the expected/actual behaviors when trying to set a value in a locked or hidden network?

It most likely will allow to visually change the fields, but the server will still correctly respect the lock. Perhaps a disabled input check is needed here.

// Possible parameters: name, host, port, password, tls, nick, username, realname, join
for (var key in params) {
var value = params[key];
key = key.replace(/\W/g, "");

This comment has been minimized.

@astorije

astorije Oct 9, 2016

Member

Add a comment here to detail what \W is doing and/or why it's necessary.

This comment has been minimized.

@MaxLeiter
element.prop("checked", (value === "1" || value === "true") ? true : false);
} else {
element.val(value);
}

This comment has been minimized.

@astorije

astorije Oct 9, 2016

Member

$("#connect input[name='foo']").is(":checkbox") is always false if there is no input foo, and $("#connect input[name='foo']").val("bar") returns [].
Not a big deal but a bit confusing when reading the code, plus it could lead to unexpected stuff if/when this code evolves. I would add an extra check to make sure there is a match. What do you think, @xPaw?

This comment has been minimized.

@MaxLeiter
var params = window.URI(document.location.search);
params = params.search(true);
// Possible parameters: name, host, port, password, tls, nick, username, realname, join
for (var key in params) {

This comment has been minimized.

@astorije

astorije Oct 9, 2016

Member

for (var ... in ...) includes properties that are inherited through the prototype chain, and this is generally not what you want. See http://eslint.org/docs/rules/guard-for-in for an alternative.
Also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#Iterating_over_own_properties_only.

var element = $("#connect input[name='" + key + "']");
if (element.is(":checkbox")) {
element.prop("checked", (value === "1" || value === "true") ? true : false);

This comment has been minimized.

@astorije

astorije Oct 9, 2016

Member

Can't we use element.is(':checked') for this instead?

This comment has been minimized.

@MaxLeiter

MaxLeiter Oct 9, 2016

Member

This is setting the checked property. I could do an if(is(':checked')) but I don't think thats an improvement.

var value = params[key];
key = key.replace(/\W/g, "");
var element = $("#connect input[name='" + key + "']");

This comment has been minimized.

@astorije

astorije Oct 9, 2016

Member

This also needs to check that:

  • input is not disabled (when lockNetwork is true)
  • input is not hidden (when displayNetwork is false). This might require a bit more fiddling than the one above, but it would be pretty nasty to let someone send you to a connect window with a bad host that you cannot see anywhere.

This comment has been minimized.

@MaxLeiter
@astorije

This comment has been minimized.

Member

astorije commented Oct 9, 2016

In addition to my review above:

  • We probably want to force displaying connect window when there are params in the URL. In public mode, people get sent to the connect window directly, but in private mode, it's not obvious these params are used until one clicks on the "Connect" button.
  • In general, even though it technically works, I don't think this is quite ready for private mode. Use case for private mode is very limited, and the only use case I can think of would be a service that add links to channels to click on. Problem is, at the moment this would create a brand new connection for the same host every time. A solution could be to join the channel if already connected to this host, but maybe for a latter time. I just think this is weird for private mode at the moment. I won't strongly object though, it's just my 2c.
  • Not too far from the first point here, when loading a URL with such params, params not specified should be empty instead of relying on defaults IMO.
  • We might want to add a list of allowed fields, similarly to what this other PR was doing. I can't think of a use case where specifying password in URL is a good idea. I might be wrong though, but I think it's safer to whitelist things. Thoughts?
@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Oct 9, 2016

We probably want to force displaying connect window when there are params in the URL. In public mode, people get sent to the connect window directly, but in private mode, it's not obvious these params are used until one clicks on the "Connect" button.

I agree

In general, even though it technically works,... I just think this is weird for private mode at the moment. I won't strongly object though, it's just my 2c.

Once this is merged (and maybe more functionality is added) I plan to make a chrome extension that will add support forirc:// protocol. I'll add a check to see if you're already connected for private mode.

Not too far from the first point here, when loading a URL with such params, params not specified should be empty instead of relying on defaults IMO.

I disagree - I think the defaults should stay, because the person providing a link with URL parameters is probably the host of the lounge instance, and has the defaults set for a reason.

We might want to add a list of allowed fields, ... but I think it's safer to whitelist things. Thoughts?

I don't think it will really make a difference; it's 'locked-in' to the #connect window anyways. And again, the person providing this URL probably hosts the lounge instance/channel they want you to connect to, so may be providing the password for a reason.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Oct 9, 2016

Addressed @astorije's review comments, haven't fixed anything I responded to last comment.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Oct 9, 2016

After some messing around, turns out it's pretty difficult to handle private mode well. I ended up simply removing the functionality in private mode, and I think that can be addressed in another PR.

Ultimately, I think we'll need to move the parameter code into a new function (like handleParameters()) that will have a callback. If we're in private mode, we'll need to check if we're already connected to the network and such in the callback.

@astorije

Great stuff!
Tried a couple bobby-tables-like (actually, things like http://localhost:9000/?name=%22%3E%3Cscript%3Ealert(%27bazinga%27)%3C/script%3E) and nothing terrible happened.

@astorije

This comment has been minimized.

Member

astorije commented Oct 17, 2016

Both comments by maxpoulin64 were addressed, and I don't want to overwhelm him with things that were already reviewed twice :-)
Merging, thanks a lot @MaxLeiter!

@astorije astorije merged commit f5af8a4 into thelounge:master Oct 17, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@astorije astorije added this to the 2.2.0 milestone Oct 17, 2016

@MaxLeiter MaxLeiter deleted the MaxLeiter:MaxLeiter/override-defaults branch Oct 17, 2016

@MaxLeiter MaxLeiter referenced this pull request Jan 24, 2017

Closed

Channel assign with URL #888

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment