Drop special headers from redirected requests #289

sberrevoets · 2018-09-15T23:10:48Z

Checklist

I've checked that all new and existing tests pass
I've updated the documentation if necessary
I've added an entry in the CHANGELOG to credit myself

Description

The change in #263 means a redirected request had the same headers as the original request (because it was a copy and not a brand new NSURLRequest instance).

While the new behavior was correct, it didn't account for "special headers" that URLSession "handles". The docs aren't entirely clear what that means, but we've at least confirmed it removes the "Authorization" headers as that poses a security risk, so this PR removes all those headers + a test.

keith · 2018-09-16T00:42:59Z

I think these test failures are related to your changes @sberrevoets ? It looks like master is mostly green (besides some flakes)

sberrevoets · 2018-09-16T01:10:32Z

I was able to reproduce this once (via rake, I never saw them fail in Xcode 9.4.1), but now even rake is succeeding locally.

@AliSoftware any idea what might be going on?

AliSoftware · 2018-09-16T09:47:11Z

I think the build failures might be related to the race condition bug we discovered in URLSession in #230 (comment) (don't hesitate to duplicate the radar to help us make Apple fix it btw)

See also #280

sberrevoets · 2018-09-16T23:17:57Z

Ah thanks for the heads up, I've duped the radar. What would you like to see here in the meantime?

sberrevoets · 2018-09-20T22:59:25Z

Friendly ping @AliSoftware :)

AliSoftware · 2018-09-23T14:02:55Z

Sorry for the late reply.
I honestly don't really know. I mean those redirect tests are probably always gonna fail until that Apple bug gets fixed or we find a workaround.
So maybe we should merge that anyway given that the failures are not our fault?

Or a better solution might be to introduce a small delay in the redirection (like what we done in the other PR to confirm the bug was a race condition) as it seemed to at least reduce a lot the bug from happening; even if that means introducing a delay and slowing down stuff a little, that's probably better than failing 70% of the time…?

sberrevoets · 2018-09-24T06:49:53Z

Yeah that's a very good question. For me it'd probably depend on how much it fails. If it really is 70% of the time right now then adding a delay seems worth it. If it's every now and then, it might be possible to (somehow) auto-retry the affected tests, or take a simpler approach of adding a comment explaining the problem.

I'm also open to alternatives, framework bugs like these are a huge PITA 😕

jeffctown · 2019-03-08T18:31:52Z

@sberrevoets - we're reviewing all outstanding PRs before making a release, and then updating the version of Swift being used. the hope is to do all of this before Xcode 10.2 comes out, so our timeline is a bit tight.

If you are still interested in getting this in, please resolve the conflicts. If you are not interested, having that feedback would be helpful too.

sberrevoets · 2019-03-08T18:47:28Z

Happy to resolve conflicts (though can't promise I'll have time for that before the 10.2 release), but how depends on what approach is preferable. Based on the conversation above I'm not sure if there's a clear path forward.

jeffctown · 2019-03-08T18:53:55Z

@sberrevoets - if you mean the failing tests - those were disabled in CI (#301 & #302), so you don't need to worry about that part. We only need the conflicts resolved. If you want to give me permission to commit to your fork's master branch I don't mind resolving them myself. the tests should pass once the conflicts are resolved.

sberrevoets · 2019-03-08T19:01:10Z

Ah I missed that, that makes resolving conflicts a bit easier. Pushed an update but also gave edit access just in case.

AliSoftware

A little sad that the tests that were previously added and which were testing if proper headers were dropped or preserved… got removed.

I mean I know they are flaky due to the Apple Bug but with #301 and #302 the goal was to disable those… only on CI. While still be able to run them on Xcode (which seemed less prone to the race condition bug for some reason probably related to our personal machines speeds)

So I think it would still be valuable to reintroduce that test that we had before the recent commit fixing the conflict, even though it won't be run by CI, as it seemed valuable and properly written and could still be useful to have when we run the tests on Xcode and not on CI.

AliSoftware · 2019-03-08T20:32:22Z

OHHTTPStubs/Sources/OHHTTPStubs.m

+                            [mReq setValue:nil forHTTPHeaderField:@"Host"];
+                            [mReq setValue:nil forHTTPHeaderField:@"Proxy-Authenticate"];
+                            [mReq setValue:nil forHTTPHeaderField:@"Proxy-Authorization"];
+                            [mReq setValue:nil forHTTPHeaderField:@"WWW-Authenticate"];


This could maybe be a for header in @[@"…",@"…",…] loop instead?

I pushed a commit to put this into it's own method since where it is now is already a bit large.

jeffctown · 2019-03-08T23:02:50Z

@AliSoftware I updated this branch with a for loop, and I still see the conditional tests.

AliSoftware · 2019-03-08T23:17:52Z

I was talking about a new test case that @sberrevoets wrote in his initial commits, and that I remember about from when I reviewed the diff on that PR before the conflict resolution today. That new test case which was added in that PR was essentially creating a request, with all the headers of interest here + another header — named "preserved" iirc —, and after using that dummy request and redirecting it, did XCTAssertNil() on all proper headers to check they were properly removed and an XCTAssertEqual() on the one named "preserved" to check it wasn't removed.

That test case seems not to be there anymore (or is my GitHub view of the PR diff on my phone making me missing something?). It probably was removed by the force-push done by @sberrevoets today I guess. While I think it would be worth to have that test case back (even if it ends up not being run on CI because we disabled them there, but they would still be able to be run locally in Xcode)

jeffctown · 2019-03-08T23:33:22Z

Oh! My bad man. I thought you meant the buggy test.

_{Sent with GitHawk}

sberrevoets · 2019-03-08T23:35:20Z

I did remove that test case because it was one of the conflicts. Working on adding that back now.

sberrevoets · 2019-03-08T23:41:06Z

Added that back and ran it locally - it still passed.

AliSoftware · 2019-03-08T23:45:00Z

OHHTTPStubs/UnitTests/Test Suites/NSURLSessionTests.m

+                                          @"Proxy-Authenticate": @"proxy-authenticate",
+                                          @"Proxy-Authorization": @"proxy-authorization",
+                                          @"WWW-Authenticate": @"www-authenticate",
+                                          @"Preserved": @"preserved"


Just because if, say, we change the implementation one day and accidentally make the new implementation behave ( wrongly) as "remove all headers but the last" or "only keep one header" (instead of doing what it's supposed to do like today), I would try to put one or two more "headers to be preserved" in there and intertwine them in the middle of that list instead of all in the end 😉 #paranoiaModeOn

But since it's a dictionary order isn't guaranteed anyway? Is that different under the NSURLSession hood?

Oh good point!

Though I suspect that dictionary literals, even though they don't guarantee order by contract as it's not a expected requirement for dicts, might end up always being iterated in the same deterministic order in practice (because of current implantation details of dictionary literals in ObjC)?

Yeah it doesn't hurt to add - done. (In a separate commit for reviewing purposes, happy to rebase at the end if you'd like.)

AliSoftware · 2019-03-09T00:16:16Z

Thanks @sberrevoets 🎉

sberrevoets · 2019-03-09T00:18:21Z

Thanks for the reviews and suggestions!

jeffctown added the Waiting for input label Mar 8, 2019

Drop special headers from redirected requests

119c7ff

jeffctown removed the Waiting for input label Mar 8, 2019

jeffctown requested review from AliSoftware and Liquidsoul and removed request for AliSoftware March 8, 2019 19:11

Update CHANGELOG.md

2fecbda

AliSoftware reviewed Mar 8, 2019

View reviewed changes

Put clearing of auth headers in it’s own method and using a for loop.

ed13794

Re-introduce header retention policy tests for 3xx

4d0f05b

AliSoftware reviewed Mar 8, 2019

View reviewed changes

fixup! Re-introduce header retention policy tests for 3xx

29ff8d8

AliSoftware approved these changes Mar 9, 2019

View reviewed changes

jeffctown merged commit e2dd12e into AliSoftware:master Mar 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop special headers from redirected requests #289

Drop special headers from redirected requests #289

sberrevoets commented Sep 15, 2018

keith commented Sep 16, 2018

sberrevoets commented Sep 16, 2018 •

edited

Loading

AliSoftware commented Sep 16, 2018 •

edited

Loading

sberrevoets commented Sep 16, 2018

sberrevoets commented Sep 20, 2018

AliSoftware commented Sep 23, 2018

sberrevoets commented Sep 24, 2018

jeffctown commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

jeffctown commented Mar 8, 2019 •

edited

Loading

sberrevoets commented Mar 8, 2019

AliSoftware left a comment

AliSoftware Mar 8, 2019

jeffctown Mar 8, 2019

jeffctown commented Mar 8, 2019 •

edited

Loading

AliSoftware commented Mar 8, 2019

jeffctown commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

AliSoftware Mar 8, 2019 •

edited

Loading

sberrevoets Mar 8, 2019

AliSoftware Mar 8, 2019

sberrevoets Mar 9, 2019

AliSoftware commented Mar 9, 2019

sberrevoets commented Mar 9, 2019

Drop special headers from redirected requests #289

Drop special headers from redirected requests #289

Conversation

sberrevoets commented Sep 15, 2018

Checklist

Description

keith commented Sep 16, 2018

sberrevoets commented Sep 16, 2018 • edited Loading

AliSoftware commented Sep 16, 2018 • edited Loading

sberrevoets commented Sep 16, 2018

sberrevoets commented Sep 20, 2018

AliSoftware commented Sep 23, 2018

sberrevoets commented Sep 24, 2018

jeffctown commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

jeffctown commented Mar 8, 2019 • edited Loading

sberrevoets commented Mar 8, 2019

AliSoftware left a comment

Choose a reason for hiding this comment

AliSoftware Mar 8, 2019

Choose a reason for hiding this comment

jeffctown Mar 8, 2019

Choose a reason for hiding this comment

jeffctown commented Mar 8, 2019 • edited Loading

AliSoftware commented Mar 8, 2019

jeffctown commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

sberrevoets commented Mar 8, 2019

AliSoftware Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

sberrevoets Mar 8, 2019

Choose a reason for hiding this comment

AliSoftware Mar 8, 2019

Choose a reason for hiding this comment

sberrevoets Mar 9, 2019

Choose a reason for hiding this comment

AliSoftware commented Mar 9, 2019

sberrevoets commented Mar 9, 2019

sberrevoets commented Sep 16, 2018 •

edited

Loading

AliSoftware commented Sep 16, 2018 •

edited

Loading

jeffctown commented Mar 8, 2019 •

edited

Loading

jeffctown commented Mar 8, 2019 •

edited

Loading

AliSoftware Mar 8, 2019 •

edited

Loading