-
Notifications
You must be signed in to change notification settings - Fork 408
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
Auto-remove trailing slash from Nightscout URL #190
Conversation
Thanks! @bjorkert recently added some URL sanitization to Loop Follow. Posting that PR here for inspiration: loopandlearn/LoopFollow#286 |
Ok - I made changes to the code to not only remove the trailing / but also do the same checks Jonas added (symbols, capital letters, etc.) to LoopFollow. Should be force pushed thanks to @MikePlante1. Better? :) |
I failed to test that LoopFollow PR well enough. I discovered this while testing a later PR. I don't know if your implementation has the same problem. "Try typing in https://sitename/, instead of pasting in a URL, and you'll notice it won't allow you to enter the / characters." @bjorkert made an additional change, please see this commit: |
@marionbarker I'm not sure I understand the problem or I cannot reproduce it. :) When I enter https://sitename/ in to the URL-field, I can continue to modify it however I wish. Maybe a difference, but I haven't looked at how LoopFollow does it, is that in this instance within Trio the checks are not ran until you actually tap "Connect" - so when you're supposedly finished entering the URL. If LoopFollow does it on the fly (whilst it is still being entered), maybe that is a difference? Just something that sprang to mind, but either way: I don't think I can reproduce that particular problem you mention. :) -edit- Even if I enter https://sitename/ and press "Connect": it just removes the trailing slash and nothing weird happens. (Of course it errors out with a DNS lookup failure, but that's expected haha. :)) |
I was not sure, looking at your implementation, whether you would have the same problem. Thanks for checking. |
LoopFollow does cleaning on each detected change, so it did drop "/" when you are enter https:/, while paste worked fine. If you trigger this using a manual "Connect" button you would be ok. |
In testing this, I found out that without this PR, a valid token will actually allow it to connect to a Nightscout, even though it won't actually let it download or upload anything from that Nightscout without an API Secret. If the correct API Secret is entered, though, a URL with a token at the end allows Trio to upload and download to Nightscout successfully, even if the token is invalid. This makes me think I was wrong for suggesting sanitization of the URL, since if someone copies their NS with a token attached and doesn't bother to delete their token, but also adds their API Secret, that would currently work fine in dev now. But after this PR, it would turn the query parameters into a path and cause an error. So maybe it's best to revert to your original commit of only deleting a trailing I also found that my Nightscout works just fine in dev (without this PR) with a trailing |
@MikePlante1 So you want me to force push the previously force pushed away code huh :P Alright, I'll try to do that tomorrow. :) |
Regarding @MikePlante1 ‘s comment, I think @bjorkert made some changes to LF to allow a url with token to be pasted without the API secret. Perhaps we could port that as well? |
When users enter a trailing / in their Nightscout URL (Eg: "https://url/" rather than "https://url"), this will immediately result in a "Not found!" (404) error, because a / is already being appended. (Resulting in "https://url//"). This check removes the trailing slash to prevent 404's.
Alright, I've just reverted the commit back to the original that just checks for the trailing /. I think in general that's likely enough anyway as typically a.) capital letters are no problem for DNS resolution, b.) users will not quickly enter other symbols in their URL. When they do: they'll get a connection failure and can probably spot the error. The trailing / being an issue is just an obscure problem that may not be so easily understood. If we do want to do something else/more to the URL-field check, let me know and I can make another commit/PR. :P For now, let's just push this one for the trailing / issue and start with fixing that :D |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks, @LiroyvH! Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch works as expected, as tested by adding a trailing /, which is removed upon tapping
Connect`. Thanks!
However, Xcode wanted to change some indentation upon build, and presented the following diff for L52-L55 of NightscoutConfigStateModel.swift:
if let CheckURL = url.last, CheckURL == "/" {
- let fixedURL = url.dropLast()
- url = String(fixedURL)
- }
+ if let CheckURL = url.last, CheckURL == "/" {
+ let fixedURL = url.dropLast()
+ url = String(fixedURL)
+ }
You probably want to build and then commit or amend those changes.
I made a PR to the pull request branch to fix my change request: I will approve when this is merged :-) |
@LiroyvH what is your status on merging @bjornoleh 's PR to your feature branch so this can move forward here? |
@dnzxy Haven't had the time to look at it, probably somewhere this week (hopefully today) |
Xcode auto changes of indentation in NightscoutConfigStateModel.swift
@dnzxy I suppose this is fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Hi, I‘d like to propose a more elegant solution that
func isValidURL(_ urlString: String) -> Bool {
let pattern = "^(http|https)://[^\\s/$.?#].[^\\s]*$"
let regex = try! NSRegularExpression(pattern: pattern, options: .caseInsensitive)
let range = NSRange(location: 0, length: urlString.utf16.count)
return regex.firstMatch(in: urlString, options: [], range: range) != nil
}
func reformatURL(_ url: String) -> String {
var formattedURL = url
if isValidURL(formattedURL) {
if let lastSlashRange = formattedURL.range(of: "/", options: .backwards) {
formattedURL = String(formattedURL[..<lastSlashRange.upperBound])
formattedURL.removeLast()
}
}
return formattedURL
} Example usage: var url = "https://www.my-example.com/some/path/?foobar=baz"
url = reformatURL(url)
print(url) // Output: "https://www.my-example.com/some/path" Used to guard URL entering via if isValidURL(url) {
url = reformatURL(url)
// …
} |
@LiroyvH and reviewers (@bjornoleh, @MikePlante1 , @marionbarker ) – how's this one looking? |
Maybe you could make a PR with the suggested changes to the PR branch, or as a separate PR to dev/alpha? The current PR is ok, but could of course be improved. |
I disagree in that this current PR is okay because it’s lacking validation if the entered URL is a valid URL. I‘m okay with keeping the logic to only remove trailing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does what it says but it is not necessary and not particularly useful. I would suggest we just close this.
edited to add remove original comment and add:
I tested only with Nightscout created using the Google Cloud which evidently does not care if there are final slashes.
@bjornoleh got different results from me and @LiroyvH explained why.
Configuration:
-
iPhone SE running 17.5.1 running latest Trio dev, add patch
-
iPhone 8 running 16.7.8 running latest Trio dev (no patch)
-
Enter a URL that is incorrect (ending in
.c
instead of.com
):Error: The request timed out
-
Enter a URL that is correct but with extra slashes (ending in
.com////
):- with this patch, one of the slashes is removed
- Trio connects with extra slashes
- repeat with the iPhone 8 running without the patch
- Trio connects with extra slashes
- with this patch, one of the slashes is removed
@marionbarker , without the fix, I am finding that connection is blocked with one or several trailing slashes, with the message "Error: Not Found" Branch: current dev aeb4ee1 |
@LiroyvH , please sync your PR branch with dev, as a breaking change for Omnipod just landed in dev. Thanks! |
@bjornoleh Yeah I don't know if that worked out. I did a sync on there and it resulted in adding an extra commit to this PR...? I don't think that's right? Pls let me know. @marionbarker Some of the nightscout providers ignore extra slashes on their end, such as yours by the sounds of what you're writing. That's great, but not the target for this fix. :) This fix is for the ones that don't, which typically includes self-hosted solutions. If you add ten /'s to the end: yes, this PR doesn't fix that - this is just a fix for the uttermost common occurrence: one / at the end of an URL, which is a very normal thing to happen but can lead to errors and thus this PR ensures its stripped. Nothing more, nothing less. (And if someone doesn't notice ten slashes at the end, I think we have bigger issues. :P) I'm sure we can enhance this and add more checks and balances to the URL sanity check (note: must be checked that it doesn't break in-URL variables!); but as long as there are no PR's for that I'd suggest doing the obvious: at least merge this first, we can look at doing more later. Otherwise indeed please just close this PR so no more time is spent on it. It's an extremely simple fix to an extremely often occurring URL-notation and I'm a wee bit surprised there's so much discussion over 4 lines of extensively tested code for a simple URL enhancement. Let's make a choice now to merge it or decline it. |
The merge of dev into your branch was good! :-) |
I made a proposition to enhance your work to which you don’t really have reacted so far. If you don’t intend to incorporate the suggestion, that’s okay; ignoring it but then demanding a PR to be either merged now or closed, is not really a nice way of going about it, especially with different tester feedback by Marion. I tested the as-is PR and it works on my end, I‘m fine with either way. Since Liroy took the time out of his day to propose this PR, I deemed it a good opportunity to at least add a URL validity check. It’s okay if that doesn’t go in, we can improve input validation another day 😊 |
I tested only with Nightscout created using the Google Cloud which evidently does not care if there are final slashes. |
The base branch was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM. Works as intended, let’s not get out of scope.
When users enter a trailing / in their Nightscout URL (Eg: "https://url.night.scout/" rather than "https://url.night.scout"), this will immediately result in a "Not found!" (404) error, because a / is already being appended. (Resulting in "https://url.night.scout//" being requested from the NS-server).
This check removes the trailing slash when the user enters it in to the URL field, to prevent 404's.