Skip to content

Conversation

@lkappeler
Copy link
Contributor

@lkappeler lkappeler commented Jan 19, 2018

Description

I tried to uptade ejs to version 2.5.7 to get rid of the security warnings mentioned in the connected issue.
Currently the tests are failing. The suggested linter does not give any hints.
Read the issue conversation for further information.

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@lkappeler lkappeler requested a review from bajtos as a code owner January 19, 2018 15:21
@slnode
Copy link

slnode commented Jan 19, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos
Copy link
Member

bajtos commented Jan 22, 2018

@slnode test please

@bajtos
Copy link
Member

bajtos commented Jan 22, 2018

@lkappeler thank you for the pull request. First of all, please sign our Contributor License Agreement: https://cla.strongloop.com/agreements/strongloop/loopback-sdk-angular

// angular.module('app', [lbServices]);
//
module.exports = <%-: moduleName | q %>;
module.exports = <%-: helpers.quotedString(moduleName) %>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to replace all occurences of <%-: with <%-. IIRC, the : suffix was enabling filters via the | operator. AFAICT, it's no longer available in the latest EJS version (see e.g. here).

Could you please try to make this change and see if it fixes the problem you are observing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already tried that (#280 (comment)) without any changes in the behavior.

Copy link
Contributor Author

@lkappeler lkappeler Jan 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message has now changed to Cannot generate services script: SyntaxError: missing ) after argument list while compiling ejs. The linter still doesn't give my any output.
In the documentation is described that the linter ignores other tags than:

EJS-Lint parses scriptlet tags (<%, %>, <%_, _%>, and -%>). It ignores all other tags (i.e. <%=).

https://github.com/ryanzim/ejs-lint

@bajtos
Copy link
Member

bajtos commented Jan 23, 2018

Looks like the new EJS is choking on the following syntax:

<% } else { params.forEach(function(arg) { -%>

I fixed the problem together with few others I discovered along the way, see e4182ad 9097bc3

Please let me know if you are ok with the result, so that I can clean up git history and proceed with landing this patch. Perhaps test the new version on your project, to make sure we are still generating a good client code?

@bajtos bajtos force-pushed the bugfix/update-ejs branch from e4182ad to 9097bc3 Compare January 23, 2018 13:49
@lkappeler
Copy link
Contributor Author

Thanks!, the code looks fine to me. I'm quiet busy right not, will test it with our application as soon as I'll find some time.

Get rid of warnings reported by nsp/snyk for a security vulnerability
in the older version of ejs (note that we are not affected by it).
@bajtos bajtos force-pushed the bugfix/update-ejs branch from 9097bc3 to d37d9fe Compare January 25, 2018 07:01
@bajtos bajtos merged commit b891f43 into strongloop:master Jan 25, 2018
@bajtos
Copy link
Member

bajtos commented Jan 25, 2018

Landed 🎉 Thank you for the contribution! ❤️

@bajtos
Copy link
Member

bajtos commented Jan 25, 2018

Released in loopback-sdk-angular@3.3.0

@slnode
Copy link

slnode commented Jan 25, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants