Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Be more defensive when creating a buffer #155

Merged
merged 5 commits into from
Jun 4, 2019
Merged

Be more defensive when creating a buffer #155

merged 5 commits into from
Jun 4, 2019

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented May 29, 2019

fixes zapier/zapier-platform-cli#428. We don't addressed nested data in sensitive values, but i've added a failing test to address it though. This doesn't fix that root issue, but it does at least stop throwing errors.

I've also incorporated cooksey's test where we were changing everything to a string. did some related fixes for being better about mutating input.

@xavdid xavdid requested a review from codebycaleb May 29, 2019 02:41
Copy link
Contributor

@codebycaleb codebycaleb left a comment

Choose a reason for hiding this comment

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

This looks pretty defensive to me! You might get a merge conflict as I had pushed changes trying to solve a similar problem (https://github.com/zapier/zapier-platform-core/pull/152/files). I think it's probably for the best to discard my line where I casted val to a String (which felt dirty anyways) and just be okay with uncensored base64 numbers. 👍

@xavdid
Copy link
Contributor Author

xavdid commented May 29, 2019

Ah, nice! I should have pulled master.

I still can't get a failing test here, so I think i'll hold off merging. I like catching numbers too, since numbers and strings are probably the big offenders. I'll probably end up with a combination of our two solutions, so we censor numbers but don't trip over other things

@xavdid
Copy link
Contributor Author

xavdid commented May 31, 2019

ok, so I spent some time debugging this today and didn't quite get there. I incorporated the test from #156 so we could track it down. I'll get back to it Monday!

@xavdid
Copy link
Contributor Author

xavdid commented Jun 4, 2019

ok, got this all set. @codebycaleb can you re-review, it's changed a fair amount. I've updated the description accordingly.

Copy link
Contributor

@codebycaleb codebycaleb left a comment

Choose a reason for hiding this comment

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

Re-approving - these changes look great! Very readable and easy to follow mentally! 👍

@xavdid xavdid merged commit fe73422 into master Jun 4, 2019
@xavdid xavdid deleted the defensive-buffer branch June 4, 2019 22:37
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.

8.x OAuth fails to find access_token which worked in 7.x
3 participants