Replace html-parse-stringify2 with html-parse-stringify #1283

kachkaev · 2021-03-19T19:07:13Z

Checklist

only relevant code is changed (make a diff before you submit the PR)
run tests npm run test
~~tests are included~~
~~documentation is changed or added~~

kachkaev · 2021-03-19T21:18:27Z

The upstream issue is fixed, but a bunch of tests for <Trans /> fail now. Example:

FAIL  test/trans.render.list.spec.js (28.115s)
  ● Trans should render nested components › should render dynamic ul as components property

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `Trans should render nested components should render dynamic ul as components property 1`

    - Snapshot
    + Received

    - <div>
    -   Result should be a list: 
    -   <ul>
    -     <li>
    -       li1
    -     </li>
    -     <li>
    -       li2
    -     </li>
    -   </ul>
    - </div>
    + <div />

Any thoughts?

PS: If it’s easier to fix this than to explain me where to dig, happy for my commits to be recycled in another PR 🙂

adrai · 2021-03-20T07:49:24Z

Practically all Trans tests are failing...
I suspect the generated ast looks completely different.... https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L136

adrai · 2021-03-21T16:51:51Z

rayd/html-parse-stringify2#27

kachkaev · 2021-03-22T10:47:18Z

@adrai thanks for your alternative solution! To increase our chances for a fast resolution, do you know what I could change in this PR for the tests to pass? I haven’t used html-parse-stringify or html-parse-stringify2 before, so am not familiar with their differences or expectations. Intuitively, it feels better to return back to html-parse-stringify from the fork.

jamuhl · 2021-03-22T11:07:19Z

@kachkaev guess you have to write new snapshots -> and manually check first if received output still matches expected and the changes are pure formatting and not wrong html

kachkaev · 2021-03-22T14:42:02Z

Hmmm here’s the catch. When we call:

const ast = HTML.parse(`<0>${interpolatedString}</0>`);

using html-parse-stringify@2.0.3, the result is an empty array. This is because of:

const tagRE = /<[a-zA-Z\-\!\/](?:"[^"]*"|'[^']*'|[^'">])*>/g

Thus, tags like <0>, <1> etc. are ignored.

If rayd/html-parse-stringify2#27 is merged as is, we’ll end up with the same problem in html-parse-stringify2 too.

adrai · 2021-03-22T17:06:59Z

PR rayd/html-parse-stringify2#27 is adapted

kachkaev · 2021-03-22T18:07:12Z

Two blockers on the path to html-parse-stringify:

stale · 2021-03-29T21:53:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kachkaev · 2021-03-29T22:13:47Z

bump

kachkaev · 2021-04-01T19:04:44Z

@HenrikJoreteg has just released html-parse-stringify@2.1.0, which is great! The new version includes HenrikJoreteg/html-parse-stringify#43 so we are half-way there!

Once HenrikJoreteg/html-parse-stringify#41 is merged and released, all tests should be passing and we’ll be able to switch from html-parse-stringify2 to html-parse-stringify 👍

kachkaev · 2021-04-01T19:32:19Z

I went into my local nodes_modules and manually tweaked html-parse-stringify/dist code so that it included HenrikJoreteg/html-parse-stringify#41. Interestingly, I noticed that some snapshots still ended up failing, so I updated them. See diff.

Is this a regression or just an artifact in snapshots? 🤔 If it is a regression, is it something to fix here or upstream? Potentially, we might have a lost space between </closingTag> <openingTag>.

adrai · 2021-04-01T19:36:58Z

Is this a regression or just an artifact in snapshots? 🤔 If it is a regression, is it something to fix here or upstream? Potentially, we might have a lost space between </closingTag> <openingTag>.

No errors on master:

kachkaev · 2021-04-01T19:45:39Z

No errors on master is an expected thing, because in there we’ve got "html-parse-stringify2": "^2.0.1". This PR replaces that dependency with html-parse-stringify, which either has a slightly different shape of an AST or misses a space in an AST in the first place. I don’t have a solution on top of my head right now, so if anyone could do some extra digging, that’d be great!

Where I’m going is that we might need to either submit an extra patch to html-parse-stringify in addition to HenrikJoreteg/html-parse-stringify#41 or maybe amend AST post-processing here.

rayd/html-parse-stringify2#27 is still a good alternative solution and I’m happy with any that can land first. The main goal here is to close #1275 🙂

coveralls · 2021-04-01T22:03:19Z

Coverage remained the same at 96.205% when pulling c85d72a on kachkaev:cve-2021-23346 into 4c005be on i18next:master.

kachkaev · 2021-04-01T22:07:53Z

Thanks to @HenrikJoreteg’s work on merging HenrikJoreteg/html-parse-stringify#41 and releasing of 2.1.1, CI is finally green now! However, for the tests to work I had to remove extra new lines from the snapshots. We need to investigate this.

My gut feeling is that there’s another small bug in html-parse-stringify, in particular to do with treating </a>  as </a>. The alternative cause could be related to AST handling in react-i18next (which would be easier to fix).

To confirm or deny the above hypotheses we’ll need to add some extra test cases upstream. I’m not sure when exactly I’ll be able to do this, so if anyone wants to help, feel free to 👍

We are much closer to publishing this PR than we were a couple of days ago, so thanks to everyone who is involved 🙌

HenrikJoreteg · 2021-04-02T00:19:44Z

If you could give me a sample of the failing input I might be able to look into it.

…

On Thu, Apr 1, 2021, 15:08 Alexander Kachkaev ***@***.***> wrote: Thanks to @HenrikJoreteg <https://github.com/HenrikJoreteg>’s work on merging HenrikJoreteg/html-parse-stringify#41 <HenrikJoreteg/html-parse-stringify#41> and releasing of 2.1.1, we have CI passing now! However, for this to work I had to remove extra new lines from the snapshots, which we’ll need to investigate. My gut feeling is that there’s another small bug in html-parse-stringify, in particular to do with treating </a> as </a>. The alternative cause could be to do with some AST handling here (which would be easier to fix). To confirm or deny the above hypotheses we’ll need to add some extra test cases upstream. I’m not sure when exactly I’ll be able to do this, so if anyone wants to help, feel free to 🙌 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1283 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQSVLEMYATPANK6RHJYLDTGTVENANCNFSM4ZPLDDCA> .

adrai · 2021-04-02T06:24:33Z

@HenrikJoreteg I had a quick look at the generated ast of this failing test: https://github.com/i18next/react-i18next/blob/master/test/trans.render.spec.js#L129

Just a simple console.log(JSON.stringify(ast, null, 2)) here: https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L137

The output of html-parse-stringify differs from html-parse-stringify2 in the following way:

This text element is missing in html-parse-stringify before the br element:

      {
        "type": "text",
        "content": " "
      }

html-parse-stringify2:

[
    {
        "type": "tag",
        "name": "0",
        "voidElement": false,
        "attrs": {},
        "children": [
            {
                "type": "tag",
                "name": "strong",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "Go"
                    }
                ]
            },
            {
                "type": "text",
                "content": " "
            },
            {
                "type": "tag",
                "name": "br",
                "voidElement": true,
                "attrs": {},
                "children": []
            },
            {
                "type": "tag",
                "name": "1",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "there"
                    }
                ]
            },
            {
                "type": "text",
                "content": "."
            }
        ]
    }
]

html-parse-stringify:

[
    {
        "type": "tag",
        "name": "0",
        "voidElement": false,
        "attrs": {},
        "children": [
            {
                "type": "tag",
                "name": "strong",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "Go"
                    }
                ]
            },
            {
                "type": "tag",
                "name": "br",
                "voidElement": true,
                "attrs": {},
                "children": []
            },
            {
                "type": "tag",
                "name": "1",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "there"
                    }
                ]
            },
            {
                "type": "text",
                "content": "."
            }
        ]
    }
]

kachkaev · 2021-04-06T09:19:26Z

Hi @pavanjava! The goal of this PR is to mitigate CVE-2021-23346 – you are right about it. The ETA for a new react-i18next release depends on how fast we will resolve this bug in html-parse-stringify: #1283 (comment). Feel free to help!

adrai · 2021-04-06T13:47:03Z

Tried to create a PR: HenrikJoreteg/html-parse-stringify#45
Really not sure if this is the correct way to do so...
this in combination with passing { respectWhitespace: true } in react-i18next:

could fix the tests...
But honestly I do not even know if having or not having that whitespace matters?

kachkaev · 2021-04-06T13:49:10Z

Tried to create a PR

Great stuff, thanks @adrai!

I do not even know if having or not having that whitespace matters?

I guess it might matter. If we have hello world and the whitespace token is removed, we end up with helloworld.

alon24 · 2021-04-11T08:51:51Z

any news?

HenrikJoreteg · 2021-04-11T15:07:38Z

I think I'm the hangup, i'm trying to sort out if it should be implemented as an option (as was done) in the PR or if it should just be the normal behavior to always preserve it. I'd rather not make it an option unless we can think of a reason why anyone would want to not have the whitespace.

kachkaev · 2021-04-11T15:28:31Z

I second an option that avoids having options. If anyone wants to not have space between two tags, they can remove it in the source string 🙂

adrai · 2021-04-11T15:37:14Z

I second an option that avoids having options. If anyone wants to not have space between two tags, they can remove it in the source string 🙂

new major version of html-parse-stringify ;-) HenrikJoreteg/html-parse-stringify#45 (comment)

adrai · 2021-04-12T15:00:44Z

Good news: HenrikJoreteg/html-parse-stringify#46 (comment)

adrai · 2021-04-12T15:02:55Z

@kachkaev you can choose:

Do you want to update this PR?
I'll do it directly on master and create a new version?

kachkaev · 2021-04-12T15:08:57Z

Great news! 🎉 Happy to update the PR shortly 🙌

kachkaev · 2021-04-12T15:15:23Z

adrai · 2021-04-12T15:19:25Z

patch?

adrai · 2021-04-12T15:23:20Z

🎉🎉🎉🎉🎉🎉 PUBLISHED v11.8.13 🎉🎉🎉🎉🎉🎉🎉

kachkaev mentioned this pull request Mar 19, 2021

Is react-i18next vulnerable to CVE-2021-23346 ? #1275

Closed

This was referenced Mar 22, 2021

Support numeric tags HenrikJoreteg/html-parse-stringify#43

Merged

Add broken tests for multiple nested tags HenrikJoreteg/html-parse-stringify#44

Closed

stale bot added the stale label Mar 29, 2021

stale bot removed the stale label Mar 29, 2021

adrai added the pr hold label Mar 30, 2021

kachkaev added 3 commits April 1, 2021 19:58

Replace html-parse-stringify2 with html-parse-stringify

08987f4

Bump html-parse-stringify to 2.0.3

8b932ba

Bump html-parse-stringify to 2.1.0

402880b

Update snaphots

2576842

Bump html-parse-stringify to 2.1.1

2462b31

SeinopSys mentioned this pull request Apr 6, 2021

New Security Vulnarability is detected in the library CVE-2021-23346 rayd/html-parse-stringify2#28

Closed

adrai mentioned this pull request Apr 6, 2021

introduce respectWhitespace parse option HenrikJoreteg/html-parse-stringify#45

Closed

adrai mentioned this pull request Apr 11, 2021

MAJOR: respect whitespaces HenrikJoreteg/html-parse-stringify#46

Merged

kachkaev added 3 commits April 12, 2021 16:11

Revert changes in tests

b1f88b4

Bump html-parse-stringify to v3.0.0

d0fa8e0

Revert one more test file

c85d72a

kachkaev marked this pull request as ready for review April 12, 2021 15:14

adrai merged commit 4b9b8c5 into i18next:master Apr 12, 2021

kachkaev deleted the cve-2021-23346 branch April 12, 2021 15:19

si1k mentioned this pull request Apr 19, 2021

CVE-2021-23346 (Medium) detected in html-parse-stringify2-2.0.1.tgz Path-Check/gaen-mobile#883

Closed

joekur mentioned this pull request Apr 21, 2021

Trans - reserved component keys #1304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace html-parse-stringify2 with html-parse-stringify #1283

Replace html-parse-stringify2 with html-parse-stringify #1283

kachkaev commented Mar 19, 2021 •

edited

Loading

kachkaev commented Mar 19, 2021 •

edited

Loading

adrai commented Mar 20, 2021

adrai commented Mar 21, 2021

kachkaev commented Mar 22, 2021

jamuhl commented Mar 22, 2021

kachkaev commented Mar 22, 2021 •

edited

Loading

adrai commented Mar 22, 2021

kachkaev commented Mar 22, 2021

stale bot commented Mar 29, 2021

kachkaev commented Mar 29, 2021

kachkaev commented Apr 1, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

adrai commented Apr 1, 2021

kachkaev commented Apr 1, 2021 •

edited

Loading

coveralls commented Apr 1, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

HenrikJoreteg commented Apr 2, 2021 via email

adrai commented Apr 2, 2021 •

edited

Loading

kachkaev commented Apr 6, 2021 •

edited

Loading

adrai commented Apr 6, 2021

kachkaev commented Apr 6, 2021 •

edited

Loading

alon24 commented Apr 11, 2021

HenrikJoreteg commented Apr 11, 2021

kachkaev commented Apr 11, 2021

adrai commented Apr 11, 2021

adrai commented Apr 12, 2021

adrai commented Apr 12, 2021

kachkaev commented Apr 12, 2021

kachkaev commented Apr 12, 2021

adrai commented Apr 12, 2021

adrai commented Apr 12, 2021

Replace html-parse-stringify2 with html-parse-stringify #1283

Replace html-parse-stringify2 with html-parse-stringify #1283

Conversation

kachkaev commented Mar 19, 2021 • edited Loading

Checklist

kachkaev commented Mar 19, 2021 • edited Loading

adrai commented Mar 20, 2021

adrai commented Mar 21, 2021

kachkaev commented Mar 22, 2021

jamuhl commented Mar 22, 2021

kachkaev commented Mar 22, 2021 • edited Loading

adrai commented Mar 22, 2021

kachkaev commented Mar 22, 2021

stale bot commented Mar 29, 2021

kachkaev commented Mar 29, 2021

kachkaev commented Apr 1, 2021 • edited Loading

kachkaev commented Apr 1, 2021 • edited Loading

adrai commented Apr 1, 2021

kachkaev commented Apr 1, 2021 • edited Loading

coveralls commented Apr 1, 2021 • edited Loading

kachkaev commented Apr 1, 2021 • edited Loading

HenrikJoreteg commented Apr 2, 2021 via email

adrai commented Apr 2, 2021 • edited Loading

kachkaev commented Apr 6, 2021 • edited Loading

adrai commented Apr 6, 2021

kachkaev commented Apr 6, 2021 • edited Loading

alon24 commented Apr 11, 2021

HenrikJoreteg commented Apr 11, 2021

kachkaev commented Apr 11, 2021

adrai commented Apr 11, 2021

adrai commented Apr 12, 2021

adrai commented Apr 12, 2021

kachkaev commented Apr 12, 2021

kachkaev commented Apr 12, 2021

adrai commented Apr 12, 2021

adrai commented Apr 12, 2021

kachkaev commented Mar 19, 2021 •

edited

Loading

kachkaev commented Mar 19, 2021 •

edited

Loading

kachkaev commented Mar 22, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

coveralls commented Apr 1, 2021 •

edited

Loading

kachkaev commented Apr 1, 2021 •

edited

Loading

adrai commented Apr 2, 2021 •

edited

Loading

kachkaev commented Apr 6, 2021 •

edited

Loading

kachkaev commented Apr 6, 2021 •

edited

Loading