GNOME Bugzilla – Bug 755617
Authorization header not set for root dir when login happened in a subdir
Last modified: 2015-10-03 15:47:44 UTC
This happens with the Basic auth. If you visit /login and you successfully provide the HTTP credentials, the Authorization header is not set if you then visit /. This is because the protection space is wrong in this particular case, because we are not correctly handling the case of being / the parent directory of the current URI in soup_auth_basic_get_protection_space() and the returned protection spec is not the parent dir, but the same URI.
Created attachment 312120 [details] [review] soup-auth-basic: Correctly handle the protection space for subdirectories of root dir
Comment on attachment 312120 [details] [review] soup-auth-basic: Correctly handle the protection space for subdirectories of root dir thanks
Comment on attachment 312120 [details] [review] soup-auth-basic: Correctly handle the protection space for subdirectories of root dir Pushed to git master
Created attachment 312596 [details] [review] soup-auth-basic: Fix potential NULL dereference when checking URI path The fix in commit c6c95600f00d36d6e1c76e2f6bc6286019de0688 introduced a potential NULL pointer dereference if the SoupURI.path does not contain a slash ('/'). Spotted by Coverity (CID: 130092).
This introduced a potential NULL pointer dereference crash for URI paths which do not contain a slash. Patch attached.
Comment on attachment 312596 [details] [review] soup-auth-basic: Fix potential NULL dereference when checking URI path > p = strrchr (space, '/'); >- if (p == space && p[1]) >+ if (p && p == space && p[1]) We're checking "p == space". So if p is NULL, then space is NULL. But that means the previous line passed NULL to strrchr(), which means we would crash before getting to "p[1]" anyway. (And why didn't coverity point that out?) With the patch, we would now not crash, but we'd return a GSList containing NULL, which would just cause a crash later on because we're supposed to be returning a list of strings. The more-correct behavior would be to return an empty list, but as it happens, space cannot be NULL here anyway (though that's beyond coverity's ability to prove).
(In reply to Dan Winship from comment #6) > Comment on attachment 312596 [details] [review] [review] > soup-auth-basic: Fix potential NULL dereference when checking URI path > > > p = strrchr (space, '/'); > >- if (p == space && p[1]) > >+ if (p && p == space && p[1]) > > We're checking "p == space". So if p is NULL, then space is NULL. But that > means the previous line passed NULL to strrchr(), which means we would crash > before getting to "p[1]" anyway. (And why didn't coverity point that out?) > > With the patch, we would now not crash, but we'd return a GSList containing > NULL, which would just cause a crash later on because we're supposed to be > returning a list of strings. The more-correct behavior would be to return an > empty list, but as it happens, space cannot be NULL here anyway (though > that's beyond coverity's ability to prove). Ah, yes. Drive-by patching doesn't work when I don't do the analysis properly. :-( Sorry for the noise.