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

Allowing steem URI scheme in markdown content #2132

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@aaroncox
Contributor

aaroncox commented Dec 9, 2017

These two small changes allow posts to render links that use the steem:// URI scheme. I have successfully tested these changes running a dev version of condenser and viewing the following post:

https://steemit.com/spam/@webdev/test-6#@jesta/re-webdev-test-6-2017128t175053871z

This URI scheme will allow launching of external apps (e.g. Vessel) to further interact with readers.

Currently you can workaround this issue and embed steem:// links into posts by using a URL shortener (e.g. tinyurl.com). This takes the links and converts them shorter https:// links, which render as valid links. This isn't preferable though - since the user would have to generate the steem:// link and then run each one through a URL shortener.

@gl2748 gl2748 self-assigned this Dec 10, 2017

@gl2748 gl2748 referenced this pull request Dec 10, 2017

Merged

support steem URI scheme #2134

@gl2748

This comment has been minimized.

Show comment
Hide comment
@gl2748

gl2748 Dec 10, 2017

Contributor

@aaroncox Vessel looks really cool - a lot like metamask? Great stuff, I have opened a PR that includes this work along with tests to cover the changes:
#2134

Contributor

gl2748 commented Dec 10, 2017

@aaroncox Vessel looks really cool - a lot like metamask? Great stuff, I have opened a PR that includes this work along with tests to cover the changes:
#2134

@aaroncox

This comment has been minimized.

Show comment
Hide comment
@aaroncox

aaroncox Dec 10, 2017

Contributor

@gl2748 Awesome, thank you very much!

Yeah - I think it's evolving in a similar direction as metamask, except as a standalone app rather than a browser extension. Excited to see what people end up building with it!

Contributor

aaroncox commented Dec 10, 2017

@gl2748 Awesome, thank you very much!

Yeah - I think it's evolving in a similar direction as metamask, except as a standalone app rather than a browser extension. Excited to see what people end up building with it!

@gl2748 gl2748 added the WIP label Dec 21, 2017

@gl2748

This comment has been minimized.

Show comment
Hide comment
@gl2748

gl2748 Dec 21, 2017

Contributor

@sneak how do we want to handle this work? 'steem' is obviously not a standard URI scheme per:
https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

Contributor

gl2748 commented Dec 21, 2017

@sneak how do we want to handle this work? 'steem' is obviously not a standard URI scheme per:
https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

@bnchdrff

This comment has been minimized.

Show comment
Hide comment
@bnchdrff

bnchdrff Dec 21, 2017

Contributor

i think we should close this PR and discuss in another forum, probably on a steemit.com post

Contributor

bnchdrff commented Dec 21, 2017

i think we should close this PR and discuss in another forum, probably on a steemit.com post

@aaroncox

This comment has been minimized.

Show comment
Hide comment
@aaroncox

aaroncox Dec 21, 2017

Contributor

Does this need to be a standard in order to allow through the sanitization filters?

This is quite discouraging.

Contributor

aaroncox commented Dec 21, 2017

Does this need to be a standard in order to allow through the sanitization filters?

This is quite discouraging.

@sneak

This comment has been minimized.

Show comment
Hide comment
@sneak

sneak Dec 21, 2017

Contributor

@aaroncox We need to do a security analysis of the potential security impact and attack vectors such a change could potentially enable. For obvious reasons, we take a conservative approach. I would love to support this feature once we’ve had a chance to do that.

Contributor

sneak commented Dec 21, 2017

@aaroncox We need to do a security analysis of the potential security impact and attack vectors such a change could potentially enable. For obvious reasons, we take a conservative approach. I would love to support this feature once we’ve had a chance to do that.

@aaroncox

This comment has been minimized.

Show comment
Hide comment
@aaroncox

aaroncox Dec 21, 2017

Contributor

It's a reasonable concern and should be the utmost priority, but please realize this change doesn't allow for anything you can't already do today. If you discover attack vectors that are capable of being performed through a custom protocol - those attacks are possible today (using a URL shortener and swapping the link to HTTPS) and wouldn't actually be "enabled" by allowing steem: links through the sanitizer.

If a security analysis is what's needed to proceed, I'll respect the process and await the results. I'm happy to help if and where I can.

Contributor

aaroncox commented Dec 21, 2017

It's a reasonable concern and should be the utmost priority, but please realize this change doesn't allow for anything you can't already do today. If you discover attack vectors that are capable of being performed through a custom protocol - those attacks are possible today (using a URL shortener and swapping the link to HTTPS) and wouldn't actually be "enabled" by allowing steem: links through the sanitizer.

If a security analysis is what's needed to proceed, I'll respect the process and await the results. I'm happy to help if and where I can.

@gl2748

This comment has been minimized.

Show comment
Hide comment
@gl2748

gl2748 Dec 22, 2017

Contributor

In the event this is all o-k, the PR here covers this work and has tests for the changes. #2134

Contributor

gl2748 commented Dec 22, 2017

In the event this is all o-k, the PR here covers this work and has tests for the changes. #2134

@roadscape roadscape removed the WIP label Mar 12, 2018

@roadscape

This comment has been minimized.

Show comment
Hide comment
@roadscape

roadscape Mar 12, 2018

Contributor

@gl2748 - PR #2134 was closed yet it contains tests for this feature. It's also on a local branch which is required for testing. So I'm reopening it and moving the feature/discussion there.

Contributor

roadscape commented Mar 12, 2018

@gl2748 - PR #2134 was closed yet it contains tests for this feature. It's also on a local branch which is required for testing. So I'm reopening it and moving the feature/discussion there.

@roadscape roadscape closed this Mar 12, 2018

@gl2748

This comment has been minimized.

Show comment
Hide comment
@gl2748

gl2748 Mar 13, 2018

Contributor

@roadscape sounds good. I would say that this has been languishing for quite some time and perhaps the consensus (albeit by implication) is that this work will never be merged and that all associated PRs ( including #2134) can be closed?

Contributor

gl2748 commented Mar 13, 2018

@roadscape sounds good. I would say that this has been languishing for quite some time and perhaps the consensus (albeit by implication) is that this work will never be merged and that all associated PRs ( including #2134) can be closed?

@roadscape

This comment has been minimized.

Show comment
Hide comment
@roadscape

roadscape Mar 13, 2018

Contributor

The change is simple; I'll dust it off, and check still relevant/safe.

Contributor

roadscape commented Mar 13, 2018

The change is simple; I'll dust it off, and check still relevant/safe.

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