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

Any def string in code block prevents block detection #1006

Closed
rndstr opened this issue Jan 8, 2018 · 19 comments
Closed

Any def string in code block prevents block detection #1006

rndstr opened this issue Jan 8, 2018 · 19 comments
Labels
category: code blocks help wanted L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue

Comments

@rndstr
Copy link

rndstr commented Jan 8, 2018

When there is a def at the beginning of any line within a code block, it is not recognized as such.

Sample inputs

A

`
def`

B

```ruby
def foo(x)
  return 3
end
```

Expectation

The code block should be recognized as code block.

Result

The code block is not recognized and just rendered as paragraphs.

A

<p>`</p>
    <p>def
    `</p>

B

<p>`
    ruby</p>
    <p>def foo(x)
      return 3
    end
    `</p>

More information

This broke by commit 98ac7a4
which seemed to have originated in #974
and was supposed to fix #465

@joshbruce joshbruce added L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue category: code blocks help wanted labels Jan 8, 2018
@joshbruce joshbruce added this to the 0.4.0 - No known defects milestone Jan 8, 2018
@Feder1co5oave
Copy link
Contributor

I think we need to revert and review #974 as soon as possible. I don't recall how that could have slipped my checking.
This can be quite tricky since we changed the whole test system, so expect to manually resolve some conflicts.

@KostyaTretyak
Copy link
Contributor

KostyaTretyak commented Jan 8, 2018

Agree, we should to revert 98ac7a4 and a477d1d commits. They are incorrect.

@joshbruce
Copy link
Member

joshbruce commented Jan 8, 2018

@Feder1co5oave and @KostyaTretyak: Agreed.

I know of three ways to do that - and seem to only be able to do one of them myself. (Note: Believe it or not, I've never had to do a reversion.)

  1. Copy-paste and submit a new PR...not revert.
  2. https://help.github.com/articles/reverting-a-pull-request/ - in theory, there should be a button at the bottom of the PRs to allow reversion - there is not.
  3. Using git via terminal - https://git-scm.com/docs/git-revert.html - not comfortable enough to do that one myself for a live project in production.

@KostyaTretyak
Copy link
Contributor

@joshbruce, please do the following via terminal from root the project:

git checkout master
git revert a477d1d --no-edit
git push origin master

It should go through without problems. And now I will pull request for revert 98ac7a4.

@Feder1co5oave
Copy link
Contributor

@KostyaTretyak can you not open a PR to revert both commits? That would be neater.

@KostyaTretyak
Copy link
Contributor

Well, I fixed my mistake, but now you need to fix the tests.

@KostyaTretyak
Copy link
Contributor

KostyaTretyak commented Jan 8, 2018

Interestingly, CommonMark Spec have such a rule, but it seems that in our case it does not work for some reason (see tab HTML and [1]: foo at bottom).

> hello
> [1]: hello

* * *

> hello
[2]: hello


* hello
* [3]: hello


* hello
[4]: hello


> foo
> bar
[1]: foo
> bar

Output:

<blockquote>
<p>hello
[1]: hello</p>
</blockquote>
<hr />
<blockquote>
<p>hello
[2]: hello</p>
</blockquote>
<ul>
<li>
<p>hello</p>
</li>
<li></li>
<li>
<p>hello
[4]: hello</p>
</li>
</ul>
<blockquote>
<p>foo
bar
[1]: foo
bar</p>
</blockquote>

@KostyaTretyak
Copy link
Contributor

KostyaTretyak commented Jan 8, 2018

And this is not works too

[foo]

> first row
> [foo]: /url
> third row

Output:

<p>[foo]</p>
<blockquote>
<p>first row
[foo]: /url
third row</p>
</blockquote>

@Feder1co5oave
Copy link
Contributor

It's because link definitions cannot interrupt a paragraph.

@Feder1co5oave
Copy link
Contributor

WAIT

I cannot reproduce the issue reported by @rndstr

$ bin/marked
```ruby
def foo(x)
  return 3
end
```
^D
<pre><code class="lang-ruby">def foo(x)
  return 3
end
</code></pre>

Guys, this is about codeblocks, #974 was about blockquotes, very different beasts.

@Feder1co5oave
Copy link
Contributor

Whereas this is a problem:

$ bin/marked
`
def ?`

`
ciao?`

^D

<p>`</p>
<p>def ?`</p>
<p><code>ciao?</code></p>

@Feder1co5oave
Copy link
Contributor

Looks like this is a problem with the paragraph block rule.

@joshbruce
Copy link
Member

joshbruce commented Jan 8, 2018

All right, I'm gonna just hold off until we answer a couple of things:

  • This issue is unrelated to the commits being reverted (a new issue).
  • Fixing this is something we should correct before performing the 0.3.10 release, which includes Declare undeclared variables #991, which is related to the highest priority XSS-related defects.

@KostyaTretyak
Copy link
Contributor

This issue is unexpected related to #1007.

@Feder1co5oave
Copy link
Contributor

Yes it is correlated, because regexp are built via a cascade of replaces. By changing the blockquote regexp, the paragraph one is now changed too, and it breaks

`
def`

into two separate paragraphs, so that the code inline rule cannot parse correctly the item.
I'm working around this issue. Still, my recent review of your #974 is still very valid and we need to take a look at the blockquote rule too, which is one of the most complicated ones, alas.

@KostyaTretyak
Copy link
Contributor

Still, my recent review of your #974 is still very valid

Agree. I did #974 when I studied marked just a couple of days.

@KostyaTretyak
Copy link
Contributor

However, in two of your three comments, you were wrong with the rules. Now we know that my test edits were correct, but I was incorrect when I deleted rule with blockquote from marked.js.

@rndstr
Copy link
Author

rndstr commented Jan 9, 2018

Okay, so #1007 and 0.3.12 brought this back to the pre-0.3.9 behaviour. Thanks for the quick release!

Not sure I follow your discussion, feel free to close this (and re-open whatever #974 was meant to fix).

@joshbruce
Copy link
Member

@rndstr: Thanks for the information. Gonna go ahead and close this per your comment.

Think we've completed all the issues related to XSS vulnerabilities; so, we can get to the other parser issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: code blocks help wanted L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue
Projects
No open projects
Development

No branches or pull requests

4 participants