Issue 29329256: Issue 3122 - Restrict allowed URLs for the sitekey_frame (Closed)
DescriptionIssue 3122 - Restrict allowed URLs for the sitekey_frame
Patch Set 1 #
Total comments: 4
Patch Set 2 : Gone with Manvel's suggestion #MessagesTotal messages: 6
Patch Set 1
https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... File templates/sitekey_frame.tmpl (right): https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... templates/sitekey_frame.tmpl:9: return base_href = href.substr( The description says Restrict allowed URLs for the sitekey_frame, I'm not sure what the frame_url suppose to return and why it's extracting the origin of the URL and concatenating the hash value without hash sign assigned to it ?
https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... File templates/sitekey_frame.tmpl (right): https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... templates/sitekey_frame.tmpl:9: return base_href = href.substr( On 2015/11/16 13:53:25, saroyanm wrote: > The description says Restrict allowed URLs for the sitekey_frame, I'm not sure > what the frame_url suppose to return and why it's extracting the origin of the > URL and concatenating the hash value without hash sign assigned to it ? So the sitekey_frame page contains a big iFrame and sets its source based on the hash part of the URL using JavaScript. The reason for that is that we can have a dynamically generated sitekey_frame page that has the correct sitekey headers etc that then displays a statically generated page inside. That way we can write tests for sitekey functionality in the future without further changes to the sitescripts repository. All I'm doing here is adding a bit of security, previously I set the src of the iFrame to be hash part of the URL without any restrictions. In theory someone could have typed ...#http://google.com , which would have set the source of the iframe to http://google.com. This change avoids that by prepending the current domain to it so only relative paths will work.
https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... File templates/sitekey_frame.tmpl (right): https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... templates/sitekey_frame.tmpl:9: return base_href = href.substr( On 2015/11/16 23:01:09, kzar wrote: > On 2015/11/16 13:53:25, saroyanm wrote: > > The description says Restrict allowed URLs for the sitekey_frame, I'm not sure > > what the frame_url suppose to return and why it's extracting the origin of the > > URL and concatenating the hash value without hash sign assigned to it ? > > So the sitekey_frame page contains a big iFrame and sets its source based on the > hash part of the URL using JavaScript. The reason for that is that we can have a > dynamically generated sitekey_frame page that has the correct sitekey headers > etc that then displays a statically generated page inside. That way we can write > tests for sitekey functionality in the future without further changes to the > sitescripts repository. > > All I'm doing here is adding a bit of security, previously I set the src of the > iFrame to be hash part of the URL without any restrictions. In theory someone > could have typed ...#http://google.com , which would have set the source of the > iframe to http://google.com. This change avoids that by prepending the current > domain to it so only relative paths will work. I see. What about: return parent_location.origin + parent_location.hash.substring(1) ?
Patch Set 2 : Gone with Manvel's suggestion https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... File templates/sitekey_frame.tmpl (right): https://codereview.adblockplus.org/29329256/diff/29329257/templates/sitekey_f... templates/sitekey_frame.tmpl:9: return base_href = href.substr( On 2015/11/17 11:28:33, saroyanm wrote: > On 2015/11/16 23:01:09, kzar wrote: > > On 2015/11/16 13:53:25, saroyanm wrote: > > > The description says Restrict allowed URLs for the sitekey_frame, I'm not > sure > > > what the frame_url suppose to return and why it's extracting the origin of > the > > > URL and concatenating the hash value without hash sign assigned to it ? > > > > So the sitekey_frame page contains a big iFrame and sets its source based on > the > > hash part of the URL using JavaScript. The reason for that is that we can have > a > > dynamically generated sitekey_frame page that has the correct sitekey headers > > etc that then displays a statically generated page inside. That way we can > write > > tests for sitekey functionality in the future without further changes to the > > sitescripts repository. > > > > All I'm doing here is adding a bit of security, previously I set the src of > the > > iFrame to be hash part of the URL without any restrictions. In theory someone > > could have typed ...#http://google.com , which would have set the source of > the > > iframe to http://google.com. This change avoids that by prepending the current > > domain to it so only relative paths will work. > > I see. > What about: > return parent_location.origin + parent_location.hash.substring(1) ? Good point, much better. Done.
LGTM 
|
