search

DanAtkinson / Fuskr

Fuskr - an image gallery extension for Google Chrome
http://danatkinson.github.io/Fuskr/
MIT License
22 stars 7 forks source link

Fusks with parameters in the querystring throw error #25

Closed DanAtkinson closed 7 years ago

DanAtkinson commented 8 years ago

Hi Jonny,

I noticed an issue when looking at video implementation where the id is in the querystring.

For example, take the following url - http://example.com/images?image=[1-10].jpg.

When you attempt to Fusk that url, it tells me that it's invalid, but there's no reason why it should be.

I think that the issue is in the $stateProvider in images.js where it's taking the stateparams.path, but is missing off the query params. This would also cause a problem on sites where a token/session id is passed in the url as, when the gallery is generated, the querystring is missing.

Is it possible for you to have a look and identify what the problem with it is?

Many thanks, Dan

DanAtkinson commented 7 years ago

I'm going to modify the functionality of Fuskr so that the url to scrape is not stored in the url but in the page. It should be linked with an id in the url though.

jbolster commented 7 years ago

How have I completely missed this? Whoops.

Store the URL in the page? What would you do to pass the URL from the page to the gallery? This could just be a fairly easy fix by just encoding the URL when constructing the gallery URL.

DanAtkinson commented 7 years ago

Hey Jonny!

To be honest, the url to scrape being contained within the url itself seems to cause an awful lot of problems.

I've seen image urls with '#' which also cause a problem because Angular doesn't seem to be able to grab the rest of the url. If the url is encoded, the the server doesn't recognise it and throws a 404.

If the url to scrape is distinctly separate from the address bar, then this shouldn't be a problem, which is what I've been trying to work towards in my spare time.

I've also been breaking everything out into an application that uses npm and grunt to build, concat, clean, check (jslint) and then drop the files into a single 'dist' directory.

Thanks, Dan

DanAtkinson commented 7 years ago

There is a branch (Url-Refactor) where I began making some changes but I don't have a great deal of free time lately. :-(

jbolster commented 7 years ago

Yeah - it was a bit ill-thought out. I can take a look at a better way to pass this through - we don't particularly need UI-Router on background.js (I think it was my golden hammer at the time).

I've a few side projects with a grunt file that'll do the linting / tests / minification / etc so I can look into dropping that and tidying up everything (as I now have spare time for the next week!) :)

DanAtkinson commented 7 years ago

Thanks! I'd really appreciate that as the extension has had a few issues with it becoming corrupted, crashing and needing to be repaired (see support page).

I'm not sure what causes it, but it seems to be something in the options page.

jbolster commented 7 years ago

Only thing I can quickly spot with that is that the options.html page is 3<!doctype html>. There seem to have been a few problems with this 'The extension is corrupted' message in Chrome from a quick view. Not sure if the doctype would have anything to do with it but it wouldn't hurt to tidy everything up a bit.

DanAtkinson commented 7 years ago

Yeah, I noticed that I'd added this. The intention was to move the options page to a popup dialog (as part of the OptionsV2 API) rather than a full-blown page.

jbolster commented 7 years ago

Already have the pop-up working locally :) (just needs a bit of styling).

As for this issue, I am thinking of passing it as a hash and using $location.hash() to retrieve the value. On first glance, this seems to be working just fine - even when the hash has a # itself, or a query string, etc.

jbolster commented 7 years ago

My updates in PR#30 seem to sort this. Images will get a proper refactor but that PR is just to get a working build.

jbolster commented 7 years ago

Fixed in PR#33 too

Test URL: http://lorempixel.com/400/200/sports/[0-2]?image[1-2].bmp

In this instance, the mime type is checked so it knows the extension isn't to be a bmp

jbolster commented 7 years ago

Closing after having merged PR#33

Let me know if there's anything else