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

Do not send CSP for Android Browser < 4.4. #82

Closed
wants to merge 1 commit into from

Conversation

pinktrink
Copy link

Android Browser < 4.4 doesn't simply silently fail when CSP headers are sent, as one might expect. It just doesn't load any resources.

@pinktrink pinktrink force-pushed the master branch 2 times, most recently from 68267a1 to ffdf129 Compare October 21, 2014 22:55
@EvanHahn
Copy link
Member

I'm going to make sure this is true before I merge (not that I don't trust you!), but before I do:

  1. Can platform determine the Android OS version? I'd prefer to use the library over the regexp if we can. (I assume we can't, but worth a look.)
  2. Let's not redefine the regexp every time. Move the var statement higher in scope so we only define it once.
  3. Some of your indentation is a little off. Looks like lines 203, 203, and 204 need to be indented.

After you fix that and I give this a test in an Android simulator, I'll merge! Thanks for finding this.

@pinktrink
Copy link
Author

  1. I took a look at the platform code and I didn't see anything that allowed that to happen. I definitely agree that it should be done in platform, and have considered making a pull request to them to allow this to happen. The thing is that there are definite inconsistencies when it comes to different platform versions. Simply making the second matching group of the regex optional could potentially fix any issues that it might run into, but that can't be guaranteed.
  2. Good point, my fault.
  3. I was trying to follow the overall indentation scheme. It might be that Vim was rendering something improperly. I'll take a look at it in some other editors.

@pinktrink
Copy link
Author

So after taking a look at it, I did follow the surrounding indentation scheme. This is a screenshot taken from Sublime Text:
Screenshot

I have defined the regex higher in scope.

@EvanHahn
Copy link
Member

I made a small server that sends some CSP to only resources from the same origin (sets the following headers), and then loads an HTML page to test CSP.

Content-Security-Policy: default-src 'self';
X-Content-Security-Policy: default-src 'self';
X-Webkit-CSP: default-src 'self';

I tried this in the Android Simulator for version 4.0.2, and while it didn't seem to support CSP, it was able to load things:

Android 4.0.2 demo

Are we sure that CSP breaks Android <4.4?

@pinktrink
Copy link
Author

Try setting it up with a fake CDN of sorts. We don't simply have 'self' in our CSP. It's interesting that you're getting these results. I'll continue doing some investigation on my end.

@EvanHahn
Copy link
Member

Could you find/make a test page that I could try? I made one myself and it doesn't seem to have the issue.

@pinktrink
Copy link
Author

Place this on hold for a bit. I'm going to set up a basic test app and see if I can't figure out a way to replicate the issue we're having.

@EvanHahn
Copy link
Member

Sounds good! Thanks for your help.

@EvanHahn
Copy link
Member

EvanHahn commented Jan 8, 2015

Because we've moved csp to its own module, the PR now just looks like this:

break;

+ case 'Android Browser':
+ break;
+

default:

Closing for now; feel free to reopen on the CSP module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants