[prettierx] Yarn audit issues in transitive dependencies

[brodybits]

• outdated postcss via outdated postcss-scss dependency - https://npmjs.com/advisories/1693 - no longer an issue with npm installation due to recent postcss@7.0.36 update; however PostCSS v8 update is needed since PostCSS v7 will no longer be supported ref: Advisory for v7 postcss/postcss#1574
• outdated trim via outdated remark-parse dependency - https://npmjs.com/advisories/1700 - now resolved by PR [WIP] prettierx-update-branch-001 - UPDATE WIP #593
• Yarn audit warnings from other outdated sub-dependencies in yarn.lock

should be resolved by proposed update from Prettier 2.3.1 in PR #569

Updates:

• It looks to me like the PostCSS dependencies are updated in the next branch of Prettier: Build(deps): Bump postcss-less from 3.1.4 to 4.0.0 prettier/prettier#9471, Update postcss-scss prettier/prettier#10490
• Unfortunately remark-parse is still outdated in the next branch of Prettier, and it is not a simple update without using @brodybits fork (see comment below).
• @ai generously patched PostCSS v7, which will not be supported any longer.

P.S. While I do not expect these issues to have much of any real-world impact in the near term, I would like to get these resolved asap.

/cc @adalinesimonian @aMarCruz @ai

[adalinesimonian]

I took a cursory glance at both postcss-scss and remark-parse. The upstream postcss fix is minor; we could easily backport it.

The remark-parse situation is not so simple, however — since the last version affected by the trim dependency issue, the internals have entirely changed, and there are breaking changes in the API. The update will have to take place upstream.

[brodybits]

The upstream postcss fix is minor; we could easily backport it.

Do you mean that you can update postcss-scss in your PR (PR #569) without breaking any failing too many tests?

If so, that would be really awesome.

I think you can imagine that I would love to avoid merging more updates from their next branch before they would at least make a pre-release. (Yes I did do this kind of merge before, though.)

The remark-parse situation is not so simple

Right, but at least they would be ahead of existing prettierX 0.18.1, maybe good enough.

[ai]

postcss-scss major update replace PostCSS 7 to PostCSS 8. New PostCSS has the same AST and supports PostCSS 7 plugins.

It is very likely that update will be safe. Maybe a few new warnings.

[adalinesimonian]

I dug deeper: simply updating postcss-scss to 3.x breaks tests and behaviour:

Summary of all failing tests
 FAIL  tests/format/scss/no-semicolon/jsfmt.spec.js
  ● url.scss › [scss] second format

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      @import ur<LF>
    -   l(; //fonts.googleapis.com/css?family=Open+Sans:400,400italic);<LF>
    +   l(;; //fonts.googleapis.com/css?family=Open+Sans:400,400italic);<LF>
      ↵

      365 |         expect(secondOutput).not.toEqual(firstOutput);
      366 |       } else {
    > 367 |         expect(secondOutput).toEqual(firstOutput);
          |                              ^
      368 |       }
      369 |     });
      370 |   }

      at Object.<anonymous> (tests/config/format-test.js:367:30)

  ● url.scss › [scss] compare AST

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -4,11 +4,11 @@
            "import": true,
            "name": "import",
            "params": Object {
              "type": "value-unknown",
              "value": "ur
    -   l(",
    +   l(;",
            },
            "type": "css-atrule",
          },
        ],
        "type": "css-root",

      380 |         expect(formattedAst).not.toEqual(originalAst);
      381 |       } else {
    > 382 |         expect(formattedAst).toEqual(originalAst);
          |                              ^
      383 |       }
      384 |     });
      385 |   }

      at Object.<anonymous> (tests/config/format-test.js:382:30)

 FAIL  tests/format/css/comments/jsfmt.spec.js
  ● selectors.css › format

    expect(received).toMatchSnapshot()

    Snapshot name: `selectors.css format 1`

    - Snapshot  - 5
    + Received  + 2

    @@ -384,13 +384,10 @@
      }

      /* custom properties set & @apply rule */
      :root {
        /* comments 192 */
    -   --centered /* comments 193 */ : /* comments 194 */ {
    -     display: flex;
    -     align-items: center;
    -     justify-content: center;
    -   }
    +   --centered/* comments 193 */ : /* comments 194 */ {display: flex; align-items:
    +     center; justify-content: center;};
      }

      ================================================================================

      336 |         CURSOR_PLACEHOLDER,
      337 |       })
    > 338 |     ).toMatchSnapshot();
          |       ^
      339 |   });
      340 |
      341 |   if (!FULL_TEST) {

      at Object.<anonymous> (tests/config/format-test.js:338:7)

The appropriate fixes were made upstream — but they are based on functionality in prettier/prettier#7933 that itself created breaking changes. As a result, as mentioned in prettier/prettier#9482 (comment), the update and related changes were reverted in prettier/prettier#9500. The implementation of the appropriate fixes upstream was delayed to 2.3 but got delayed again to 3.x (prettier/prettier#9584).

[ai]

l(; //fonts.googleapis.com/css?family=Open+Sans:400,400italic);

Is it a valid CSS?

[brodybits]

@adalinesimonian I am thinking we could just cherry-pick prettier/prettier#7933 for now, maybe with some special marker comments to avoid losing these changes when we finish the merge from upstream Prettier 2.3.1. And minor "breaking" changes are fine since prettierX is still pre-1.0.

@ai I hope you don't think we see an issue with PostCSS itself, we are dealing with the brittleness of this whole software fork. Thanks for your input.

[adalinesimonian]

l(; //fonts.googleapis.com/css?family=Open+Sans:400,400italic);

Is it a valid CSS?

Definitely not. In prettier/prettier#8675 (comment) it was decided that this semicolon is fine since the code is invalid anyway. I'm not sure why the second pass of format is adding an additional semicolon, however. As it stands this would result in prettier adding semicolons over and over again to an invalid line. I don't mind invalid code being invalidly formatted, but I would rather us not have compounding changes to files when formatting.

Nonetheless, this seems to be something on prettier/prettierX's end, not an issue with postcss.

@adalinesimonian I am thinking we could just cherry-pick prettier/prettier#7933 for now, maybe with some special marker comments to avoid losing these changes when we finish the merge from upstream Prettier 2.3.1. And minor "breaking" changes are fine since prettierX is still pre-1.0.

I agree that we don't have an issue with breaking changes at this stage in the project. At the moment, I'm trying to see if there's a simpler fix, first, before bringing in a larger number of changes from upstream.

[adalinesimonian]

Regarding the issue with the compounding semicolons, I've narrowed down the change in behaviour in the parser. Here's the input line:

@import ur
  l(//fonts.googleapis.com/css?family=Open+Sans:400,400italic);

The AST from postcss-scss 2.x is (with extra details redacted):

{
  "raws": { // semicolon is false even though line ends with semicolon
    "semicolon": false,
    "after": "//fonts.googleapis.com/css?family=Open+Sans:400,400italic);\n"
  },
  "type": "root",
  "nodes": [
    {
      "raws": {
        "before": "",
        "between": "",
        "afterName": " "
      },
      "type": "atrule",
      "name": "import",
      "params": "ur\n  l("
    }
  ]
}

The AST in 3.0.5 is identical.

semicolon is false, which leads prettier to add a semicolon after the presumed end of the node:

@import ur
  l(; //fonts.googleapis.com/css?family=Open+Sans:400,400italic);

The formatted line results in the following AST in postcss-scss 2.x:

{
  "raws": { // semicolon is now true
    "semicolon": true,
    "after": "\n"
  },
  "type": "root",
  "nodes": [
    {
      "raws": {
        "before": "",
        "between": "",
        "afterName": " "
      },
      "type": "atrule",
      "name": "import",
      "params": "ur\n  l("
    },
    { // the contents of the rule are now parsed as a node
      "raws": {
        "before": " ",
        "inline": true,
        "left": "",
        "right": "",
        "text": "fonts.googleapis.com/css?family=Open+Sans:400,400italic);"
      },
      "type": "comment",
      "text": "fonts.googleapis.com/css?family=Open+Sans:400,400italic);"
    }
  ]
}

However, the AST outputted by 3.0.5 differs:

{
  "raws": { // semicolon is false even though last child node ends with semi
    "semicolon": false,
    "after": " //fonts.googleapis.com/css?family=Open+Sans:400,400italic);\n"
  },
  "type": "root",
  "nodes": [
    {
      "raws": {
        "before": "",
        "between": "",
        "afterName": " "
      },
      "type": "atrule",
      "name": "import",
      "params": "ur\n  l(;"  // last child does have a semicolon
    }
  ]
}

No matter how many times you add a semicolon, semicolon stays false. This leads to circular behaviour in prettier, adding semicolons every time the file is printed.

The line that adds the unwanted semicolon in this case is https://github.com/prettier/prettier/blob/2.3.1/src/language-css/printer-postcss.js#L304 if anyone wants to take a look at it. I'm signing off for the day.

[brodybits]

I am now thinking to move the PostCSS and remark-parse into optional peer dependencies until they get these issues resolved in a stable release. While these security issues would not very likely affect very many users, I would rather publish a release clean from these issues if possible.

[brodybits]

Many thanks to @ai for updating PostCSS v7. I hope Prettier will make a stable version with prettier/prettier#9584 (PostCSS updated to v8) in the near future.

Side question to @adalinesimonian and @ai if you both speak Russian? I do not but have met many Russian speakers over the past couple decades.

[ai]

Я говорю ☺️

[adalinesimonian]

I am now thinking to move the PostCSS and remark-parse into optional peer dependencies until they get these issues resolved in a stable release. While these security issues would not very likely affect very many users, I would rather publish a release clean from these issues if possible.

remark-parse drives all markdown functionality - if we make it optional, I doubt that almost anyone would opt to not install it, seeing as virtually every JS project has at least a few markdown files lying around. That said, I'm not opposed to putting it into peer deps; instead, I'm not confident that it would change much practically. Do we have any metrics on the use of prettierX that would be useful?

Many thanks to @ai for updating PostCSS v7. I hope Prettier will make a stable version with prettier/prettier#9584 (PostCSS updated to v8) in the near future.

@ai rocks, thank you so much for the one-time exception. I'm looking forward to Prettier updating to 8.x so that we can pull down their changes into this fork and resolve this issue for good — this certainly won't be the last time a postcss dependency has a vulnerability.

Side question to @adalinesimonian and @ai if you both speak Russian? I do not but have met many Russian speakers over the past couple decades.

Да, знаю русский! Funny enough, yes — I grew up in a multi-lingual household; I've got middle eastern and white ancestry. The common language between my parents was Russian, so I grew up with it.

[brodybits]

😄 My mothers family which is Jewish left Lithuania in the 1800s which I think you know was part of the Russian empire at that time :)

---

I think I have good news for prettierX:

• Merging in a single change from Prettier's next branch in prettier/prettier@8616182 will give us the update to PostCSS version 8.
• I have published @brodybits/remark-parse (version 8.0.5) which uses updated trim to resolve the warning from https://npmjs.com/advisories/1700.

[adalinesimonian]

😄 My mothers family which is Jewish left Lithuania in the 1800s which I think you know was part of the Russian empire at that time :)

It's a small world out there; my mom's family is Jewish and Russian.

I think I have good news for prettierX:

• Merging in a single change from Prettier's next branch in prettier/prettier@8616182 will give us the update to PostCSS version 8.
• I have published @brodybits/remark-parse (version 8.0.5) which uses updated trim to resolve the warning from https://npmjs.com/advisories/1700.

Sounds great! I'm a bit occupied this week, but I'll have a little bit of time tomorrow morning and Thursday/Friday morning to be available to work on any aspect of the project. Let me know how I can help, and I'll be glad to do it.

[brodybits]

Shalom @adalinesimonian that is so cool:)

I have now switched to @brodybits/remark-parse fork of remark-parse in PR #598 to resolve the outdated trim dependency. There are some other Yarn audit warnings coming from outdated sub-dependencies in yarn.lock which should never affect installation from either npm or GitHub once I make upcoming patch release 0.18.2.

I did try regenerating yarn.lock but discovered this would lead to some GitHub test failures.

I am now thinking it would be most straight-forward to make the 0.18.2 patch release then resolve the remaining Yarn audit issues after merging updates from Prettier 2.3.1 as proposed in PR #569.

Let me know how I can help, and I'll be glad to do it.

Thanks, I will comment in PR #569.

[adalinesimonian]

Happy to report that in #569:

╭─adaline@ultima-weapon ~/github/brodybits/prettierx  ‹adalinesimonian/merge-prettier-2.3.1›
╰─➤  yarn audit
yarn audit v1.22.10
0 vulnerabilities found - Packages audited: 1496
Done in 0.68s.

[brodybits]

Yeah thanks!!

I have made the new release with updates from Prettier 2.3.2, with many of the updates proposed in PR #603.

I sincerely hope that they will find a way to resolve these warnings in the upstream Prettier repository.

[brodybits]

Just to clarify:

• prettierX 0.19.0 includes some updates from Prettier to start using PostCSS 8
• prettierX is now using @brodybits/remark-parse 8.x, with updated trim sub-dependency, as a workaround until they update the upstream Prettier code base to use remark-parse 0