Skip to content
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

Handle storage of javascript: URIs #46

Closed
nodiscc opened this issue Nov 8, 2014 · 11 comments
Closed

Handle storage of javascript: URIs #46

nodiscc opened this issue Nov 8, 2014 · 11 comments
Labels
easy good place to start contributing enhancement feedback needed

Comments

@nodiscc
Copy link
Member

nodiscc commented Nov 8, 2014

Copied from sebsauvage#174

At https://github.com/shaarli/Shaarli/blob/master/index.php#L1479, add && !startsWith($url,'javascript:')

This allows to pervent bookmarklets' URL to be prepended with http:// prefix.
That keeps the bookmarklet link functional and dragable "as is" to the browser's bookmarks bar.

@e2jk
Copy link

e2jk commented Nov 8, 2014

OK

@Ginko-Aloe
Copy link

Hi @nodiscc,

I closed sebsauvage#174 as you suggested.

Just a question: what do you mean by a "temporary" fork? Sounds like a good idea to update shaarli so why a temporary repo ?

@nodiscc
Copy link
Member Author

nodiscc commented Nov 8, 2014

@Ginko-Aloe thank you for closing the old issue.

what do you mean by a "temporary" fork

See sebsauvage#191. @sebsauvage does not have time to maintain Shaarli, add fixes, merge pull requests and cleanup the bug tracker currently. This organization tries to keep the project alive and fix common problems/requests. We hope this situation is temporary, it would be better to have only 1 repository with the latest changes (as you can see here https://github.com/shaarli/Shaarli/network I think we're on the right path)

@Ginko-Aloe
Copy link

Ok, I got it :)

Glad to see shaarli live again !

@nodiscc nodiscc added the easy good place to start contributing label Jan 9, 2015
@nodiscc nodiscc added this to the 0.9beta milestone Jan 9, 2015
@nodiscc
Copy link
Member Author

nodiscc commented Feb 23, 2015

Ongoing work in pull request #119. Please add your voice to the discussion there.

Edit: considering it's an enhancement and there is no agreement on how to properly add this in #119, I've removed the 0.9beta milestone so this doesn't block us for the release. You can still merge #119 in your own install if you desperately need this :)

@nodiscc nodiscc removed this from the 0.9beta milestone Feb 25, 2015
@nicolasdanelon
Copy link

-1 to add javascript:*something*

@nodiscc
Copy link
Member Author

nodiscc commented Mar 4, 2015

@nicolasdanelon +1s an -1s without technical or usability argumentation spams everyone subscribed to this issue (mail notification, RSS notification and Github notification). Please refrain in the future, thanks.

@nicolasdanelon
Copy link

ok, sorry.
is not a good practice for coding. developers shouldn't code like that and we shouldn't be agree with this kind of practices. -1

@e2jk
Copy link

e2jk commented Mar 5, 2015

@nicolasdanelon It's not Shaarli's job to influence "coding practices".
With HTML5 "apps", Firefox mobile and all, if you just take a "all JavaScript is bad" approach you haven't got a bright future in front of you.
Plus, this doesn't have anything to do with the developers of the websites, since this would be used primarily to store bookmarklets.
Sorry, not a convincing argument to me.

@nodiscc
Copy link
Member Author

nodiscc commented Mar 7, 2015

Copied from #119

I've just re-read a bug report on the RequestPolicyContinued bug tracker. There are a lot of existing URI schemes aside from http: https: ftp: magnet: (the linked bug report references feed: mailto: magnet: mediasource: about: jar: gopher: UT2004: spotify: chrome-extension: floatnotes: greasemonkey: bank-id: and mooore) and I think we should allow users to bookmark them.

@e2jk has concerns about the security of this, and the workaround (popup) is IMHO not so good.

I propose allowing users to configure extra URI schemes they want to be able to store. Eg. a config array $GLOBALS['config']['EXTRA_ALLOWED_URIS'] = array('javascript:', 'mailto:', mediasource'); and loop over this for if (!startsWith($url,$allowed_uri).

Would that that be ok?

@nodiscc
Copy link
Member Author

nodiscc commented Mar 8, 2015

Another solution is to have have an option/checkbox (heh) in the tools dialog, checked by default:

[x] Only allow bookmarking http://, https:// and magnet: URIs

Knah-Tsaeb pushed a commit to Knah-Tsaeb/myShaarli that referenced this issue May 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy good place to start contributing enhancement feedback needed
Projects
None yet
Development

No branches or pull requests

4 participants