Add CloudflareProtectedEmailAddress #6

ondenman · 2017-05-25T12:53:29Z

This is a step towards fixing: everypolitician/everypolitician-data#34867

Member email addresses have been obfuscated and the scraper is currently capturing a string of javascript instead of the email address:

:email=>"[email protected]/* <![CDATA[ */!function(t,e,r,n,c,a,p){try{t=document.currentScript||function(){for(t=document.getElementsByTagName('script'),e=t.length;e--;)if(t[e].getAttribute('data-cfhash'))return t[e]}();if(t&&(c=t.previousSibling)){p=t.parentNode;if(a=c.getAttribute('data-cfemail')){for(e='',r='0x'+a.substr(0,2)|0,n=2;a.length-n;n+=2)e+='%'+('0'+('0x'+a.substr(n,2)^r).toString(16)).slice(-2);p.replaceChild(document.createTextNode(decodeURIComponent(e)),c)}p.removeChild(t)}}catch(u){}}()/* ]]> */"

For example, when the obfuscated address is b3d2dfd7dcc5d6c1d2f3d7dac3c6c7d2d7dcc09dd4dcc59dc3ca, calling

CloudflareProtectedEmailAddress.new('b3d2dfd7dcc5d6c1d2f3d7dac3c6c7d2d7dcc09dd4dcc59dc3ca').human_readable

will return:
[email protected]

The javascript comes from the Paraguay member pages. The key part is:

a.substr(n,2)^r

r is the first character of the input string. The remainder is the obfuscated email. Two digit hexadecimals represent each character.

The next step will be to use this class within MemberPage.

This method of obfuscation is not particular to this site. For example, it is being used on the New Zealand site everypolitician/everypolitician-data#36492. The intention is to develop the class in this scraper before extracting it to a gem.

tmtmtmtm

I'm assuming the plan is to get this working here, and then extract to a gem? Would be good to note that if so.

As per previous discussion, you also need to explain where the algorithm comes from.

tmtmtmtm · 2017-05-25T14:32:19Z

lib/cloudflare_protected_email_address.rb

+require 'cgi'
+
+class CloudflareProtectedEmailAddress
+  def initialize(obfuscated:)


What's your thinking behind making this require a named parameter, rather than just a positional one? If there are likely to be other arguments that might make sense, but here it seems like it just complicates the interface.

I was leaning towards verbosity but I see why it complicates the interface.

tmtmtmtm · 2017-05-25T14:32:39Z

lib/cloudflare_protected_email_address.rb

+  end
+
+  def key
+    @key ||= obfuscated[0..1].hex


is this really expensive enough to need caching? (Especially compared to unobfuscated)

tmtmtmtm · 2017-05-25T14:34:11Z

lib/cloudflare_protected_email_address.rb

+    @key ||= obfuscated[0..1].hex
+  end
+
+  def unobfuscated


The value returned from this still seems fairly obfuscated to me…

tmtmtmtm · 2017-05-25T14:35:14Z

test/cloudflare_protected_email_address_test.rb

+  end
+
+  it 'will make an obfuscated email address human readable' do
+    email.must_equal '[email protected]'


it would be good to have another test here as well. I could make this test suite pass with a MUCH simpler implementation :D

ondenman · 2017-05-25T16:53:17Z

#human_readable returns the original string if the unobfuscated one does not look like at email (it tests to see if contains an @). I'm not sure if it's desirable behaviour but I think it may be more useful than returning something unintelligible if, for instance, the class was initialised with a plain email address.

tmtmtmtm · 2017-05-25T17:06:29Z

lib/cloudflare_protected_email_address.rb

+
+  def key
+    obfuscated[0..1].hex
+  end


key and obfuscated_characters here are both taking responsibility for the same thing, but doing it in slightly different ways. This would be cleaner and more maintainable if those both used an underlying method that took care of the conversion e.g.

def hex_pairs obfuscated.scan(/.{2}/).map(&:hex) end def obfuscated_characters hex_pairs.drop(1) end def key hex_pairs.first end

(not sure if hex_pairs is the right name here)

tmtmtmtm · 2017-05-25T17:10:42Z

lib/cloudflare_protected_email_address.rb

+  def escaped_email_address
+    obfuscated_characters.reduce('') do |email, encoded_character|
+      email + (encoded_character ^ key).to_s(16).prepend('%')
+    end


This is a bit over-complicated.

Perhaps a simple map+join would be clearer?

obfuscated_characters.map { |char| (char ^ key).to_s(16).prepend('%') }.join

tmtmtmtm · 2017-05-25T17:12:49Z

lib/cloudflare_protected_email_address.rb

+
+  def human_readable
+    return CGI.unescape(escaped_email_address) if CGI.unescape(escaped_email_address).include?('@')
+    obfuscated


This has become much too complicated and confusing now, I fear. If you really need to do this check, I'd suggest at least factoring out that CGI.unescape(escaped_email_address) to a named method.

The logic of this also seems back-to-front. A guard clause should be used to break out in unusual cases, not be the main path, with a fall through to the odd ones.

Again, I'm not convinced you need this at all, but if you do, then it would be better as:

return obfuscated unless unescaped.include? '@' unescaped

The Paraguay members page contains obfuscated email addresses. When initialized with an encrypted email address from the Paraguay members list page, CloudFlareProtectedEmail#unobfuscated_email will return the human readable address.

tmtmtmtm · 2017-06-08T17:05:26Z

lib/cloudflare_protected_email_address.rb

+  attr_accessor :obfuscated
+
+  def components
+    obfuscated.scan(/.{2}/).map(&:hex)


I'm curious here why you're using /.{2}/ rather than just ..

tmtmtmtm · 2017-06-08T17:06:17Z

lib/cloudflare_protected_email_address.rb

+
+  attr_accessor :obfuscated
+
+  def components


components is a bit too overloaded a term for me to be entirely comfortable with it here — I'm not sure it really conveys what's actually going on, and could be misleading. Is there a better name you can think of?

tmtmtmtm · 2017-06-08T17:11:47Z

lib/cloudflare_protected_email_address.rb

+    obfuscated.scan(/.{2}/).map(&:hex)
+  end
+
+  def obfuscated_characters


This is actually quite a misleading name too, as it returns integers, not characters

tmtmtmtm · 2017-06-08T17:18:15Z

lib/cloudflare_protected_email_address.rb

+    components.first
+  end
+
+  def escaped


I think this would be easier to follow if it actually mentioned percent encoding (or hex encoding) somewhere. Ideally that would be obvious from the name of the method, though if that's not sensible, then a comment might be called for.

tmtmtmtm · 2017-06-08T17:24:38Z

lib/cloudflare_protected_email_address.rb

+  end
+
+  def escaped
+    obfuscated_characters.map { |char| (char ^ key).to_s(16).prepend('%') }.join


Now that I've worked out what's actually going on here, I'm not entirely sure that .to_s(16) gets you the correct format. This approach will convert 15 to '%f' when I think it's strictly meant to be '%0F'.

sprintf("#%02X" % i) should get you the correct value, and means you wouldn't need the prepend either.

ondenman force-pushed the add-cloudflare-protected-email-address branch from 26fc736 to d6d9c80 Compare May 25, 2017 12:57

This was referenced May 25, 2017

WIP: Fix email field everypolitician-scrapers/new-zealand-parliament#17

Open

New Zealand: failing scraper -- protected email addresses everypolitician/everypolitician-data#36492

Open

ondenman changed the title ~~Add CloudflareProtectedEmailAddress class~~ Add CloudflareProtectedEmailAddress May 25, 2017

ondenman requested a review from tmtmtmtm May 25, 2017 14:03

ondenman assigned tmtmtmtm May 25, 2017

tmtmtmtm suggested changes May 25, 2017

View reviewed changes

tmtmtmtm assigned ondenman and unassigned tmtmtmtm May 25, 2017

ondenman force-pushed the add-cloudflare-protected-email-address branch from ae51cff to 2d0f184 Compare May 25, 2017 16:51

ondenman requested a review from tmtmtmtm May 25, 2017 16:54

ondenman assigned tmtmtmtm May 25, 2017

tmtmtmtm reviewed May 25, 2017

View reviewed changes

tmtmtmtm suggested changes May 25, 2017

View reviewed changes

tmtmtmtm removed their assignment May 25, 2017

ondenman assigned tmtmtmtm and unassigned ondenman Jun 6, 2017

ondenman requested a review from tmtmtmtm June 6, 2017 08:53

Oliver Denman added 2 commits June 8, 2017 18:02

Add CloudFlareProtectedEmail class

b0d18ff

The Paraguay members page contains obfuscated email addresses. When initialized with an encrypted email address from the Paraguay members list page, CloudFlareProtectedEmail#unobfuscated_email will return the human readable address.

Test CloudflareProtectedEmailAddress

9199f3c

tmtmtmtm force-pushed the add-cloudflare-protected-email-address branch from 1488b48 to 9199f3c Compare June 8, 2017 17:03

tmtmtmtm suggested changes Jun 14, 2017

View reviewed changes

tmtmtmtm assigned ondenman and unassigned tmtmtmtm Jun 14, 2017

ondenman force-pushed the add-cloudflare-protected-email-address branch from c7ef388 to ed9f546 Compare June 16, 2017 10:01

ondenman assigned tmtmtmtm and unassigned ondenman Jun 16, 2017

ondenman requested review from tmtmtmtm and removed request for tmtmtmtm June 16, 2017 11:24

ondenman assigned ondenman and unassigned tmtmtmtm Jun 16, 2017

fixup! Add CloudFlareProtectedEmail class

54f0000

ondenman force-pushed the add-cloudflare-protected-email-address branch from ed9f546 to 54f0000 Compare June 16, 2017 15:38

ondenman requested a review from tmtmtmtm June 19, 2017 08:46

ondenman assigned tmtmtmtm and unassigned ondenman Jun 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CloudflareProtectedEmailAddress #6

Add CloudflareProtectedEmailAddress #6

ondenman commented May 25, 2017 •

edited

Loading

tmtmtmtm left a comment

tmtmtmtm May 25, 2017

ondenman May 25, 2017

tmtmtmtm May 25, 2017

tmtmtmtm May 25, 2017

tmtmtmtm May 25, 2017

ondenman commented May 25, 2017

tmtmtmtm May 25, 2017

tmtmtmtm May 25, 2017 •

edited

Loading

tmtmtmtm May 25, 2017

tmtmtmtm May 25, 2017

tmtmtmtm Jun 8, 2017

tmtmtmtm Jun 8, 2017

tmtmtmtm Jun 8, 2017

tmtmtmtm Jun 8, 2017

tmtmtmtm Jun 8, 2017

Add CloudflareProtectedEmailAddress #6

Are you sure you want to change the base?

Add CloudflareProtectedEmailAddress #6

Conversation

ondenman commented May 25, 2017 • edited Loading

tmtmtmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondenman commented May 25, 2017

Choose a reason for hiding this comment

tmtmtmtm May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondenman commented May 25, 2017 •

edited

Loading

tmtmtmtm May 25, 2017 •

edited

Loading