Skip to content

When ammonia supports allowing only certain values for attributes, re-enable syntax highlighting #1008

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

Closed
3 tasks
carols10cents opened this issue Aug 24, 2017 · 5 comments
Labels
A-readme C-bug 🐞 Category: unintended, undesired behavior E-has-mentor

Comments

@carols10cents
Copy link
Member

carols10cents commented Aug 24, 2017

We had to disable classes on code tags because you could use arbitrary classes to do arbitrary things. So for now, syntax highlighting in readmes is disabled.

I've filed an issue with Ammonia to request being able to specify allowed values for attributes (or at least classes); these are the classes that marky-markdown (used by npm) allows for their README rendering.

Instructions for implementing:

  • We've already updated to ammonia 0.7.0, which now has support for this!
  • Add the allowed_classes field to the ammonia configuration struct with a hash map that contains the tag name "code" and a value of a HashSet containing all the languages we want to support highlighting prefixed by language-, so language-bash, language-clike, etc.
  • Add a test in here like these tests that calls markdown_to_html with a code block that should get syntax highlighting, and assert that the code tag has class="language-whatever"
  • Add another test in there that tries to add classes that aren't allowed to a code element, or other classes to other elements, and assert that they aren't in the rendered HTML
  • BONUS: I'm happy to test this for real, but if you're feeling ambitious, publish a crate that has a README with some code in it to your local crates.io instance with the code change (instructions on publishing to your own instance are here) and make sure it is highlighted.
@notriddle
Copy link
Contributor

notriddle commented Sep 12, 2017

Ammonia supports it now.

@carols10cents carols10cents added this to the impl period milestone Sep 15, 2017
@carols10cents carols10cents removed this from the impl period milestone Sep 15, 2017
@treiff
Copy link
Contributor

treiff commented Sep 20, 2017

I'd like to give this a shot. I'll let you know if I have any questions, thanks!

treiff added a commit to treiff/crates.io that referenced this issue Sep 21, 2017
- Added `code` tag and corresponding `class` attribute to the whitelisted `tag_attributes`.
- Added `allowed-classes` for the `code` tag.
treiff added a commit to treiff/crates.io that referenced this issue Sep 21, 2017
- Added `code` tag and corresponding `class` attribute to the whitelisted `tag_attributes`.
- Added `allowed-classes` for the `code` tag.
@treiff
Copy link
Contributor

treiff commented Sep 21, 2017

Hi @carols10cents, quick question on this. I've pretty much wrapped up all the checklist items and had published a create locally with some syntax highlighting. That said, when I view the crate details the README.md I created is not displayed and 404's.

The URL it's trying to reach is http://localhost:4200/readmes/hello/hello-0.1.0.html (hello is the name of my test crate obviously), and the server response is 404 with {"errors":[{"detail":"Not Found"}]}

Any thought's as to why the readme won't render? I'm suspecting possibly something with S3?

@carols10cents
Copy link
Member Author

Hi @treiff! Have you set up an s3 bucket locally, and put S3 config vars in your .env? If not, the files should be "uploaded" to your crates.io directory and served from there. When you run cargo run --bin server, do you see this?

$ cargo run --bin server
    Finished dev [unoptimized + debuginfo] target(s) in 0.2 secs
     Running `target/debug/server`
Using local uploader, crate files will be in the local_uploads directory

If so, that means we can rule out s3 being the problem :)

Where you have crates.io's code checked out, can you look in local_uploads/readmes? Is there a directory in there for hello, and is there a hello-0.1.0.html file in there?

If that file does exist, then for some reason the server isn't seeing that. We can poke around in https://github.com/rust-lang/crates.io/blob/264dd1a53d5055693a44654aad006cb6cf68bf7f/src/local_upload.rs to see where it might be going wrong.

If that file doesn't exist, then the readme HTML isn't getting created/"uploaded" on crate publish :( The rendering happens here, which calls this markdown_to_html function. The "uploading" happens here which calls this upload method.

We'd need to try publishing more crate versions and narrowing down where that might be failing.

Let me know if you'd like to jump on a screensharing session or something to figure this out in real time!

@treiff
Copy link
Contributor

treiff commented Sep 21, 2017

Thanks @carols10cents ! Okay, sounds like we can rule out S3.

I didn't ever setup a local bucket and can confirm that when running cargo run --bin server, I receive the local upload message.

Using local uploader, crate files will be in the local_uploads directory

Also, interestingly I don't have the readmes directory within local_uploads. My directory structure looks like this (run from root of crates.io directory):

→ tree local_uploads                                                                                                                                                                                                                                                                                                [dc108ff]
local_uploads
└── crates
    └── hello
        └── hello-0.1.0.crate

2 directories, 1 file

So based on that, I guess we're looking into the readme HTML not getting created/uploaded when I published the crate. I can experiment bumping the version and republishing while adding some debugging statements within the related rendering and uploading functions.

I'll have another look at this tonight and see how I make out, if i'm still having issues then I'd say setting up a screen share would be great! Thanks for your help!

EDIT

Got it! It helps if you add readme = "README.md" within the Cargo.toml 🤦‍♂️ , should have a PR up tonight.

bors-voyager bot added a commit that referenced this issue Sep 23, 2017
1070: Re-enable syntax highlighting. r=carols10cents

Syntax highlighting seems to be working as intended now.  I tested by publishing a create locally with a `README.md` including a couple of code samples, see below for a screenshot. Thanks for the help, and please let me know if you have any questions.

Closes #1008 

<img width="963" alt="screen shot 2017-09-21 at 1 07 28 pm" src="https://user-images.githubusercontent.com/5225538/30719271-d79969e8-9ef0-11e7-878d-064a62a4dbeb.png">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-readme C-bug 🐞 Category: unintended, undesired behavior E-has-mentor
Projects
None yet
Development

No branches or pull requests

3 participants