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

auth params in image urls don't work in android #7200

Closed
npomfret opened this issue Apr 24, 2016 · 10 comments
Closed

auth params in image urls don't work in android #7200

npomfret opened this issue Apr 24, 2016 · 10 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Resolution: Locked This issue was locked by the bot. Type: Enhancement A new feature or enhancement of an existing feature.

Comments

@npomfret
Copy link
Contributor

If I have an image url like http://**admin:pass@**host/image.jpg it appears that the username and password get stripped from it by the component. Is this by design?

@grabbou
Copy link
Contributor

grabbou commented Apr 24, 2016

I would say it probably gets striped out at some point -> if you could do some debugging by putting breakpoints here and there in this method https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L261 - it will probably tell you more. If not - no worries, I'll try to look into that when I get to the office in the morning.

Sounds like a small bugfix, is it iOS or Android?

@grabbou grabbou added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Feature Request labels Apr 24, 2016
@npomfret
Copy link
Contributor Author

I've narrowed it down to an android bug, but I can't find the problem. I think what's supposed to happen when you specify a username and password in the url, is that an authorization header gets created with a base64 encode of the credentials. At least that's what it looks like my browser does.

It seems that the Android java client isn't doing this. I've stepped though the code and nowhere does it explicitly remove that text from the url. I guess its just ignoring it and not creating the auth header.

@npomfret
Copy link
Contributor Author

npomfret commented Apr 24, 2016

It might be the OkHttp library not supporting basic auth: square/okhttp#2143

Or maybe the fresco code needs a couple of lines adding to it. Help!

@jhack32
Copy link

jhack32 commented Apr 26, 2016

It could also be the image size, when you console logged the url, does it show the url you're expecting? If it does, you can check the adb logs to see if it gives you an image too big error. adb logcat -t 500 to see the logs. If you do see the error, the image will need to be resized

@npomfret
Copy link
Contributor Author

Its not the image size. OkHttp doesn't honour the Basic auth pattern, and Fesco doesn't put the auth headers in (in android at least). I don't know which is incorrect, maybe both? The problem doesn't happen on IOS.

An incorrect image size wouldn't result in a 401.

@npomfret
Copy link
Contributor Author

I think that it's going to be addressed in the OkHttp library. square/okhttp#2143

@grabbou
Copy link
Contributor

grabbou commented Apr 27, 2016

Feel free to close and thanks for digging into that. There's an issue in this repo about okhttp upgrading (there are some blockers on FB end). So if that ends up being fixed by them and released in next version - please reference it :)

@npomfret npomfret closed this as completed Jun 8, 2016
@npomfret
Copy link
Contributor Author

npomfret commented Sep 19, 2016

Its not being fixed by them unfortunately. I'm not sure what do do now. There's a proposed solution but for some reason they won't implemented it. Could it be added to RN?

Ideally, I'd like to be able to add auth header props to the <Image ..

See: #7791 and facebook/fresco#1459

@npomfret npomfret reopened this Sep 19, 2016
@npomfret npomfret changed the title auth params in image urls get removed auth params in image urls don't work in android Sep 19, 2016
@npomfret
Copy link
Contributor Author

npomfret commented Sep 19, 2016

Ah, looks like support for adding headers to image requests has been added to Fresco in com/facebook/imagepipeline/backends/okhttp3/OkHttpNetworkFetcher.java on Jul 6th. Looks like version 13 got released since then so I guess this change is in. But... I think RN is using Fresco version 11 still.

@lacker
Copy link
Contributor

lacker commented Dec 15, 2016

Yeah, this is just pending on that Fresco upgrade and getting it in. I'm going to close this issue because I think we are already sufficiently tracking problems with the old fresco, thanks for reporting this!

@lacker lacker closed this as completed Dec 15, 2016
@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
@hramos hramos added the Type: Enhancement A new feature or enhancement of an existing feature. label Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Resolution: Locked This issue was locked by the bot. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

6 participants