Skip to content

Added big libraries to safelist#21484

Merged
armanio123 merged 6 commits into
microsoft:masterfrom
armanio123:AddBigLibrariesToSafeList
Jan 31, 2018
Merged

Added big libraries to safelist#21484
armanio123 merged 6 commits into
microsoft:masterfrom
armanio123:AddBigLibrariesToSafeList

Conversation

@armanio123
Copy link
Copy Markdown
Contributor

Added library datatables.net for the safelist. Check https://www.datatables.net/ for documentation about the library.

Comment thread src/server/typesMap.json Outdated
"exclude": [["^", 1, "$"]]
},
"Datatables.net": {
"match": "(jquery\\.)?dataTables(\\.all)?(\\.min)?\\.js$",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This RegExp would also match mydataTables.js. There's a reason all the others uses a ^.*\/-like pattern at the start, please do similar.

Also note all the other patterns are lower-cased. I believe the path is normalized to lowercase before the regexp is applied. Please follow this pattern also, else it may not match due to casing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Notice that the jquery library is also missing that same pattern. Let me know if that is expected or should be fixed.

Comment thread src/server/editorServices.ts Outdated
"Datatables.net": {
// e.g. /wwwroot/lib/datatables.all.min.js
match: /(jquery\.)?dataTables(\.all)?(\.min)?\.js$/i,
types: ["datatables.net"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to add this to the baked in default safelist also. It's not that common a library. Just having it in the typesMap.json should be fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Copy Markdown
Member

@billti billti left a comment

Choose a reason for hiding this comment

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

See inline comments.

Comment thread src/server/typesMap.json
"types": ["datatables.net"]
},
"Ace": {
"match": "^(.*)\\/ace.js",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please note that ace.js has a very generic folder name, ranging between "src", "src-min", "src-noconflict", etc. thus the need to match any folder name as a user is very likely to append the name.

In the data analyzed a user use the name "ace-src-min".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's interesting looking at the repo at https://github.com/ajaxorg/ace-builds . I see multiple folders with different builds all including ace.js, so I get why this is needed. Does the safelist mechanism detect and exclude multiple folders for the same filename? It would be good to (have a) test to make sure. cc @RyanCavanaugh .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does, I tested manually having an src-min folder and on another path src-noconflict, and it excluded both of the folders. I will work on an automated test on another PR.

@armanio123 armanio123 changed the title Added datatables library to safelist Added big libraries to safelist Jan 31, 2018
@armanio123 armanio123 merged commit e7ddb84 into microsoft:master Jan 31, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants