Skip to content

Conversation

@caub
Copy link
Contributor

@caub caub commented Dec 5, 2021

See #796 #793

@pokoli @wcerfgba Hey guys, could you check this PR?

It adds more cases for escapeFormulae option, cf https://owasp.org/www-community/attacks/CSV_Injection

@caub
Copy link
Contributor Author

caub commented Dec 5, 2021

I added possibility to pass a RegExp as escapeFormulae option, but not sure it's needed, just tell me, I can remove that

@pokoli
Copy link
Collaborator

pokoli commented Dec 9, 2021

Hi @caub!

Thanks for your suggestion. Could you please keep the current tests cases as is (so we ensure nothing is borken) and add a new test case for the regexp validation (the new feature) ?

Thanks in advance!

@caub
Copy link
Contributor Author

caub commented Dec 9, 2021

Hey @pokoli
Current tests are actually changed, https://owasp.org/www-community/attacks/CSV_Injection recommanded to escape values anyway as soon as a formulae is used

Not sure if really necessary but what's your thought? can we revert my last commit if you prefer? (and then previous tests will be identical)

@caub
Copy link
Contributor Author

caub commented Dec 9, 2021

I'll start separating anyway so it's clearer, you're right

@pokoli
Copy link
Collaborator

pokoli commented Dec 9, 2021

@caub feel free to remove some test suit if it is no longer valid (but please keep the valid ones)

@caub
Copy link
Contributor Author

caub commented Dec 9, 2021

ok done, https://github.com/mholt/PapaParse/pull/904/files you can see 2 of the previous tests are affected, because for example =test was converted in '=test as it doesn't require escaping the whole value, with owasp recommandation we get either "'=test" or '''=test'

@pokoli
Copy link
Collaborator

pokoli commented Dec 9, 2021

Hi @caub,

Tests failed due to node15 (which seems unrelated). I removed the version from github actions in 26a86fd

Could you please rebase the PR so I can relauch the test suite to ensure everything is in place.

Sorry for the inconvenience.

@caub
Copy link
Contributor Author

caub commented Dec 9, 2021

yep, done

@pokoli pokoli merged commit 1f2c733 into mholt:master Dec 10, 2021
@pokoli
Copy link
Collaborator

pokoli commented Dec 10, 2021

Merged it. Thanks

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.

2 participants