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

sandbox iframe used for github star #1542

Closed
wants to merge 1 commit into from

Conversation

sonalkr132
Copy link
Member

iframe only needs permission to excute script and open new window.
It doesn't need other permission like form submission, navigation of
top-level browsing content etc. Sandboxing is not supported by IE 10
and below.
Read: https://msdn.microsoft.com/en-us/hh563496

iframe only needs permission to excute script and open new window.
It doesn't need other permission like form submission, navigation of
top-level browsing content etc. Sandboxing is not supported by IE 10
and below.
Read: https://msdn.microsoft.com/en-us/hh563496
@simi
Copy link
Member

simi commented Jan 12, 2017

I'm not sure if using 3rd party (even used script is not from GitHub itself) application in iframe is the best solution here.

@sonalkr132
Copy link
Member Author

This makes me scared 😱
screenshot from 2017-01-12 23-59-21

I have only seen ghbtns used in static website like a github page for ones library. At least we should have content security policy in place first.
I would still suggest what cocapods does. They use a different app to update a separate table for github metrics. They blogged a few details but one can always get the story together from their repo.
Stars icon is no magic, we can build of our own.

@simi
Copy link
Member

simi commented Jan 12, 2017

I think that's not needed. We can get numbers from GH and vendor button styles.

@sonalkr132
Copy link
Member Author

What about rate limits of Github API? I hope eventually we will show open issue and pull requests as well.

@simi
Copy link
Member

simi commented Jan 12, 2017

I mean on client side.

@sonalkr132
Copy link
Member Author

I am not sure if doing it on client side would be a feasible solution either. We will be making unauthenticated requests to github api and rate limit for that is 60 requests per hour.
Given that they use ip address to track the rate limit, can you suggest how we can deal with scenario where our multiple users are on the same network and they have same public ip address?

@sonalkr132
Copy link
Member Author

sonalkr132 commented Jan 13, 2017

Turns out we are already making requests to github api 😂 ghbtns probably just provides icon and style.
screenshot from 2017-01-13 13-30-49

We should so remove this 🙅‍♂️ @dwradcliffe what do you say?

@dwradcliffe
Copy link
Member

I'm totally ok with removing the 3rd party part and doing this ourselves. 👍 I'd like to keep the github api stuff client side though.

@simi
Copy link
Member

simi commented Jan 13, 2017

@dwradcliffe I can try to craft this.

@dwradcliffe
Copy link
Member

Is this still valid?

@sonalkr132
Copy link
Member Author

See #1827

@sonalkr132 sonalkr132 closed this Nov 4, 2018
@sonalkr132 sonalkr132 deleted the sandbox-iframe branch November 4, 2018 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants