Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Atom/Core URL handler support for Markdown inside Datatip #16888

Closed
BoykoAlex opened this issue Mar 5, 2018 · 12 comments · Fixed by #16940
Closed

Atom/Core URL handler support for Markdown inside Datatip #16888

BoykoAlex opened this issue Mar 5, 2018 · 12 comments · Fixed by #16940
Labels

Comments

@BoykoAlex
Copy link
Contributor

BoykoAlex commented Mar 5, 2018

Prerequisites

[ X] * Reproduced the problem in Safe Mode: https://flight-manual.atom.io/hacking-atom/sections/debugging/#using-safe-mode
[ X] * Followed all applicable steps in the debugging guide: https://flight-manual.atom.io/hacking-atom/sections/debugging/
[ X] * Checked the FAQs on the message board for common solutions: https://discuss.atom.io/c/faq
[ X] * Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=is%3Aissue+user%3Aatom
[ X] * Checked that there is not already an Atom package that provides the described functionality: https://atom.io/packages

Description

MD preview and MD datatips (hovers) don't support atom/core URL handlers from #15935. Would be useful if I want to provide source code links inside the datatip (hover)

Steps to Reproduce

  1. Create an MD file with a link similar to [This is a link](atom://core/open/file?filename=<path to file>)
  2. Open MD preview
  3. Click on the link - nothing happens. At the same time if one switches the URL to http://www.bbc.com the link would work

Expected behavior:
The file specified by its path open in Atom text editor

Actual behavior:
Core URLs not supported

Reproduces how often:
100%

Versions

All verions up to 1.25-beta2

Additional Details

Looking for directions... What to look at if I am to provide a patch for this

@BinaryMuse
Copy link
Contributor

BinaryMuse commented Mar 6, 2018

This is due to dompurify removing unsafe links in this block of code in markdown-preview:

render = (text, filePath, callback) ->
  roaster ?= require 'roaster'
  options =
    sanitize: false
    breaks: atom.config.get('markdown-preview.breakOnSingleNewline')


  # Remove the <!doctype> since otherwise marked will escape it
  # https://github.com/chjj/marked/issues/354
  text = text.replace(/^\s*<!doctype(\s+.*)?>\s*/i, '')


  roaster text, options, (error, html) ->
    return callback(error) if error?


    html = createDOMPurify().sanitize(html, {ALLOW_UNKNOWN_PROTOCOLS: atom.config.get('markdown-preview.allowUnsafeProtocols')})
    html = resolveImagePaths(html, filePath)
    callback(null, html.trim())

If you check "Allow Unsafe Protocols" in the markdown-preview options, atom:// links work correctly, but also allows other protocols you likely don't want enabled. Luckily, dompurify allows passing options to control this, so it shouldn't be too crazy to fix:

// allow external protocol handlers in URL attributes (default is false)
// by default only http, https, ftp, ftps, tel, mailto, callto, cid and xmpp are allowed.
var clean = DOMPurify.sanitize(dirty, {ALLOW_UNKNOWN_PROTOCOLS: true});

// allow specific protocols handlers in URL attributes (default is false)
// by default only http, https, ftp, ftps, tel, mailto, callto, cid and xmpp are allowed.
// Default RegExp: /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|cid|xmpp):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i;
var clean = DOMPurify.sanitize(dirty, {ALLOWED_URI_REGEXP: /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|cid|xmpp|xxx):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i;});

[Edit] This is only for the markdown-preview part of this bug; the datatips are handled separately.

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Mar 6, 2018

@BinaryMuse i think the setting ALLOW_UNKNOWN_PROTOCOLS and ALLOWED_URI_REGEXP only helps to properly set href attribute to the atom://... URL value. Is that correct?
I've tried modifying the DOM directly via dev tools and set the href attribute to: atom://core/open/file?filename=<path to file>. I think the result of this is the same as passing those 2 settings mentioned above. However, clicking on the link does nothing still.
Anything i need to set in Atom config for the atom core URLs to work within Atom? These URLs work when i try open atom:// URL in my OS, but not in Atom...

@BoykoAlex
Copy link
Contributor Author

@BinaryMuse hmm... maybe my atom core URL is in incorrect format. The URL I'm trying is: atom://core/open/file?filename=/Users/aboyko/Documents/Language-Server/demo/src/main/java/com/example/DemoApplication.java
The file name attribute being the absolute path to a file.
Does that look ok?

@BoykoAlex
Copy link
Contributor Author

I still can't get Atom core URLs to work from MD preview even after setting the prefernce Allow Unsafe Protocols. I see with the DOM inspector (DevTools) that href attribute has the right Atom coe URL value, but clicking on the link doesn't nothing...
Can you provide me a Atom Core URL that worked for you? Or an MD content that worked from the MD preview? (just need it as an example, I understand that file name/path would be different)

@UziTech
Copy link
Contributor

UziTech commented Mar 8, 2018

I was looking for the exact format too according to #15935 it's:

atom://core/open/file?filename=urlEncodedFileName&line=n&column=n

@BoykoAlex
Copy link
Contributor Author

@UziTech I have tried encoding the file path and it again only works from the OS, i.e. Mac OSX URL in the Text Editor, right-click Open URL - works.
However, if I'm to put this URL in an MD file in a link format it doesn't work from MarkDown preview in Atom UI (same goes for hovers/datatips). Does this work for you?

@UziTech
Copy link
Contributor

UziTech commented Mar 12, 2018

You are correct. It does seem to work with hyperlink-hyperclick.

After a bit of research it looks like the "Open link" explicitly only works with http/https links https://github.com/atom/link/blob/add8170e69c84e1ea438e22b6a023a32f6582eaf/lib/link.js#L28

And markdown-preview doesn't seem to work with anything other than http or https (I tried mailto://) Even with allowUnsafeProtocols set as @BinaryMuse suggested in #16888 (comment) , it seems atom would have to register the other protocols with electron.protocol.registerHttpProtocol to allow links to open with those protocols.

@BoykoAlex
Copy link
Contributor Author

@UziTech Thanks very much for the info! I think there is enough info for me to attempt a patch :-)

@UziTech
Copy link
Contributor

UziTech commented Mar 12, 2018

I added a PR for language-hyperlink to include atom:// URIs

atom/language-hyperlink#21

@BoykoAlex
Copy link
Contributor Author

Looks like I found the right solution for the problem. Wonder if anyone can merge this PR #16940 ?

@lee-dohm
Copy link
Contributor

@daviwil Would you mind checking this out?

@lock
Copy link

lock bot commented Mar 5, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. If you can still reproduce this issue in Safe Mode then please open a new issue and fill out the entire issue template to ensure that we have enough information to address your issue. Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants