Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix request cache when filtering sensitive params. #34

Merged
merged 17 commits into from
Apr 16, 2017

Conversation

thhermansen
Copy link
Member

@thhermansen thhermansen commented Apr 13, 2017

This fixes an issue where filtering the URL actually mutated the given URL string. That string was also used as a cache key, but it didn't hit the cache as the string was mutated.

Fixes #32

PS. There are some a lot of rubocop related changes in this pull request as well as the fix. Sorry for that, but I didn't spend time keeping the rubocop commits separate from the fix.

@thhermansen
Copy link
Member Author

thhermansen commented Apr 13, 2017

This pull request filters the URL logged, but it uses the URL (with sensitive URL params) as cache key.

Maybe a more correct fix is to only use URL with sensitive data when doing a request to Google's API, but as cache key and when logging we'll use the filtered one.

What do you think, @aceunreal?

@thhermansen
Copy link
Member Author

I think I'll push the code in the direction where we have a sensitive_url and a filtered_url from the UrlBuilder class., thus remove the responsibility of filtering the URL from the log subscriber.

Maybe we we could hash the sensitive_url when we add it to the cache as well, instead of use the filtered_url as hash key, as people may want to filter params which should be included as cache key.

@thhermansen thhermansen merged commit 168d571 into master Apr 16, 2017
@thhermansen thhermansen deleted the fix-request-cache branch April 16, 2017 18:30
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.

Request caching not working
1 participant